Message-ID: <020b8036-8959-4733-a5ab-ce8c963ce869@samsung.com>
Date: Fri, 11 Jul 2025 14:36:37 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Danilo Krummrich <dakr@...nel.org>, Uwe Kleine-König
	<ukleinek@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas
	Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor
	Gross <tmgross@...ch.edu>, Guo Ren <guoren@...nel.org>, Fu Wei
	<wefu@...hat.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Paul Walmsley
	<paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
	<aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, Marek Szyprowski
	<m.szyprowski@...sung.com>, Benno Lossin <lossin@...nel.org>, Michael
	Turquette <mturquette@...libre.com>, Drew Fustini <fustini@...nel.org>,
	linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
	rust-for-linux@...r.kernel.org, linux-riscv@...ts.infradead.org,
	devicetree@...r.kernel.org, Krzysztof Kozlowski
	<krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH v10 0/7] Rust Abstractions for PWM subsystem with TH1520
 PWM driver



On 7/10/25 23:19, Danilo Krummrich wrote:
> 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,
> 	}
}

Hello

Thank you both for the detailed feedback and suggestions.

Danilo, you are right, we should absolutely use the subclassing pattern
to have a single allocation for the chip and driver data. This is a much
cleaner design.

As I looked into this, the main difference is that the C struct pwm_chip
doesn't have a fixed size because of the pwms[] array at the end. This
prevents us from using the exact struct layout you suggested.

pub pwms: __IncompleteArrayField<pwm_device>,

Therefore, to correctly implement the subclassing pattern, it would be
sufficient to leave the current struct as is and use pwmchip_get_drvdata to
acquire pointers to the allocated drvdata.

pub struct Chip<T: PwmOps>(Opaque<bindings::pwm_chip>, PhantomData<T>);

This will still achieve the goal of a single allocation via
pwmchip_alloc's sizeof_priv argument, while working around the DST
limitation.

> 
> 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>>();

I think this would use pwmchip_get_drvdata instead.

> 
>         // 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
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@...sung.com>

Powered by blists - more mailing lists