Message-ID: <2025032158-embezzle-life-8810@gregkh>
Date: Fri, 21 Mar 2025 20:25:07 -0700
From: Greg KH <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: bhelgaas@...gle.com, rafael@...nel.org, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
	linux-pci@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] rust: pci: impl TryFrom<&Device> for &pci::Device

On Fri, Mar 21, 2025 at 10:47:54PM +0100, Danilo Krummrich wrote:
> Implement TryFrom<&device::Device> for &Device.
> 
> This allows us to get a &pci::Device from a generic &Device in a safe
> way; the conversion fails if the device' bus type does not match with
> the PCI bus type.

In thinking about this some more, I am starting to not really like it at
all, more below...

> +impl TryFrom<&device::Device> for &Device {
> +    type Error = kernel::error::Error;
> +
> +    fn try_from(dev: &device::Device) -> Result<Self, Self::Error> {
> +        #[allow(unused_unsafe)]
> +        // SAFETY: rustc < 1.82 falsely treats this as unsafe, see:
> +        // https://blog.rust-lang.org/2024/10/17/Rust-1.82.0.html#safely-addressing-unsafe-statics
> +        if dev.bus_type_raw() != unsafe { addr_of!(bindings::pci_bus_type) } {
> +            return Err(EINVAL);
> +        }

For the record, and to write it down so it's just not a discussion in
person like I've had in the past about this topic, I figured I'd say
something about why we didn't do this in C.

When we wrote the driver model/core all those decades ago, we made the
explicit decision to NOT encode the device type in the device structure.
We did this to "force" the user to "know" the type before casting it
away to the real type, allowing it to be done in a way that would always
"just work" because an error could never occur.

This is not a normal "design pattern" for objects, even then (i.e.
gobject does not do this), and over the years I still think this was a
good decision.  It allowed us to keep the sysfs callbacks simpler in
that it saved us yet-another-function-call-deep, and it also allowed us
to do some "fun" pointer casting tricks for sysfs attributes (much to
CFI's nightmare many years later), and it kept drivers simple, which in
the end is the key.  It also forced driver authors from doing "tricks"
where they could just try to figure out the device type and then do
something with that for many many years until we had to give in and
allow this to happen due to multi-device-sharing subsystems.  And even
then, I think that was the wrong choice.

Yes, over time, many subsystems were forced to come up with ways of
determining the device type, look at dev_is_pci() and where it's used
all over the place (wait, why can't you just use that here too?)  But
those usages are the rare exception.  And I still think that was the
wrong choice.

What I don't want to see is this type of function to be the only way you
can get a pci device from a struct device in rust.  That's going to be
messy as that is going to be a very common occurance (maybe, I haven't
seen how you use this), and would force everyone to always test the
return value, adding complexity and additional work for everyone, just
because the few wants it.

So where/how are you going to use this?  Why can't you just store the
"real" reference to the pci device?  If you want to save off a generic
device reference, where did it come from?  Why can't you just explicitly
save off the correct type at the time you stored it?  Is this just
attempting to save a few pointers?  Is this going to be required to be
called as the only way to get from a device to a pci device in a rust
driver?

Along these lines, if you can convince me that this is something that we
really should be doing, in that we should always be checking every time
someone would want to call to_pci_dev(), that the return value is
checked, then why don't we also do this in C if it's going to be
something to assure people it is going to be correct?  I don't want to
see the rust and C sides get "out of sync" here for things that can be
kept in sync, as that reduces the mental load of all of us as we travers
across the boundry for the next 20+ years.

Note, it's almost 10x more common to just use to_pci_dev() on it's own
and not call dev_is_pci(), so that gives you a sense of just how much
work every individual driver would have to do for such a rare
requirement.

thanks,

greg k-h

Powered by blists - more mailing lists