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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DB8I5J8ZY7QF.2D8HEN6JX4HSZ@kernel.org>
Date: Thu, 10 Jul 2025 18:06:26 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: "Michal Wilczynski" <m.wilczynski@...sung.com>, "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 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. :)