Re: [PATCH v10 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 2 potential
issues to consider:
- [High] rust: serio: invalid irqreturn constants in
  Registration::interrupt_callback()
- [Medium] rust: serio: unsound Device<Bound> cast in
  Registration::connect_callback()
--

commit 2cd8430f1d5375be91eaa8f7c615434521c81d61
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/rust/kernel/serio.rs b/rust/kernel/serio.rs
> new file mode 100644
> index 0000000000000..a1b29965e8708
> --- /dev/null
> +++ b/rust/kernel/serio.rs

[ ... ]

> +pub trait Driver: Send + Sync + Sized {
> +    /// Context data stored per connected device instance.
> +    type Data: ForeignOwnable + Send + Sync;
> +
> +    /// Pending resources allocated during probe to be published after the port opens.
> +    type Pending;
> +
> +    /// Published device handle created after the port opens.
> +    type Published;
> +
> +    /// Device ID match table.
> +    const ID_TABLE: IdTable;
> +
> +    /// Initializes driver data before the port is opened.
> +    fn init(dev: &Device<device::Bound>) -> Result<(Self::Data, Self::Pending)>;

[ ... ]

> +    unsafe extern "C" fn connect_callback(
> +        serio: *mut bindings::serio,
> +        drv: *mut bindings::serio_driver,
> +    ) -> c_int {
> +        if serio.is_null() {
> +            return crate::error::code::EINVAL.to_errno();
> +        }
> +
> +        // SAFETY: `serio` is valid. `Device<Bound>` is repr(transparent).
> +        let dev = unsafe { &*(serio.cast::<Device<device::Bound>>()) };
> +
> +        // 1. Initialize driver data before port is opened.
> +        let (data, pending) = match T::init(dev) {
> +            Ok(d) => d,
> +            Err(e) => return e.to_errno(),
> +        };

[Severity: Medium]
Is it sound to pass &Device<device::Bound> to T::init() here?

Because serio::Device<Ctx> implements AsRef<device::Device<Ctx>>, safe
Driver::init() implementations can obtain &device::Device<device::Bound> and
register devres resources via Devres::new() or devres::register(). However,
when binding manually through sysfs drvctl, serio_bind_driver() invokes
connect outside really_probe() and does not call devres_release_all() on
failure:

drivers/input/serio/serio.c:serio_bind_driver() {
	...
		serio->dev.driver = &drv->driver;
		if (serio_connect_driver(serio, drv)) {
			serio->dev.driver = NULL;
			return -ENODEV;
		}
	...
}

If T::init(), serio_open(), T::start(), or device_bind_driver() fails in that
path, any devres resource registered during T::init() is left unrevoked on an
unbound device, violating Devres revocation guarantees and causing subsequent
really_probe() attempts on the port to fail with -EBUSY. Passing
&Device<device::Normal> (or &Device) to Driver::init() would avoid exposing
Bound-only APIs before binding is guaranteed.

[ ... ]

> +    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 interrupt_callback() fail to compile because bindings::irqreturn::IRQ_NONE
and bindings::irqreturn::IRQ_HANDLED do not exist?

bindgen generates bindings::irqreturn as a type alias for ffi::c_uint with
top-level constants bindings::irqreturn_IRQ_NONE and
bindings::irqreturn_IRQ_HANDLED (as used in rust/kernel/irq/request.rs), so
resolving IRQ_NONE and IRQ_HANDLED as associated items on bindings::irqreturn
fails with rustc error E0599.

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