Re: [PATCH v2 3/4] rust: core abstractions for HID drivers

[Rahul Rameshbabu <sergeantsagara@protonmail.com>]

On Tue, 15 Jul, 2025 17:04:07 +0200 "Danilo Krummrich" <dakr@kernel.org> wrote:
> On Sun Jul 13, 2025 at 11:12 PM CEST, Rahul Rameshbabu wrote:
>> +// SAFETY: Instances of `Device` are always reference-counted.
>> +unsafe impl crate::types::AlwaysRefCounted for Device {
>> +    fn inc_ref(&self) {
>> +        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
>> +        unsafe { bindings::kref_get(&mut ((*self.as_raw()).ref_)) }
>
> I'm confused, what's the lifecycle of a struct hid_device? It embedds a struct
> device, so it also inherits its reference count. Additionally it also has a
> struct kref. Can you elaborate please?
>
> I don't know what the struct kref is for, but I'm pretty sure you want to manage
> the reference count of the embedded struct device here.
>

Hi Danilo,

The hid subsystem uses a separate ref counter from the one maintained by
the embedded struct device. hid_device_release is assigned to the
embedded struct device's release method and makes use of this separate
reference (in drivers/hid/hid-core.c).

drivers/hid/hid-debug.c makes use of the same reference in a similar
manner to what I do here in hid_debug_events_release and
hid_debug_events_open.

Basically, the hid subsystem has a separate reference counter on top of
the one embedded in the kobject of struct device;

I decided to follow the same pattern used in the core C HID subsystem.
This pattern is found in hid-core + hid_debug.

However, thank you for bringing this up since I think that you are right
that it would be better to increment the reference count for the
underlying device rather than use this separate external reference
count.

It might even make sense to refactor core C HID to remove this separate
kref and have hid_debug increase the reference count of the underlying
struct device's kobject.

Right now, we can have the following situation in the hid subsystem.

1. struct device's kobject decrements to 0
2. struct device's release callback is called
3. This invokes hid_device_ref and decrements struct hid_device's ref
4. hid_debug is in use, so the ref has not gone to 0 yet
5. When hid_debug is ready to tear down the instance, hiddev_free will
   get called

The above creates a weird situation where the HID device can still be
in-use even though the underlying device instance has reached a
reference count of 0.

I will first start with fixing this in my Rust code, but I will take a
look at cleaning this up in the core HID subsystem as well.

>> +    }
>> +
>> +    unsafe fn dec_ref(obj: NonNull<Self>) {
>> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
>> +        unsafe {
>> +            bindings::kref_put(
>> +                &mut ((*obj.cast::<bindings::hid_device>().as_ptr()).ref_),
>
> I think you want &raw mut instead.
>
>> +                Some(bindings::hiddev_free),
>> +            )
>> +        }
>> +    }
>> +}
>> +
>> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
>> +    fn as_ref(&self) -> &device::Device<Ctx> {
>> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
>> +        // `struct hid_device`.
>> +        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
>
> You also use &raw mut meanwhile.
>
>> +
>> +        // SAFETY: `dev` points to a valid `struct device`.
>> +        unsafe { device::Device::as_ref(dev) }
>> +    }
>> +}

https://doc.rust-lang.org/std/primitive.pointer.html#3-create-it-using-raw

Yes, I want &raw mut in the places you mentioned. Thanks :)

Thanks,
Rahul Rameshbabu