Message-ID: <8ad10cc3-6e7d-4a8b-b6f6-9568403ee2b3@samsung.com>
Date: Tue, 5 Aug 2025 00:29:07 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>, 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>, Drew Fustini
	<drew@...7.com>, 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
Subject: Re: [PATCH v12 3/3] rust: pwm: Add complete abstraction layer


On 7/25/25 17:56, Daniel Almeida wrote:
>> +
>> +    /// Gets the label for this PWM device, if any.
>> +    pub fn label(&self) -> Option<&CStr> {
>> +        // SAFETY: self.as_raw() provides a valid pointer.
>> +        let label_ptr = unsafe { (*self.as_raw()).label };
>> +        if label_ptr.is_null() {
>> +            None
>> +        } else {
>> +            // SAFETY: label_ptr is non-null and points to a C string
>> +            // managed by the kernel, valid for the lifetime of the PWM device.
>> +            Some(unsafe { CStr::from_char_ptr(label_ptr) })
>> +        }
>> +    }
> 
> nit: this can be written more concisely, but I personally don’t mind.

Do you have something specific in mind ? I think the alternative way of
expressing this would use NonNull, but somehow this feels less readable
for me.


>> +
>> +/// Trait defining the operations for a PWM driver.
>> +pub trait PwmOps: 'static + Sized {
>> +    /// The driver-specific hardware representation of a waveform.
>> +    ///
>> +    /// This type must be [`Copy`], [`Default`], and fit within `PWM_WFHWSIZE`.
>> +    type WfHw: Copy + Default;
> 
> Can’t you use a build_assert!() here? i.e.:
> 
>     #[doc(hidden)]
>     const _CHECK_SZ: () = {
>         build_assert!(core::mem::size_of::<Self::WfHw>() <= bindings::PWM_WFHWSIZE as usize);
>     };

This doesn't work i.e the driver using oversized WfHw compiles
correctly, but putting the assert inside the serialize did work, please
see below.


> 
>> +        Err(ENOTSUPP)
>> +    }
>> +
>> +    /// Convert a hardware-specific representation back to a generic waveform.
>> +    /// This is typically a pure calculation and does not perform I/O.
>> +    fn round_waveform_fromhw(
>> +        _chip: &Chip<Self>,
>> +        _pwm: &Device,
>> +        _wfhw: &Self::WfHw,
>> +        _wf: &mut Waveform,
>> +    ) -> Result<c_int> {
>> +        Err(ENOTSUPP)
>> +    }
> 
> Please include at least a description of what this returns.

Instead I think it should just return Result, reviewed the code and it's
fine.

> 
>> +/// Bridges Rust `PwmOps` to the C `pwm_ops` vtable.
>> +struct Adapter<T: PwmOps> {
>> +    _p: PhantomData<T>,
>> +}
>> +
>> +impl<T: PwmOps> Adapter<T> {
>> +    const VTABLE: PwmOpsVTable = create_pwm_ops::<T>();
>> +
>> +    /// # Safety
>> +    ///
>> +    /// `wfhw_ptr` must be valid for writes of `size_of::<T::WfHw>()` bytes.
>> +    unsafe fn serialize_wfhw(wfhw: &T::WfHw, wfhw_ptr: *mut c_void) -> Result {
>> +        let size = core::mem::size_of::<T::WfHw>();
>> +        if size > bindings::PWM_WFHWSIZE as usize {
>> +            return Err(EINVAL);
>> +        }
> 
> See my previous comment on using build_assert if possible.

So I did try this and it does work, however it results in a cryptic
linker error:
ld.lld: error: undefined symbol: rust_build_error
>>> referenced by pwm_th1520.2c2c3938312114c-cgu.0
>>>               drivers/pwm/pwm_th1520.o:(<kernel::pwm::Adapter<pwm_th1520::Th1520PwmDriverData>>::read_waveform_callback) in archive vmlinux.a
>>> referenced by pwm_th1520.2c2c3938312114c-cgu.0
>>>               drivers/pwm/pwm_th1520.o:(<kernel::pwm::Adapter<pwm_th1520::Th1520PwmDriverData>>::round_waveform_tohw_callback) in archive vmlinux.a
make[2]: *** [scripts/Makefile.vmlinux:91: vmlinux] Error 1
  
I assume this could be fixed at some point to better explain what
failed? I think putting the assert in serialize functions is fine and
the proposed _CHECK_SZ isn't really required.

I would love to do some debugging and find out why that is myself if
time allows :-)

> 
>> +        // 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`.
>> +        unsafe { device::Device::as_ref(&raw mut (*self.as_raw()).dev) }
>> +    }
> 
> IIRC, these are supposed to be prefixed with “-“ to highlight that it’s a bulleted list.
> 
>> +
>> +    /// Gets the *typed* driver specific data associated with this chip's embedded device.
> 
> I don’t think this emphasis adds anything of value. (IMHO)
> 
>> +    pub fn drvdata(&self) -> &T {
> 
> This is off-topic (sorry), but I wonder if this shouldn’t be renamed “driver_data()” across the tree.

Agree


> 
> 
> — Daniel
> 
> [0] https://lore.kernel.org/rust-for-linux/20250711-device-as-ref-v2-0-1b16ab6402d7@google.com/
> 
> 

For readability cut the rest of the comments, but they will be fixed

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

Powered by blists - more mailing lists