Message-ID: <00ea8f31-140f-467f-8083-e1c044afa391@samsung.com>
Date: Tue, 12 Aug 2025 11:27:40 +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 8/6/25 14:49, Daniel Almeida wrote:
> Hi Michal,
> 
>> On 4 Aug 2025, at 19:29, Michal Wilczynski <m.wilczynski@...sung.com> wrote:
>>
>>
>> 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.
> 
> Yes, an early return, i.e.:
> 
> if label_ptr.is_null() {
>   return None
> }
> 
> It saves you one level of indentation by removing the else branch.
> 
>>
>>
>>>> +
>>>> +/// 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.
> 
> Can you show how it looks like with the build_assert included? Just as a sanity check.

For a sanity check, here’s the code I added to the PwmOps trait, exactly
as you suggested:

#[doc(hidden)]
const _CHECK_SZ: () = {
    build_assert!(core::mem::size_of::<Self::WfHw>() <= bindings::PWM_WFHWSIZE as usize);
};

To test it, I went into the pwm-th1520 driver and changed its WfHw
implementation to be larger than PWM_WFHWSIZE. I expected the build to
fail because of the build_assert!, but it compiled without any errors.

This is why I concluded it "doesn't work" in this position, whereas
placing the check inside the serialize function did cause a (linker)
error as expected. I'm probably missing something subtle here.

> 
>>
>>
>>>
>>>> +        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.
>>
> 
> Ack.
> 
>>>
>>>> +/// 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 :-)
> 
> There is nothing wrong here. A canonical Rust-for-Linux experience is stumbling
> upon the error generated by build_assert and being rightly confused. People ask
> about this every few months :)
> 
> This just means that the build_assert triggered and the build failed as a
> result. IOW, it means that your build_assert is working properly to catch
> errors.

Yeah it is working correctly, I was just hoping errors can be somehow
made more informative :-), but it is hard and would require some support
from compiler as I imagine.

> 
> — Daniel
> 
> 

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

Powered by blists - more mailing lists