Thread (43 messages) 43 messages, 8 authors, 2026-02-27

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

From: "Danilo Krummrich" <dakr@kernel.org>
Date: 2026-02-22 15:29:53
Also in: linux-rtc

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 [off-list ref] wrote:
quoted
On Sat Feb 21, 2026 at 12:19 PM CET, Rafael J. Wysocki wrote:
quoted
On Sat, Feb 21, 2026 at 12:16 PM Alexandre Belloni
[off-list ref] wrote:
quoted
quoted
quoted
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
quoted
  (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)
	    }
	}
quoted
  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.
quoted
  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/ (local)
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
quoted
  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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help