Message-Id: <DD8A27ESH61G.306ZAIGZCMJ97@kernel.org>
Date: Fri, 03 Oct 2025 02:57:53 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Jason Gunthorpe" <jgg@...dia.com>
Cc: "John Hubbard" <jhubbard@...dia.com>, "Alexandre Courbot"
 <acourbot@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "Timur
 Tabi" <ttabi@...dia.com>, "Alistair Popple" <apopple@...dia.com>, "Zhi
 Wang" <zhiw@...dia.com>, "Surath Mitra" <smitra@...dia.com>, "David Airlie"
 <airlied@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Alex Williamson"
 <alex.williamson@...hat.com>, "Bjorn Helgaas" <bhelgaas@...gle.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>, "Miguel
 Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun
 Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice
 Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 <nouveau@...ts.freedesktop.org>, <linux-pci@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, "LKML" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] rust: pci: skip probing VFs if driver doesn't
 support VFs

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.

Powered by blists - more mailing lists