Nouveau - Oct 2025 - [PATCH v2 1/2] rust: pci: skip probing VFs if driver doesn't support VFs

On Thu, Oct 02, 2025 at 11:32:44PM +0200, Danilo Krummrich wrote:
> So, when we call pdev.physfn().drvdata_borrow::<NovaCore>() the
checks are
> included already.
I'm not keen on hiding this reasoning inside an physfn() accessor like
this. ie one that returns a Device<Bound>. The reasoning for this is
tricky and special. We have enough cases where physfn won't be a bound
driver. I think it is big stretch just to declare that unconditionally
safe.

There is a reason pci_iov_get_pf_drvdata() has such a big comment..

So I'd rather see you follow the C design and have an explicit helper
function to convert a VF bound device to a PF bound device and check
the owner, basically split up pci_iov_get_pf_drvdata() into a part to
get the struct device and an inline to get the drvdata. Rust still has an
ops pointer it can pass in so it can be consistent with the C code
even if it does another check inside its drvdata_borrow.

This way we keep the reasoning and explanation in one place.

Jason

On Fri Oct 3, 2025 at 1:40 AM CEST, Jason Gunthorpe
wrote:
> On Thu, Oct 02, 2025 at 11:32:44PM +0200, Danilo Krummrich wrote:
>
>> So, when we call pdev.physfn().drvdata_borrow::<NovaCore>() the
checks are
>> included already.
>
> I'm not keen on hiding this reasoning inside an physfn() accessor like
> this. ie one that returns a Device<Bound>. The reasoning for this is
> tricky and special. We have enough cases where physfn won't be a bound
> driver. I think it is big stretch just to declare that unconditionally
> safe.
In this example physfn() would return a &pci::Device<Bound>.

This is what I mentioned previously; I want the subsystem to guarantee (at least
under certain circumstances) that if the VF device is bound that the PF device
must be bound as well.
> There is a reason pci_iov_get_pf_drvdata() has such a big comment..
>
> So I'd rather see you follow the C design and have an explicit helper
> function to convert a VF bound device to a PF bound device
Well, this is exactly what physfn() does (with the precondition that we can get
the guarantee from the subsystem).
> and check
> the owner, basically split up pci_iov_get_pf_drvdata() into a part to
> get the struct device and an inline to get the drvdata. Rust still has an
> ops pointer it can pass in so it can be consistent with the C code
Which ops pointer are you referring to? Do you mean the struct pci_driver
pointer? If so, no we can't access this one. We could make it accessible,
but it
would result into horrible code, wouldn't make it possible to implement the
check generically for any device (which we need anyways) and would have some
other bad implications.

I try to be as consistent as possible with C code, but sometimes it just
doesn't
fit at all and would even hurt. This is one of those cases.

To give at least some background: A driver structure (like struct pci_driver) is
embedded in a module structure, which is a global static and intentionally not
directly accessible for drivers.

Even if we'd make it accessible, the driver field within a module structure
depends on the exact implementation, i.e. it depends on whether a module is
declared "by hand", or whether it is generated by a module_driver!()
macro (e.g.
module_pci_driver!().

It probably also helps to have a look at samples/rust/rust_driver_auxiliary.rs,
which open codes driver registrations, since it registers two drivers in the
same module for the purpose of illustrating an auxiliary connection, i.e. it
doesn't use a module_driver!() macro.

The struct struct auxiliary_driver resides within the
driver::Registration<auxiliary::Adapter<T>>, where
driver::Registration is a
generic type for registering drivers, auxiliary::Adapter defines the auxiliary
specific bits for this registration and T is the driver specific type that
implements the auxiliary::Driver trait, containing the bus callbacks etc.
> even if it does another check inside its drvdata_borrow.
It's the exact same check; just a better fitting implementation.

I.e. instead of checking a pointer which is hard to access, we check if the type
ID we gave to the device matches the driver specific type (the T in
driver::Registration<auxiliary::Adapter<T>>).

Semantically, there is no difference, but it results in less cumbersome and more
flexible code.