Re: [PATCH v10 0/7] Rust Abstractions for PWM subsystem with TH1520 PWM driver

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

On Thu Jul 10, 2025 at 10:57 PM CEST, Uwe Kleine-König wrote:
> On Thu, Jul 10, 2025 at 06:06:26PM +0200, Danilo Krummrich wrote:
>> On Thu Jul 10, 2025 at 5:25 PM CEST, Uwe Kleine-König wrote:
>> > Hello Michal,
>> >
>> > On Thu, Jul 10, 2025 at 03:48:08PM +0200, Michal Wilczynski wrote:
>> >> On 7/10/25 15:10, Uwe Kleine-König wrote:
>> >> > On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>> >> >> On 7/7/25 11:48, Michal Wilczynski wrote:
>> >> >>> The series is structured as follows:
>> >> >>>  - Expose static function pwmchip_release.
>> >> > 
>> >> > Is this really necessary? I didn't try to understand the requirements
>> >> > yet, but I wonder about that. If you get the pwmchip from
>> >> > __pwmchip_add() the right thing to do to release it is to call
>> >> > pwmchip_remove(). Feels like a layer violation.
>> >> 
>> >> It's required to prevent a memory leak in a specific, critical failure
>> >> scenario. The sequence of events is as follows:
>> >> 
>> >>     pwm::Chip::new() succeeds, allocating both the C struct pwm_chip and
>> >>     the Rust drvdata.
>> >> 
>> >>     pwm::Registration::register() (which calls pwmchip_add()) fails for
>> >>     some reason.
>> >
>> 
>> (Just trying to help clear up the confusion.)
>
> Very appreciated!
>
>> > If you called pwmchip_alloc() but not yet pwmchip_add(), the right
>> > function to call for cleanup is pwmchip_put().
>> 
>> That is exactly what is happening when ARef<Chip> is dropped. If the reference
>> count drops to zero, pwmchip_release() is called, which frees the chip. However,
>> this would leave the driver's private data allocation behind, which is owned by
>> the Chip instance.
>
> I don't understand that. The chip and the driver private data both are
> located in the same allocation. How is this a problem of the driver
> private data only then? The kfree() in pwmchip_release() is good enough
> for both?!

Not in the current abstractions, there are two allocations, one for the Chip and
one for the driver's private data, or in other words the abstraction uses
pwmchip_set_drvdata() and pwmchip_get_drvdata().

Having a brief look at pwmchip_alloc(), it seems to me that PWM supports the
subclassing pattern with pwmchip_priv().

We should probably take advantage of that. Assuming we do that, the Rust
abstraction still needs a release() callback because we still need to call
drop_in_place() in order to get the destructor of the driver's private data
type called. We actually missed this in DRM and I fixed it up recently [1].

@Michal: With the subclassing pattern the Chip structure would look like this:

	#[repr(C)]
	#[pin_data]
	pub struct Chip<T> {
	   inner: Opaque<bindings::pwm_chip>,
	   #[pin]
	   data: T,
	}

And in the release() callback would look like this:

    extern "C" fn release(ptr: *mut bindings::pwm_chip) {
        // CAST: Casting `ptr` to `Chip<T>` is valid, since [...].
        let this = ptr.cast<Chip<T>>();

        // SAFETY:
        // - When `release` runs it is guaranteed that there is no further access to `this`.
        // - `this` is valid for dropping.
        unsafe { core::ptr::drop_in_place(this) };
    }

This is exactly what we're doing in DRM as well, I would have recommended this
to begin with, but I didn't recognize that PWM supports subclassing. :)

I recommend having a look at [2].

[1] https://lore.kernel.org/all/20250629153747.72536-1-dakr@kernel.org/
[2] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-fixes/rust/kernel/drm/device.rs