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

["Danilo Krummrich" <dakr@kernel.org>]

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.