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

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 4492 bytes --]

On Wed, 2026-09-30 at 14:52 +0100, Gary Guo wrote:
> On Wed Sep 30, 2026 at 1:59 PM BST, Markus Probst wrote:
> > On Wed, 2026-09-30 at 13:13 +0900, FUJITA Tomonori wrote:
> > > 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. Clamp a negative timeout to
> > > 0, as the current code does for a value larger than isize::MAX.
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > > ---
> > >  rust/kernel/serdev.rs | 27 ++++++++++++---------------
> > >  1 file changed, 12 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> > > index 17ca504b7f8d..094503f059ff 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, //
> > > @@ -481,22 +484,19 @@ pub fn set_parity(&self, parity: Parity) -> Result {
> > >      /// [`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 = isize::max(timeout.as_jiffies(), 0);
> > > +
> > >          // 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`.
> > > @@ -547,14 +547,11 @@ pub fn write_flush(&self) {
> > >      ///
> > >      /// Use a timeout of 0 to wait indefinitely.
> > >      #[inline]
> > > -    pub fn wait_until_sent(&self, timeout: Jiffies) {
> > > +    pub fn wait_until_sent(&self, timeout: Delta<Jiffy>) {
> > > +        let timeout = isize::max(timeout.as_jiffies(), 0);
> > > +
> > >          // 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: f1850e443b0e4f2429ddf42a8d5033ea54ae8a90
> > 
> > Both functions have "Use a timeout of 0 to wait indefinitely." inside
> > the rustdoc.
> > 
> > Make sure `Delta::ZERO` is also usable for `Delta<Jiffy>` and replace
> > the 0 in the rustdoc with "[`Delta::<Jiffy>::ZERO`]". You might also
> > remove the "timeout of" part, but I don't mind if it stays.
> 
> I think the API should ideally use `Option<Delta<Jiffies>>` for this case, and
> use `None` to represent indefinite wait.
Yes, this would make it more explicit.
> 
> We might need to round 0 jiffies to 1 to avoid C API change. But ideally the C
> API should be using MAX_JIFFY_OFFSET to mean indefinite..
In both cases MAX_SCHEDULE_TIMEOUT is also accepted (even if not
documented).

At least both pass through such call:

if (timeout == 0)
        timeout = MAX_SCHEDULE_TIMEOUT;

Thanks
- Markus Probst

> 
> Best,
> Gary


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]