linux-kernel - Re: [PATCH v3 15/16] rust: platform: add basic platform device / driver abstractions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_JsqLVdoQNSSDCfGcf0wCZE9VQphRhHKANxhpei_UoFzkN9g@mail.gmail.com>
Date: Wed, 30 Oct 2024 07:23:47 -0500
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 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.

> >
> > > > > +
> > > > > +    /// 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.

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.

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