Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The `remove_callback` function in the Rust `serdev` abstraction fails to free the driver's private data (`PrivateData`), leading to a memory leak when the device is unbound. - [Medium] The `receive` callback returns the number of bytes written to the Tx buffer rather than the number of bytes consumed from the Rx buffer, which can permanently stall the TTY Rx path. - [Low] The driver queries the non-standard device tree property `baudrate` instead of the standard `current-speed` to configure the serial speed. - [Low] The driver registers an ACPI ID table but this behavior is entirely omitted from the commit message. --
--- Patch [2]: [PATCH v14 2/3] samples: rust: add Rust serial device bus sample device driver --- commit 119f675ea28625315fe04a8ca0cdd59f9d0f26bc 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] Does this description omit the ACPI ID table? Looking at ACPI_TABLE in rust_driver_serdev.rs, the driver also registers an ACPI ID table for matching: kernel::acpi_device_table!( ACPI_TABLE, MODULE_ACPI_TABLE, <SampleDriver as serdev::Driver>::IdInfo, [(acpi::DeviceId::new(c"LNUXBEEF"), ())] ); Could the commit message be updated to mention the ACPI matching behavior as well? > diff --git a/samples/rust/rust_driver_serdev.rs > b/samples/rust/rust_driver_serdev.rs > new file mode 100644 > index 0000000000000..a12b1412db30c > --- /dev/null > +++ b/samples/rust/rust_driver_serdev.rs > @@ -0,0 +1,90 @@ [ ... ] > + 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: Low] Does this code use an incorrect, non-standard device tree property? It looks like it queries c"baudrate" instead of the standard c"current-speed" property, which could cause it to fail to configure the speed on platforms compliant with standard bindings. > + .unwrap_or(115200), > + ) > + .is_err() > + { > + 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 { > + sdev.write(data).unwrap_or_default() as usize [Severity: Medium] Could this permanently stall the TTY Rx path? The receive callback appears to return the number of bytes written to the Tx buffer rather than the number of bytes consumed from the Rx buffer. If the Tx buffer is full, sdev.write() returns 0, and the TTY layer will stop pushing data. Without a mechanism to wake up the Rx path when Tx space becomes available, does this permanently stall the receiver? > + } > +} I also noticed an issue in rust/kernel/serdev.rs during driver unbind: rust/kernel/serdev.rs:remove_callback() { ... let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() }; // SAFETY: No one has exclusive access to private_data.driver. let data = unsafe { &*private_data.driver.get() }; ... T::unbind(sdev, data_pinned); } [Severity: High] Does this code leak the driver's private data? The remove_callback() function appears to fail to free PrivateData because it uses drvdata_borrow() instead of drvdata_obtain() on the teardown path, which would take ownership back and allow the driver data allocation to be dropped when the device is unbound. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
