On Sun, 2026-07-12 at 17:19 +0000, [email protected] wrote: > 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 <[email protected]> > > 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? There is a todo comment in serdev_device_match, so I will leave it for now.
>
> > 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?
Just an example.
>
> > + .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.
Just an example.
>
> > + {
> > + 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.
Might or might not be an issue, I have to look into this. But its still
an example.
>
> > + 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.
driver.rs should take care of dropping PrivateData, which will also
drop the underlying driver data.
>
> 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.
See above.
signature.asc
Description: This is a digitally signed message part
