[PATCH v2] rust: serdev: use Delta<Jiffy> for timeouts

[PATCH v2] rust: serdev: use Delta<Jiffy> for timeouts

[FUJITA Tomonori]

From: FUJITA Tomonori <fujita.tomonori@gmail.com>

write_all() and wait_until_sent() take the timeout as a raw Jiffies,
which is a c_ulong alias. The type does not show the unit.

Take the timeout as Delta<Jiffy> instead.

The C side treats a timeout of 0 as "wait indefinitely". The Rust API
does not keep that meaning. A timeout of 0 or negative is rounded up
to one jiffy. A caller that wants to wait indefinitely passes
Delta::from_jiffies(isize::MAX). That value is MAX_SCHEDULE_TIMEOUT.
The C side replaces 0 with MAX_SCHEDULE_TIMEOUT before it waits, so
this gives the same wait as a timeout of 0 did before.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
v2:
- Round a timeout of 0 or negative up to one jiffy instead of passing 0,
  which the C side treats as "wait indefinitely" (Gary)
- Document Delta::from_jiffies(isize::MAX) for an indefinite wait
v1: https://lore.kernel.org/rust-for-linux/20260930041339.1551129-1-tomo@flapping.org/
---
 rust/kernel/serdev.rs | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index 17ca504b7f8d..656598602d7a 100644
--- a/rust/kernel/serdev.rs
+++ b/rust/kernel/serdev.rs
@@ -20,7 +20,10 @@
         aref::AlwaysRefCounted,
         Mutex, //
     },
-    time::Jiffies,
+    time::{
+        Delta,
+        Jiffy, //
+    },
     types::{
         Opaque,
         ScopeGuard, //
@@ -433,6 +436,16 @@ fn as_raw(&self) -> *mut bindings::serdev_device {
     }
 }
 
+/// Convert a timeout to the `long` the C side takes.
+///
+/// The C side treats a timeout of 0 as "wait indefinitely", so a timeout of 0 or negative is
+/// rounded up to one jiffy. `isize::MAX` is `MAX_SCHEDULE_TIMEOUT`, so it is passed through
+/// unchanged and the C side waits indefinitely.
+#[inline]
+fn timeout_jiffies(timeout: Delta<Jiffy>) -> isize {
+    isize::max(timeout.as_jiffies(), 1)
+}
+
 impl Device<device::Bound> {
     /// Set the baudrate in bits per second.
     ///
@@ -475,28 +488,26 @@ pub fn set_parity(&self, parity: Parity) -> Result {
     /// [`Device::wait_until_sent`] to make sure the controller write buffer has actually been
     /// emptied.
     ///
-    /// Use a timeout of 0 to wait indefinitely.
+    /// A `timeout` of 0 or negative is rounded up to one jiffy. Pass
+    /// `Delta::from_jiffies(isize::MAX)` to wait indefinitely.
     ///
     /// Returns the number of bytes written (less than `data.len()` if interrupted).
     /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
     /// before any bytes were written. [`kernel::error::code::EINVAL`] if `data.len() > i32::MAX`.
     #[inline]
-    pub fn write_all(&self, data: &[u8], timeout: Jiffies) -> Result<usize> {
+    pub fn write_all(&self, data: &[u8], timeout: Delta<Jiffy>) -> Result<usize> {
         if data.len() > i32::MAX as usize {
             return Err(EINVAL);
         }
 
+        let timeout = timeout_jiffies(timeout);
+
         // SAFETY:
         // - `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
         // - `data.as_ptr()` is guaranteed to be a valid array pointer with the size of
         //   `data.len()`.
         let ret = unsafe {
-            bindings::serdev_device_write(
-                self.as_raw(),
-                data.as_ptr(),
-                data.len(),
-                isize::try_from(timeout).unwrap_or_default(),
-            )
+            bindings::serdev_device_write(self.as_raw(), data.as_ptr(), data.len(), timeout)
         };
         // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
         // which always fit into a `i32`.
@@ -545,16 +556,14 @@ pub fn write_flush(&self) {
     /// After this function, the write buffer of the controller should be empty or the timeout
     /// elapsed.
     ///
-    /// Use a timeout of 0 to wait indefinitely.
+    /// A `timeout` of 0 or negative is rounded up to one jiffy. Pass
+    /// `Delta::from_jiffies(isize::MAX)` to wait indefinitely.
     #[inline]
-    pub fn wait_until_sent(&self, timeout: Jiffies) {
+    pub fn wait_until_sent(&self, timeout: Delta<Jiffy>) {
+        let timeout = timeout_jiffies(timeout);
+
         // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
-        unsafe {
-            bindings::serdev_device_wait_until_sent(
-                self.as_raw(),
-                isize::try_from(timeout).unwrap_or_default(),
-            )
-        };
+        unsafe { bindings::serdev_device_wait_until_sent(self.as_raw(), timeout) };
     }
 }
 

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.43.0

Re: [PATCH v2] rust: serdev: use Delta<Jiffy> for timeouts

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: serdev: outdated timeout type in Driver doctest for write_all()
--

commit 2726c2fee6d94012455c660974ada63f17dd12c7
Author: FUJITA Tomonori <fujita.tomonori@gmail.com>

rust: serdev: use Delta<Jiffy> for timeouts

This commit updates serdev::Device::write_all() and
serdev::Device::wait_until_sent() to accept a Delta<Jiffy> timeout instead
of a raw Jiffies value, rounding non-positive timeouts up to one jiffy.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d6..656598602d7a9 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs

[ ... ]

> @@ -475,28 +488,26 @@ pub fn set_parity(&self, parity: Parity) -> Result {
>      /// [`Device::wait_until_sent`] to make sure the controller write buffer has actually been
>      /// emptied.
>      ///
> -    /// Use a timeout of 0 to wait indefinitely.
> +    /// A `timeout` of 0 or negative is rounded up to one jiffy. Pass
> +    /// `Delta::from_jiffies(isize::MAX)` to wait indefinitely.
>      ///
>      /// Returns the number of bytes written (less than `data.len()` if interrupted).
>      /// [`kernel::error::code::ETIMEDOUT`] or [`kernel::error::code::ERESTARTSYS`] if interrupted
>      /// before any bytes were written. [`kernel::error::code::EINVAL`] if `data.len() > i32::MAX`.
>      #[inline]
> -    pub fn write_all(&self, data: &[u8], timeout: Jiffies) -> Result<usize> {
> +    pub fn write_all(&self, data: &[u8], timeout: Delta<Jiffy>) -> Result<usize> {

[Severity: Low]
Does the rustdoc example on the serdev::Driver trait in rust/kernel/serdev.rs
also need to be updated for this signature change?

///     fn probe<'bound>(
///         sdev: &'bound serdev::Device<Core<'_>>,
///         _id_info: Option<&'bound Self::IdInfo>,
///     ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
///         sdev.set_baudrate(115200);
///         sdev.write_all(b"Hello\n", 0)?;
///         Ok(MyDriver)
///     }

It still passes an integer literal 0 to sdev.write_all(), which fails to
compile when building with CONFIG_RUST_SERIAL_DEV_BUS_ABSTRACTIONS=y and
CONFIG_RUST_KERNEL_DOCTESTS=y.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102005125.626125-1-tomo@flapping.org?part=1