Re: [PATCH v6 2/2] rust: serio: add serio bus abstractions and port Gravis Stinger driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] rust: serio: non-existent bindings::irqreturn::IRQ_* in
  interrupt_callback()
- [Low] Input: stinger_rust: built-in modalias uses stinger_rust instead
  of stinger
- [Low] rust: serio: borrow_mut() lifetime spans across
  serio_continue_rx()
--

commit 8182b04fefab6df9f076e7e3b941b950b2bc4c52
Author: Vecheslav Filatov <harepro29@gmail.com>

rust: serio: add serio bus abstractions and port Gravis Stinger driver

This commit adds Rust abstractions for the serio bus subsystem and ports
the Gravis Stinger serial gamepad driver to Rust.

> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 9976f596a9208..2ba3391af7c9f 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -33,6 +33,9 @@ obj-$(CONFIG_JOYSTICK_SENSEHAT)	+= sensehat-joystick.o
>  obj-$(CONFIG_JOYSTICK_SIDEWINDER)	+= sidewinder.o
>  obj-$(CONFIG_JOYSTICK_SPACEBALL)	+= spaceball.o
>  obj-$(CONFIG_JOYSTICK_SPACEORB)		+= spaceorb.o
> +ifdef CONFIG_JOYSTICK_STINGER_RUST
> +stinger-objs				:= stinger_rust.o
> +endif
>  obj-$(CONFIG_JOYSTICK_STINGER)		+= stinger.o

[Severity: Low]
Does building stinger_rust.o into composite stinger.o emit
stinger_rust.alias instead of stinger.alias into modules.builtin.modinfo
when CONFIG_JOYSTICK_STINGER=y?

rustc derives the crate name and module_path!() from the source filename
stinger_rust.rs, which serio_device_table!() embeds after __kmod_ in the
__mod_device_table__ symbol, whereas module! declares the module name as
"stinger".

> diff --git a/rust/kernel/serio.rs b/rust/kernel/serio.rs
> new file mode 100644
> index 0000000000000..fa5fd93182938
> --- /dev/null
> +++ b/rust/kernel/serio.rs

[ ... ]

> +        // 5. Attach published resources under `serio_pause_rx` to eliminate races with interrupts.
> +        // SAFETY: `serio_pause_rx` holds `serio->lock`, preventing concurrent interrupt execution.
> +        unsafe {
> +            bindings::serio_pause_rx(serio);
> +            let mut borrowed = <T::Data as ForeignOwnable>::borrow_mut(raw_data);
> +            T::attach(&mut borrowed, published);
> +            bindings::serio_continue_rx(serio);
> +        }

[Severity: Low]
Does borrowed remain in scope across bindings::serio_continue_rx(serio)?

Because ForeignOwnable::BorrowedMut may have drop glue, borrowed is not
dropped until the end of the unsafe block after serio->lock is released.
An interrupt arriving right after serio_continue_rx() can run
interrupt_callback() and call ForeignOwnable::borrow_mut(raw) while
connect_callback()'s BorrowedMut is still alive, violating
ForeignOwnable::borrow_mut()'s non-overlapping lifetime requirement.
Explicitly dropping borrowed before serio_continue_rx(serio) avoids this.

> +
> +        0
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// Must only be invoked by the serio subsystem during an interrupt on a connected port.
> +    /// `serio` must be a valid pointer.
> +    unsafe extern "C" fn interrupt_callback(
> +        serio: *mut bindings::serio,
> +        data: c_uchar,
> +        flags: c_uint,
> +    ) -> bindings::irqreturn_t {
> +        // SAFETY: `serio` is valid.
> +        let raw = unsafe { bindings::serio_get_drvdata(serio) };
> +        if raw.is_null() {
> +            return bindings::irqreturn::IRQ_NONE;
> +        }
> +
> +        // SAFETY: `raw` was created via `into_foreign()` in `connect_callback`.
> +        let mut borrowed = unsafe { <T::Data as ForeignOwnable>::borrow_mut(raw) };
> +        match T::interrupt(&mut borrowed, data, flags) {
> +            IrqReturn::None => bindings::irqreturn::IRQ_NONE,
> +            IrqReturn::Handled => bindings::irqreturn::IRQ_HANDLED,
> +        }
> +    }

[Severity: High]
Does this fail to compile when CONFIG_SERIO=y?  bindgen generates
bindings::irqreturn as a u32 type alias with top-level constants
bindings::irqreturn_IRQ_NONE and bindings::irqreturn_IRQ_HANDLED, so
bindings::irqreturn::IRQ_NONE and bindings::irqreturn::IRQ_HANDLED do not
exist:

rust/kernel/irq/request.rs:IrqReturn {
    #[repr(u32)]
    pub enum IrqReturn {
        /// The interrupt was not from this device or was not handled.
        None = bindings::irqreturn_IRQ_NONE,

        /// The interrupt was handled by this device.
        Handled = bindings::irqreturn_IRQ_HANDLED,
    }
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107053953.471972-1-harepro29@gmail.com?part=2