Re: [PATCH v13 2/3] samples: rust: add Rust serial device bus sample device driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `receive` callback issues an unbounded blocking write (`serdev::Timeout::Max`), which can stall the TTY receive worker indefinitely and cause kernel hangs during module unload.
- [High] The serdev abstraction's `remove_callback` fails to drop the driver's private data, leaking memory and leaving the device open, which leads to a null-pointer dereference in the receive workqueue.
- [Medium] The driver attempts to read a non-standard "baudrate" property instead of the standard "current-speed" property.
- [Medium] The driver's probe function will fail on many valid hardware platforms because it rejects inexact baud rates.
- [Low] The commit message falsely claims the driver supports device/driver name matching and omits mention of ACPI support.
--

--- Patch [2]: [PATCH v13 2/3] samples: rust: add Rust serial device bus sample device driver ---
commit 44e3438d1218c2205d779354d11c50718f82dc2f
Author: Markus Probst <markus.probst@posteo.de>

samples: rust: add Rust serial device bus sample device driver

Add a sample Rust serial device bus device driver illustrating the usage
of the serial device bus abstractions.

> This drivers probes through either a match of device / driver name or a
> match within the OF ID table.

[Severity: Low]
This isn't a bug, but the commit message inaccurately describes the bus matching
behavior.

serdev_device_match() exclusively checks ACPI and OF ID tables and does not
fall back to device/driver name matching. The implemented ACPI matching is also
omitted from the description.

Could the commit message be updated to accurately reflect the supported
matching mechanisms?

> diff --git a/samples/rust/rust_driver_serdev.rs b/samples/rust/rust_driver_serdev.rs
> new file mode 100644
> index 0000000000000..824affbf65936
> --- /dev/null
> +++ b/samples/rust/rust_driver_serdev.rs
[ ... ]
> +    fn probe<'bound>(
> +        sdev: &'bound serdev::Device<Core<'_>>,
> +        _info: Option<&'bound Self::IdInfo>,
> +    ) -> impl PinInit<Self, Error> + 'bound {
> +        let dev = sdev.as_ref();
> +
> +        dev_dbg!(dev, "Probe Rust Serial device bus device driver sample.\n");
> +
> +        if sdev
> +            .set_baudrate(
> +                dev.fwnode()
> +                    .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())

[Severity: Medium]
Will this fail to read the configured speed from standard device trees?

The driver attempts to read a non-standard "baudrate" property instead of the
standard "current-speed" property. This causes a silent fallback to the 115200
default.

Should this use the standard "current-speed" property instead?

> +                    .unwrap_or(115200),
> +            )
> +            .is_err()

[Severity: Medium]
Does strictly rejecting any Err with EINVAL cause the driver to fail probe when
an inexact baud rate is achieved?

The bindings::serdev_device_set_baudrate() function returns the actual
configured speed. The Rust set_baudrate() abstraction returns Err(actual_speed)
if the hardware cannot exactly match the requested speed due to clock dividers.

> +        {
> +            return Err(EINVAL);
> +        }
> +        sdev.set_flow_control(false);
> +        sdev.set_parity(serdev::Parity::None)?;
> +
> +        Ok(Self { sdev: sdev.into() })
> +    }
> +
> +    fn receive<'bound>(
> +        sdev: &'bound serdev::Device<Bound>,
> +        _this: Pin<&Self>,
> +        data: &[u8],
> +    ) -> usize {
> +        let _ = sdev.write_all(data, serdev::Timeout::Max);

[Severity: High]
Can this unbounded blocking write stall the TTY receive worker indefinitely?

The receive() callback executes in the TTY RX workqueue. serdev::Timeout::Max
maps to a 0 timeout in serdev_device_write(), which waits indefinitely. If the
hardware TX buffer fills up due to a slow remote device or flow control, the
worker thread will block forever.

This deadlock of the TTY RX worker leads to an unkillable task and system hang
when cancel_work_sync() is called during module unload.

> +        data.len()
> +    }
> +}

Additionally, I noticed an issue in the underlying serdev abstraction code in
rust/kernel/serdev.rs:

[Severity: High]
Does Adapter::remove_callback() in rust/kernel/serdev.rs fail to drop the
driver's private data during unbind?

During device unbind, remove_callback() uses drvdata_borrow() instead of
drvdata_obtain(). This fails to take ownership of the KBox<PrivateData>,
leaking the memory.

Because PrivateData::drop never runs, bindings::serdev_device_close() is never
called. The C driver core then resets dev->driver_data to NULL. Because the
device is still open, incoming serial data triggers receive_buf_callback(),
which calls drvdata_borrow() on the now-NULL drvdata pointer, causing a panic
when locking the mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-rust_serdev-v13-0-2c4060bc8e30@posteo.de?part=2