Message-ID: <e77eab1c-446f-4620-95be-d343684d1e95@samsung.com>
Date: Thu, 10 Jul 2025 15:48:08 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: 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>, Danilo Krummrich <dakr@...nel.org>, 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 15:10, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Jul 10, 2025 at 10:42:07AM +0200, Michal Wilczynski wrote:
>> On 7/7/25 11:48, Michal Wilczynski wrote:
>>> This patch series introduces Rust support for the T-HEAD TH1520 PWM
>>> controller and demonstrates its use for fan control on the Sipeed Lichee
>>> Pi 4A board.
>>>
>>> The primary goal of this patch series is to introduce a basic set of
>>> Rust abstractions for the Linux PWM subsystem. As a first user and
>>> practical demonstration of these abstractions, the series also provides
>>> a functional PWM driver for the T-HEAD TH1520 SoC. This allows control
>>> of its PWM channels and ultimately enables temperature controlled fan
>>> support for the Lichee Pi 4A board. This work aims to explore the use of
>>> Rust for PWM drivers and lay a foundation for potential future
>>> Rust based PWM drivers.
>>>
>>> The core of this series is a new rust/kernel/pwm.rs module that provides
>>> abstractions for writing PWM chip provider drivers in Rust. This has
>>> been significantly reworked from v1 based on extensive feedback. The key
>>> features of the new abstraction layer include:
>>>
>>>  - Ownership and Lifetime Management: The pwm::Chip wrapper is managed
>>>    by ARef, correctly tying its lifetime to its embedded struct device
>>>    reference counter. Chip registration is handled by a pwm::Registration
>>>    RAII guard, which guarantees that pwmchip_add is always paired with
>>>    pwmchip_remove, preventing resource leaks.
>>>
>>>  - Modern and Safe API: The PwmOps trait is now based on the modern
>>>    waveform API (round_waveform_tohw, write_waveform, etc.) as recommended
>>>    by the subsystem maintainer. It is generic over a driver's
>>>    hardware specific data structure, moving all unsafe serialization logic
>>>    into the abstraction layer and allowing drivers to be written in 100%
>>>    safe Rust.
>>>
>>>  - Ergonomics: The API provides safe, idiomatic wrappers for other PWM
>>>    types (State, Args, Device, etc.) and uses standard kernel error
>>>    handling patterns.
>>>
>>> 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.

Hi Uwe,
Thank you for the feedback.

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.

    The ARef<Chip> returned by new() is dropped, its reference count
    goes to zero, and our custom release_callback is called.

In this state:

Calling pwmchip_remove() would be incorrect because the chip was never
successfully added. If our callback only frees the Rust drvdata, the
memory for the C struct pwm_chip is leaked.

Therefore, our custom release_callback  must perform both cleanup tasks:
freeing the Rust drvdata and then calling pwmchip_release to free the C
struct. This "chaining" of the release handlers seems to be the most
robust way to guarantee cleanup in all scenarios.

Note that the bindings still use pwmchip_remove function in 'normal'
failure path.

> 
>> [...]
>>> ---
>>> base-commit: 47753b5a1696283930a78aae79b29371f96f5bca
> 
> I have problems applying this series and don't have this base commit in
> my repo.

Sorry for the confusion. Base commit doesn't exist in the mainline
kernel or linux-next, cause I've added some dependecies for compilation,
like IoMem for the driver (uploaded full branch on github [1]). The
bindings however doesn't depend on anything that's not in linux-next.
Anyway series applies cleanly on linux-next/master like so:

$ git fetch linux-next
$ git checkout linux-next/master
$ b4 shazam https://lore.kernel.org/all/20250707-rust-next-pwm-working-fan-for-sending-v10-0-d0c5cf342004@samsung.com/
Grabbing thread from lore.kernel.org/all/20250707-rust-next-pwm-working-fan-for-sending-v10-0-d0c5cf342004@...sung.com/t.mbox.gz
Checking for newer revisions
Grabbing search results from lore.kernel.org
Analyzing 14 messages in the thread
Looking for additional code-review trailers on lore.kernel.org
Analyzing 165 code-review messages
Checking attestation on all messages, may take a moment...
---
  ✓ [PATCH v10 1/7] pwm: Export `pwmchip_release` for external use
  ✓ [PATCH v10 2/7] rust: pwm: Add Kconfig and basic data structures
  ✓ [PATCH v10 3/7] rust: pwm: Add complete abstraction layer
  ✓ [PATCH v10 4/7] pwm: Add Rust driver for T-HEAD TH1520 SoC
  ✓ [PATCH v10 5/7] dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
  ✓ [PATCH v10 6/7] riscv: dts: thead: Add PWM controller node
  ✓ [PATCH v10 7/7] riscv: dts: thead: Add PWM fan and thermal control
  ---
  ✓ Signed: DKIM/samsung.com
---
Total patches: 7
---
 Base: using specified base-commit 47753b5a1696283930a78aae79b29371f96f5bca
Applying: pwm: Export `pwmchip_release` for external use
Applying: rust: pwm: Add Kconfig and basic data structures
Applying: rust: pwm: Add complete abstraction layer
Applying: pwm: Add Rust driver for T-HEAD TH1520 SoC
Applying: dt-bindings: pwm: thead: Add T-HEAD TH1520 PWM controller
Applying: riscv: dts: thead: Add PWM controller node
Applying: riscv: dts: thead: Add PWM fan and thermal control


> 
> Best regards
> Uwe

[1] - https://github.com/mwilczy/linux/commits/rust-next-pwm-working-fan-for-sending-v15/

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

Powered by blists - more mailing lists