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 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.)

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

So, in Rust we not only have to free the chip itself on release, but also the
driver's private data. The solution Michal went for is overwriting the PWM
chip's dev->release() with a callback that drops the driver's private data and
subsequently calls the "original" pwmchip_release().

This is a common pattern in Rust that we use in DRM as well. One thing that is
different in DRM is, that a struct drm_device (equivalent of struct pwm_chip in
this case), has it's own release callback for drivers that we can attach to.

PWM does not have such a callback AFAICS, hence the Rust abstraction uses the
underlying device's release callback and then forwards to pwmchip_release().

Hope this helps. :)