From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0A7172E3AFD for ; Thu, 3 Jul 2025 11:37:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=210.118.77.12 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751542669; cv=none; b=D4Juw6bUKreJ90SVydMTrXU5g+8HIuu3d+hMlYZEAdqmpiatakdyWnBNFn5tvmm3H0RRy6/6+yDCcdkNfMvV7M8z2VAyMaCL3sSQnf2yKWCu1nwcO9XGibY9GCA/Eu1HGVcxzUPoNnBChrz0hErbojFcMFOmiKr48qRh+NBeT+Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751542669; c=relaxed/simple; bh=evYGdWXWw+/6PRQoXuVCrMVKNTai1x93mZvTubwFAMQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:From:In-Reply-To: Content-Type:References; b=rLt1ONL3+drls1GOxMu1WLjlflLO4n6uwDnmWSppRuk0UOBsZVQfd/AewIAaf89gc/hjpZWMqBFlnCujViwrHVG7zO7tRxs4yl6MVFuOSLU+0OhY5jPTH5TzRjwmbBa4mqEW24ZHyLTGJBqOKajdWqyBZkNvcD2bVTku33lJpog= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=samsung.com; spf=pass smtp.mailfrom=samsung.com; dkim=pass (1024-bit key) header.d=samsung.com header.i=@samsung.com header.b=fTHWtAb9; arc=none smtp.client-ip=210.118.77.12 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=samsung.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=samsung.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=samsung.com header.i=@samsung.com header.b="fTHWtAb9" Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20250703113745euoutp02a3ab0566edced2857d32262eba3b28a2~OusYqZ7bw2882828828euoutp02H for ; Thu, 3 Jul 2025 11:37:45 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20250703113745euoutp02a3ab0566edced2857d32262eba3b28a2~OusYqZ7bw2882828828euoutp02H DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1751542665; bh=IaUSrP5vF7c7LGO5H4Eij7O+D07WlO1LwH0IQDoV9Z8=; h=Date:Subject:To:Cc:From:In-Reply-To:References:From; b=fTHWtAb9YYu5inKO0J3HoW674ZkV25N5g2HW/kyUJ50zhaNeytdMrVsog2TjpZqdG Jj0WC7gFvDyT+IEt+0PceeUd8tVS+W8U4ugRgC6jAbNCcnsF/1vOlJdsKktQ6zK0Bx CrQg+OofpOOAJosMNakVfLKwYgn6YHGLQqR8QFR8= Received: from eusmtip1.samsung.com (unknown [203.254.199.221]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20250703113744eucas1p157ec555de6855afe8b1d1280030fc06d~OusX8uDZc2942729427eucas1p1O; Thu, 3 Jul 2025 11:37:44 +0000 (GMT) Received: from [192.168.1.44] (unknown [106.210.136.40]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20250703113743eusmtip18065c1273aae5e6af7f4987e4c46d79b~OusW4AM3g1070910709eusmtip1C; Thu, 3 Jul 2025 11:37:43 +0000 (GMT) Message-ID: <6d9ce601-b81e-4c2a-b9c3-4cba6fa87b8b@samsung.com> Date: Thu, 3 Jul 2025 13:37:43 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v7 3/8] rust: pwm: Add core 'Device' and 'Chip' object wrappers To: Danilo Krummrich Cc: =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= , Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Alice Ryhl , Trevor Gross , Guo Ren , Fu Wei , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Paul Walmsley , Palmer Dabbelt , Albert Ou , Alexandre Ghiti , Marek Szyprowski , Benno Lossin , Michael Turquette , Drew Fustini , linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org, rust-for-linux@vger.kernel.org, linux-riscv@lists.infradead.org, devicetree@vger.kernel.org Content-Language: en-US From: Michal Wilczynski In-Reply-To: Content-Transfer-Encoding: 7bit X-CMS-MailID: 20250703113744eucas1p157ec555de6855afe8b1d1280030fc06d X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20250702134957eucas1p1d84f2ed3014cf98ea3a077c7fae6dea6 X-EPHeader: CA X-CMS-RootMailID: 20250702134957eucas1p1d84f2ed3014cf98ea3a077c7fae6dea6 References: <20250702-rust-next-pwm-working-fan-for-sending-v7-0-67ef39ff1d29@samsung.com> <20250702-rust-next-pwm-working-fan-for-sending-v7-3-67ef39ff1d29@samsung.com> On 7/2/25 17:13, Danilo Krummrich wrote: > On Wed, Jul 02, 2025 at 03:45:31PM +0200, Michal Wilczynski wrote: >> Building on the basic data types, this commit introduces the central >> object abstractions for the PWM subsystem: Device and Chip. It also >> includes the core trait implementations that make the Chip wrapper a >> complete, safe, and managed object. >> >> The main components of this change are: >> - Device and Chip Structs: These structs wrap the underlying struct >> pwm_device and struct pwm_chip C objects, providing safe, idiomatic >> methods to access their fields. >> >> - High-Level `Device` API: Exposes safe wrappers for the modern >> `waveform` API, allowing consumers to apply, read, and pre-validate >> hardware configurations. >> >> - Core Trait Implementations for Chip: >> - AlwaysRefCounted: Links the Chip's lifetime to its embedded >> struct device reference counter. This enables automatic lifetime >> management via ARef. >> - Send and Sync: Marks the Chip wrapper as safe for use across >> threads. This is sound because the C core handles all necessary >> locking for the underlying object's state. >> >> These wrappers and traits form a robust foundation for building PWM >> drivers in Rust. >> >> Signed-off-by: Michal Wilczynski > > Few more comments below, with those fixed: > > Reviewed-by: Danilo Krummrich > >> +/// Wrapper for a PWM device [`struct pwm_device`](srctree/include/linux/pwm.h). >> +#[repr(transparent)] >> +pub struct Device(Opaque); >> + >> +impl Device { > > > >> + /// Gets a reference to the parent `Chip` that this device belongs to. >> + pub fn chip(&self) -> &Chip { >> + // SAFETY: `self.as_raw()` provides a valid pointer. (*self.as_raw()).chip >> + // is assumed to be a valid pointer to `pwm_chip` managed by the kernel. >> + // Chip::as_ref's safety conditions must be met. >> + unsafe { Chip::as_ref((*self.as_raw()).chip) } > > I assume the C API does guarantee that a struct pwm_device *always* holds a > valid pointer to a struct pwm_chip? > >> + >> +/// Wrapper for a PWM chip/controller ([`struct pwm_chip`](srctree/include/linux/pwm.h)). >> +#[repr(transparent)] >> +pub struct Chip(Opaque); >> + >> +impl Chip { >> + /// Creates a reference to a [`Chip`] from a valid pointer. >> + /// >> + /// # Safety >> + /// >> + /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the >> + /// returned [`Chip`] reference. >> + pub(crate) unsafe fn as_ref<'a>(ptr: *mut bindings::pwm_chip) -> &'a Self { >> + // SAFETY: The safety requirements guarantee the validity of the dereference, while the >> + // `Chip` type being transparent makes the cast ok. >> + unsafe { &*ptr.cast::() } >> + } >> + >> + /// Returns a raw pointer to the underlying `pwm_chip`. >> + pub(crate) fn as_raw(&self) -> *mut bindings::pwm_chip { >> + self.0.get() >> + } >> + >> + /// Gets the number of PWM channels (hardware PWMs) on this chip. >> + pub fn npwm(&self) -> u32 { >> + // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime. >> + unsafe { (*self.as_raw()).npwm } >> + } >> + >> + /// Returns `true` if the chip supports atomic operations for configuration. >> + pub fn is_atomic(&self) -> bool { >> + // SAFETY: `self.as_raw()` provides a valid pointer for `self`'s lifetime. >> + unsafe { (*self.as_raw()).atomic } >> + } >> + >> + /// Returns a reference to the embedded `struct device` abstraction. >> + pub fn device(&self) -> &device::Device { >> + // SAFETY: `self.as_raw()` provides a valid pointer to `bindings::pwm_chip`. >> + // The `dev` field is an instance of `bindings::device` embedded within `pwm_chip`. >> + // Taking a pointer to this embedded field is valid. >> + // `device::Device` is `#[repr(transparent)]`. >> + // The lifetime of the returned reference is tied to `self`. >> + let dev_field_ptr = unsafe { core::ptr::addr_of!((*self.as_raw()).dev) }; > > I think you can use `&raw` instead. > >> + // SAFETY: `dev_field_ptr` is a valid pointer to `bindings::device`. >> + // Casting and dereferencing is safe due to `repr(transparent)` and lifetime. >> + unsafe { &*(dev_field_ptr.cast::()) } > > Please use Device::as_ref() instead. > >> + } >> + >> + /// Gets the *typed* driver-specific data associated with this chip's embedded device. >> + pub fn drvdata(&self) -> &T { > > You need to make the whole Chip structure generic over T, i.e. > Chip. > > Otherwise the API is unsafe, since the caller can pass in any T when calling > `chip.drvdata()` regardless of whether you actually stored as private data > through Chip::new(). You were right that the original drvdata() method was unsafe. The most direct fix, making Chip generic to Chip, unfortunately creates a significant cascade effect: - If Chip becomes Chip, then anything holding it, like ARef, must become ARef>. - This in turn forces container structs like Registration to become generic (Registration). - Finally, the PwmOps trait itself needs to be aware of T, which complicates the trait and all driver implementations. This chain reaction adds a lot of complexity. To avoid it, I've figured an alternative: The new idea keeps Chip simple and non generic but ensures type safety through two main improvements to the abstraction layer: 1. A Thread Safe DriverData Wrapper The pwm.rs module now provides a generic pwm::DriverData struct. Its only job is to wrap the driver's private data and provide the necessary unsafe impl Send + Sync. // In `rust/kernel/pwm.rs` // SAFETY: The contained data is guaranteed by the kernel to have // synchronized access during callbacks. pub struct DriverData(T); unsafe impl Send for DriverData {} unsafe impl Sync for DriverData {} // In the driver's `probe` function let safe_data = pwm::DriverData::new(Th1520PwmDriverData{ }); 2. A More Ergonomic PwmOps Trait The PwmOps trait methods now receive the driver's data directly as &self, which is much more intuitive. We achieve this by providing a default associated type for the data owner, which removes boilerplate from the driver. // In `rust/kernel/pwm.rs` pub trait PwmOps: 'static + Sized { type Owner: Deref> + ForeignOwnable = Pin>>; /// For now I'm getting compiler error here: `associated type defaults are unstable` /// So the driver would need to specify this for now, until this feature /// is stable // Methods now receive `&self`, making them much cleaner to implement. fn round_waveform_tohw(&self, chip: &Chip, pwm: &Device, wf: &Waveform) -> Result<...>; } // In the driver impl pwm::PwmOps for Th1520PwmDriverData { type WfHw = Th1520WfHw; fn round_waveform_tohw(&self, chip: &pwm::Chip, ...) -> Result<...> { // no drvdata() call here :-) let rate_hz = self.clk.rate().as_hz(); // ... } } This solution seem to address to issue you've pointed (as the user of the API never deals with drvdata directly at this point), while making it easier to develop PWM drivers in Rust. Please let me know what you think. Best regards, -- Michal Wilczynski