Re: [PATCH v3 3/9] rust: pwm: Add driver operations trait and registration support

[Danilo Krummrich]

On Tue, Jun 17, 2025 at 04:07:26PM +0200, Michal Wilczynski wrote:
> +/// Manages the registration of a PWM chip, ensuring `pwmchip_remove` is called on drop.
> +pub struct Registration {
> +    chip: ManuallyDrop<ARef<Chip>>,

Why is this ManuallyDrop when you call ManuallyDrop::drop(&mut self.chip) as
the last thing in Registration::drop()?

I think you don't need ManuallyDrop here.

> +}
> +
> +impl Registration {
> +    /// Registers a PWM chip (obtained via `Chip::new`) with the PWM subsystem.
> +    ///
> +    /// Takes an [`ARef<Chip>`]. On `Drop` of the returned `Registration` object,
> +    /// `pwmchip_remove` is called for the chip.
> +    pub fn new(chip: ARef<Chip>, ops_vtable: &'static PwmOpsVTable) -> Result<Self> {

For the reason mentioned in [1] this should either return Result<Devres<Self>>
or just Result, if you use Devres::new_foreign_owned() (see also [2]).

In case of the latter, the Registration instance is automatically dropped once
the parent device is unbound.

If you go with the first, you can drop the Devres<Registration> (and hence the
inner Registration) at any arbitrary point of time, but Devres will still
gurarantee that the inner Registration is dropped once the parent device is
unbound, i.e. it can't out-live driver unbind.

This guarantees that the &Device<Bound> instance you provide in the callbacks
below is guaranteed to be valid.

[1] https://lore.kernel.org/lkml/aFF7qqlexxh540FW@pollux/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/rust/kernel/drm/driver.rs#n134

> +        // Get the raw C pointer from ARef<Chip>.
> +        let c_chip_ptr = chip.as_raw().cast::<bindings::pwm_chip>();
> +
> +        // SAFETY: `c_chip_ptr` is valid (guaranteed by ARef existing).
> +        // `ops_vtable.as_raw()` provides a valid `*const bindings::pwm_ops`.
> +        // `bindings::__pwmchip_add` preconditions (valid pointers, ops set on chip) are met.
> +        unsafe {
> +            (*c_chip_ptr).ops = ops_vtable.as_raw();
> +            to_result(bindings::__pwmchip_add(c_chip_ptr, core::ptr::null_mut()))?;
> +        }

Please split this up into separate unsafe blocks.

> +        Ok(Registration {
> +            chip: ManuallyDrop::new(chip),
> +        })
> +    }
> +}
> +
> +impl Drop for Registration {
> +    fn drop(&mut self) {
> +        let chip = &**self.chip;
> +        let chip_raw: *mut bindings::pwm_chip = chip.as_raw();
> +
> +        // SAFETY: `chip_raw` points to a chip that was successfully registered via `Self::new`.
> +        // `bindings::pwmchip_remove` is the correct C function to unregister it.
> +        unsafe {
> +            bindings::pwmchip_remove(chip_raw);
> +            ManuallyDrop::drop(&mut self.chip); // Drops the ARef<Chip>
> +        }

Same here, but I don't think ManuallyDrop is needed anyways.

> +    }
> +}