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 for Android: free password hash cracker in your pocket

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

Message-ID: <Zx9kR4OhT1pErzEk@pollux>
Date: Mon, 28 Oct 2024 11:15:35 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Rob Herring <robh@...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 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?

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

> 
> Rob
>