Message-ID: <CAL_Jsq+TV486zw=hAWkFnNbPeA08mJh_4kVVJLSXiYkzWcOVDg@mail.gmail.com>
Date: Tue, 26 Nov 2024 08:44:19 -0600
From: Rob Herring <robh@...nel.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, 
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, 
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, 
	tmgross@...ch.edu, a.hindborg@...sung.com, aliceryhl@...gle.com, 
	airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net, 
	pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com, 
	daniel.almeida@...labora.com, saravanak@...gle.com, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 15/16] rust: platform: add basic platform device /
 driver abstractions

On Tue, Nov 26, 2024 at 6:39 AM Danilo Krummrich <dakr@...nel.org> wrote:
>
> On Wed, Oct 30, 2024 at 07:23:47AM -0500, Rob Herring wrote:
> > On Mon, Oct 28, 2024 at 5:15 AM Danilo Krummrich <dakr@...nel.org> wrote:
> > >
> > > On Wed, Oct 23, 2024 at 09:23:55AM -0500, Rob Herring wrote:
> > > > On Wed, Oct 23, 2024 at 08:44:42AM +0200, Danilo Krummrich wrote:
> > > > > On Tue, Oct 22, 2024 at 06:47:12PM -0500, Rob Herring wrote:
> > > > > > On Tue, Oct 22, 2024 at 11:31:52PM +0200, Danilo Krummrich wrote:
> > > > > > > +///     ]
> > > > > > > +/// );
> > > > > > > +///
> > > > > > > +/// impl platform::Driver for MyDriver {
> > > > > > > +///     type IdInfo = ();
> > > > > > > +///     const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
> > > > > > > +///
> > > > > > > +///     fn probe(
> > > > > > > +///         _pdev: &mut platform::Device,
> > > > > > > +///         _id_info: Option<&Self::IdInfo>,
> > > > > > > +///     ) -> Result<Pin<KBox<Self>>> {
> > > > > > > +///         Err(ENODEV)
> > > > > > > +///     }
> > > > > > > +/// }
> > > > > > > +///```
> > > > > > > +/// Drivers must implement this trait in order to get a platform driver registered. Please refer to
> > > > > > > +/// the `Adapter` documentation for an example.
> > > > > > > +pub trait Driver {
> > > > > > > +    /// The type holding information about each device id supported by the driver.
> > > > > > > +    ///
> > > > > > > +    /// TODO: Use associated_type_defaults once stabilized:
> > > > > > > +    ///
> > > > > > > +    /// type IdInfo: 'static = ();
> > > > > > > +    type IdInfo: 'static;
> > > > > > > +
> > > > > > > +    /// The table of device ids supported by the driver.
> > > > > > > +    const ID_TABLE: IdTable<Self::IdInfo>;
> > > >
> > > > Another thing. I don't think this is quite right. Well, this part is
> > > > fine, but assigning the DT table to it is not. The underlying C code has
> > > > 2 id tables in struct device_driver (DT and ACPI) and then the bus
> > > > specific one in the struct ${bus}_driver.
> > >
> > > The assignment of this table in `Adapter::register` looks like this:
> > >
> > > `pdrv.driver.of_match_table = T::ID_TABLE.as_ptr();`
> > >
> > > What do you think is wrong with this assignment?
> >
> > Every bus implementation will need the DT and ACPI tables, so they
> > should not be declared and assigned in platform driver code, but in
> > the generic device/driver abstractions just like the underlying C
> > code. The one here should be for platform_device_id. You could put all
> > 3 tables here, but that's going to be a lot of duplication I think.
>
> That's indeed true. But I'm not sure that at this point we need a generalized
> `Driver` abstraction just for assigning the DT and ACPI tables.

Why not? Practically *every* non-discoverable bus type needs that.
That's essentially everything except USB and PCI.

> Maybe it's better to do this in a subsequent series?

Sure, but only because there will be a limited number of users to fix.
It looks to me like things are designed for exactly 1 IdInfo
type/table per driver and that's not a correct assumption.

> > > > > > > +
> > > > > > > +    /// Platform driver probe.
> > > > > > > +    ///
> > > > > > > +    /// Called when a new platform device is added or discovered.
> > > > > > > +    /// Implementers should attempt to initialize the device here.
> > > > > > > +    fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
> > > > > > > +
> > > > > > > +    /// Find the [`of::DeviceId`] within [`Driver::ID_TABLE`] matching the given [`Device`], if any.
> > > > > > > +    fn of_match_device(pdev: &Device) -> Option<&of::DeviceId> {
> > > > > >
> > > > > > Is this visible to drivers? It shouldn't be.
> > > > >
> > > > > Yeah, I think we should just remove it. Looking at struct of_device_id, it
> > > > > doesn't contain any useful information for a driver. I think when I added this I
> > > > > was a bit in "autopilot" mode from the PCI stuff, where struct pci_device_id is
> > > > > useful to drivers.
> > > >
> > > > TBC, you mean other than *data, right? If so, I agree.
> > >
> > > Yes.
> > >
> > > >
> > > > The DT type and name fields are pretty much legacy, so I don't think the
> > > > rust bindings need to worry about them until someone converts Sparc and
> > > > PowerMac drivers to rust (i.e. never).
> > > >
> > > > I would guess the PCI cases might be questionable, too. Like DT, drivers
> > > > may be accessing the table fields, but that's not best practice. All the
> > > > match fields are stored in pci_dev, so why get them from the match
> > > > table?
> > >
> > > Fair question, I'd like to forward it to Greg. IIRC, he explicitly requested to
> > > make the corresponding struct pci_device_id available in probe() at Kangrejos.

Making it available is not necessarily the same thing as passing it in
via probe. I agree it may need to be available in probe(), but that
can be an explicit call to get it.

> > Which table gets passed in though? Is the IdInfo parameter generic and
> > can be platform_device_id, of_device_id or acpi_device_id? Not sure if
> > that's possible in rust or not.
>
> Not sure I can follow you here.
>
> The `IdInfo` parameter is of a type given by the driver for driver specific data
> for a certain ID table entry.
>
> It's analogue to resolving `pci_device_id::driver_data` in C.

As I said below, the PCI case is simpler than for platform devices.
Platform devices have 3 possible match tables. The *_device_id type we
end up with is determined at runtime (because matching is done at
runtime), so IdInfo could be any of those 3 types. Is the exact type
opaque to probe() and will that magically work in rust? Or do we need
to pass in the 'driver_data' ptr (or reference) itself? The matched
driver data is generally all the driver needs or cares about. We can
probably assume that it is the same type no matter which match table
is used whether it is platform_device_id::driver_data,
of_device_id::data, or acpi_device_id::driver_data. Nothing in the C
API guarantees that, but that's just best practice. Best practice in C
looks like this:

my_probe()
{
  struct my_driver_data *data = device_get_match_data();
  ...
}

device_get_match_data() is just a wrapper to handle the 3 possible match tables.

The decision for rust is whether we pass in "data" to probe or have an
explicit call. There is a need to get to the *_device_id entry, but
that's the exception. I would go as far as saying we may never need
that in rust drivers.

Rob

> > PCI is the exception, not the rule here, in that it only matches with
> > pci_device_id. At least I think that is the case currently, but it is
> > entirely possible we may want to do ACPI/DT matching like every other
> > bus. There are cases where PCI devices are described in DT.
> >
> > Rob
> >

Powered by blists - more mailing lists