Message-Id: <DEF5EC79OOT4.2MT1ET4IKXS5Y@kernel.org>
Date: Sat, 22 Nov 2025 23:23:16 +1300
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Jason Gunthorpe" <jgg@...pe.ca>
Cc: "Peter Colberg" <pcolberg@...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>, "Abdiel Janulgue"
 <abdiel.janulgue@...il.com>, "Daniel Almeida"
 <daniel.almeida@...labora.com>, "Robin Murphy" <robin.murphy@....com>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>, "Dave Ertman"
 <david.m.ertman@...el.com>, "Ira Weiny" <ira.weiny@...el.com>, "Leon
 Romanovsky" <leon@...nel.org>, <linux-pci@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 "Alexandre Courbot" <acourbot@...dia.com>, "Alistair Popple"
 <apopple@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "John
 Hubbard" <jhubbard@...dia.com>, "Zhi Wang" <zhiw@...dia.com>
Subject: Re: [PATCH 7/8] rust: pci: add physfn(), to return PF device for VF
 device

On Sat Nov 22, 2025 at 12:26 PM NZDT, Jason Gunthorpe wrote:
> On Wed, Nov 19, 2025 at 05:19:11PM -0500, Peter Colberg wrote:
>> Add a method to return the Physical Function (PF) device for a Virtual
>> Function (VF) device in the bound device context.
>> 
>> Unlike for a PCI driver written in C, guarantee that when a VF device is
>> bound to a driver, the underlying PF device is bound to a driver, too.
>
> You can't do this as an absolutely statement from rust code alone,
> this statement is confused.

Indeed! However, I'd like to see that we actually provide this guarantee from
the C PCI code.

So far I haven't heard a convincing reason for not providing this guarantee. The
only reason not to guarantee this I have heard is that some PF drivers only
enable SR-IOV and hence could be unloaded afterwards. However, I think there is
no strong reason to do so.

What I would like to see is that we unbind VF drivers when the PF driver is
unbound in general, analogous to what we are guaranteed by the auxiliary bus.

>> +    #[cfg(CONFIG_PCI_IOV)]
>> +    pub fn physfn(&self) -> Result<&Device<device::Bound>> {
>> +        if !self.is_virtfn() {
>> +            return Err(EINVAL);
>> +        }
>> +        // SAFETY:
>> +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
>> +        //
>> +        // `physfn` is a valid pointer to a `struct pci_dev` since self.is_virtfn() is `true`.
>> +        //
>> +        // `physfn` may be cast to a `Device<device::Bound>` since `pci::Driver::remove()` calls
>> +        // `disable_sriov()` to remove all VF devices, which guarantees that the underlying
>> +        // PF device is always bound to a driver when the VF device is bound to a driver.
>
> Wrong safety statement. There are drivers that don't call
> disable_sriov(). You need to also check that the driver attached to
> the PF is actually working properly.

Indeed, with this patch, only Rust drivers provide this guarantee of the VF
being bound when the PF is bound.

> I do not like to see this attempt to open code the tricky login of
> pci_iov_get_pf_drvdata() in rust without understanding the issues :(

I discussed this with Peter in advance (thanks Peter for your work on this
topic!), and as mentioned above I'd like to see this series to propose that we
always guarantee that a VF is bound when the corresponding PF is bound.

With this, the above code will be correct and a driver can use the generic
infrastructure to:

  1) Call pci::Device<Bound>::physfn() returning a Result<pci::Device<Bound>>
  2) Grab the driver's device private data from the returned Device<Bound>

Note that 2) (i.e. accessing the driver's device private data with
Device::drvdata() [1]) ensures that the device is actually bound (drvdata() is
only implemented for Device<Bound>) and that the returned type actually matches
the type of the object that has been stored.

Since we always need those two checks when accessing a driver's device private
data, it is already done in the generic drvdata() accessor.

Therefore the only additional guarantee we have to give is that VF bound implies
PF bound. Otherwise physfn() would need to be unsafe and the driver would need
to promise that this is the case. From there on drvdata() already does the other
checks as mentioned.

I suggest to have a look at [2] for an example with how this turns out with the
auxiliary bus [2][3], since in the end it's the same problem, i.e. an auxiliary
driver calling into its parent, except that the auxiliary bus already guarantees
that the parent is bound when the child is bound.

Given that, there is no value in using pci_iov_get_pf_drvdata(), in Rust you'd
just call

	// `vfdev` must be a `pci::Device<Bound>` for `physfn()` to be
	// available; `pfdev` will therefore be a `pci::Device<Bound>` too
	// (assuming we provide the guarantee for this, otherwise this would
	// need to be unsafe).
	let pfdev = vfdev.phyfn();

	// `FooData` is the type of the PF drvier's device private data. The
	// call to `drvdata()` will fail with an error of the asserted type is
	// wrong.
	let drvdata = pfdev.drvdata::<FooData>()?;

So, if we'd provide a Rust accessor for the PF's device driver data, we'd
implement it like above, because Device::drvdata() is already safe. If we want
pci::Device::pf_drvdata() to be safe, we'd otherwise need to do all the checks
Device::drvdata() already does again before we call into
pci_iov_get_pf_drvdata().

(Note that I'm currently travelling, hence I might not be as responsive as
usual. I'm travelling until after LPC; I plan to take a detailed look at this
series in the week of the conference).

[1] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/rust/kernel/device.rs?h=driver-core-next#n313
[2] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tree/samples/rust/rust_driver_auxiliary.rs?h=driver-core-next#n81
[3] https://lore.kernel.org/all/20251020223516.241050-1-dakr@kernel.org/

Powered by blists - more mailing lists