On Thu, 30 May 2024 08:47:25 +0200
Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
>> >> >> For a Rust PHY driver, you know that you have a valid pointer to C's
>> >> >> device object of C's PHY device during the probe callback. The driver
>> >> >> creates a Rust device object to wrap the C pointer to the C's device
>> >> >> object and passes it to the firmware abstractions. The firmware
>> >> >> abstractions gets the C's pointer from the Rust object and calls C's
>> >> >> function to load firmware, returns the result.
>> >> >>
>> >> >> You have concerns about the simple code like the following?
>> >> >>
>> >> >>
>> >> >> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
>> >> >> new file mode 100644
>> >> >> index 000000000000..6144437984a9
>> >> >> --- /dev/null
>> >> >> +++ b/rust/kernel/device.rs
>> >> >> @@ -0,0 +1,30 @@
>> >> >> +// SPDX-License-Identifier: GPL-2.0
>> >> >> +
>> >> >> +//! Generic devices that are part of the kernel's driver model.
>> >> >> +//!
>> >> >> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>> >> >> +
>> >> >> +use crate::types::Opaque;
>> >> >> +
>> >> >> +#[repr(transparent)]
>> >> >> +pub struct Device(Opaque<bindings::device>);
>> >> >> +
>> >> >> +impl Device {
>> >> >> + /// Creates a new [`Device`] instance from a raw pointer.
>> >> >> + ///
>> >> >> + /// # Safety
>> >> >> + ///
>> >> >> + /// For the duration of 'a, the pointer must point at a valid `device`.
>> >> >
>> >> > If the following rust code does what this comment says, then sure, I'm
>> >> > ok with it for now if it helps you all out with stuff like the firmware
>> >> > interface for the phy rust code.
>> >>
>> >> Great, thanks a lot!
>> >>
>> >> Danilo and Wedson, are there any concerns about pushing this patch [1]
>> >> for the firmware abstractions?
>> >
>> > Well, if everyone is fine with this one I don't see why we can't we go with [1]
>> > directly? AFAICS, we'd only need the following fix:
>> >
>> > -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
>> > +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
>> >
>> > [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@xxxxxxxxxx/
>>
>> The difference is that your patch touches the reference count of a
>> struct device. My patch doesn't.
>>
>> The following part in your patch allows the rust code to freely play
>> with the reference count of a struct device. Your Rust drm driver
>> interact with the reference count in a different way than C's drm
>> drivers if I understand correctly. I'm not sure that Greg will be
>> convinenced with that approach.
>
> I don't see how this is different than what we do in C. If you (for whatever
> reason) want to keep a pointer to a struct device you should make sure to
> increase the reference count of this device, such that it can't get freed for
> the time being.
>
> This is a 1:1 representation of that and conceptually identical.
A drm driver does such? If a drm driver allocates two types of
driver-specific data and keep a pointer to the pci device, then the
driver calls get_device() twice to increase the reference count of the
pci's device?
At the very least, network device drivers don't. a driver doesn't play
with the reference count. The network subsystem does. And, the network
subsystem doesn't care about how many pointers to a pci device a
driver keeps.
With the patch [1], a driver plays with the reference count of a
device and directly calls get/put_device. It's a different than C
drivers for me (not sure about drm subsystem though).
But I might be misunderstanding that Greg isn't convinced with this
reference count thing. We need to wait for his response.
>> +// SAFETY: Instances of `Device` are always ref-counted.
>> +unsafe impl crate::types::AlwaysRefCounted for Device {
>> + fn inc_ref(&self) {
>> + // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero.
>> + unsafe { bindings::get_device(self.as_raw()) };
>> + }
>> +
>> + unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>> + // SAFETY: The safety requirements guarantee that the refcount is nonzero.
>> + unsafe { bindings::put_device(obj.cast().as_ptr()) }
>> + }
>> +}
>>
>> The following comments give the impression that Rust abstractions
>> wrongly interact with the reference count; callers check out the
>> reference counter. Nobody should do that.
>
> No, saying that the caller must ensure that the device "has a non-zero reference
> count" is a perfectly valid requirement.
>
> It doensn't mean that the calling code has to peek the actual reference count,
> but it means that it must be ensured that while we try to increase the reference
> count it can't drop to zero unexpectedly.
Yeah, the same requirements but expressed differently.
But again, I might be misunderstanding. Greg might have a different reason.
> Your patch, as a subset of this one, has the same requirements as listed below.
>
>>
>> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
>> + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
>>
>> + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for
>> + /// the entire duration when the returned reference exists.
>> + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self {
>> + // SAFETY: Guaranteed by the safety requirements of the function.
>> + unsafe { &*ptr.cast() }
>> + }
>>
>
>
