Re: [PATCH v14 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 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 <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]
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/20260715-rust_serdev-v14-0-b4ed5c93606f@posteo.de?part=2