Claude review: rust: add basic serial device bus abstractions

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the main patch. Several observations:

**1. Timeout::Max maps to 0, which is wrong:**

```rust
+impl Timeout {
+    fn into_jiffies(self) -> isize {
+        match self {
+            ...
+            Self::Max => 0,
+        }
+    }
+}
```

`MAX_SCHEDULE_TIMEOUT` is `LONG_MAX` (i.e., `c_long::MAX`), not 0. A timeout of 0 means "don't wait at all" in `serdev_device_write()`. The `Timeout::Max` variant's doc says "Wait as long as possible" and references `MAX_SCHEDULE_TIMEOUT`, but then passes 0, which is the exact opposite. This should be:

```rust
Self::Max => isize::MAX, // or kernel::task::MAX_SCHEDULE_TIMEOUT
```

**2. Race condition in probe/receive synchronization:**

The `PrivateData` struct uses a `Completion` to synchronize `receive_buf_callback` with probe completion:

```rust
+        private_data.probe_complete.wait_for_completion();
+
+        // SAFETY: No one has exclusive access to `private_data.error`.
+        if unsafe { *private_data.error.get() } {
+            return length;
+        }
```

The `error` field uses `UnsafeCell<bool>` without any synchronization primitive. The SAFETY comment "No one has exclusive access to `private_data.error`" doesn't actually justify the safety — it's the *opposite* of what you'd want. The concern is whether the write to `error` in probe is guaranteed to be visible in `receive_buf_callback` on another thread. The `Completion` itself should provide a memory barrier (via `complete_all`/`wait_for_completion`), so the ordering is likely correct in practice, but this should be documented explicitly. Using an `AtomicBool` would be cleaner and avoid the need for `unsafe` altogether.

**3. `set_baudrate` returns `Result<(), u32>` instead of idiomatic kernel `Result`:**

```rust
+    pub fn set_baudrate(&self, speed: u32) -> Result<(), u32> {
```

Returning the actual baudrate set on failure is useful information, but the `Result<(), u32>` type is non-standard for kernel Rust code. The C function `serdev_device_set_baudrate` returns the actual speed set (which may differ from requested). The sample driver checks `is_err()` and returns `EINVAL`, losing the actual value. Consider either returning `Result<u32>` (the actual speed set) or `Result` with a standard error code.

**4. `write` returns `Result<u32>` but `write_all` returns `Result<usize>` — inconsistent types:**

```rust
+    pub fn write_all(&self, data: &[u8], timeout: Timeout) -> Result<usize> {
...
+    pub fn write(&self, data: &[u8]) -> Result<u32> {
```

Both should consistently use `usize` for byte counts.

**5. The `write` function wraps `serdev_device_write_buf` which returns `ssize_t`:**

```rust
+        let ret =
+            unsafe { bindings::serdev_device_write_buf(self.as_raw(), data.as_ptr(), data.len()) };
+        to_result(ret as i32).map(|()| ret.unsigned_abs())
```

Casting `ssize_t` (which is `isize`/`i64` on 64-bit) to `i32` could truncate positive values. Since this returns the number of bytes written, if `data.len()` > `i32::MAX`, the cast would produce an incorrect error. The same issue exists in `write_all`. Use `ret as i32` carefully — `to_result` expects a C `int`, but the value range should be OK in practice since write buffers won't exceed `i32::MAX`.

**6. `set_baudrate`, `set_flow_control`, `set_parity`, `write`, `write_all`, `write_flush`, `wait_until_sent` are all on `Device<Bound>`, but probe receives `Device<Core>`:**

Looking at the sample driver, `sdev.set_baudrate(...)` is called in `probe` which receives `&Device<Core>`. This compiles because of `impl_device_context_deref` which lets `Device<Core>` deref to `Device<Bound>`. This is the standard pattern, so it's fine.

**7. Typo in doc comments:**

```rust
+    /// Even partiy.
```

Should be "parity".

```rust
+    /// [ Device::wait_until_sent`]
```

Missing `[` — should be `[`Device::wait_until_sent`]`. This appears twice.

**8. The `Kconfig` entry `RUST_SERIAL_DEV_BUS_ABSTRACTIONS` should probably be inside the `if SERIAL_DEV_BUS` block:**

```diff
+config RUST_SERIAL_DEV_BUS_ABSTRACTIONS
+	bool "Rust Serial device bus abstractions"
+	depends on RUST
+	select SERIAL_DEV_BUS
```

It uses `select SERIAL_DEV_BUS` rather than `depends on`, which means enabling the Rust abstractions will force-enable `SERIAL_DEV_BUS`. This is placed *before* `if SERIAL_DEV_BUS`, which is a bit odd. Consider using `depends on SERIAL_DEV_BUS` instead.

**9. `remove_callback` doesn't call `drvdata_obtain` to free the driver data:**

```rust
+    extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
+        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal>>() };
+        let data = unsafe { sdev.as_ref().drvdata_borrow::<T>() };
+        T::unbind(sdev, data);
+    }
```

It borrows but never takes ownership of the drvdata. The data appears to be freed by devres when the device is released, since `set_drvdata` stores it via `dev_set_drvdata`. But wait — `set_drvdata` stores data via `into_foreign()` and there's no corresponding `from_foreign()` call. The data should be cleaned up by the devres system if `set_drvdata` registers a devres action, but looking at the `device.rs` code, `set_drvdata` does *not* register a devres — it just calls `dev_set_drvdata`. This looks like a **memory leak** — the `Pin<KBox<T>>` stored via `set_drvdata` is never freed. Other bus drivers (platform, etc.) likely call `drvdata_obtain` in their remove callbacks. This needs to be verified.

---
Generated by Claude Code Patch Reviewer