| Message ID | 20260116162203.296844-2-sunke@kylinos.cn |
|---|---|
| State | Under Review |
| Headers | show |
| Series | rust: Add RTC driver support | expand |
On Fri Jan 16, 2026 at 5:21 PM CET, Ke Sun wrote: > diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c > index baf1a8ca8b2b1..eddcc5a69db3b 100644 > --- a/drivers/rtc/dev.c > +++ b/drivers/rtc/dev.c > @@ -410,7 +410,7 @@ static long rtc_dev_ioctl(struct file *file, > } > default: > if (rtc->ops->param_get) > - err = rtc->ops->param_get(rtc->dev.parent, ¶m); > + err = rtc->ops->param_get(rtc_ops_dev(rtc), ¶m); > else > err = -EINVAL; > } <snip> > +/** > + * rtc_ops_dev - Get the device pointer for RTC ops callbacks > + * @rtc: RTC device > + * > + * Returns &rtc->dev if RTC_OPS_USE_RTC_DEV flag is set, > + * otherwise returns rtc->dev.parent. > + */ > +static inline struct device *rtc_ops_dev(struct rtc_device *rtc) > +{ > + if (test_bit(RTC_OPS_USE_RTC_DEV, &rtc->flags)) > + return &rtc->dev; > + return rtc->dev.parent; > +} I understand that the idea is to gradually convert all drivers to use the RTC device, rather than it's parent device in RTC device callbacks. My main concern is that once that has been achieved it's still not what we want to have eventually, i.e. RTC device callbacks should ideally take a struct rtc_device as argument and not the embedded base struct device. I.e. we'd kick off a conversion process that won't reach the actual desired state.
On 1/19/26 22:32, Danilo Krummrich wrote: > On Fri Jan 16, 2026 at 5:21 PM CET, Ke Sun wrote: >> diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c >> index baf1a8ca8b2b1..eddcc5a69db3b 100644 >> --- a/drivers/rtc/dev.c >> +++ b/drivers/rtc/dev.c >> @@ -410,7 +410,7 @@ static long rtc_dev_ioctl(struct file *file, >> } >> default: >> if (rtc->ops->param_get) >> - err = rtc->ops->param_get(rtc->dev.parent, ¶m); >> + err = rtc->ops->param_get(rtc_ops_dev(rtc), ¶m); >> else >> err = -EINVAL; >> } > <snip> > >> +/** >> + * rtc_ops_dev - Get the device pointer for RTC ops callbacks >> + * @rtc: RTC device >> + * >> + * Returns &rtc->dev if RTC_OPS_USE_RTC_DEV flag is set, >> + * otherwise returns rtc->dev.parent. >> + */ >> +static inline struct device *rtc_ops_dev(struct rtc_device *rtc) >> +{ >> + if (test_bit(RTC_OPS_USE_RTC_DEV, &rtc->flags)) >> + return &rtc->dev; >> + return rtc->dev.parent; >> +} > I understand that the idea is to gradually convert all drivers to use the RTC > device, rather than it's parent device in RTC device callbacks. > > My main concern is that once that has been achieved it's still not what we want > to have eventually, i.e. RTC device callbacks should ideally take a struct > rtc_device as argument and not the embedded base struct device. > > I.e. we'd kick off a conversion process that won't reach the actual desired > state. Hi Danilo, This is indeed an intermediate step. Full cleanup is in progress, but it's large and untested. I'm working on a complete cleanup involving ~190+ files across arch/, drivers/rtc/, and drivers/virtio/. Most changes are straightforward interface replacements, but some drivers need additional modifications. Given the scale, I haven't fully tested everything and can't guarantee correctness yet. The intermediate step enables gradual migration, allowing us to: - Clean up and test each rtc driver incrementally - Ensure correctness through gradual changes - Avoid breaking existing functionality Once all cleanup is complete and tested, changing all rtc_class_ops callbacks to use struct rtc_device * will be much simpler and safer. Currently there seem to be only these two approaches. I'm still waiting for Alexandre's suggestion on how to proceed specifically, but haven't received a response yet. Best regard, Ke Sun
On 20/01/2026 16:01:40+0800, Ke Sun wrote: > > On 1/19/26 22:32, Danilo Krummrich wrote: > > On Fri Jan 16, 2026 at 5:21 PM CET, Ke Sun wrote: > > > diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c > > > index baf1a8ca8b2b1..eddcc5a69db3b 100644 > > > --- a/drivers/rtc/dev.c > > > +++ b/drivers/rtc/dev.c > > > @@ -410,7 +410,7 @@ static long rtc_dev_ioctl(struct file *file, > > > } > > > default: > > > if (rtc->ops->param_get) > > > - err = rtc->ops->param_get(rtc->dev.parent, ¶m); > > > + err = rtc->ops->param_get(rtc_ops_dev(rtc), ¶m); > > > else > > > err = -EINVAL; > > > } > > <snip> > > > > > +/** > > > + * rtc_ops_dev - Get the device pointer for RTC ops callbacks > > > + * @rtc: RTC device > > > + * > > > + * Returns &rtc->dev if RTC_OPS_USE_RTC_DEV flag is set, > > > + * otherwise returns rtc->dev.parent. > > > + */ > > > +static inline struct device *rtc_ops_dev(struct rtc_device *rtc) > > > +{ > > > + if (test_bit(RTC_OPS_USE_RTC_DEV, &rtc->flags)) > > > + return &rtc->dev; > > > + return rtc->dev.parent; > > > +} > > I understand that the idea is to gradually convert all drivers to use the RTC > > device, rather than it's parent device in RTC device callbacks. > > > > My main concern is that once that has been achieved it's still not what we want > > to have eventually, i.e. RTC device callbacks should ideally take a struct > > rtc_device as argument and not the embedded base struct device. > > > > I.e. we'd kick off a conversion process that won't reach the actual desired > > state. > Hi Danilo, > > This is indeed an intermediate step. > > Full cleanup is in progress, but it's large and untested. I'm working on a > complete cleanup involving ~190+ files across arch/, drivers/rtc/, and > drivers/virtio/. Most changes are straightforward interface replacements, > but some drivers need additional modifications. Given the scale, I haven't > fully tested everything and can't guarantee correctness yet. > > The intermediate step enables gradual migration, allowing us to: > - Clean up and test each rtc driver incrementally > - Ensure correctness through gradual changes > - Avoid breaking existing functionality > > Once all cleanup is complete and tested, changing all rtc_class_ops > callbacks to use struct rtc_device * will be much simpler and safer. > > Currently there seem to be only these two approaches. I'm still waiting > for Alexandre's suggestion on how to proceed specifically, but haven't > received a response yet. I'm sorry, I still don't see the point of doing this. The driver will almost always need to set its driver data in the parent device because we need to be able to handle interrupts, suspend/resume or .remove(). So while intellectually, this would be more satisfying to have the callbacks take a struct rtc_device, functionally this doesn't have any purpose because we need to use the parent drvdata anyway. Said differently, you should explain why a device driver must not call amba_set_drvdata() ? Out of 29 drivers, 18 are doing so.
On 2/21/26 06:53, Alexandre Belloni wrote: > On 20/01/2026 16:01:40+0800, Ke Sun wrote: >> On 1/19/26 22:32, Danilo Krummrich wrote: >>> On Fri Jan 16, 2026 at 5:21 PM CET, Ke Sun wrote: >>>> diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c >>>> index baf1a8ca8b2b1..eddcc5a69db3b 100644 >>>> --- a/drivers/rtc/dev.c >>>> +++ b/drivers/rtc/dev.c >>>> @@ -410,7 +410,7 @@ static long rtc_dev_ioctl(struct file *file, >>>> } >>>> default: >>>> if (rtc->ops->param_get) >>>> - err = rtc->ops->param_get(rtc->dev.parent, ¶m); >>>> + err = rtc->ops->param_get(rtc_ops_dev(rtc), ¶m); >>>> else >>>> err = -EINVAL; >>>> } >>> <snip> >>> >>>> +/** >>>> + * rtc_ops_dev - Get the device pointer for RTC ops callbacks >>>> + * @rtc: RTC device >>>> + * >>>> + * Returns &rtc->dev if RTC_OPS_USE_RTC_DEV flag is set, >>>> + * otherwise returns rtc->dev.parent. >>>> + */ >>>> +static inline struct device *rtc_ops_dev(struct rtc_device *rtc) >>>> +{ >>>> + if (test_bit(RTC_OPS_USE_RTC_DEV, &rtc->flags)) >>>> + return &rtc->dev; >>>> + return rtc->dev.parent; >>>> +} >>> I understand that the idea is to gradually convert all drivers to use the RTC >>> device, rather than it's parent device in RTC device callbacks. >>> >>> My main concern is that once that has been achieved it's still not what we want >>> to have eventually, i.e. RTC device callbacks should ideally take a struct >>> rtc_device as argument and not the embedded base struct device. >>> >>> I.e. we'd kick off a conversion process that won't reach the actual desired >>> state. >> Hi Danilo, >> >> This is indeed an intermediate step. >> >> Full cleanup is in progress, but it's large and untested. I'm working on a >> complete cleanup involving ~190+ files across arch/, drivers/rtc/, and >> drivers/virtio/. Most changes are straightforward interface replacements, >> but some drivers need additional modifications. Given the scale, I haven't >> fully tested everything and can't guarantee correctness yet. >> >> The intermediate step enables gradual migration, allowing us to: >> - Clean up and test each rtc driver incrementally >> - Ensure correctness through gradual changes >> - Avoid breaking existing functionality >> >> Once all cleanup is complete and tested, changing all rtc_class_ops >> callbacks to use struct rtc_device * will be much simpler and safer. >> >> Currently there seem to be only these two approaches. I'm still waiting >> for Alexandre's suggestion on how to proceed specifically, but haven't >> received a response yet. > I'm sorry, I still don't see the point of doing this. The driver will > almost always need to set its driver data in the parent device because > we need to be able to handle interrupts, suspend/resume or .remove(). So > while intellectually, this would be more satisfying to have the > callbacks take a struct rtc_device, functionally this doesn't have any > purpose because we need to use the parent drvdata anyway. > > Said differently, you should explain why a device driver must not call > amba_set_drvdata() ? As in platform.rs [1] and i2c.rs [2], set_drvdata is always called by the bus Adapter's probe_callback, not by the device driver. [1]: https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/platform.rs#L80 [2]: https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/i2c.rs#L160 In Rust the Adapter already sets drvdata on the bus device in probe. If the driver also calls amba_set_drvdata() there, it overwrites that pointer; on remove/shutdown the framework then gets wrong data and can hit use-after-free or crashes. So only the framework must set drvdata on the bus device. This applies only to the Rust implementation; in C, calling amba_set_drvdata() is fine. In the Rust design the bus device owns the bus device's drvdata and the class device owns the class device's drvdata, so the class driver must not set drvdata on the bus device. This is my understanding of the Rust device driver abstraction design. Danilo is the authority on this. Best regards, Ke Sun > Out of 29 drivers, 18 are doing so. >
On 21/02/2026 17:31:09+0800, Alvin Sun wrote: > As in platform.rs [1] and i2c.rs [2], set_drvdata is always called by > the bus Adapter's probe_callback, not by the device driver. > > [1]: > https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/platform.rs#L80 > [2]: > https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/i2c.rs#L160 > > In Rust the Adapter already sets drvdata on the bus device in probe. If > the driver also calls amba_set_drvdata() there, it overwrites that > pointer; on remove/shutdown the framework then gets wrong data and can > hit use-after-free or crashes. So only the framework must set drvdata > on the bus device. But this is wrong, how do you then handle the class device on suspend/resume or on .remove? > > This applies only to the Rust implementation; in C, calling > amba_set_drvdata() is fine. In the Rust design the bus device owns the > bus device's drvdata and the class device owns the class device's > drvdata, so the class driver must not set drvdata on the bus device. > > This is my understanding of the Rust device driver abstraction design. > Danilo is the authority on this. > > Best regards, > Ke Sun > > > Out of 29 drivers, 18 are doing so. > >
On Sat, Feb 21, 2026 at 12:16 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 21/02/2026 17:31:09+0800, Alvin Sun wrote: > > As in platform.rs [1] and i2c.rs [2], set_drvdata is always called by > > the bus Adapter's probe_callback, not by the device driver. > > > > [1]: > > https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/platform.rs#L80 > > [2]: > > https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/i2c.rs#L160 > > > > In Rust the Adapter already sets drvdata on the bus device in probe. If > > the driver also calls amba_set_drvdata() there, it overwrites that > > pointer; on remove/shutdown the framework then gets wrong data and can > > hit use-after-free or crashes. So only the framework must set drvdata > > on the bus device. > > But this is wrong, how do you then handle the class device on > suspend/resume or on .remove? > > > > > This applies only to the Rust implementation; in C, calling > > amba_set_drvdata() is fine. In the Rust design the bus device owns the > > bus device's drvdata and the class device owns the class device's > > drvdata, so the class driver must not set drvdata on the bus device. > > > > This is my understanding of the Rust device driver abstraction design. > > Danilo is the authority on this. > > > > Best regards, > > Ke Sun > > > > > Out of 29 drivers, 18 are doing so. +1 The vast majority of around 50 platform drivers I've inspected recently use platform_set_drvdata() or equivalent in probe.
On Sat Feb 21, 2026 at 12:19 PM CET, Rafael J. Wysocki wrote: > On Sat, Feb 21, 2026 at 12:16 PM Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: >> > > Out of 29 drivers, 18 are doing so. > > The vast majority of around 50 platform drivers I've inspected > recently use platform_set_drvdata() or equivalent in probe. This thread seems to contain quite a bit of confusion and misunderstandings -- let me try to clarify. (1) How Rust handles bus device private data. In Rust the probe() function of a bus implementation (platform, PCI, etc.) returns an initializer (impl PinInit<T, Error>) for the driver's device private data. The bus implementation takes this initializer and passes it (together with the underlying struct device) to the driver-core. The driver-core allocates the required memory, initializes the memory with the given initializer and stores a pointer to the corresponding object with dev_set_drvdata(). So, technically, in Rust all platform drivers call platform_set_drvdata(). (Note that this is also true when the driver's device private data type is empty (i.e. it has no fields). In this case it could still have a destructor that must be called when the device private data structure is destroyed. Of course there is no real memory allocation when the struct's size is zero.) The driver's device private data can only be accessed when the bus device is bound to the driver, i.e. the driver can only access it with a &Device<Bound>; it (the driver's device private data) is automatically freed by the driver-core when remove() and all devres callbacks have been completed. I.e. the rules are - of course - the same as on the C side, but they are enforced by the type system and the driver-core code. (2) Bus device private data vs. class device private data. The change to pass a struct rtc_device in class device callbacks of RTC, rather than the base struct device of the corresponding bus device (e.g. AMBA, platform, etc.) should not aim at storing all data in rtc->dev.private_data that was previously stored in rtc->dev.parent->private_data. Instead, it gives drivers the option to differentiate in terms of ownership and lifetime. While the bus device private data has a very defined lifetime from probe() until the device is unbound from the driver, class device private data might live shorter than this, or might even out-live driver unbind in some cases. It really depends on the lifetime of the class device itself, which is not generally defined. Now, from a C side point of view this may not look like a big deal, as it (unfortunately) is not that uncommon that struct fields are just initialized and destroyed whenever needed and the code just takes it into account. But at the same time, this is what leads to a lot of lifetime problems and memory bugs and it is one of those things that Rust aims at avoiding by being very strict about initialization, ownership and lifetimes. However, I do also recognize that drivers creating an RTC device are typically very simple and in practice I would not be surprised if it turns out that it happens that drivers keep the struct rtc_device alive from probe() until the bus device is unbound from the driver, i.e. lifetimes just end up being almost the same. But I don't know if that's always the case. Regardless of that, I think it would be good to keep driver authors finding a common pattern, where class device callbacks carry the corresponding class device struct (instead of the parent base struct device). Especially on the Rust side we now have the chance to make the experience of writing drivers as consistent as possible, which should help (new) driver authors a lot in terms of learning the driver lifetime patterns. I hope this helps. - Danilo
On 2/21/26 19:16, Alexandre Belloni wrote: > On 21/02/2026 17:31:09+0800, Alvin Sun wrote: >> As in platform.rs [1] and i2c.rs [2], set_drvdata is always called by >> the bus Adapter's probe_callback, not by the device driver. >> >> [1]: >> https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/platform.rs#L80 >> [2]: >> https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/i2c.rs#L160 >> >> In Rust the Adapter already sets drvdata on the bus device in probe. If >> the driver also calls amba_set_drvdata() there, it overwrites that >> pointer; on remove/shutdown the framework then gets wrong data and can >> hit use-after-free or crashes. So only the framework must set drvdata >> on the bus device. > But this is wrong, how do you then handle the class device on > suspend/resume or on .remove? There is a patch adding runtime PM for Tyr (platform device driver): https://gitlab.freedesktop.org/panfrost/linux/-/merge_requests/60/diffs#dbdd2c5024317f2c80128c91a823e224b3a41550_240_256 If you are interested in adding Rust support for RTC, We can do some research on top of it for RTC Rust drivers. > >> This applies only to the Rust implementation; in C, calling >> amba_set_drvdata() is fine. In the Rust design the bus device owns the >> bus device's drvdata and the class device owns the class device's >> drvdata, so the class driver must not set drvdata on the bus device. >> >> This is my understanding of the Rust device driver abstraction design. >> Danilo is the authority on this. >> >> Best regards, >> Ke Sun >> >>> Out of 29 drivers, 18 are doing so. >>>
On Sat Feb 21, 2026 at 12:16 PM CET, Alexandre Belloni wrote: > On 21/02/2026 17:31:09+0800, Alvin Sun wrote: >> As in platform.rs [1] and i2c.rs [2], set_drvdata is always called by >> the bus Adapter's probe_callback, not by the device driver. >> >> [1]: >> https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/platform.rs#L80 >> [2]: >> https://elixir.bootlin.com/linux/v6.19-rc5/source/rust/kernel/i2c.rs#L160 >> >> In Rust the Adapter already sets drvdata on the bus device in probe. If >> the driver also calls amba_set_drvdata() there, it overwrites that >> pointer; on remove/shutdown the framework then gets wrong data and can >> hit use-after-free or crashes. So only the framework must set drvdata >> on the bus device. > > But this is wrong, how do you then handle the class device on > suspend/resume or on .remove? I think you misunderstand what Alvin is saying here. We do have bus device private data in all drivers. It's just that drivers do not set it randomly through dev_set_drvdata() et al. Please see [1] for details. [1] https://lore.kernel.org/all/DGKPPQI0QE73.S8I1M5NCI2BV@kernel.org/
On 21/02/2026 15:33:48+0100, Danilo Krummrich wrote: > On Sat Feb 21, 2026 at 12:19 PM CET, Rafael J. Wysocki wrote: > > On Sat, Feb 21, 2026 at 12:16 PM Alexandre Belloni > > <alexandre.belloni@bootlin.com> wrote: > >> > > Out of 29 drivers, 18 are doing so. > > > > The vast majority of around 50 platform drivers I've inspected > > recently use platform_set_drvdata() or equivalent in probe. > > This thread seems to contain quite a bit of confusion and misunderstandings -- > let me try to clarify. > > (1) How Rust handles bus device private data. > > In Rust the probe() function of a bus implementation (platform, PCI, etc.) > returns an initializer (impl PinInit<T, Error>) for the driver's device > private data. > > The bus implementation takes this initializer and passes it (together with the > underlying struct device) to the driver-core. The driver-core allocates the > required memory, initializes the memory with the given initializer and stores > a pointer to the corresponding object with dev_set_drvdata(). > > So, technically, in Rust all platform drivers call platform_set_drvdata(). > > (Note that this is also true when the driver's device private data type is > empty (i.e. it has no fields). In this case it could still have a destructor > that must be called when the device private data structure is destroyed. Of > course there is no real memory allocation when the struct's size is zero.) > > The driver's device private data can only be accessed when the bus device is > bound to the driver, i.e. the driver can only access it with a &Device<Bound>; > it (the driver's device private data) is automatically freed by the > driver-core when remove() and all devres callbacks have been completed. > > I.e. the rules are - of course - the same as on the C side, but they are > enforced by the type system and the driver-core code. > This still doesn't explain how you get the class private data that you need when you are in a driver callback that is called from the bus (e.g. suspend/resume) from what you explain, the driver doesn't have any chance to pass it. The whole goal of a device driver is to be the glue between a class device and a bus device as essentially this is the exact physical device, just represented differently. An i2c RTC is not either an i2c device or an RTC, it is a single device and we need to be able to tell the framework when something happens on the bus or we need to be able to talk on the bus when we want to do something with the device. > (2) Bus device private data vs. class device private data. > > The change to pass a struct rtc_device in class device callbacks of RTC, > rather than the base struct device of the corresponding bus device (e.g. AMBA, > platform, etc.) should not aim at storing all data in rtc->dev.private_data > that was previously stored in rtc->dev.parent->private_data. > But what you explain here is that the drive is forbidden to use rtc->dev.parent->private_data at all because the rust core is already using it. What I'm saying is that it won't work because more than half of the drivers currently need it. I get that you are trying to avoid most of the issues by using devres but I'm pretty sure there are cases where this doesn't work. > Instead, it gives drivers the option to differentiate in terms of ownership > and lifetime. > > While the bus device private data has a very defined lifetime from probe() > until the device is unbound from the driver, class device private data might > live shorter than this, or might even out-live driver unbind in some cases. It > really depends on the lifetime of the class device itself, which is not > generally defined. > > Now, from a C side point of view this may not look like a big deal, as it > (unfortunately) is not that uncommon that struct fields are just initialized > and destroyed whenever needed and the code just takes it into account. > > But at the same time, this is what leads to a lot of lifetime problems and > memory bugs and it is one of those things that Rust aims at avoiding by being > very strict about initialization, ownership and lifetimes. > > However, I do also recognize that drivers creating an RTC device are typically > very simple and in practice I would not be surprised if it turns out that it > happens that drivers keep the struct rtc_device alive from probe() until the > bus device is unbound from the driver, i.e. lifetimes just end up being almost > the same. But I don't know if that's always the case. > > Regardless of that, I think it would be good to keep driver authors finding a > common pattern, where class device callbacks carry the corresponding class > device struct (instead of the parent base struct device). > > Especially on the Rust side we now have the chance to make the experience of > writing drivers as consistent as possible, which should help (new) driver > authors a lot in terms of learning the driver lifetime patterns. > The whole point of the RTC is to outlive the system but as Linux can't do anything with the RTC unless it has a bus device to talk to, the class device and the bus device have the exact same lifetime and we do handle device appearing and disappearing from the bus. You may think RTC drivers afe simple but there are plenty of lifetime issues that you will never get with any other devices because they simply die with the system.
On Sat, Feb 21, 2026 at 3:33 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Sat Feb 21, 2026 at 12:19 PM CET, Rafael J. Wysocki wrote: > > On Sat, Feb 21, 2026 at 12:16 PM Alexandre Belloni > > <alexandre.belloni@bootlin.com> wrote: > >> > > Out of 29 drivers, 18 are doing so. > > > > The vast majority of around 50 platform drivers I've inspected > > recently use platform_set_drvdata() or equivalent in probe. > > This thread seems to contain quite a bit of confusion and misunderstandings -- > let me try to clarify. > > (1) How Rust handles bus device private data. > > In Rust the probe() function of a bus implementation (platform, PCI, etc.) > returns an initializer (impl PinInit<T, Error>) for the driver's device > private data. > > The bus implementation takes this initializer and passes it (together with the > underlying struct device) to the driver-core. The driver-core allocates the > required memory, initializes the memory with the given initializer and stores > a pointer to the corresponding object with dev_set_drvdata(). > > So, technically, in Rust all platform drivers call platform_set_drvdata(). So do I understand correctly that the driver is required to tell the core what type its driver_data will be and then the core will allocate memory for it and clean it up on remove? > (Note that this is also true when the driver's device private data type is > empty (i.e. it has no fields). In this case it could still have a destructor > that must be called when the device private data structure is destroyed. Of > course there is no real memory allocation when the struct's size is zero.) So in the simplest case when the driver doesn't need driver_data at all, it will just use a struct with no fields as that driver_data type, IIUC. > The driver's device private data can only be accessed when the bus device is > bound to the driver, i.e. the driver can only access it with a &Device<Bound>; > it (the driver's device private data) is automatically freed by the > driver-core when remove() and all devres callbacks have been completed. Well, that's what happens on the C side of things too most of the time because driver_data is allocated at probe time, very often using devm_kzalloc() or equivalent. > I.e. the rules are - of course - the same as on the C side, but they are > enforced by the type system and the driver-core code. OK > (2) Bus device private data vs. class device private data. > > The change to pass a struct rtc_device in class device callbacks of RTC, > rather than the base struct device of the corresponding bus device (e.g. AMBA, > platform, etc.) should not aim at storing all data in rtc->dev.private_data > that was previously stored in rtc->dev.parent->private_data. > > Instead, it gives drivers the option to differentiate in terms of ownership > and lifetime. > > While the bus device private data has a very defined lifetime from probe() > until the device is unbound from the driver, class device private data might > live shorter than this, or might even out-live driver unbind in some cases. It > really depends on the lifetime of the class device itself, which is not > generally defined. > > Now, from a C side point of view this may not look like a big deal, as it > (unfortunately) is not that uncommon that struct fields are just initialized > and destroyed whenever needed and the code just takes it into account. > > But at the same time, this is what leads to a lot of lifetime problems and > memory bugs and it is one of those things that Rust aims at avoiding by being > very strict about initialization, ownership and lifetimes. As a general rule, I agree, but I would advise against applying general rules automatically everywhere. > However, I do also recognize that drivers creating an RTC device are typically > very simple and in practice I would not be surprised if it turns out that it > happens that drivers keep the struct rtc_device alive from probe() until the > bus device is unbound from the driver, i.e. lifetimes just end up being almost > the same. But I don't know if that's always the case. > > Regardless of that, I think it would be good to keep driver authors finding a > common pattern, where class device callbacks carry the corresponding class > device struct (instead of the parent base struct device). TBH I'm not really convinced about this particular thing and I think I can provide an illustrative example. Namely, quite incidentally, I've recently set out to add an RTC class device to an existing driver, which is the ACPI time and alarm device (TAD) one. The TAD functionality is based on ACPI control methods provided by the platform firmware and it may (or may not) include RTC-equivalent functions. So far, the driver has been providing a completely custom sysfs interface to user space, but since more and more platforms contain an ACPI TAD and some of them may not contain a "traditional" RTC, having an RTC class device interface in that driver may be quite useful. I have a prototype of the requisite change (I'll post it shortly for reference) and it turns out that because the RTC class callbacks take the parent device pointer as an argument, wrapping them around the existing driver routines backing the existing sysfs interface is super-straightforward. Had the RTC class passed an RTC device pointer to those callbacks, the driver would have had to do something to get back from it to the parent device (which is what the driver really works with). If there are more similar drivers, that would have led to some code duplication that is in fact unnecessary. Moreover, the RTC device pointer doesn't even need to be stored anywhere in that driver because the driver need not use it directly at all and the RTC device object memory is freed by the core when the driver unbinds. > Especially on the Rust side we now have the chance to make the experience of > writing drivers as consistent as possible, which should help (new) driver > authors a lot in terms of learning the driver lifetime patterns. Well, I'm not sure if "the experience of writing drivers as consistent as possible" is more important than less code duplication and simpler code.
On Sun Feb 22, 2026 at 1:05 AM CET, Alexandre Belloni wrote: > On 21/02/2026 15:33:48+0100, Danilo Krummrich wrote: >> (2) Bus device private data vs. class device private data. >> >> The change to pass a struct rtc_device in class device callbacks of RTC, >> rather than the base struct device of the corresponding bus device (e.g. AMBA, >> platform, etc.) should not aim at storing all data in rtc->dev.private_data >> that was previously stored in rtc->dev.parent->private_data. >> > > But what you explain here is that the drive is forbidden to use > rtc->dev.parent->private_data at all because the rust core is already > using it. What I'm saying is that it won't work because more than half > of the drivers currently need it. I think I was explaining the exact opposite, i.e. the driver's bus device private data is stored in the exact same way as in C, but it has a defined lifetime (from probe() until remove() and all devres callbacks have been completed) and is managed by the driver-core. Look at this example: // The bus device private data. struct SampleDriver { foo: u32, bar: u32, } impl pci::Driver for SampleDriver { fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { // Return the driver's bus device private data. The driver-core // will do all the work for us and eventually call // `dev_set_drvdata()` on the base `struct device` of `pdev`. Ok(Foo { foo: 42, bar: 24, }) } fn unbind(pdev: &pci::Device<Core>, self: Pin<&Self>) { // Use the driver's device private data from the method's `self` argument. dev_info!(pdev, "foo: {}, bar: {}\n", self.foo, self.bar); // There's also the `Device<Bound>::drvdata()` accessor, let's // use that as well. // Get a generic `device::Device` from the `pci::Device`. let dev = pdev.as_ref(); // Get the `drvdata`; we have to assert the type and let the driver-core // validate that the asserted type is correct. let drvdata = dev.drvdata::<SampleDriver>()?; dev_info!(pdev, "foo: {}, bar: {}\n", drvdata.foo, drvdata.bar); } } >> This thread seems to contain quite a bit of confusion and misunderstandings -- >> let me try to clarify. >> >> (1) How Rust handles bus device private data. >> >> In Rust the probe() function of a bus implementation (platform, PCI, etc.) >> returns an initializer (impl PinInit<T, Error>) for the driver's device >> private data. >> >> The bus implementation takes this initializer and passes it (together with the >> underlying struct device) to the driver-core. The driver-core allocates the >> required memory, initializes the memory with the given initializer and stores >> a pointer to the corresponding object with dev_set_drvdata(). >> >> So, technically, in Rust all platform drivers call platform_set_drvdata(). >> >> (Note that this is also true when the driver's device private data type is >> empty (i.e. it has no fields). In this case it could still have a destructor >> that must be called when the device private data structure is destroyed. Of >> course there is no real memory allocation when the struct's size is zero.) >> >> The driver's device private data can only be accessed when the bus device is >> bound to the driver, i.e. the driver can only access it with a &Device<Bound>; >> it (the driver's device private data) is automatically freed by the >> driver-core when remove() and all devres callbacks have been completed. >> >> I.e. the rules are - of course - the same as on the C side, but they are >> enforced by the type system and the driver-core code. >> > > This still doesn't explain how you get the class private data that you > need when you are in a driver callback that is called from the bus (e.g. > suspend/resume) from what you explain, the driver doesn't have any > chance to pass it. The whole goal of a device driver is to be the glue > between a class device and a bus device as essentially this is the exact > physical device, just represented differently. It is not always as simple as "one bus device corresponds to one class device". Sometimes drivers have to deal with multiple class devices for a single bus device, sometimes they are separated through the auxiliary bus or MFD. For instance, take struct net_device. In the context of cfg80211 wireless drivers may create arbitrary struct net_device instances, depending on how often the add_virtual_intf() callback is called (through netlink); example in [1]. Now, regarding your question "How to access class device private data from bus device callbacks?". Nothing prevents a driver from embedding the class device in its bus device private data in one or the other way. In the net device example above, the driver would probably keep a list (or xarray, etc.) of net devices in its bus device private data, as they can be created and removed at any point of time. (Note that this is also a good example for when the class device private data lives shorter than the bus device private data.) Here is an example of this looks like in code using a DRM device as class device. struct SampleIrqData; // The class device private data. struct SampleDrm { sched: drm::GpuScheduler, } // The bus device private data. #[pin_data] struct SampleDriver { #[pin] irq: irq::Registration<SampleIrqData>, drm: ARef<drm::Device<SampleDrm>, // Refcount of a `drm::Device`. } impl pci::Driver for SampleDriver { fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { let dev = pdev.as_ref(); let drm = drm::Device::new(dev, SampleDrm::new()?)?; drm.register(); Ok(impl_pin_init!(Self { irq <- irq::Registration::new(...); drm, }) } fn unbind(pdev: &pci::Device<Core>, self: Pin<&Self>) { self.irq.synchronize(); self.drm.flush_scheduler(); } } Note that the irq::Registration is dropped when the bus device is unbound, whereas the GpuScheduler is dropped when the DRM device is dropped. Of course, depending on the actual class device this difference may be more or less important, i.e. absolutely crucial in the net device example, less important for RTC apparently. But it is always good to be precise about ownership and lifetime of data. As I already mentioned, I don't think it is good to fold ownership and lifetime for bus and class devices together in cases where it happens that they are mostly identical, i.e.: >> I think it would be good to keep driver authors finding a common pattern, >> where class device callbacks carry the corresponding class device struct >> (instead of the parent base struct device). >> >> Especially on the Rust side we now have the chance to make the experience >> of writing drivers as consistent as possible, which should help (new) >> driver authors a lot in terms of learning the driver lifetime patterns. [1] https://elixir.bootlin.com/linux/v6.19.2/source/drivers/net/wireless/microchip/wilc1000/netdev.c#L948
On Sun, Feb 22, 2026 at 1:49 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Sun Feb 22, 2026 at 1:05 AM CET, Alexandre Belloni wrote: > > On 21/02/2026 15:33:48+0100, Danilo Krummrich wrote: > >> (2) Bus device private data vs. class device private data. > >> > >> The change to pass a struct rtc_device in class device callbacks of RTC, > >> rather than the base struct device of the corresponding bus device (e.g. AMBA, > >> platform, etc.) should not aim at storing all data in rtc->dev.private_data > >> that was previously stored in rtc->dev.parent->private_data. > >> > > > > But what you explain here is that the drive is forbidden to use > > rtc->dev.parent->private_data at all because the rust core is already > > using it. What I'm saying is that it won't work because more than half > > of the drivers currently need it. > > I think I was explaining the exact opposite, i.e. the driver's bus device > private data is stored in the exact same way as in C, but it has a defined > lifetime (from probe() until remove() and all devres callbacks have been > completed) and is managed by the driver-core. > > Look at this example: > > // The bus device private data. > struct SampleDriver { > foo: u32, > bar: u32, > } > > impl pci::Driver for SampleDriver { > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > // Return the driver's bus device private data. The driver-core > // will do all the work for us and eventually call > // `dev_set_drvdata()` on the base `struct device` of `pdev`. So that's how I thought it would work. > Ok(Foo { > foo: 42, > bar: 24, > }) > } > > fn unbind(pdev: &pci::Device<Core>, self: Pin<&Self>) { > // Use the driver's device private data from the method's `self` argument. > dev_info!(pdev, "foo: {}, bar: {}\n", self.foo, self.bar); > > // There's also the `Device<Bound>::drvdata()` accessor, let's > // use that as well. > > // Get a generic `device::Device` from the `pci::Device`. > let dev = pdev.as_ref(); > > // Get the `drvdata`; we have to assert the type and let the driver-core > // validate that the asserted type is correct. > let drvdata = dev.drvdata::<SampleDriver>()?; > > dev_info!(pdev, "foo: {}, bar: {}\n", drvdata.foo, drvdata.bar); > } > } This helps, thanks! > >> This thread seems to contain quite a bit of confusion and misunderstandings -- > >> let me try to clarify. > >> > >> (1) How Rust handles bus device private data. > >> > >> In Rust the probe() function of a bus implementation (platform, PCI, etc.) > >> returns an initializer (impl PinInit<T, Error>) for the driver's device > >> private data. > >> > >> The bus implementation takes this initializer and passes it (together with the > >> underlying struct device) to the driver-core. The driver-core allocates the > >> required memory, initializes the memory with the given initializer and stores > >> a pointer to the corresponding object with dev_set_drvdata(). > >> > >> So, technically, in Rust all platform drivers call platform_set_drvdata(). > >> > >> (Note that this is also true when the driver's device private data type is > >> empty (i.e. it has no fields). In this case it could still have a destructor > >> that must be called when the device private data structure is destroyed. Of > >> course there is no real memory allocation when the struct's size is zero.) > >> > >> The driver's device private data can only be accessed when the bus device is > >> bound to the driver, i.e. the driver can only access it with a &Device<Bound>; > >> it (the driver's device private data) is automatically freed by the > >> driver-core when remove() and all devres callbacks have been completed. > >> > >> I.e. the rules are - of course - the same as on the C side, but they are > >> enforced by the type system and the driver-core code. > >> > > > > This still doesn't explain how you get the class private data that you > > need when you are in a driver callback that is called from the bus (e.g. > > suspend/resume) from what you explain, the driver doesn't have any > > chance to pass it. The whole goal of a device driver is to be the glue > > between a class device and a bus device as essentially this is the exact > > physical device, just represented differently. > > It is not always as simple as "one bus device corresponds to one class device". > > Sometimes drivers have to deal with multiple class devices for a single bus > device, sometimes they are separated through the auxiliary bus or MFD. > > For instance, take struct net_device. In the context of cfg80211 wireless > drivers may create arbitrary struct net_device instances, depending on how often > the add_virtual_intf() callback is called (through netlink); example in [1]. > > Now, regarding your question "How to access class device private data from bus > device callbacks?". Nothing prevents a driver from embedding the class device in > its bus device private data in one or the other way. > > In the net device example above, the driver would probably keep a list (or > xarray, etc.) of net devices in its bus device private data, as they can be > created and removed at any point of time. > > (Note that this is also a good example for when the class device private data > lives shorter than the bus device private data.) > > Here is an example of this looks like in code using a DRM device as class > device. > > struct SampleIrqData; > > // The class device private data. > struct SampleDrm { > sched: drm::GpuScheduler, > } > > // The bus device private data. > #[pin_data] > struct SampleDriver { > #[pin] > irq: irq::Registration<SampleIrqData>, > drm: ARef<drm::Device<SampleDrm>, // Refcount of a `drm::Device`. > } > > impl pci::Driver for SampleDriver { > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > let dev = pdev.as_ref(); > > let drm = drm::Device::new(dev, SampleDrm::new()?)?; > drm.register(); > > Ok(impl_pin_init!(Self { > irq <- irq::Registration::new(...); > drm, > }) > } > > fn unbind(pdev: &pci::Device<Core>, self: Pin<&Self>) { > self.irq.synchronize(); > self.drm.flush_scheduler(); > } > } > > Note that the irq::Registration is dropped when the bus device is unbound, > whereas the GpuScheduler is dropped when the DRM device is dropped. > > Of course, depending on the actual class device this difference may be more or > less important, i.e. absolutely crucial in the net device example, less > important for RTC apparently. But it is always good to be precise about > ownership and lifetime of data. > > As I already mentioned, I don't think it is good to fold ownership and lifetime > for bus and class devices together in cases where it happens that they are > mostly identical, i.e.: > > >> I think it would be good to keep driver authors finding a common pattern, > >> where class device callbacks carry the corresponding class device struct > >> (instead of the parent base struct device). > >> > >> Especially on the Rust side we now have the chance to make the experience > >> of writing drivers as consistent as possible, which should help (new) > >> driver authors a lot in terms of learning the driver lifetime patterns. > > [1] https://elixir.bootlin.com/linux/v6.19.2/source/drivers/net/wireless/microchip/wilc1000/netdev.c#L948 General considerations aside, regarding the $subject patch, I don't think that it is an improvement because it would confuse the existing RTC class device interface quite a bit. Regarding whether or not switching over the RTC class device interface to passing RTC device pointers to its class callbacks is a good idea, I think that the difference really boils down to what data needs to be accessed by the driver. As it stands today, in an RTC class callback, the driver gets directly to the parent of the given RTC device and if it does not need to get to the RTC device at all, that's convenient. However, if the driver needs to get to the RTC device, it will most likely need to use driver_data in the parent device for this purpose (and store the RTC device pointer). If the interface were changed to pass RTC device pointers to class callbacks, the driver would get to the RTC device and from there it would need to get to the parent device in the majority of cases. Even though getting from an RTC device to its parent device is straightforward (and does not require using driver_data at all), in some drivers that would be an extra step that would make the picture somewhat less clear than it is today (and arguably less clear than it really needs to be). Still, the change of the interface might allow some other drivers that do access the RTC device in class callback functions to be simplified. Granted, the above options had already been there when the RTC class interface was designed and the choice was made at that time. Revising it now would require clear technical reasons IMV. How hard would it be for Rust to cope with the existing calling convention in the RTC class?
On Sun, Feb 22, 2026 at 1:25 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Sat, Feb 21, 2026 at 3:33 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Sat Feb 21, 2026 at 12:19 PM CET, Rafael J. Wysocki wrote: > > > On Sat, Feb 21, 2026 at 12:16 PM Alexandre Belloni > > > <alexandre.belloni@bootlin.com> wrote: > > >> > > Out of 29 drivers, 18 are doing so. > > > > > > The vast majority of around 50 platform drivers I've inspected > > > recently use platform_set_drvdata() or equivalent in probe. > > > > This thread seems to contain quite a bit of confusion and misunderstandings -- > > let me try to clarify. > > > > (1) How Rust handles bus device private data. > > > > In Rust the probe() function of a bus implementation (platform, PCI, etc.) > > returns an initializer (impl PinInit<T, Error>) for the driver's device > > private data. > > > > The bus implementation takes this initializer and passes it (together with the > > underlying struct device) to the driver-core. The driver-core allocates the > > required memory, initializes the memory with the given initializer and stores > > a pointer to the corresponding object with dev_set_drvdata(). > > > > So, technically, in Rust all platform drivers call platform_set_drvdata(). > > So do I understand correctly that the driver is required to tell the > core what type its driver_data will be and then the core will allocate > memory for it and clean it up on remove? > > > (Note that this is also true when the driver's device private data type is > > empty (i.e. it has no fields). In this case it could still have a destructor > > that must be called when the device private data structure is destroyed. Of > > course there is no real memory allocation when the struct's size is zero.) > > So in the simplest case when the driver doesn't need driver_data at > all, it will just use a struct with no fields as that driver_data > type, IIUC. > > > The driver's device private data can only be accessed when the bus device is > > bound to the driver, i.e. the driver can only access it with a &Device<Bound>; > > it (the driver's device private data) is automatically freed by the > > driver-core when remove() and all devres callbacks have been completed. > > Well, that's what happens on the C side of things too most of the time > because driver_data is allocated at probe time, very often using > devm_kzalloc() or equivalent. > > > I.e. the rules are - of course - the same as on the C side, but they are > > enforced by the type system and the driver-core code. > > OK > > > (2) Bus device private data vs. class device private data. > > > > The change to pass a struct rtc_device in class device callbacks of RTC, > > rather than the base struct device of the corresponding bus device (e.g. AMBA, > > platform, etc.) should not aim at storing all data in rtc->dev.private_data > > that was previously stored in rtc->dev.parent->private_data. > > > > Instead, it gives drivers the option to differentiate in terms of ownership > > and lifetime. > > > > While the bus device private data has a very defined lifetime from probe() > > until the device is unbound from the driver, class device private data might > > live shorter than this, or might even out-live driver unbind in some cases. It > > really depends on the lifetime of the class device itself, which is not > > generally defined. > > > > Now, from a C side point of view this may not look like a big deal, as it > > (unfortunately) is not that uncommon that struct fields are just initialized > > and destroyed whenever needed and the code just takes it into account. > > > > But at the same time, this is what leads to a lot of lifetime problems and > > memory bugs and it is one of those things that Rust aims at avoiding by being > > very strict about initialization, ownership and lifetimes. > > As a general rule, I agree, but I would advise against applying > general rules automatically everywhere. > > > However, I do also recognize that drivers creating an RTC device are typically > > very simple and in practice I would not be surprised if it turns out that it > > happens that drivers keep the struct rtc_device alive from probe() until the > > bus device is unbound from the driver, i.e. lifetimes just end up being almost > > the same. But I don't know if that's always the case. > > > > Regardless of that, I think it would be good to keep driver authors finding a > > common pattern, where class device callbacks carry the corresponding class > > device struct (instead of the parent base struct device). > > TBH I'm not really convinced about this particular thing and I think I > can provide an illustrative example. > > Namely, quite incidentally, I've recently set out to add an RTC class > device to an existing driver, which is the ACPI time and alarm device > (TAD) one. The TAD functionality is based on ACPI control methods > provided by the platform firmware and it may (or may not) include > RTC-equivalent functions. So far, the driver has been providing a > completely custom sysfs interface to user space, but since more and > more platforms contain an ACPI TAD and some of them may not contain a > "traditional" RTC, having an RTC class device interface in that driver > may be quite useful. > > I have a prototype of the requisite change (I'll post it shortly for > reference) and it turns out that because the RTC class callbacks take > the parent device pointer as an argument, wrapping them around the > existing driver routines backing the existing sysfs interface is > super-straightforward. Had the RTC class passed an RTC device pointer > to those callbacks, the driver would have had to do something to get > back from it to the parent device (which is what the driver really > works with). If there are more similar drivers, that would have led > to some code duplication that is in fact unnecessary. > > Moreover, the RTC device pointer doesn't even need to be stored > anywhere in that driver because the driver need not use it directly at > all and the RTC device object memory is freed by the core when the > driver unbinds. The above is referring specifically to this patch: https://lore.kernel.org/linux-acpi/10819001.nUPlyArG6x@rafael.j.wysocki/
On Sun Feb 22, 2026 at 1:25 PM CET, Rafael J. Wysocki wrote: > On Sat, Feb 21, 2026 at 3:33 PM Danilo Krummrich <dakr@kernel.org> wrote: >> >> On Sat Feb 21, 2026 at 12:19 PM CET, Rafael J. Wysocki wrote: >> > On Sat, Feb 21, 2026 at 12:16 PM Alexandre Belloni >> > <alexandre.belloni@bootlin.com> wrote: >> >> > > Out of 29 drivers, 18 are doing so. >> > >> > The vast majority of around 50 platform drivers I've inspected >> > recently use platform_set_drvdata() or equivalent in probe. >> >> This thread seems to contain quite a bit of confusion and misunderstandings -- >> let me try to clarify. >> >> (1) How Rust handles bus device private data. >> >> In Rust the probe() function of a bus implementation (platform, PCI, etc.) >> returns an initializer (impl PinInit<T, Error>) for the driver's device >> private data. >> >> The bus implementation takes this initializer and passes it (together with the >> underlying struct device) to the driver-core. The driver-core allocates the >> required memory, initializes the memory with the given initializer and stores >> a pointer to the corresponding object with dev_set_drvdata(). >> >> So, technically, in Rust all platform drivers call platform_set_drvdata(). > > So do I understand correctly that the driver is required to tell the > core what type its driver_data will be and then the core will allocate > memory for it and clean it up on remove? Yes, but it's not really that the driver actively has to tell the driver-core, etc. probe() functions return an initializer for the driver's device private data, so the type is known anyways. fn probe( pdev: &pci::Device<Core>, info: &Self::IdInfo, ) -> impl PinInit<T, Error> { ... } So, the return type is a fallible initializer for T, where T is the type of the driver's device private data. I assume this may sound a bit odd with little or no Rust experience. Hence, for people without much Rust experience reading along a quick explanation: On the more general side of things, Rust has a very powerful type system, which includes generics, hence modeling such kind of things with generics is pretty straight forward and preferred over passing void pointers. But there is also a much more specific reason; In C dev_get_drvdata() has two pitfalls: (1) The pointer returned by dev_get_drvdata() is only valid as long as the bus device is bound to the driver. (2) The driver has to cast the pointer returned by dev_get_drvdata() to the correct type. Since Rust is a memory safe language, we can't allow UB for safe APIs. Hence, the Rust Device::drvdata() [1] method has to consider both (1) and (2). (1) is rather simple as we have a type state found bound devices, i.e. Device<Bound>, so drvdata() is simply only implemented for Device<Bound>. (2) is much more tricky as we can't statically type the device over its private data, as a single device instance can be bound to multiple drivers at runtime. Hence, we need a runtime check, which the driver-core does for us. When a driver calls the drvdata() method it looks like this: fn foo(dev: &Device<Bound>) -> Result { let data = dev.drvdata::<MyDataType>()?; data.bar(); } The driver-core takes care of checking that the private data associated with `dev` actually is of type MyDataType. If this is not the case, the call simply fails. The alternative would be an infallible unsafe API, such as: fn foo(dev: &Device<Bound>) -> Result { // SAFETY: // - `dev` is guaranteed to be bound, because ... // - The private data type of `dev` is guaranteed to be // `MyDataType`, since ... let data = unsafe { dev.drvdata::<MyDataType>() }; data.bar(); } [1] https://rust.docs.kernel.org/kernel/device/struct.Device.html#method.drvdata >> (Note that this is also true when the driver's device private data type is >> empty (i.e. it has no fields). In this case it could still have a destructor >> that must be called when the device private data structure is destroyed. Of >> course there is no real memory allocation when the struct's size is zero.) > > So in the simplest case when the driver doesn't need driver_data at > all, it will just use a struct with no fields as that driver_data > type, IIUC. Yes, it would look like this: struct SampleDriver; impl pci::Driver for SampleDriver { fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { if !validate_something() { return Err(EINVAL); } Ok(Self) } } >> But at the same time, this is what leads to a lot of lifetime problems and >> memory bugs and it is one of those things that Rust aims at avoiding by being >> very strict about initialization, ownership and lifetimes. > > As a general rule, I agree, but I would advise against applying > general rules automatically everywhere. > >> However, I do also recognize that drivers creating an RTC device are typically >> very simple and in practice I would not be surprised if it turns out that it >> happens that drivers keep the struct rtc_device alive from probe() until the >> bus device is unbound from the driver, i.e. lifetimes just end up being almost >> the same. But I don't know if that's always the case. >> >> Regardless of that, I think it would be good to keep driver authors finding a >> common pattern, where class device callbacks carry the corresponding class >> device struct (instead of the parent base struct device). > > TBH I'm not really convinced about this particular thing and I think I > can provide an illustrative example. > > Namely, quite incidentally, I've recently set out to add an RTC class > device to an existing driver, which is the ACPI time and alarm device > (TAD) one. The TAD functionality is based on ACPI control methods > provided by the platform firmware and it may (or may not) include > RTC-equivalent functions. So far, the driver has been providing a > completely custom sysfs interface to user space, but since more and > more platforms contain an ACPI TAD and some of them may not contain a > "traditional" RTC, having an RTC class device interface in that driver > may be quite useful. > > I have a prototype of the requisite change (I'll post it shortly for > reference) and it turns out that because the RTC class callbacks take > the parent device pointer as an argument, wrapping them around the > existing driver routines backing the existing sysfs interface is > super-straightforward. Had the RTC class passed an RTC device pointer > to those callbacks, the driver would have had to do something to get > back from it to the parent device (which is what the driver really > works with). If there are more similar drivers, that would have led > to some code duplication that is in fact unnecessary. The type system in Rust is powerfuly enough, so drivers can even get the exact type of the parent device the RTC device has as an additional argument to the callback. The infrastructure for this is in place and it is used by subsystems. I.e. in RTC we can do something like this: impl rtc::Ops for MyRtcOps { type BusDeviceType = platform::Device<Bound>; fn read_time( rtc: &rtc::Device<MyRtcData>, parent: &platform::Device<Bound>, time: rtc::Time, ) -> Result { ... } } where the corresponding rtc::Device::register() method would ensure that the associated BusDeviceType matches the parent device type that is passed to rtc::Device::register(). A real example of this can be found in the LED class device abstractions [2]. Note that in contrast to a bus device, class devices can be statically typed over their private data: rtc::Device<MyRtcData>. We usually also implment the Deref trait, such that rtc::Device<MyRtcData> automatically dereferences to MyRtcData. Having that said, if RTC drivers *never* have any private data that should be associated with the RTC device exclusively (i.e. data that is only relevant in the context of class device callbacks and class device infrastructure in general, or logically belongs to the class device), then the `rdev` argument would indeed be always unused and hence pointless. I usually would assume that there are such cases, but if that's really never the case for any RTC drivers, then I agree we should change the above code to: impl rtc::Ops for MyRtcOps { type BusDeviceType = platform::Device<Bound>; fn read_time( parent: &platform::Device<Bound>, time: rtc::Time, ) -> Result { ... } } This way it is at least clear what kind of device is passed through the class device callbacks. [2] https://lore.kernel.org/all/20260207-rust_leds-v12-1-fdb518417b75@posteo.de/ > Moreover, the RTC device pointer doesn't even need to be stored > anywhere in that driver because the driver need not use it directly at > all and the RTC device object memory is freed by the core when the > driver unbinds. I don't think that is true, I think there are a few drivers accessing the RTC device from IRQs or workqueues. Besides that, quite a lot of RTC drivers actually seem to save a pointer to the struct rtc_device within their bus device private data, e.g. [3] and [4]. [3] https://elixir.bootlin.com/linux/v6.19.2/source/drivers/rtc/rtc-ac100.c#L91 [4] https://elixir.bootlin.com/linux/v6.19.2/source/drivers/rtc/rtc-cros-ec.c#L30 >> Especially on the Rust side we now have the chance to make the experience of >> writing drivers as consistent as possible, which should help (new) driver >> authors a lot in terms of learning the driver lifetime patterns. > > Well, I'm not sure if "the experience of writing drivers as consistent > as possible" is more important than less code duplication and simpler > code. Yeah, it always depends, and if there *really* is no point in having any class device private data in RTC, then that's of course fine too. I just want to make sure we do not encourage to just through all the private data a driver may potentially have into the bus device private data struct. I saw this a lot in C drivers and it actually causes ownership and lifetime problems.
On Sun, Feb 22, 2026 at 4:29 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Sun Feb 22, 2026 at 1:25 PM CET, Rafael J. Wysocki wrote: > > On Sat, Feb 21, 2026 at 3:33 PM Danilo Krummrich <dakr@kernel.org> wrote: > >> > >> On Sat Feb 21, 2026 at 12:19 PM CET, Rafael J. Wysocki wrote: > >> > On Sat, Feb 21, 2026 at 12:16 PM Alexandre Belloni > >> > <alexandre.belloni@bootlin.com> wrote: > >> >> > > Out of 29 drivers, 18 are doing so. > >> > > >> > The vast majority of around 50 platform drivers I've inspected > >> > recently use platform_set_drvdata() or equivalent in probe. > >> > >> This thread seems to contain quite a bit of confusion and misunderstandings -- > >> let me try to clarify. > >> > >> (1) How Rust handles bus device private data. > >> > >> In Rust the probe() function of a bus implementation (platform, PCI, etc.) > >> returns an initializer (impl PinInit<T, Error>) for the driver's device > >> private data. > >> > >> The bus implementation takes this initializer and passes it (together with the > >> underlying struct device) to the driver-core. The driver-core allocates the > >> required memory, initializes the memory with the given initializer and stores > >> a pointer to the corresponding object with dev_set_drvdata(). > >> > >> So, technically, in Rust all platform drivers call platform_set_drvdata(). > > > > So do I understand correctly that the driver is required to tell the > > core what type its driver_data will be and then the core will allocate > > memory for it and clean it up on remove? > > Yes, but it's not really that the driver actively has to tell the driver-core, > etc. > > probe() functions return an initializer for the driver's device private data, so > the type is known anyways. > > fn probe( > pdev: &pci::Device<Core>, > info: &Self::IdInfo, > ) -> impl PinInit<T, Error> { > ... > } > > So, the return type is a fallible initializer for T, where T is the type of the > driver's device private data. > > I assume this may sound a bit odd with little or no Rust experience. Hence, for > people without much Rust experience reading along a quick explanation: > > On the more general side of things, Rust has a very powerful type system, which > includes generics, hence modeling such kind of things with generics is pretty > straight forward and preferred over passing void pointers. > > But there is also a much more specific reason; In C dev_get_drvdata() has two > pitfalls: > > (1) The pointer returned by dev_get_drvdata() is only valid as long as the > bus device is bound to the driver. > > (2) The driver has to cast the pointer returned by dev_get_drvdata() to the > correct type. > > Since Rust is a memory safe language, we can't allow UB for safe APIs. Hence, > the Rust Device::drvdata() [1] method has to consider both (1) and (2). > > (1) is rather simple as we have a type state found bound devices, i.e. > Device<Bound>, so drvdata() is simply only implemented for Device<Bound>. > > (2) is much more tricky as we can't statically type the device over its private > data, as a single device instance can be bound to multiple drivers at runtime. > > Hence, we need a runtime check, which the driver-core does for us. When a driver > calls the drvdata() method it looks like this: > > fn foo(dev: &Device<Bound>) -> Result { > let data = dev.drvdata::<MyDataType>()?; > > data.bar(); > } > > The driver-core takes care of checking that the private data associated with > `dev` actually is of type MyDataType. If this is not the case, the call simply > fails. > > The alternative would be an infallible unsafe API, such as: > > fn foo(dev: &Device<Bound>) -> Result { > // SAFETY: > // - `dev` is guaranteed to be bound, because ... > // - The private data type of `dev` is guaranteed to be > // `MyDataType`, since ... > let data = unsafe { dev.drvdata::<MyDataType>() }; > > data.bar(); > } > > [1] https://rust.docs.kernel.org/kernel/device/struct.Device.html#method.drvdata > > >> (Note that this is also true when the driver's device private data type is > >> empty (i.e. it has no fields). In this case it could still have a destructor > >> that must be called when the device private data structure is destroyed. Of > >> course there is no real memory allocation when the struct's size is zero.) > > > > So in the simplest case when the driver doesn't need driver_data at > > all, it will just use a struct with no fields as that driver_data > > type, IIUC. > > Yes, it would look like this: > > struct SampleDriver; > > impl pci::Driver for SampleDriver { > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > if !validate_something() { > return Err(EINVAL); > } > > Ok(Self) > } > } > > >> But at the same time, this is what leads to a lot of lifetime problems and > >> memory bugs and it is one of those things that Rust aims at avoiding by being > >> very strict about initialization, ownership and lifetimes. > > > > As a general rule, I agree, but I would advise against applying > > general rules automatically everywhere. > > > >> However, I do also recognize that drivers creating an RTC device are typically > >> very simple and in practice I would not be surprised if it turns out that it > >> happens that drivers keep the struct rtc_device alive from probe() until the > >> bus device is unbound from the driver, i.e. lifetimes just end up being almost > >> the same. But I don't know if that's always the case. > >> > >> Regardless of that, I think it would be good to keep driver authors finding a > >> common pattern, where class device callbacks carry the corresponding class > >> device struct (instead of the parent base struct device). > > > > TBH I'm not really convinced about this particular thing and I think I > > can provide an illustrative example. > > > > Namely, quite incidentally, I've recently set out to add an RTC class > > device to an existing driver, which is the ACPI time and alarm device > > (TAD) one. The TAD functionality is based on ACPI control methods > > provided by the platform firmware and it may (or may not) include > > RTC-equivalent functions. So far, the driver has been providing a > > completely custom sysfs interface to user space, but since more and > > more platforms contain an ACPI TAD and some of them may not contain a > > "traditional" RTC, having an RTC class device interface in that driver > > may be quite useful. > > > > I have a prototype of the requisite change (I'll post it shortly for > > reference) and it turns out that because the RTC class callbacks take > > the parent device pointer as an argument, wrapping them around the > > existing driver routines backing the existing sysfs interface is > > super-straightforward. Had the RTC class passed an RTC device pointer > > to those callbacks, the driver would have had to do something to get > > back from it to the parent device (which is what the driver really > > works with). If there are more similar drivers, that would have led > > to some code duplication that is in fact unnecessary. > > The type system in Rust is powerfuly enough, so drivers can even get the exact > type of the parent device the RTC device has as an additional argument to the > callback. The infrastructure for this is in place and it is used by subsystems. > I.e. in RTC we can do something like this: > > impl rtc::Ops for MyRtcOps { > type BusDeviceType = platform::Device<Bound>; > > fn read_time( > rtc: &rtc::Device<MyRtcData>, > parent: &platform::Device<Bound>, > time: rtc::Time, > ) -> Result { > ... > } > } > > where the corresponding rtc::Device::register() method would ensure that the > associated BusDeviceType matches the parent device type that is passed to > rtc::Device::register(). > > A real example of this can be found in the LED class device abstractions [2]. > > Note that in contrast to a bus device, class devices can be statically typed > over their private data: rtc::Device<MyRtcData>. > > We usually also implment the Deref trait, such that rtc::Device<MyRtcData> > automatically dereferences to MyRtcData. > > Having that said, if RTC drivers *never* have any private data that should be > associated with the RTC device exclusively (i.e. data that is only relevant in > the context of class device callbacks and class device infrastructure in > general, or logically belongs to the class device), then the `rdev` argument > would indeed be always unused and hence pointless. > > I usually would assume that there are such cases, but if that's really never the > case for any RTC drivers, then I agree we should change the above code to: > > impl rtc::Ops for MyRtcOps { > type BusDeviceType = platform::Device<Bound>; > > fn read_time( > parent: &platform::Device<Bound>, > time: rtc::Time, > ) -> Result { > ... > } > } > > This way it is at least clear what kind of device is passed through the class > device callbacks. > > [2] https://lore.kernel.org/all/20260207-rust_leds-v12-1-fdb518417b75@posteo.de/ > > > Moreover, the RTC device pointer doesn't even need to be stored > > anywhere in that driver because the driver need not use it directly at > > all and the RTC device object memory is freed by the core when the > > driver unbinds. > > I don't think that is true, I think there are a few drivers accessing the RTC > device from IRQs or workqueues. I was referring to the specific case I had in mind, I didn't mean that this was always the case. Sorry for being unclear. > Besides that, quite a lot of RTC drivers actually seem to save a pointer to the > struct rtc_device within their bus device private data, e.g. [3] and [4]. > > [3] https://elixir.bootlin.com/linux/v6.19.2/source/drivers/rtc/rtc-ac100.c#L91 > [4] https://elixir.bootlin.com/linux/v6.19.2/source/drivers/rtc/rtc-cros-ec.c#L30 Right. > >> Especially on the Rust side we now have the chance to make the experience of > >> writing drivers as consistent as possible, which should help (new) driver > >> authors a lot in terms of learning the driver lifetime patterns. > > > > Well, I'm not sure if "the experience of writing drivers as consistent > > as possible" is more important than less code duplication and simpler > > code. > > Yeah, it always depends, and if there *really* is no point in having any class > device private data in RTC, then that's of course fine too. > > I just want to make sure we do not encourage to just through all the private > data a driver may potentially have into the bus device private data struct. > > I saw this a lot in C drivers and it actually causes ownership and lifetime > problems. Sure. Alexandre doesn't seem to be convinced that it is necessary to change the RTC class device interface, so if there are any existing RTC drivers that would benefit from doing so, it would be good to refer to them specifically as examples. Alvin, can you find any?
On Sun Feb 22, 2026 at 3:01 PM CET, Rafael J. Wysocki wrote: > General considerations aside, regarding the $subject patch, I don't > think that it is an improvement because it would confuse the existing > RTC class device interface quite a bit. > > Regarding whether or not switching over the RTC class device interface > to passing RTC device pointers to its class callbacks is a good idea, > I think that the difference really boils down to what data needs to be > accessed by the driver. > > As it stands today, in an RTC class callback, the driver gets directly > to the parent of the given RTC device and if it does not need to get > to the RTC device at all, that's convenient. However, if the driver > needs to get to the RTC device, it will most likely need to use > driver_data in the parent device for this purpose (and store the RTC > device pointer). > > If the interface were changed to pass RTC device pointers to class > callbacks, the driver would get to the RTC device and from there it > would need to get to the parent device in the majority of cases. Even > though getting from an RTC device to its parent device is > straightforward (and does not require using driver_data at all), in > some drivers that would be an extra step that would make the picture > somewhat less clear than it is today (and arguably less clear than it > really needs to be). Still, the change of the interface might allow > some other drivers that do access the RTC device in class callback > functions to be simplified. > > Granted, the above options had already been there when the RTC class > interface was designed and the choice was made at that time. Revising > it now would require clear technical reasons IMV. > > How hard would it be for Rust to cope with the existing calling > convention in the RTC class? There is no limitation from the Rust side of things, as mentioned in [1], this should work perfectly fine, but it might be slightly less convinient: Example 1: impl rtc::Ops for MyRtcOps { type BusDeviceType = platform::Device<Bound>; fn read_time( parent: &platform::Device<Bound>, time: &mut rtc::Time, ) -> Result { let drvdata = pdev.as_ref().drvdata::<MyDriver>()?; ... } } We can improve this by avoiding that the driver has to extract its bus device private data all the time by doing this instead: Example 2: impl rtc::Ops for MyRtcOps { type BusDeviceType = platform::Device<Bound>; type BusDeviceData = MyDriver; fn read_time( parent: &platform::Device<Bound>, drvdata: &MyDriver, time: &mut rtc::Time, ) -> Result { ... } } However, this would require to type the rtc::Device over the bus device private data type, which works perfectly fine, but arguably is a bit odd. The alternative to make it convinient for drivers and avoid this oddity is to just let drivers store the data that needs to be accessed from class device callbacks in the class device private data and data that is only needed in bus device callbacks in the bus device private data. Additionally, drivers can of course save a refcount of the class device in their bus device private data as well. This is mostly what patch 5 of this series currently does (with a few modifications) and I personally think it's the most clean approach in terms of how it turns out for drivers: Example 3: // The bus device private data. #[pin_data] struct MyDriver { #[pin] irq: irq::Registration<MyIrqHandler>, } // The class device private data. struct MyRtcData { io: Devres<IoMem<PL031_REG_SIZE>>, hw_variant: VendorVariant, } impl rtc::Ops for MyRtcOps { type BusDeviceType = platform::Device<Bound>; fn read_time( rtc: &rtc::Device<MyRtcData> parent: &platform::Device<Bound>, time: &mut rtc::Time, ) -> Result { let io = rtc.io.access(parent)?; match rtc.hw_variant { VendorVariant::Arm | VendorVariant::StV1 => { let my_time = io.read(...); my_time.write_into(time); }, VendorVariant::StV2 => { ... }, } } } (Note how we can directly access the `io` and `hw_variant` fields from `rtc`.) If the data from struct MyRtcData is also needed from other bus callbacks, we can simply extend struct MyDriver: #[pin_data] struct MyDriver { #[pin] irq: irq::Registration<MyIrqHandler>, rtc: ARef<rtc::Device<MyRtcData>>, } But again, I just want to make sure we do not encourage to just throw all the private data a driver may potentially have into the bus device private data struct. Besides that, it is of course a subsystem decision and I don't want to be presumptuous or anything. My recommendation is to go with something like in (3), otherwise I'd choose (1) and (2) is my least favorite as I think it is a bit odd to type a class device over bus device private data. [1] https://lore.kernel.org/all/DGLLJ541SEJW.160MET6OCQHKS@kernel.org/
On Sun Feb 22, 2026 at 5:13 PM CET, Danilo Krummrich wrote: > There is no limitation from the Rust side of things, as mentioned in [1], this > should work perfectly fine, but it might be slightly less convinient: When I wrote this on Sunday, I should have been much more precise about this, since it actually does not work perfectly fine without further changes that would (re-)introduce ordering constraints for drivers on the probe() side of things that I want to avoid. The "re-" is in braces as we never had those ordering constraints in Rust, but we have quite a lot of them in C, where they cause endless problems. Which is why I want to avoid them in Rust. > Example 1: > > impl rtc::Ops for MyRtcOps { > type BusDeviceType = platform::Device<Bound>; > > fn read_time( > parent: &platform::Device<Bound>, > time: &mut rtc::Time, > ) -> Result { > let drvdata = pdev.as_ref().drvdata::<MyDriver>()?; > > ... > } > } Let's have a look at the corresponding probe() side of things. struct SampleIrqData; // The bus device private data. #[pin_data] struct SampleDriver { #[pin] irq: irq::Registration<SampleIrqData>, rtc: ARef<rtc::Device>, ..., } impl pci::Driver for SampleDriver { fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { let dev = pdev.as_ref(); let rtc = rtc::Device::new(dev)?; Ok(impl_pin_init!(Self { irq <- irq::Registration::new(...), rtc, _: { devres::register(rtc::Registration::new(rtc))?; } }) } } This is a problem, since even though the registration of the RTC device is the last thing in the initializer, rtc::Ops::read_time() above could race and Device::drvdata() could just fail as it might not be set yet. This is fixable, but - as mentioned - at the price of introducing ordering constraints for drivers. For instance, this could be solved by introducing a pci::ProbeDevice<'a> type, which ensures that pci::ProbeDevice::set_drvdata() can only be called from probe() and can only be called once, as it consumes the pci::ProbeDevice. struct SampleIrqData; // The bus device private data. #[pin_data] struct SampleDriver { #[pin] irq: irq::Registration<SampleIrqData>, rtc: ARef<rtc::Device>, ..., } impl pci::Driver for SampleDriver { fn probe<'a>(pdev: pci::ProbeDevice<'a>, info: &Self::IdInfo) -> Result { let dev = pdev.as_ref(); let rtc = rtc::Device::new(dev)?; let data = impl_pin_init!(Self { irq <- irq::Registration::new(...), rtc, }); // The `pci::ProbeDevice` has been consumed by value, and // returned a `&pci::Device<Core>`. let pdev = pdev.set_drvdata(data)?; devres::register(rtc::Registration::new(rtc)) } } However, note how this wouldn't solve the same problem for irq::Registration, as it lives within the the driver's device private data. If this would be C you would probably just partially initialize the driver's device private data, but in Rust uninitialized data is not allowed for safety reasons. We could probably find other ways to somehow handle this, e.g. by introducing new device context states, additional callbacks, etc., but in the end it would only be workarounds for not having defined ownership, that eventually ends up being more complicated and more error prone. For this reason an irq::Registration has its own private data (which in this example is just an empty struct). Similarly, all other kinds of registrations (and driver entry points) have their own private data, such as class devices, work and workqueues, file operations, timers, etc. I.e. there is a clear separation of ownership and lifetime, which makes more or less (often more) suble ordering constraints obsolete. In fact, in the beginning I did not even plan to expose Device::drvdata() at all, making the driver's device private data only available in bus device callbacks, etc. The reason why I added it was that it was required (and makes sense) when drivers interact with other drivers, e.g. through an auxiliary device. I did also mention this in the commit message of commit 6f61a2637abe ("rust: device: introduce Device::drvdata()"): (2) Having a direct accessor to the driver's private data is not commonly required (at least in Rust): Bus callback methods already provide access to the driver's device private data through a &self argument, while other driver entry points such as IRQs, workqueues, timers, IOCTLs, etc. have their own private data with separate ownership and lifetime. In other words, a driver's device private data is only relevant for driver model contexts (such a file private is only relevant for file contexts). Having that said, the motivation for accessing the driver's device private data with Device<Bound>::drvdata() are interactions between drivers. For instance, when an auxiliary driver calls back into its parent, the parent has to be capable to derive its private data from the corresponding device (i.e. the parent of the auxiliary device). Let's have a look at how probe() looks like for the example below, which is what we do in other subsystems, such as DRM or PWM. > Example 3: > > struct SampleIrqData { > rtc: ARef<rtc::Device>, > }; > > // The bus device private data. > #[pin_data] > struct SampleDriver { > #[pin] > irq: irq::Registration<SampleIrqHandler>, > rtc: ARef<rtc::Device<SampleRtcData>>, > } > > // The class device private data. > struct SampleRtcData { > io: Devres<IoMem<PL031_REG_SIZE>>, > hw_variant: VendorVariant, > } > > impl rtc::Ops for MyRtcOps { > type BusDeviceType = platform::Device<Bound>; > > fn read_time( > rtc: &rtc::Device<SampleRtcData> > parent: &platform::Device<Bound>, > time: &mut rtc::Time, > ) -> Result { > let io = rtc.io.access(parent)?; > > match rtc.hw_variant { > VendorVariant::Arm | VendorVariant::StV1 => { > let my_time = io.read(...); > > my_time.write_into(time); > }, > VendorVariant::StV2 => { ... }, > } > } > } > impl pci::Driver for SampleDriver { fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { let dev = pdev.as_ref(); let rtc_data = impl_pin_init!(SampleRtcData { io: iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, hw_variant: VendorVariant::StV1, }); let rtc = rtc::Device::new(dev, rtc_data)?; // Internally calls `devres::register(rtc::Registration::new())`. rtc::Registration::register(rtc)?; Ok(impl_pin_init!(Self { // Give the IRQ handler a reference count of the `rtc::Device`. irq <- irq::Registration::new(..., rtc.clone()), rtc, }) } } With this there are no (subtle) ordering constraints the driver has to get right; ownership and lifetimes are well defined. (I.e. whatever order a driver picks, it either works properly or it does not compile in the first place, which is a huge improvement over the situation we have in C.)
On Tue, Feb 24, 2026 at 1:12 AM Danilo Krummrich <dakr@kernel.org> wrote: > > On Sun Feb 22, 2026 at 5:13 PM CET, Danilo Krummrich wrote: > > There is no limitation from the Rust side of things, as mentioned in [1], this > > should work perfectly fine, but it might be slightly less convinient: > > When I wrote this on Sunday, I should have been much more precise about this, > since it actually does not work perfectly fine without further changes that > would (re-)introduce ordering constraints for drivers on the probe() side of > things that I want to avoid. > > The "re-" is in braces as we never had those ordering constraints in Rust, but > we have quite a lot of them in C, where they cause endless problems. Which is > why I want to avoid them in Rust. > > > Example 1: > > > > impl rtc::Ops for MyRtcOps { > > type BusDeviceType = platform::Device<Bound>; > > > > fn read_time( > > parent: &platform::Device<Bound>, > > time: &mut rtc::Time, > > ) -> Result { > > let drvdata = pdev.as_ref().drvdata::<MyDriver>()?; > > > > ... > > } > > } > > Let's have a look at the corresponding probe() side of things. > > struct SampleIrqData; > > // The bus device private data. > #[pin_data] > struct SampleDriver { > #[pin] > irq: irq::Registration<SampleIrqData>, > rtc: ARef<rtc::Device>, > ..., > } > > impl pci::Driver for SampleDriver { > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > let dev = pdev.as_ref(); > > let rtc = rtc::Device::new(dev)?; > > Ok(impl_pin_init!(Self { > irq <- irq::Registration::new(...), > rtc, > _: { > devres::register(rtc::Registration::new(rtc))?; > } > }) > } > } > > This is a problem, since even though the registration of the RTC device is the > last thing in the initializer, rtc::Ops::read_time() above could race and > Device::drvdata() could just fail as it might not be set yet. > > This is fixable, but - as mentioned - at the price of introducing ordering > constraints for drivers. > > For instance, this could be solved by introducing a pci::ProbeDevice<'a> type, > which ensures that pci::ProbeDevice::set_drvdata() can only be called from > probe() and can only be called once, as it consumes the pci::ProbeDevice. > > struct SampleIrqData; > > // The bus device private data. > #[pin_data] > struct SampleDriver { > #[pin] > irq: irq::Registration<SampleIrqData>, > rtc: ARef<rtc::Device>, > ..., > } > > impl pci::Driver for SampleDriver { > fn probe<'a>(pdev: pci::ProbeDevice<'a>, info: &Self::IdInfo) -> Result { > let dev = pdev.as_ref(); > > let rtc = rtc::Device::new(dev)?; > > let data = impl_pin_init!(Self { > irq <- irq::Registration::new(...), > rtc, > }); > > // The `pci::ProbeDevice` has been consumed by value, and > // returned a `&pci::Device<Core>`. > let pdev = pdev.set_drvdata(data)?; > > devres::register(rtc::Registration::new(rtc)) > } > } > > However, note how this wouldn't solve the same problem for irq::Registration, as > it lives within the the driver's device private data. > > If this would be C you would probably just partially initialize the driver's > device private data, but in Rust uninitialized data is not allowed for safety > reasons. > > We could probably find other ways to somehow handle this, e.g. by introducing > new device context states, additional callbacks, etc., but in the end it would > only be workarounds for not having defined ownership, that eventually ends up > being more complicated and more error prone. > > For this reason an irq::Registration has its own private data (which in this > example is just an empty struct). > > Similarly, all other kinds of registrations (and driver entry points) have their > own private data, such as class devices, work and workqueues, file operations, > timers, etc. > > I.e. there is a clear separation of ownership and lifetime, which makes more or > less (often more) suble ordering constraints obsolete. > > In fact, in the beginning I did not even plan to expose Device::drvdata() at > all, making the driver's device private data only available in bus device > callbacks, etc. > > The reason why I added it was that it was required (and makes sense) when > drivers interact with other drivers, e.g. through an auxiliary device. > > I did also mention this in the commit message of commit 6f61a2637abe ("rust: > device: introduce Device::drvdata()"): > > (2) Having a direct accessor to the driver's private data is not > commonly required (at least in Rust): Bus callback methods already > provide access to the driver's device private data through a &self > argument, while other driver entry points such as IRQs, > workqueues, timers, IOCTLs, etc. have their own private data with > separate ownership and lifetime. > > In other words, a driver's device private data is only relevant > for driver model contexts (such a file private is only relevant > for file contexts). > > Having that said, the motivation for accessing the driver's device > private data with Device<Bound>::drvdata() are interactions between > drivers. For instance, when an auxiliary driver calls back into its > parent, the parent has to be capable to derive its private data from the > corresponding device (i.e. the parent of the auxiliary device). > > Let's have a look at how probe() looks like for the example below, which is what > we do in other subsystems, such as DRM or PWM. > > > Example 3: > > > > struct SampleIrqData { > > rtc: ARef<rtc::Device>, > > }; > > > > // The bus device private data. > > #[pin_data] > > struct SampleDriver { > > #[pin] > > irq: irq::Registration<SampleIrqHandler>, > > rtc: ARef<rtc::Device<SampleRtcData>>, > > } > > > > // The class device private data. > > struct SampleRtcData { > > io: Devres<IoMem<PL031_REG_SIZE>>, > > hw_variant: VendorVariant, > > } > > > > impl rtc::Ops for MyRtcOps { > > type BusDeviceType = platform::Device<Bound>; > > > > fn read_time( > > rtc: &rtc::Device<SampleRtcData> > > parent: &platform::Device<Bound>, > > time: &mut rtc::Time, > > ) -> Result { > > let io = rtc.io.access(parent)?; > > > > match rtc.hw_variant { > > VendorVariant::Arm | VendorVariant::StV1 => { > > let my_time = io.read(...); > > > > my_time.write_into(time); > > }, > > VendorVariant::StV2 => { ... }, > > } > > } > > } > > > > impl pci::Driver for SampleDriver { > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > let dev = pdev.as_ref(); > > let rtc_data = impl_pin_init!(SampleRtcData { > io: iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > // Internally calls `devres::register(rtc::Registration::new())`. > rtc::Registration::register(rtc)?; > > Ok(impl_pin_init!(Self { > // Give the IRQ handler a reference count of the `rtc::Device`. > irq <- irq::Registration::new(..., rtc.clone()), > rtc, > }) > } > } > > With this there are no (subtle) ordering constraints the driver has to get > right; ownership and lifetimes are well defined. > > (I.e. whatever order a driver picks, it either works properly or it does not > compile in the first place, which is a huge improvement over the situation we > have in C.) Thanks for all of this information and let me confirm my understanding. In both Example 1 and Example 3 there is a dependency between rtc's read_time() callback and the PCI driver probe() because calling the former before the latter is complete would be premature (and it would fail). However, in Example 1 the compiler does not take that dependency into account because rtc is not passed to the callback, while in Example 3 the dependency is visible to the compiler and it will refuse to compile the code if the dependency may be missed. If the above is correct, the current calling convention of RTC class callbacks is problematic because it causes correctness checks done by the Rust compiler to be bypassed (which may allow certain ordering issues to be missed and functional problems to appear going forward). That's convincing and I think that adding an RTC device pointer to the list of parameters of RTC class callbacks on the C side would not be a very intrusive change. Arguably, it would be less intrusive than replacing the parent device pointer with the RTC device pointer in them.
On 24/02/2026 14:28:48+0100, Rafael J. Wysocki wrote: > On Tue, Feb 24, 2026 at 1:12 AM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Sun Feb 22, 2026 at 5:13 PM CET, Danilo Krummrich wrote: > > > There is no limitation from the Rust side of things, as mentioned in [1], this > > > should work perfectly fine, but it might be slightly less convinient: > > > > When I wrote this on Sunday, I should have been much more precise about this, > > since it actually does not work perfectly fine without further changes that > > would (re-)introduce ordering constraints for drivers on the probe() side of > > things that I want to avoid. > > > > The "re-" is in braces as we never had those ordering constraints in Rust, but > > we have quite a lot of them in C, where they cause endless problems. Which is > > why I want to avoid them in Rust. > > > > > Example 1: > > > > > > impl rtc::Ops for MyRtcOps { > > > type BusDeviceType = platform::Device<Bound>; > > > > > > fn read_time( > > > parent: &platform::Device<Bound>, > > > time: &mut rtc::Time, > > > ) -> Result { > > > let drvdata = pdev.as_ref().drvdata::<MyDriver>()?; > > > > > > ... > > > } > > > } > > > > Let's have a look at the corresponding probe() side of things. > > > > struct SampleIrqData; > > > > // The bus device private data. > > #[pin_data] > > struct SampleDriver { > > #[pin] > > irq: irq::Registration<SampleIrqData>, > > rtc: ARef<rtc::Device>, > > ..., > > } > > > > impl pci::Driver for SampleDriver { > > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > > let dev = pdev.as_ref(); > > > > let rtc = rtc::Device::new(dev)?; > > > > Ok(impl_pin_init!(Self { > > irq <- irq::Registration::new(...), > > rtc, > > _: { > > devres::register(rtc::Registration::new(rtc))?; > > } > > }) > > } > > } > > > > This is a problem, since even though the registration of the RTC device is the > > last thing in the initializer, rtc::Ops::read_time() above could race and > > Device::drvdata() could just fail as it might not be set yet. > > > > This is fixable, but - as mentioned - at the price of introducing ordering > > constraints for drivers. > > > > For instance, this could be solved by introducing a pci::ProbeDevice<'a> type, > > which ensures that pci::ProbeDevice::set_drvdata() can only be called from > > probe() and can only be called once, as it consumes the pci::ProbeDevice. > > > > struct SampleIrqData; > > > > // The bus device private data. > > #[pin_data] > > struct SampleDriver { > > #[pin] > > irq: irq::Registration<SampleIrqData>, > > rtc: ARef<rtc::Device>, > > ..., > > } > > > > impl pci::Driver for SampleDriver { > > fn probe<'a>(pdev: pci::ProbeDevice<'a>, info: &Self::IdInfo) -> Result { > > let dev = pdev.as_ref(); > > > > let rtc = rtc::Device::new(dev)?; > > > > let data = impl_pin_init!(Self { > > irq <- irq::Registration::new(...), > > rtc, > > }); > > > > // The `pci::ProbeDevice` has been consumed by value, and > > // returned a `&pci::Device<Core>`. > > let pdev = pdev.set_drvdata(data)?; > > > > devres::register(rtc::Registration::new(rtc)) > > } > > } > > > > However, note how this wouldn't solve the same problem for irq::Registration, as > > it lives within the the driver's device private data. > > > > If this would be C you would probably just partially initialize the driver's > > device private data, but in Rust uninitialized data is not allowed for safety > > reasons. > > > > We could probably find other ways to somehow handle this, e.g. by introducing > > new device context states, additional callbacks, etc., but in the end it would > > only be workarounds for not having defined ownership, that eventually ends up > > being more complicated and more error prone. > > > > For this reason an irq::Registration has its own private data (which in this > > example is just an empty struct). > > > > Similarly, all other kinds of registrations (and driver entry points) have their > > own private data, such as class devices, work and workqueues, file operations, > > timers, etc. > > > > I.e. there is a clear separation of ownership and lifetime, which makes more or > > less (often more) suble ordering constraints obsolete. > > > > In fact, in the beginning I did not even plan to expose Device::drvdata() at > > all, making the driver's device private data only available in bus device > > callbacks, etc. > > > > The reason why I added it was that it was required (and makes sense) when > > drivers interact with other drivers, e.g. through an auxiliary device. > > > > I did also mention this in the commit message of commit 6f61a2637abe ("rust: > > device: introduce Device::drvdata()"): > > > > (2) Having a direct accessor to the driver's private data is not > > commonly required (at least in Rust): Bus callback methods already > > provide access to the driver's device private data through a &self > > argument, while other driver entry points such as IRQs, > > workqueues, timers, IOCTLs, etc. have their own private data with > > separate ownership and lifetime. > > > > In other words, a driver's device private data is only relevant > > for driver model contexts (such a file private is only relevant > > for file contexts). > > > > Having that said, the motivation for accessing the driver's device > > private data with Device<Bound>::drvdata() are interactions between > > drivers. For instance, when an auxiliary driver calls back into its > > parent, the parent has to be capable to derive its private data from the > > corresponding device (i.e. the parent of the auxiliary device). > > > > Let's have a look at how probe() looks like for the example below, which is what > > we do in other subsystems, such as DRM or PWM. > > > > > Example 3: > > > > > > struct SampleIrqData { > > > rtc: ARef<rtc::Device>, > > > }; > > > > > > // The bus device private data. > > > #[pin_data] > > > struct SampleDriver { > > > #[pin] > > > irq: irq::Registration<SampleIrqHandler>, > > > rtc: ARef<rtc::Device<SampleRtcData>>, > > > } > > > > > > // The class device private data. > > > struct SampleRtcData { > > > io: Devres<IoMem<PL031_REG_SIZE>>, > > > hw_variant: VendorVariant, > > > } > > > > > > impl rtc::Ops for MyRtcOps { > > > type BusDeviceType = platform::Device<Bound>; > > > > > > fn read_time( > > > rtc: &rtc::Device<SampleRtcData> > > > parent: &platform::Device<Bound>, > > > time: &mut rtc::Time, > > > ) -> Result { > > > let io = rtc.io.access(parent)?; > > > > > > match rtc.hw_variant { > > > VendorVariant::Arm | VendorVariant::StV1 => { > > > let my_time = io.read(...); > > > > > > my_time.write_into(time); > > > }, > > > VendorVariant::StV2 => { ... }, > > > } > > > } > > > } > > > > > > > impl pci::Driver for SampleDriver { > > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > > let dev = pdev.as_ref(); > > > > let rtc_data = impl_pin_init!(SampleRtcData { > > io: iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > > hw_variant: VendorVariant::StV1, > > }); > > > > let rtc = rtc::Device::new(dev, rtc_data)?; > > > > // Internally calls `devres::register(rtc::Registration::new())`. > > rtc::Registration::register(rtc)?; > > > > Ok(impl_pin_init!(Self { > > // Give the IRQ handler a reference count of the `rtc::Device`. > > irq <- irq::Registration::new(..., rtc.clone()), > > rtc, > > }) > > } > > } > > > > With this there are no (subtle) ordering constraints the driver has to get > > right; ownership and lifetimes are well defined. > > > > (I.e. whatever order a driver picks, it either works properly or it does not > > compile in the first place, which is a huge improvement over the situation we > > have in C.) > > Thanks for all of this information and let me confirm my understanding. > > In both Example 1 and Example 3 there is a dependency between rtc's > read_time() callback and the PCI driver probe() because calling the > former before the latter is complete would be premature (and it would > fail). However, in Example 1 the compiler does not take that > dependency into account because rtc is not passed to the callback, > while in Example 3 the dependency is visible to the compiler and it > will refuse to compile the code if the dependency may be missed. > > If the above is correct, the current calling convention of RTC class > callbacks is problematic because it causes correctness checks done by > the Rust compiler to be bypassed (which may allow certain ordering > issues to be missed and functional problems to appear going forward). > > That's convincing and I think that adding an RTC device pointer to the > list of parameters of RTC class callbacks on the C side would not be a > very intrusive change. Arguably, it would be less intrusive than > replacing the parent device pointer with the RTC device pointer in > them. But there is no way the rtc callbacks will be called before the rtc is registered which will only happen in probe, when we have an actual device to talk to...
On 24/02/2026 01:12:32+0100, Danilo Krummrich wrote: > Let's have a look at how probe() looks like for the example below, which is what > we do in other subsystems, such as DRM or PWM. > > > Example 3: > > > > struct SampleIrqData { > > rtc: ARef<rtc::Device>, > > }; > > > > // The bus device private data. > > #[pin_data] > > struct SampleDriver { > > #[pin] > > irq: irq::Registration<SampleIrqHandler>, > > rtc: ARef<rtc::Device<SampleRtcData>>, > > } > > > > // The class device private data. > > struct SampleRtcData { > > io: Devres<IoMem<PL031_REG_SIZE>>, > > hw_variant: VendorVariant, > > } > > > > impl rtc::Ops for MyRtcOps { > > type BusDeviceType = platform::Device<Bound>; > > > > fn read_time( > > rtc: &rtc::Device<SampleRtcData> > > parent: &platform::Device<Bound>, > > time: &mut rtc::Time, > > ) -> Result { > > let io = rtc.io.access(parent)?; > > > > match rtc.hw_variant { > > VendorVariant::Arm | VendorVariant::StV1 => { > > let my_time = io.read(...); > > > > my_time.write_into(time); > > }, > > VendorVariant::StV2 => { ... }, > > } > > } > > } > > > > impl pci::Driver for SampleDriver { > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > let dev = pdev.as_ref(); > > let rtc_data = impl_pin_init!(SampleRtcData { > io: iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > // Internally calls `devres::register(rtc::Registration::new())`. > rtc::Registration::register(rtc)?; > > Ok(impl_pin_init!(Self { > // Give the IRQ handler a reference count of the `rtc::Device`. > irq <- irq::Registration::new(..., rtc.clone()), > rtc, > }) I can't really read rust yet but this seems to open a race condition with userspace if irq::Registration::new(...) fails, there is an ordering constraint you missed. > } > } > > With this there are no (subtle) ordering constraints the driver has to get > right; ownership and lifetimes are well defined. > > (I.e. whatever order a driver picks, it either works properly or it does not > compile in the first place, which is a huge improvement over the situation we > have in C.)
On Tue, Feb 24, 2026 at 3:57 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 24/02/2026 14:28:48+0100, Rafael J. Wysocki wrote: > > On Tue, Feb 24, 2026 at 1:12 AM Danilo Krummrich <dakr@kernel.org> wrote: > > > > > > On Sun Feb 22, 2026 at 5:13 PM CET, Danilo Krummrich wrote: > > > > There is no limitation from the Rust side of things, as mentioned in [1], this > > > > should work perfectly fine, but it might be slightly less convinient: > > > > > > When I wrote this on Sunday, I should have been much more precise about this, > > > since it actually does not work perfectly fine without further changes that > > > would (re-)introduce ordering constraints for drivers on the probe() side of > > > things that I want to avoid. > > > > > > The "re-" is in braces as we never had those ordering constraints in Rust, but > > > we have quite a lot of them in C, where they cause endless problems. Which is > > > why I want to avoid them in Rust. > > > > > > > Example 1: > > > > > > > > impl rtc::Ops for MyRtcOps { > > > > type BusDeviceType = platform::Device<Bound>; > > > > > > > > fn read_time( > > > > parent: &platform::Device<Bound>, > > > > time: &mut rtc::Time, > > > > ) -> Result { > > > > let drvdata = pdev.as_ref().drvdata::<MyDriver>()?; > > > > > > > > ... > > > > } > > > > } > > > > > > Let's have a look at the corresponding probe() side of things. > > > > > > struct SampleIrqData; > > > > > > // The bus device private data. > > > #[pin_data] > > > struct SampleDriver { > > > #[pin] > > > irq: irq::Registration<SampleIrqData>, > > > rtc: ARef<rtc::Device>, > > > ..., > > > } > > > > > > impl pci::Driver for SampleDriver { > > > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > > > let dev = pdev.as_ref(); > > > > > > let rtc = rtc::Device::new(dev)?; > > > > > > Ok(impl_pin_init!(Self { > > > irq <- irq::Registration::new(...), > > > rtc, > > > _: { > > > devres::register(rtc::Registration::new(rtc))?; > > > } > > > }) > > > } > > > } > > > > > > This is a problem, since even though the registration of the RTC device is the > > > last thing in the initializer, rtc::Ops::read_time() above could race and > > > Device::drvdata() could just fail as it might not be set yet. > > > > > > This is fixable, but - as mentioned - at the price of introducing ordering > > > constraints for drivers. > > > > > > For instance, this could be solved by introducing a pci::ProbeDevice<'a> type, > > > which ensures that pci::ProbeDevice::set_drvdata() can only be called from > > > probe() and can only be called once, as it consumes the pci::ProbeDevice. > > > > > > struct SampleIrqData; > > > > > > // The bus device private data. > > > #[pin_data] > > > struct SampleDriver { > > > #[pin] > > > irq: irq::Registration<SampleIrqData>, > > > rtc: ARef<rtc::Device>, > > > ..., > > > } > > > > > > impl pci::Driver for SampleDriver { > > > fn probe<'a>(pdev: pci::ProbeDevice<'a>, info: &Self::IdInfo) -> Result { > > > let dev = pdev.as_ref(); > > > > > > let rtc = rtc::Device::new(dev)?; > > > > > > let data = impl_pin_init!(Self { > > > irq <- irq::Registration::new(...), > > > rtc, > > > }); > > > > > > // The `pci::ProbeDevice` has been consumed by value, and > > > // returned a `&pci::Device<Core>`. > > > let pdev = pdev.set_drvdata(data)?; > > > > > > devres::register(rtc::Registration::new(rtc)) > > > } > > > } > > > > > > However, note how this wouldn't solve the same problem for irq::Registration, as > > > it lives within the the driver's device private data. > > > > > > If this would be C you would probably just partially initialize the driver's > > > device private data, but in Rust uninitialized data is not allowed for safety > > > reasons. > > > > > > We could probably find other ways to somehow handle this, e.g. by introducing > > > new device context states, additional callbacks, etc., but in the end it would > > > only be workarounds for not having defined ownership, that eventually ends up > > > being more complicated and more error prone. > > > > > > For this reason an irq::Registration has its own private data (which in this > > > example is just an empty struct). > > > > > > Similarly, all other kinds of registrations (and driver entry points) have their > > > own private data, such as class devices, work and workqueues, file operations, > > > timers, etc. > > > > > > I.e. there is a clear separation of ownership and lifetime, which makes more or > > > less (often more) suble ordering constraints obsolete. > > > > > > In fact, in the beginning I did not even plan to expose Device::drvdata() at > > > all, making the driver's device private data only available in bus device > > > callbacks, etc. > > > > > > The reason why I added it was that it was required (and makes sense) when > > > drivers interact with other drivers, e.g. through an auxiliary device. > > > > > > I did also mention this in the commit message of commit 6f61a2637abe ("rust: > > > device: introduce Device::drvdata()"): > > > > > > (2) Having a direct accessor to the driver's private data is not > > > commonly required (at least in Rust): Bus callback methods already > > > provide access to the driver's device private data through a &self > > > argument, while other driver entry points such as IRQs, > > > workqueues, timers, IOCTLs, etc. have their own private data with > > > separate ownership and lifetime. > > > > > > In other words, a driver's device private data is only relevant > > > for driver model contexts (such a file private is only relevant > > > for file contexts). > > > > > > Having that said, the motivation for accessing the driver's device > > > private data with Device<Bound>::drvdata() are interactions between > > > drivers. For instance, when an auxiliary driver calls back into its > > > parent, the parent has to be capable to derive its private data from the > > > corresponding device (i.e. the parent of the auxiliary device). > > > > > > Let's have a look at how probe() looks like for the example below, which is what > > > we do in other subsystems, such as DRM or PWM. > > > > > > > Example 3: > > > > > > > > struct SampleIrqData { > > > > rtc: ARef<rtc::Device>, > > > > }; > > > > > > > > // The bus device private data. > > > > #[pin_data] > > > > struct SampleDriver { > > > > #[pin] > > > > irq: irq::Registration<SampleIrqHandler>, > > > > rtc: ARef<rtc::Device<SampleRtcData>>, > > > > } > > > > > > > > // The class device private data. > > > > struct SampleRtcData { > > > > io: Devres<IoMem<PL031_REG_SIZE>>, > > > > hw_variant: VendorVariant, > > > > } > > > > > > > > impl rtc::Ops for MyRtcOps { > > > > type BusDeviceType = platform::Device<Bound>; > > > > > > > > fn read_time( > > > > rtc: &rtc::Device<SampleRtcData> > > > > parent: &platform::Device<Bound>, > > > > time: &mut rtc::Time, > > > > ) -> Result { > > > > let io = rtc.io.access(parent)?; > > > > > > > > match rtc.hw_variant { > > > > VendorVariant::Arm | VendorVariant::StV1 => { > > > > let my_time = io.read(...); > > > > > > > > my_time.write_into(time); > > > > }, > > > > VendorVariant::StV2 => { ... }, > > > > } > > > > } > > > > } > > > > > > > > > > impl pci::Driver for SampleDriver { > > > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > > > let dev = pdev.as_ref(); > > > > > > let rtc_data = impl_pin_init!(SampleRtcData { > > > io: iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > > > hw_variant: VendorVariant::StV1, > > > }); > > > > > > let rtc = rtc::Device::new(dev, rtc_data)?; > > > > > > // Internally calls `devres::register(rtc::Registration::new())`. > > > rtc::Registration::register(rtc)?; > > > > > > Ok(impl_pin_init!(Self { > > > // Give the IRQ handler a reference count of the `rtc::Device`. > > > irq <- irq::Registration::new(..., rtc.clone()), > > > rtc, > > > }) > > > } > > > } > > > > > > With this there are no (subtle) ordering constraints the driver has to get > > > right; ownership and lifetimes are well defined. > > > > > > (I.e. whatever order a driver picks, it either works properly or it does not > > > compile in the first place, which is a huge improvement over the situation we > > > have in C.) > > > > Thanks for all of this information and let me confirm my understanding. > > > > In both Example 1 and Example 3 there is a dependency between rtc's > > read_time() callback and the PCI driver probe() because calling the > > former before the latter is complete would be premature (and it would > > fail). However, in Example 1 the compiler does not take that > > dependency into account because rtc is not passed to the callback, > > while in Example 3 the dependency is visible to the compiler and it > > will refuse to compile the code if the dependency may be missed. > > > > If the above is correct, the current calling convention of RTC class > > callbacks is problematic because it causes correctness checks done by > > the Rust compiler to be bypassed (which may allow certain ordering > > issues to be missed and functional problems to appear going forward). > > > > That's convincing and I think that adding an RTC device pointer to the > > list of parameters of RTC class callbacks on the C side would not be a > > very intrusive change. Arguably, it would be less intrusive than > > replacing the parent device pointer with the RTC device pointer in > > them. > > But there is no way the rtc callbacks will be called before the rtc is > registered which will only happen in probe, when we have an actual > device to talk to... On the C side it really depends on what probe (of the parent) does. If it sets driver_data of the parent before registering the RTC device, then it'll all work. If it registers the RTC device before setting driver_data for the parent, there may be a race. IIUC, the idea on the Rust side is to use the capabilities of the compiler to eliminate the possibility of messing up the ordering in a driver.
On Tue Feb 24, 2026 at 4:23 PM CET, Rafael J. Wysocki wrote: > On the C side it really depends on what probe (of the parent) does. > If it sets driver_data of the parent before registering the RTC > device, then it'll all work. If it registers the RTC device before > setting driver_data for the parent, there may be a race. > > IIUC, the idea on the Rust side is to use the capabilities of the > compiler to eliminate the possibility of messing up the ordering in a > driver. Yes, this is all spot-on.
On Tue Feb 24, 2026 at 4:01 PM CET, Alexandre Belloni wrote: > On 24/02/2026 01:12:32+0100, Danilo Krummrich wrote: >> impl pci::Driver for SampleDriver { >> fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { >> let dev = pdev.as_ref(); >> >> let rtc_data = impl_pin_init!(SampleRtcData { >> io: iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, >> hw_variant: VendorVariant::StV1, >> }); >> >> let rtc = rtc::Device::new(dev, rtc_data)?; >> >> // Internally calls `devres::register(rtc::Registration::new())`. >> rtc::Registration::register(rtc)?; >> >> Ok(impl_pin_init!(Self { >> // Give the IRQ handler a reference count of the `rtc::Device`. >> irq <- irq::Registration::new(..., rtc.clone()), >> rtc, >> }) > > I can't really read rust yet but this seems to open a race condition > with userspace if irq::Registration::new(...) fails, there is an > ordering constraint you missed. (I did not have any specific hardware in mind when sketching this up (e.g. an IRQ could also only be needed in bus device callbacks, e.g. for loading firmware etc.). But for RTC it obviously is common that it is relevant to the class device too.) So, I assume you mean because there could already be an ioctl before the IRQ has been successfully registered, and this ioctl may wait for an IRQ? In this case the irq::Registration should go into rtc_data instead to account for this dependency. Unfortunately, this is a semantic dependency that we can't always catch at compile time. The reason we sometimes can is because, if you would need access to the irq::Registration from ioctls (e.g. for calling synchronize(), enable(), disable() etc.) it would be caught, because you couldn't access it without it being in rtc_data in the first place, and being forced to have it in rtc_data guarantees that the ordering can't be wrong.
On Tue Feb 24, 2026 at 5:35 PM CET, Danilo Krummrich wrote: > The reason we sometimes can is because, if you would need access to the > irq::Registration from ioctls (e.g. for calling synchronize(), enable(), > disable() etc.) it would be caught, because you couldn't access it without it > being in rtc_data in the first place, and being forced to have it in rtc_data > guarantees that the ordering can't be wrong. I.e. as long as we can model a data dependency we can catch those things at compile time.
On 24/02/2026 17:35:23+0100, Danilo Krummrich wrote: > On Tue Feb 24, 2026 at 4:01 PM CET, Alexandre Belloni wrote: > > On 24/02/2026 01:12:32+0100, Danilo Krummrich wrote: > >> impl pci::Driver for SampleDriver { > >> fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > >> let dev = pdev.as_ref(); > >> > >> let rtc_data = impl_pin_init!(SampleRtcData { > >> io: iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > >> hw_variant: VendorVariant::StV1, > >> }); > >> > >> let rtc = rtc::Device::new(dev, rtc_data)?; > >> > >> // Internally calls `devres::register(rtc::Registration::new())`. > >> rtc::Registration::register(rtc)?; > >> > >> Ok(impl_pin_init!(Self { > >> // Give the IRQ handler a reference count of the `rtc::Device`. > >> irq <- irq::Registration::new(..., rtc.clone()), > >> rtc, > >> }) > > > > I can't really read rust yet but this seems to open a race condition > > with userspace if irq::Registration::new(...) fails, there is an > > ordering constraint you missed. > > (I did not have any specific hardware in mind when sketching this up (e.g. an > IRQ could also only be needed in bus device callbacks, e.g. for loading firmware > etc.). But for RTC it obviously is common that it is relevant to the class > device too.) > > So, I assume you mean because there could already be an ioctl before the IRQ has > been successfully registered, and this ioctl may wait for an IRQ? > > In this case the irq::Registration should go into rtc_data instead to account > for this dependency. Unfortunately, this is a semantic dependency that we can't > always catch at compile time. > > The reason we sometimes can is because, if you would need access to the > irq::Registration from ioctls (e.g. for calling synchronize(), enable(), > disable() etc.) it would be caught, because you couldn't access it without it > being in rtc_data in the first place, and being forced to have it in rtc_data > guarantees that the ordering can't be wrong. No, once you register the rtc, the character device will appear in userspace and may be opened, at this point, probe is not allowed to fail anymore which you are allowing by trying to register the IRQ so late.
On Tue Feb 24, 2026 at 6:28 PM CET, Alexandre Belloni wrote: > On 24/02/2026 17:35:23+0100, Danilo Krummrich wrote: >> (I did not have any specific hardware in mind when sketching this up (e.g. an >> IRQ could also only be needed in bus device callbacks, e.g. for loading firmware >> etc.). But for RTC it obviously is common that it is relevant to the class >> device too.) >> >> So, I assume you mean because there could already be an ioctl before the IRQ has >> been successfully registered, and this ioctl may wait for an IRQ? >> >> In this case the irq::Registration should go into rtc_data instead to account >> for this dependency. Unfortunately, this is a semantic dependency that we can't >> always catch at compile time. >> >> The reason we sometimes can is because, if you would need access to the >> irq::Registration from ioctls (e.g. for calling synchronize(), enable(), >> disable() etc.) it would be caught, because you couldn't access it without it >> being in rtc_data in the first place, and being forced to have it in rtc_data >> guarantees that the ordering can't be wrong. > > No, once you register the rtc, the character device will appear in > userspace and may be opened, at this point, probe is not allowed to fail > anymore which you are allowing by trying to register the IRQ so late. This does not seem to correspond to my previous reply -- may I kindly ask you to read it again? Here's also some sketched up code for what I wrote above: fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { let dev = pdev.as_ref(); let rtc_data = impl_pin_init!(SampleRtcData { io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, hw_variant: VendorVariant::StV1, irq <- irq::Registration::new(...), }); let rtc = rtc::Device::new(dev, rtc_data)?; rtc::Registration::register(rtc)?; Ok(Self { rtc }) } Note that if any of the RTC callbacks would ever need to call irq.synchronize(), irq.disable(), etc. the compiler would enforce correct ordering, as there would not be any other possibility to put the irq::Registration other than into the rtc_data that goes into rtc::Device::new(). Besides that, you above mentioned "probe is not allowed to fail anymore" after the RTC device is registered and the corresponding character device becomes visible to userspace. While there most likely isn't any good reason for probe() to fail afterwards for RTC devices, it is not the case that this isn't allowed. We generally can unwind from a class device registration. In fact, this is not different to remove() being called (immediately). Imagine a case where a driver registers multiple class devices, or a class device and an auxiliary device, etc. (But I assume your point was more that for an RTC device specifically this would be odd or uncommon.)
On 24/02/2026 23:23:29+0100, Danilo Krummrich wrote: > On Tue Feb 24, 2026 at 6:28 PM CET, Alexandre Belloni wrote: > > On 24/02/2026 17:35:23+0100, Danilo Krummrich wrote: > >> (I did not have any specific hardware in mind when sketching this up (e.g. an > >> IRQ could also only be needed in bus device callbacks, e.g. for loading firmware > >> etc.). But for RTC it obviously is common that it is relevant to the class > >> device too.) > >> > >> So, I assume you mean because there could already be an ioctl before the IRQ has > >> been successfully registered, and this ioctl may wait for an IRQ? > >> > >> In this case the irq::Registration should go into rtc_data instead to account > >> for this dependency. Unfortunately, this is a semantic dependency that we can't > >> always catch at compile time. > >> > >> The reason we sometimes can is because, if you would need access to the > >> irq::Registration from ioctls (e.g. for calling synchronize(), enable(), > >> disable() etc.) it would be caught, because you couldn't access it without it > >> being in rtc_data in the first place, and being forced to have it in rtc_data > >> guarantees that the ordering can't be wrong. > > > > No, once you register the rtc, the character device will appear in > > userspace and may be opened, at this point, probe is not allowed to fail > > anymore which you are allowing by trying to register the IRQ so late. > > This does not seem to correspond to my previous reply -- may I kindly ask you to > read it again? > > Here's also some sketched up code for what I wrote above: > > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > let dev = pdev.as_ref(); > > let rtc_data = impl_pin_init!(SampleRtcData { > io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > irq <- irq::Registration::new(...), > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > rtc::Registration::register(rtc)?; > > Ok(Self { rtc }) > } > > Note that if any of the RTC callbacks would ever need to call irq.synchronize(), > irq.disable(), etc. the compiler would enforce correct ordering, as there would > not be any other possibility to put the irq::Registration other than into the > rtc_data that goes into rtc::Device::new(). Right but again, the issue is not about the irq or resource allocation ordering, it is about probe failing after the character device creation. > > Besides that, you above mentioned "probe is not allowed to fail anymore" after > the RTC device is registered and the corresponding character device becomes > visible to userspace. > > While there most likely isn't any good reason for probe() to fail afterwards for > RTC devices, it is not the case that this isn't allowed. We generally can unwind > from a class device registration. In fact, this is not different to remove() > being called (immediately). It is actually different, this was the race back then: CPU0: CPU1: sys_load_module() do_init_module() do_one_initcall() cmos_do_probe() rtc_device_register() __register_chrdev() cdev->owner = struct module* open("/dev/rtc0") rtc_device_unregister() module_put() free_module() module_free(mod->module_core) /* struct module *module is now freed */ chrdev_open() spin_lock(cdev_lock) cdev_get() try_module_get() module_is_live() /* dereferences already freed struct module* */ I don't think it has been solved since then. > > Imagine a case where a driver registers multiple class devices, or a class > device and an auxiliary device, etc. > > (But I assume your point was more that for an RTC device specifically this would > be odd or uncommon.)
On 2026-02-24 22:44, Alexandre Belloni wrote: > On 24/02/2026 23:23:29+0100, Danilo Krummrich wrote: >> On Tue Feb 24, 2026 at 6:28 PM CET, Alexandre Belloni wrote: >> > On 24/02/2026 17:35:23+0100, Danilo Krummrich wrote: >> >> (I did not have any specific hardware in mind when sketching this up (e.g. an >> >> IRQ could also only be needed in bus device callbacks, e.g. for loading firmware >> >> etc.). But for RTC it obviously is common that it is relevant to the class >> >> device too.) >> >> >> >> So, I assume you mean because there could already be an ioctl before the IRQ has >> >> been successfully registered, and this ioctl may wait for an IRQ? >> >> >> >> In this case the irq::Registration should go into rtc_data instead to account >> >> for this dependency. Unfortunately, this is a semantic dependency that we can't >> >> always catch at compile time. >> >> >> >> The reason we sometimes can is because, if you would need access to the >> >> irq::Registration from ioctls (e.g. for calling synchronize(), enable(), >> >> disable() etc.) it would be caught, because you couldn't access it without it >> >> being in rtc_data in the first place, and being forced to have it in rtc_data >> >> guarantees that the ordering can't be wrong. >> > >> > No, once you register the rtc, the character device will appear in >> > userspace and may be opened, at this point, probe is not allowed to fail >> > anymore which you are allowing by trying to register the IRQ so late. >> >> This does not seem to correspond to my previous reply -- may I kindly ask you to >> read it again? >> >> Here's also some sketched up code for what I wrote above: >> >> fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { >> let dev = pdev.as_ref(); >> >> let rtc_data = impl_pin_init!(SampleRtcData { >> io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, >> hw_variant: VendorVariant::StV1, >> irq <- irq::Registration::new(...), >> }); >> >> let rtc = rtc::Device::new(dev, rtc_data)?; >> >> rtc::Registration::register(rtc)?; >> >> Ok(Self { rtc }) >> } >> >> Note that if any of the RTC callbacks would ever need to call irq.synchronize(), >> irq.disable(), etc. the compiler would enforce correct ordering, as there would >> not be any other possibility to put the irq::Registration other than into the >> rtc_data that goes into rtc::Device::new(). > > Right but again, the issue is not about the irq or resource allocation > ordering, it is about probe failing after the character device creation. > >> >> Besides that, you above mentioned "probe is not allowed to fail anymore" after >> the RTC device is registered and the corresponding character device becomes >> visible to userspace. >> >> While there most likely isn't any good reason for probe() to fail afterwards for >> RTC devices, it is not the case that this isn't allowed. We generally can unwind >> from a class device registration. In fact, this is not different to remove() >> being called (immediately). > > It is actually different, this was the race back then: > > CPU0: CPU1: > sys_load_module() > do_init_module() > do_one_initcall() > cmos_do_probe() > rtc_device_register() > __register_chrdev() > cdev->owner = struct module* > open("/dev/rtc0") > rtc_device_unregister() > module_put() > free_module() > module_free(mod->module_core) > /* struct module *module is now > freed */ > chrdev_open() > spin_lock(cdev_lock) > cdev_get() > try_module_get() > module_is_live() > /* dereferences already > freed struct module* */ > > > I don't think it has been solved since then. I think it is not realistic to require module init to always complete once a char dev is registered. What if you're trying to register multiple char devs? I think this requires that either `rtc_device_unregister` to wait until all opened fds on the char dev to be closed before returning, or a reference count to the module is kept. (Or having the file ops being swapped out so further operation on the fd doesn't hit the module anymore). Anyhow, this looks exactly what all the driver-core revocable discussion is about. Best, Gary
On Tue, Feb 24, 2026 at 11:23 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Tue Feb 24, 2026 at 6:28 PM CET, Alexandre Belloni wrote: > > On 24/02/2026 17:35:23+0100, Danilo Krummrich wrote: > >> (I did not have any specific hardware in mind when sketching this up (e.g. an > >> IRQ could also only be needed in bus device callbacks, e.g. for loading firmware > >> etc.). But for RTC it obviously is common that it is relevant to the class > >> device too.) > >> > >> So, I assume you mean because there could already be an ioctl before the IRQ has > >> been successfully registered, and this ioctl may wait for an IRQ? > >> > >> In this case the irq::Registration should go into rtc_data instead to account > >> for this dependency. Unfortunately, this is a semantic dependency that we can't > >> always catch at compile time. > >> > >> The reason we sometimes can is because, if you would need access to the > >> irq::Registration from ioctls (e.g. for calling synchronize(), enable(), > >> disable() etc.) it would be caught, because you couldn't access it without it > >> being in rtc_data in the first place, and being forced to have it in rtc_data > >> guarantees that the ordering can't be wrong. > > > > No, once you register the rtc, the character device will appear in > > userspace and may be opened, at this point, probe is not allowed to fail > > anymore which you are allowing by trying to register the IRQ so late. > > This does not seem to correspond to my previous reply -- may I kindly ask you to > read it again? > > Here's also some sketched up code for what I wrote above: > > fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > let dev = pdev.as_ref(); > > let rtc_data = impl_pin_init!(SampleRtcData { > io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > irq <- irq::Registration::new(...), > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > rtc::Registration::register(rtc)?; > > Ok(Self { rtc }) > } > > Note that if any of the RTC callbacks would ever need to call irq.synchronize(), > irq.disable(), etc. the compiler would enforce correct ordering, as there would > not be any other possibility to put the irq::Registration other than into the > rtc_data that goes into rtc::Device::new(). IIUC, the interrupt handler can only access the rtc_data because the parent's driver_data may not exist yet when it runs. Or am I missing something?
On Wed Feb 25, 2026 at 2:33 PM CET, Rafael J. Wysocki wrote: > On Tue, Feb 24, 2026 at 11:23 PM Danilo Krummrich <dakr@kernel.org> wrote: >> Here's also some sketched up code for what I wrote above: >> >> fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { >> let dev = pdev.as_ref(); >> >> let rtc_data = impl_pin_init!(SampleRtcData { >> io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, >> hw_variant: VendorVariant::StV1, >> irq <- irq::Registration::new(...), >> }); >> >> let rtc = rtc::Device::new(dev, rtc_data)?; >> >> rtc::Registration::register(rtc)?; >> >> Ok(Self { rtc }) >> } >> >> Note that if any of the RTC callbacks would ever need to call irq.synchronize(), >> irq.disable(), etc. the compiler would enforce correct ordering, as there would >> not be any other possibility to put the irq::Registration other than into the >> rtc_data that goes into rtc::Device::new(). > > IIUC, the interrupt handler can only access the rtc_data because the > parent's driver_data may not exist yet when it runs. Or am I missing > something? In the code above the IRQ handler can also not access rtc_data, as struct SampleRtcData might not be fully initialized when it runs, i.e. let rtc_data = impl_pin_init!(SampleRtcData { io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, hw_variant: VendorVariant::StV1, irq <- irq::Registration::new(..., rtc_data), }); would not compile in the first place. irq::Registration, for this purpose, has its own private data on the handler itself, see also [1]. In fact, the C code has the same concept with the dev_id argument in request_threaded_irq() [2]. The difference is that the C compiler does not ensure that the IRQ handler actually owns the data behind the dev_id pointer. I.e. the driver has to somehow ensure that whatever is behind the dev_id pointer remains valid for the duration the IRQ handler is registered. In the Rust implementation the compiler does ensure that what is behind the dev_id pointer remains valid for the duration of the lifetime of the irq::Registration. Having that said, I assume you wonder what we would pass into the irq::Registration instead, if it is not rtc_data. The answer is it depends; it depends on what's actually needed, what other entities interact with the IRQ (e.g. some scheduled work, etc.) and maybe even preference to some extend. Here is one example: let irq_data = impl_pin_init!(SampleIrqData { io <- pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, hw_variant: VendorVariant::StV1, }); let rtc_data = impl_pin_init!(SampleRtcData { irq <- irq::Registration::new(..., irq_data), ..., }); let rtc = rtc::Device::new(dev, rtc_data)?; This would compile as it ensures that irq_data (struct SampleIrqData) is fully initialized before irq::Registration::new() is called. At a first glance this might look like we need an additional allocation, one for irq_data and one for rtc_data, but that is not the case. irq_data is an initializer that is passed to another initializer, i.e. rtc_data is still an initializer. The actual (single) allocation happens in rtc::Device::new(). In terms of accessing it through the the rtc::Device in an RTC device callback, we would likely use accessor methods to make it a bit more convinient, i.e. fn read_time( rtc: &rtc::Device<SampleRtcData> parent: &platform::Device<Bound>, time: &mut rtc::Time, ) -> Result { let io = rtc.io().access(parent)?; match rtc.hw_variant() { VendorVariant::Arm | VendorVariant::StV1 => { let my_time = io.read(...); my_time.write_into(time); }, VendorVariant::StV2 => { ... }, } } As mentioned above there are a few other options to implement this, depending on what's required, etc. For instance, if the I/O bar is actually shared between multiple entities we might want to initialize it within an Arc [3] (reference count it) for shared ownership. For the future we will also be able to support references within initializers to other pinned fields, which make things a bit more convinient, so you could do things like this: let irq_data = impl_pin_init!(SampleIrqData { io <- pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, hw_variant: VendorVariant::StV1, }); let rtc_data = impl_pin_init!(SampleRtcData { irq <- irq::Registration::new(..., irq_data), io: &irq.io, ..., }); let rtc = rtc::Device::new(dev, rtc_data)?; Note the additional `io: &irq.io,` in the rtc_data initializer. This would be legal as we know that `irq` is pinned within `rtc_data`, hence it is valid to hold a reference to one of its pinned fields. I am not sure how far we are from having this supported, I assume Benno and Gary can say more about this. I hope this helps, and thanks for asking those questions! [1] https://rust.docs.kernel.org/kernel/irq/struct.Registration.html [2] https://elixir.bootlin.com/linux/v6.19.3/source/kernel/irq/manage.c#L2090 [3] https://rust.docs.kernel.org/kernel/sync/struct.Arc.html
On Wed, Feb 25, 2026 at 5:26 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed Feb 25, 2026 at 2:33 PM CET, Rafael J. Wysocki wrote: > > On Tue, Feb 24, 2026 at 11:23 PM Danilo Krummrich <dakr@kernel.org> wrote: > >> Here's also some sketched up code for what I wrote above: > >> > >> fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > >> let dev = pdev.as_ref(); > >> > >> let rtc_data = impl_pin_init!(SampleRtcData { > >> io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > >> hw_variant: VendorVariant::StV1, > >> irq <- irq::Registration::new(...), > >> }); > >> > >> let rtc = rtc::Device::new(dev, rtc_data)?; > >> > >> rtc::Registration::register(rtc)?; > >> > >> Ok(Self { rtc }) > >> } > >> > >> Note that if any of the RTC callbacks would ever need to call irq.synchronize(), > >> irq.disable(), etc. the compiler would enforce correct ordering, as there would > >> not be any other possibility to put the irq::Registration other than into the > >> rtc_data that goes into rtc::Device::new(). > > > > IIUC, the interrupt handler can only access the rtc_data because the > > parent's driver_data may not exist yet when it runs. Or am I missing > > something? > > In the code above the IRQ handler can also not access rtc_data, as struct > SampleRtcData might not be fully initialized when it runs, i.e. > > let rtc_data = impl_pin_init!(SampleRtcData { > io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > irq <- irq::Registration::new(..., rtc_data), > }); > > would not compile in the first place. > > irq::Registration, for this purpose, has its own private data on the handler > itself, see also [1]. In fact, the C code has the same concept with the dev_id > argument in request_threaded_irq() [2]. > > The difference is that the C compiler does not ensure that the IRQ handler > actually owns the data behind the dev_id pointer. I.e. the driver has to somehow > ensure that whatever is behind the dev_id pointer remains valid for the duration > the IRQ handler is registered. > > In the Rust implementation the compiler does ensure that what is behind the > dev_id pointer remains valid for the duration of the lifetime of the > irq::Registration. > > Having that said, I assume you wonder what we would pass into the > irq::Registration instead, if it is not rtc_data. > > The answer is it depends; it depends on what's actually needed, what other > entities interact with the IRQ (e.g. some scheduled work, etc.) and maybe even > preference to some extend. > > Here is one example: > > let irq_data = impl_pin_init!(SampleIrqData { > io <- pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > }); > > let rtc_data = impl_pin_init!(SampleRtcData { > irq <- irq::Registration::new(..., irq_data), > ..., > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > This would compile as it ensures that irq_data (struct SampleIrqData) is fully > initialized before irq::Registration::new() is called. > > At a first glance this might look like we need an additional allocation, one for > irq_data and one for rtc_data, but that is not the case. irq_data is an > initializer that is passed to another initializer, i.e. rtc_data is still an > initializer. > > The actual (single) allocation happens in rtc::Device::new(). > > In terms of accessing it through the the rtc::Device in an RTC device callback, > we would likely use accessor methods to make it a bit more convinient, i.e. > > fn read_time( > rtc: &rtc::Device<SampleRtcData> > parent: &platform::Device<Bound>, > time: &mut rtc::Time, > ) -> Result { > let io = rtc.io().access(parent)?; > > match rtc.hw_variant() { > VendorVariant::Arm | VendorVariant::StV1 => { > let my_time = io.read(...); > > my_time.write_into(time); > }, > VendorVariant::StV2 => { ... }, > } > } > > As mentioned above there are a few other options to implement this, depending on > what's required, etc. > > For instance, if the I/O bar is actually shared between multiple entities we > might want to initialize it within an Arc [3] (reference count it) for shared > ownership. > > For the future we will also be able to support references within initializers to > other pinned fields, which make things a bit more convinient, so you could do > things like this: > > let irq_data = impl_pin_init!(SampleIrqData { > io <- pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > }); > > let rtc_data = impl_pin_init!(SampleRtcData { > irq <- irq::Registration::new(..., irq_data), > io: &irq.io, > ..., > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > Note the additional `io: &irq.io,` in the rtc_data initializer. This would be > legal as we know that `irq` is pinned within `rtc_data`, hence it is valid to > hold a reference to one of its pinned fields. > > I am not sure how far we are from having this supported, I assume Benno and Gary > can say more about this. > > I hope this helps, and thanks for asking those questions! Well, I'm using an opportunity to learn something, thank you! > [1] https://rust.docs.kernel.org/kernel/irq/struct.Registration.html > [2] https://elixir.bootlin.com/linux/v6.19.3/source/kernel/irq/manage.c#L2090 > [3] https://rust.docs.kernel.org/kernel/sync/struct.Arc.html
On Wed, Feb 25, 2026 at 5:26 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed Feb 25, 2026 at 2:33 PM CET, Rafael J. Wysocki wrote: > > On Tue, Feb 24, 2026 at 11:23 PM Danilo Krummrich <dakr@kernel.org> wrote: > >> Here's also some sketched up code for what I wrote above: > >> > >> fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> impl PinInit<Self, Error> { > >> let dev = pdev.as_ref(); > >> > >> let rtc_data = impl_pin_init!(SampleRtcData { > >> io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > >> hw_variant: VendorVariant::StV1, > >> irq <- irq::Registration::new(...), > >> }); > >> > >> let rtc = rtc::Device::new(dev, rtc_data)?; > >> > >> rtc::Registration::register(rtc)?; > >> > >> Ok(Self { rtc }) > >> } > >> > >> Note that if any of the RTC callbacks would ever need to call irq.synchronize(), > >> irq.disable(), etc. the compiler would enforce correct ordering, as there would > >> not be any other possibility to put the irq::Registration other than into the > >> rtc_data that goes into rtc::Device::new(). > > > > IIUC, the interrupt handler can only access the rtc_data because the > > parent's driver_data may not exist yet when it runs. Or am I missing > > something? > > In the code above the IRQ handler can also not access rtc_data, as struct > SampleRtcData might not be fully initialized when it runs, i.e. > > let rtc_data = impl_pin_init!(SampleRtcData { > io: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > irq <- irq::Registration::new(..., rtc_data), > }); > > would not compile in the first place. > > irq::Registration, for this purpose, has its own private data on the handler > itself, see also [1]. In fact, the C code has the same concept with the dev_id > argument in request_threaded_irq() [2]. > > The difference is that the C compiler does not ensure that the IRQ handler > actually owns the data behind the dev_id pointer. I.e. the driver has to somehow > ensure that whatever is behind the dev_id pointer remains valid for the duration > the IRQ handler is registered. > > In the Rust implementation the compiler does ensure that what is behind the > dev_id pointer remains valid for the duration of the lifetime of the > irq::Registration. > > Having that said, I assume you wonder what we would pass into the > irq::Registration instead, if it is not rtc_data. > > The answer is it depends; it depends on what's actually needed, what other > entities interact with the IRQ (e.g. some scheduled work, etc.) and maybe even > preference to some extend. > > Here is one example: > > let irq_data = impl_pin_init!(SampleIrqData { > io <- pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > }); > > let rtc_data = impl_pin_init!(SampleRtcData { > irq <- irq::Registration::new(..., irq_data), > ..., > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > This would compile as it ensures that irq_data (struct SampleIrqData) is fully > initialized before irq::Registration::new() is called. > > At a first glance this might look like we need an additional allocation, one for > irq_data and one for rtc_data, but that is not the case. irq_data is an > initializer that is passed to another initializer, i.e. rtc_data is still an > initializer. > > The actual (single) allocation happens in rtc::Device::new(). I think that the key observation here is that C and Rust are substantially different with respect to how things get initialized. In C, we first allocate memory, then initialize it, and then start services that will refer to it. All of these steps need to be taken explicitly and separately in the right order and the compiler simply processes the high-level language into CPU instructions. On the way out, all of that needs to be cleaned up directly, most of the time in reverse order. If anything is missed or forgotten, or the ordering is messed up, troubles ensue. In Rust, IIUC, the compiler is essentially told about what data will be there in the memory, how to initialize it and what services to start and all of that happens in one go when memory gets allocated (so, apparently, a good part of the code doesn't even produce CPU instructions at all, as it is all about feeding the information to the compiler). So long as the compiler has complete information, it can figure out the right ordering automatically and it will complain if something is not right. The difficulty here is to find a way to provide the compiler with complete information. > In terms of accessing it through the the rtc::Device in an RTC device callback, > we would likely use accessor methods to make it a bit more convinient, i.e. > > fn read_time( > rtc: &rtc::Device<SampleRtcData> > parent: &platform::Device<Bound>, > time: &mut rtc::Time, > ) -> Result { > let io = rtc.io().access(parent)?; > > match rtc.hw_variant() { > VendorVariant::Arm | VendorVariant::StV1 => { > let my_time = io.read(...); > > my_time.write_into(time); > }, > VendorVariant::StV2 => { ... }, > } > } > > As mentioned above there are a few other options to implement this, depending on > what's required, etc. > > For instance, if the I/O bar is actually shared between multiple entities we > might want to initialize it within an Arc [3] (reference count it) for shared > ownership. > > For the future we will also be able to support references within initializers to > other pinned fields, which make things a bit more convinient, so you could do > things like this: > > let irq_data = impl_pin_init!(SampleIrqData { > io <- pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > }); > > let rtc_data = impl_pin_init!(SampleRtcData { > irq <- irq::Registration::new(..., irq_data), > io: &irq.io, > ..., > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > Note the additional `io: &irq.io,` in the rtc_data initializer. This would be > legal as we know that `irq` is pinned within `rtc_data`, hence it is valid to > hold a reference to one of its pinned fields. > > I am not sure how far we are from having this supported, I assume Benno and Gary > can say more about this. > > I hope this helps, and thanks for asking those questions! > > [1] https://rust.docs.kernel.org/kernel/irq/struct.Registration.html > [2] https://elixir.bootlin.com/linux/v6.19.3/source/kernel/irq/manage.c#L2090 > [3] https://rust.docs.kernel.org/kernel/sync/struct.Arc.html
On Wed Feb 25, 2026 at 5:26 PM CET, Danilo Krummrich wrote: > For the future we will also be able to support references within initializers to > other pinned fields, which make things a bit more convinient, so you could do > things like this: > > let irq_data = impl_pin_init!(SampleIrqData { > io <- pdev.iomap_region_sized::<BAR0_SIZE>(0, c"my_rtc/bar0")?, > hw_variant: VendorVariant::StV1, > }); > > let rtc_data = impl_pin_init!(SampleRtcData { > irq <- irq::Registration::new(..., irq_data), > io: &irq.io, > ..., > }); > > let rtc = rtc::Device::new(dev, rtc_data)?; > > Note the additional `io: &irq.io,` in the rtc_data initializer. This would be > legal as we know that `irq` is pinned within `rtc_data`, hence it is valid to > hold a reference to one of its pinned fields. > > I am not sure how far we are from having this supported, I assume Benno and Gary > can say more about this. We added that support in 42415d163e5d ("rust: pin-init: add references to previously initialized fields"). Note that any references created during the initializer are only valid inside of that initializer. You can convert into a raw pointer though and if the struct is `!Unpin` it is sound to use that pointer at the same time as references to the pinned value. Cheers, Benno
diff --git a/drivers/rtc/dev.c b/drivers/rtc/dev.c index baf1a8ca8b2b1..eddcc5a69db3b 100644 --- a/drivers/rtc/dev.c +++ b/drivers/rtc/dev.c @@ -410,7 +410,7 @@ static long rtc_dev_ioctl(struct file *file, } default: if (rtc->ops->param_get) - err = rtc->ops->param_get(rtc->dev.parent, ¶m); + err = rtc->ops->param_get(rtc_ops_dev(rtc), ¶m); else err = -EINVAL; } @@ -440,7 +440,7 @@ static long rtc_dev_ioctl(struct file *file, default: if (rtc->ops->param_set) - err = rtc->ops->param_set(rtc->dev.parent, ¶m); + err = rtc->ops->param_set(rtc_ops_dev(rtc), ¶m); else err = -EINVAL; } @@ -450,7 +450,7 @@ static long rtc_dev_ioctl(struct file *file, default: /* Finally try the driver's ioctl interface */ if (ops->ioctl) { - err = ops->ioctl(rtc->dev.parent, cmd, arg); + err = ops->ioctl(rtc_ops_dev(rtc), cmd, arg); if (err == -ENOIOCTLCMD) err = -ENOTTY; } else { diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index b8b298efd9a9c..4c81130fb0394 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -91,7 +91,7 @@ static int __rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm) err = -EINVAL; } else { memset(tm, 0, sizeof(struct rtc_time)); - err = rtc->ops->read_time(rtc->dev.parent, tm); + err = rtc->ops->read_time(rtc_ops_dev(rtc), tm); if (err < 0) { dev_dbg(&rtc->dev, "read_time: fail to read: %d\n", err); @@ -155,7 +155,7 @@ int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm) if (!rtc->ops) err = -ENODEV; else if (rtc->ops->set_time) - err = rtc->ops->set_time(rtc->dev.parent, tm); + err = rtc->ops->set_time(rtc_ops_dev(rtc), tm); else err = -EINVAL; @@ -200,7 +200,7 @@ static int rtc_read_alarm_internal(struct rtc_device *rtc, alarm->time.tm_wday = -1; alarm->time.tm_yday = -1; alarm->time.tm_isdst = -1; - err = rtc->ops->read_alarm(rtc->dev.parent, alarm); + err = rtc->ops->read_alarm(rtc_ops_dev(rtc), alarm); } mutex_unlock(&rtc->ops_lock); @@ -441,7 +441,7 @@ static int __rtc_set_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm) else if (!test_bit(RTC_FEATURE_ALARM, rtc->features)) err = -EINVAL; else - err = rtc->ops->set_alarm(rtc->dev.parent, alarm); + err = rtc->ops->set_alarm(rtc_ops_dev(rtc), alarm); /* * Check for potential race described above. If the waiting for next @@ -568,7 +568,7 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled) else if (!test_bit(RTC_FEATURE_ALARM, rtc->features) || !rtc->ops->alarm_irq_enable) err = -EINVAL; else - err = rtc->ops->alarm_irq_enable(rtc->dev.parent, enabled); + err = rtc->ops->alarm_irq_enable(rtc_ops_dev(rtc), enabled); mutex_unlock(&rtc->ops_lock); @@ -618,7 +618,7 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled) rtc->uie_rtctimer.period = ktime_set(1, 0); err = rtc_timer_enqueue(rtc, &rtc->uie_rtctimer); if (!err && rtc->ops && rtc->ops->alarm_irq_enable) - err = rtc->ops->alarm_irq_enable(rtc->dev.parent, 1); + err = rtc->ops->alarm_irq_enable(rtc_ops_dev(rtc), 1); if (err) goto out; } else { @@ -874,7 +874,7 @@ static void rtc_alarm_disable(struct rtc_device *rtc) if (!rtc->ops || !test_bit(RTC_FEATURE_ALARM, rtc->features) || !rtc->ops->alarm_irq_enable) return; - rtc->ops->alarm_irq_enable(rtc->dev.parent, false); + rtc->ops->alarm_irq_enable(rtc_ops_dev(rtc), false); trace_rtc_alarm_irq_enable(0, 0); } @@ -1076,7 +1076,7 @@ int rtc_read_offset(struct rtc_device *rtc, long *offset) return -EINVAL; mutex_lock(&rtc->ops_lock); - ret = rtc->ops->read_offset(rtc->dev.parent, offset); + ret = rtc->ops->read_offset(rtc_ops_dev(rtc), offset); mutex_unlock(&rtc->ops_lock); trace_rtc_read_offset(*offset, ret); @@ -1111,7 +1111,7 @@ int rtc_set_offset(struct rtc_device *rtc, long offset) return -EINVAL; mutex_lock(&rtc->ops_lock); - ret = rtc->ops->set_offset(rtc->dev.parent, offset); + ret = rtc->ops->set_offset(rtc_ops_dev(rtc), offset); mutex_unlock(&rtc->ops_lock); trace_rtc_set_offset(offset, ret); diff --git a/drivers/rtc/proc.c b/drivers/rtc/proc.c index cbcdbb19d848e..bf688079d0fbb 100644 --- a/drivers/rtc/proc.c +++ b/drivers/rtc/proc.c @@ -73,7 +73,7 @@ static int rtc_proc_show(struct seq_file *seq, void *offset) seq_printf(seq, "24hr\t\t: yes\n"); if (ops->proc) - ops->proc(rtc->dev.parent, seq); + ops->proc(rtc_ops_dev(rtc), seq); return 0; } diff --git a/include/linux/rtc.h b/include/linux/rtc.h index 95da051fb155d..1dd4a45d0186e 100644 --- a/include/linux/rtc.h +++ b/include/linux/rtc.h @@ -83,6 +83,7 @@ struct rtc_timer { /* flags */ #define RTC_DEV_BUSY 0 #define RTC_NO_CDEV 1 +#define RTC_OPS_USE_RTC_DEV 2 struct rtc_device { struct device dev; @@ -167,6 +168,20 @@ struct rtc_device { #define rtc_lock(d) mutex_lock(&d->ops_lock) #define rtc_unlock(d) mutex_unlock(&d->ops_lock) +/** + * rtc_ops_dev - Get the device pointer for RTC ops callbacks + * @rtc: RTC device + * + * Returns &rtc->dev if RTC_OPS_USE_RTC_DEV flag is set, + * otherwise returns rtc->dev.parent. + */ +static inline struct device *rtc_ops_dev(struct rtc_device *rtc) +{ + if (test_bit(RTC_OPS_USE_RTC_DEV, &rtc->flags)) + return &rtc->dev; + return rtc->dev.parent; +} + /* useful timestamps */ #define RTC_TIMESTAMP_BEGIN_0000 -62167219200ULL /* 0000-01-01 00:00:00 */ #define RTC_TIMESTAMP_BEGIN_1900 -2208988800LL /* 1900-01-01 00:00:00 */
Add rtc_ops_dev() helper to allow drivers to choose whether rtc_class_ops callbacks receive rtc->dev or rtc->dev.parent. This enables Rust RTC drivers to store driver data on the RTC device. The helper checks RTC_OPS_USE_RTC_DEV flag: if set, returns &rtc->dev; otherwise returns rtc->dev.parent. Update all rtc_class_ops callback invocations to use rtc_ops_dev(rtc) instead of directly accessing rtc->dev.parent. Maintains backward compatibility for existing C drivers. Signed-off-by: Ke Sun <sunke@kylinos.cn> --- drivers/rtc/dev.c | 6 +++--- drivers/rtc/interface.c | 18 +++++++++--------- drivers/rtc/proc.c | 2 +- include/linux/rtc.h | 15 +++++++++++++++ 4 files changed, 28 insertions(+), 13 deletions(-)