Re: [RFC PATCH v3 1/5] rtc: add device selector for rtc_class_ops callbacks

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

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

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com