Re: [PATCH v2 1/3] rust: watchdog: add watchdog device abstraction

Thank you for your contribution! Sashiko AI review found 5 potential issue(=
s) to consider:
- [High] Safe `Device::set_timeout` and `Device::timeout` cause data races =
(Undefined Behavior).
- [High] Missing `set_pretimeout` method in `Device`.
- [High] Missing commonly required driver fields (`bootstatus` and `min_hw_=
heartbeat_ms`) in `Options`.
- [Low] Missing `#[inline]` annotations on small abstraction methods.
- [Low] Imports do not follow the kernel vertical import style.
--

--- Patch [1]: [PATCH v2 1/3] rust: watchdog: add watchdog device abstracti=
on ---
commit a2fbd8297d0a9de19e082cbcf7ecd7a0e4b836b1
Author: Artem Lytkin <[email protected]>
Subject: rust: watchdog: add watchdog device abstraction

This commit adds Rust abstractions for the Linux watchdog subsystem, enabli=
ng
watchdog drivers to be written in Rust. It introduces wrappers for the
watchdog device, identity info, and options, along with a trait for driver
operations and a registration mechanism.

> diff --git a/rust/kernel/watchdog.rs b/rust/kernel/watchdog.rs
> new file mode 100644
> index 0000000000000..155eecc679238
> --- /dev/null
> +++ b/rust/kernel/watchdog.rs
> @@ -0,0 +1,522 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Watchdog device support.
> +//!
> +//! C headers: [`include/linux/watchdog.h`](srctree/include/linux/watchd=
og.h).
> +
> +use crate::{bindings, device, error::*, prelude::*, types::Opaque};

[Severity: Low]
This isn't a bug, but should the imports here follow the kernel vertical
import style as documented in the general coding guidelines?

> +use core::marker::PhantomData;
> +

[ ... ]

> +impl Device {
> +    /// Creates a new [`Device`] reference from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `ptr` must point at a valid `watchdog_device`.
> +    /// - The returned reference must not outlive the callback invocatio=
n.
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::watchdog_device) -> &'a S=
elf {

[Severity: Low]
This isn't a bug, but should small or forwarding functions in abstractions,
such as from_raw(), as_raw(), and the various getter/setter methods below,
be annotated with #[inline] according to the Rust subsystem guidelines?

> +        // CAST: `Self` is a `repr(transparent)` wrapper around
> +        // `bindings::watchdog_device`.
> +        let ptr =3D ptr.cast::<Self>();
> +        // SAFETY: By the function requirements the pointer is valid.
> +        unsafe { &*ptr }
> +    }
> +
> +    /// Returns a raw pointer to the underlying `watchdog_device`.
> +    fn as_raw(&self) -> *mut bindings::watchdog_device {
> +        self.0.get()
> +    }
> +
> +    /// Returns the current timeout in seconds.
> +    pub fn timeout(&self) -> u32 {
> +        // SAFETY: The struct invariant ensures the pointer is valid. The
> +        // read of this `unsigned int` field mirrors how the C core and C
> +        // drivers access it without synchronisation.
> +        unsafe { (*self.as_raw()).timeout }
> +    }
> +
> +    /// Sets the current timeout in seconds.
> +    pub fn set_timeout(&self, timeout: u32) {
> +        // SAFETY: The struct invariant ensures the pointer is valid. The
> +        // field lives in an `Opaque`, so writing through a shared refer=
ence
> +        // is allowed; the C core accesses this field the same way.
> +        unsafe { (*self.as_raw()).timeout =3D timeout };
> +    }

[Severity: High]
Since the commit message notes that callbacks can run concurrently without
holding a lock, does exposing these safe read and write methods cause a data
race when accessed from multiple threads?

In Rust, unsynchronized concurrent access where at least one is a write
constitutes undefined behavior. Should these methods use volatile or atomic
accesses to mirror READ_ONCE/WRITE_ONCE, or require caller synchronization?

> +
> +    /// Returns the current pretimeout in seconds.
> +    pub fn pretimeout(&self) -> u32 {
> +        // SAFETY: See `timeout`.
> +        unsafe { (*self.as_raw()).pretimeout }
> +    }

[Severity: High]
If a driver provides a set_pretimeout callback, the C watchdog core expects
the driver to update the internal wdd->pretimeout field upon success.

Since the Device abstraction only provides a getter for pretimeout, how can=
 a
Rust driver correctly update this core state without a set_pretimeout() met=
hod?

> +
> +    /// Returns the minimum timeout in seconds.
> +    pub fn min_timeout(&self) -> u32 {

[ ... ]

> +pub struct Options {
> +    /// The default timeout in seconds.
> +    pub timeout: u32,
> +    /// The minimum settable timeout in seconds.
> +    pub min_timeout: u32,
> +    /// The maximum settable timeout in seconds.
> +    pub max_timeout: u32,
> +    /// Hardware limit for the maximum timeout, in milliseconds.
> +    /// Zero means no hardware limit.
> +    pub max_hw_heartbeat_ms: u32,
> +    /// If `true`, the watchdog cannot be stopped once started.
> +    pub nowayout: bool,
> +    /// If `true`, the watchdog is stopped on system reboot. Requires a
> +    /// [`WatchdogOps::stop`] implementation.
> +    pub stop_on_reboot: bool,
> +}

[Severity: High]
Are commonly required driver fields like bootstatus and min_hw_heartbeat_ms
missing from this Options struct?

Because missing fields are zero-initialized in Registration::register() and
there are no setters in Device, are drivers unable to configure these stand=
ard
features, such as reporting WDIOF_CARDRESET?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260723161529.2375=
[email protected]?part=3D1