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

[PATCH v3] 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.

Reviewed-by: Markus Probst <markus.probst@posteo.de>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
v3:
- Fix the serdev::Driver doctest example, which still passed 0
  (sashiko, Markus)
- Add Markus' Reviewed-by
v2: https://lore.kernel.org/rust-for-linux/20261102005125.626125-1-tomo@flapping.org/
- 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 | 48 ++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index 17ca504b7f8d..acad9c3cc910 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, //
@@ -316,7 +319,8 @@ macro_rules! module_serdev_device_driver {
 ///         Core, //
 ///     },
 ///     of,
-///     serdev, //
+///     serdev,
+///     time::Delta, //
 /// };
 ///
 /// struct MyDriver;
@@ -349,7 +353,7 @@ macro_rules! module_serdev_device_driver {
 ///         _id_info: Option<&'bound Self::IdInfo>,
 ///     ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
 ///         sdev.set_baudrate(115200);
-///         sdev.write_all(b"Hello\n", 0)?;
+///         sdev.write_all(b"Hello\n", Delta::from_jiffies(isize::MAX))?;
 ///         Ok(MyDriver)
 ///     }
 /// }
@@ -433,6 +437,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 +489,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 +557,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 v3] rust: serdev: use Delta<Jiffy> for timeouts

[sashiko-bot]

> 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.
> 
> Reviewed-by: Markus Probst <markus.probst@posteo.de>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Sashiko has reviewed this patch and found no issues.

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