Message-Id: <8C20C615-DBF2-4DF9-9AB3-E78C4B1E7493@collabora.com>
Date: Wed, 6 Aug 2025 09:49:21 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Michal Wilczynski <m.wilczynski@...sung.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

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.

> 
> 
>> 
>>> +        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.

— Daniel

Powered by blists - more mailing lists