Re: [PATCH v12 3/3] rust: pwm: Add complete abstraction layer

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Michal,

> On 4 Aug 2025, at 19:29, Michal Wilczynski <m.wilczynski@samsung.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