Re: [PATCH 0/8] Device: generic accessors for drvdata + Driver::unbind()

[Danilo Krummrich <dakr@kernel.org>]

On Tue, Jul 01, 2025 at 11:25:41AM +0200, Greg KH wrote:
> On Sat, Jun 21, 2025 at 09:43:26PM +0200, Danilo Krummrich wrote:
> > This patch series consists of the following three parts.
> > 
> >   1. Introduce the 'Internal' device context (semantically identical to the
> >      'Core' device context), but only accessible for bus abstractions.
> > 
> >   2. Introduce generic accessors for a device's driver_data  pointer. Those are
> >      implemented for the 'Internal' device context only, in order to only enable
> >      bus abstractions to mess with the driver_data pointer of struct device.
> > 
> >   3. Implement the Driver::unbind() callback (details below).
> > 
> > Driver::unbind()
> > ----------------
> > 
> > Currently, there's really only one core callback for drivers, which is
> > probe().
> > 
> > Now, this isn't entirely true, since there is also the drop() callback of
> > the driver type (serving as the driver's private data), which is returned
> > by probe() and is dropped in remove().
> > 
> > On the C side remove() mainly serves two purposes:
> > 
> >   (1) Tear down the device that is operated by the driver, e.g. call bus
> >       specific functions, write I/O memory to reset the device, etc.
> > 
> >   (2) Release the resources that have been allocated by a driver for a
> >       specific device.
> > 
> > The drop() callback mentioned above is intended to cover (2) as the Rust
> > idiomatic way.
> > 
> > However, it is partially insufficient and inefficient to cover (1)
> > properly, since drop() can't be called with additional arguments, such as
> > the reference to the corresponding device that has the correct device
> > context, i.e. the Core device context.
> 
> I'm missing something, why doesn't drop() have access to the device
> itself, which has the Core device context?  It's the same "object",
> right?

Not exactly, the thing in drop() is the driver's private data, which has the
exact lifetime of a driver being bound to a device, which makes drop() pretty
much identical to remove() in this aspect.

	// This is the private data of the driver.
	struct MyDriver {
	   bar: Devres<pci::Bar>,
	   ...
	}

	impl pci::Driver for MyDriver {
	   fn probe(
	      pdev: &pci::Device<Core>,
	      info: &Self::IdInfo
	   ) -> Result<Pin<KBox<Self>>> {
	      let bar = ...;
	      pdev.enable_device()?;

	      KBox::pin_init(Self { bar }, GFP_KERNEL)
	   }

	   fn unbind(&self, pdev: &Device<Core>) {
	      // Can only be called with a `&Device<Core>`.
	      pdev.disable_device();
	   }
	}

	impl Drop for MyDriver {
	   fn drop(&mut self) {
	      // We don't need to do anything here, the destructor of `self.bar`
	      // is called automatically, which is where the PCI BAR is unmapped
	      // and the resource region is released. In fact, this impl block
	      // is optional and can be omitted.
	   }
	}

The probe() method's return value is the driver's private data, which, due to
being a ForeignOwnable, is stored in dev->driver_data by the bus abstraction.

The lifetime goes until remove(), which is where the bus abstraction does not
borrow dev->driver_data, but instead re-creates the original driver data object,
which subsequently in the bus abstraction's remove() function goes out of scope
and hence is dropped.

From the bus abstraction side of things it conceptually looks like this:

	 extern "C" fn probe_callback(pdev, ...) {
	    let data = T::probe();

	    pdev.as_ref().set_drvdata(data);
	 }

	extern "C" fn remove_callback(pdev) {
	   let data = unsafe { pdev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() }

	   T::unbind(pdev, data.as_ref());
	} // data.drop() is called here, since data goes out of scope.

> > This makes it inefficient (but not impossible) to access device
> > resources, e.g. to write device registers, and impossible to call device
> > methods, which are only accessible under the Core device context.
> > 
> > In order to solve this, add an additional callback for (1), which we
> > call unbind().
> > 
> > The reason for calling it unbind() is that, unlike remove(), it is *only*
> > meant to be used to perform teardown operations on the device (1), but
> > *not* to release resources (2).
> 
> Ick.  I get the idea, but unbind() is going to get confusing fast.
> Determining what is, and is not, a "resource" is going to be hard over
> time.  In fact, how would you define it?  :)

I think the definition is simple: All teardown operations a driver needs a
&Device<Core> for go into unbind().

Whereas drop() really only is the destructor of the driver's private data.

> Is "teardown" only allowed to write to resources, but not free them?

"Teardown" is everything that involves interaction with the device when the
driver is unbound.

However, we can't free things there, that happens in the automatically when the
destructor of the driver's private data is called, i.e. in drop().

> If so, why can't that happen in drop() as the resources are available there.

For instance, some functions can only be implemented for a &Device<Core>, such
as pci_disable_device(), which hence we can't call from drop().

> I'm loath to have a 2-step destroy system here for rust only, and not
> for C, as maintaining this over time is going to be rough.

Why do you think so? To me it seems pretty clean to separate the "unbind()
teardown sequence" from the destructor of the driver's private data, even if
there wouldn't be a technical reason to do so.