rust/helpers/auxiliary.c | 10 ------ rust/helpers/device.c | 10 ++++++ rust/helpers/pci.c | 10 ------ rust/helpers/platform.c | 10 ------ rust/kernel/auxiliary.rs | 35 +++++++++----------- rust/kernel/device.rs | 57 ++++++++++++++++++++++++++++++++- rust/kernel/pci.rs | 47 +++++++++++++++++---------- rust/kernel/platform.rs | 52 +++++++++++++++++++----------- samples/rust/rust_driver_pci.rs | 11 ++++++- 9 files changed, 154 insertions(+), 88 deletions(-)
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.
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).
Danilo Krummrich (8):
rust: device: introduce device::Internal
rust: device: add drvdata accessors
rust: platform: use generic device drvdata accessors
rust: pci: use generic device drvdata accessors
rust: auxiliary: use generic device drvdata accessors
rust: platform: implement Driver::unbind()
rust: pci: implement Driver::unbind()
samples: rust: pci: reset pci-testdev in unbind()
rust/helpers/auxiliary.c | 10 ------
rust/helpers/device.c | 10 ++++++
rust/helpers/pci.c | 10 ------
rust/helpers/platform.c | 10 ------
rust/kernel/auxiliary.rs | 35 +++++++++-----------
rust/kernel/device.rs | 57 ++++++++++++++++++++++++++++++++-
rust/kernel/pci.rs | 47 +++++++++++++++++----------
rust/kernel/platform.rs | 52 +++++++++++++++++++-----------
samples/rust/rust_driver_pci.rs | 11 ++++++-
9 files changed, 154 insertions(+), 88 deletions(-)
base-commit: b29929b819f35503024c6a7e6ad442f6e36c68a0
--
2.49.0
On Sat, Jun 21, 2025 at 09:43:26PM +0200, Danilo Krummrich wrote: > rust: device: introduce device::Internal [ Rename device::Internal to device::CoreInternal. - Danilo ] > rust: device: add drvdata accessors [ Improve safety comment as proposed by Benno. - Danilo ] > rust: platform: use generic device drvdata accessors > rust: pci: use generic device drvdata accessors > rust: auxiliary: use generic device drvdata accessors > rust: platform: implement Driver::unbind() > rust: pci: implement Driver::unbind() > samples: rust: pci: reset pci-testdev in unbind() Applied to driver-core-testing, thanks!
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? > 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? :) Is "teardown" only allowed to write to resources, but not free them? If so, why can't that happen in drop() as the resources are available there. 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. thanks, greg k-h
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.
On Tue Jul 1, 2025 at 7:40 PM JST, Danilo Krummrich wrote: >> > 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(). Can't we somehow make a (renamed) `unbind` receive full ownership of the driver's private data, such that it will be freed (and its `drop` implementation called) before `unbind` returns? Or do we necessarily need to free that data later?
On Mon, Jul 07, 2025 at 04:18:09PM +0900, Alexandre Courbot wrote: > On Tue Jul 1, 2025 at 7:40 PM JST, Danilo Krummrich wrote: > >> > 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(). > > Can't we somehow make a (renamed) `unbind` receive full ownership of the > driver's private data, such that it will be freed (and its `drop` > implementation called) before `unbind` returns? Or do we necessarily > need to free that data later? No, we could do that. And I thought about this as well, but I really want to bind the lifetime of the driver's private data stored in a device to the lifetime of this driver being bound to the device. I don't think there is a valid use-case to allow this data as a whole to out-live driver unbind arbitrarily.
© 2016 - 2025 Red Hat, Inc.