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? > 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/[email protected]?part=2
