[RFC/PATCH v3 1/2] watchdog_ioctl: Add decoding of WDIOC_GETSUPPORT and WDIOC_SETOPTIONS ioctls

[Sahil]

Hi,

Thank you for your review.

On Sunday, August 25, 2024 8:04:38 AM GMT+5:30 Eugene Syromyatnikov wrote:
> On Thu, Aug 08, 2024 at 01:29:55AM +0530, Sahil Siddiq wrote:
> > [...]
> > diff --git a/src/watchdog_ioctl.c b/src/watchdog_ioctl.c
> > index b88094f0d..5f9b1824a 100644
> > --- a/src/watchdog_ioctl.c
> > +++ b/src/watchdog_ioctl.c
> > @@ -9,6 +9,9 @@
> > 
> >  #include <linux/watchdog.h>
> > 
> > +#include "xlat/watchdog_ioctl_setoptions.h"
> > 
> > +#include "xlat/watchdog_ioctl_support_options.h"
> 
> I'd rather name this one "watchdog_ioctl_flags", they are seemingly
> used in WDIOC_GET{,BOOT}STATUS as well.
> 
> > +
> >  #define XLAT_MACROS_ONLY
> >  #include "xlat/watchdog_ioctl_cmds.h"
> >  #undef XLAT_MACROS_ONLY
> > @@ -17,7 +20,29 @@ int
> >  watchdog_ioctl(struct tcb *const tcp, const unsigned int code,
> >           const kernel_ulong_t arg)
> >  {
> > +      struct watchdog_info ident;
> > +      unsigned int options;
> > +
> >        switch (code) {
> > +      case WDIOC_GETSUPPORT:
> > +              if (entering(tcp))
> > +                      return 0;
> > +              tprint_arg_next();
> 
> It's preferred to first call tprint_arg_next, and only then turn, so it
> looks nicer when a tracee switch occurs, like so:
> 
> 	if (entering(tcp)) {
> 		tprint_arg_next();
> 		return 0;
> 	}
> 
> So it yields "ioctl(..., <unfinished ...>>".
> 
> > +              if (!umove_or_printaddr(tcp, arg, &ident)) {
>
> You can opt for
> 
> 	if (umove_or_printaddr(tcp, arg, &ident))
> 		break;
> 
> here to avoid indenting the successful read case.
> 
> > +                      tprint_struct_begin();
> > +                      PRINT_FIELD_FLAGS(ident, options, watchdog_ioctl_support_options,
> 
> An overly long line.
> 
> > +                                        "WDIOF_???");
> > +                      tprint_struct_next();
> >
> > +                      if (abbrev(tcp))
> > +                              tprint_more_data_follows();
> > +                      else {
> 
> It's a bit nicer (and safer for possible future changes) to have
> the curly braces for the if branch as well.

I'll incorporate these changes in the next patch.

> > +                              PRINT_FIELD_U(ident, firmware_version);
> 
> I wonder if PRINT_FIELD_X makes more sense here—sometimes developers
> tend to encode major/minor revisions as multiples of 16.  Do you have
> any insight in what actually appears here in the wild, by chance?
>

I have an intel TCO watchdog driver and it's firmware_version field is set to 6.
In the linux kernel in "drivers/watchdog/iTCO_wdt.c", I see that this driver can
take values from 0 through 6 as the firmware version [1].

For several drivers it's hardcoded to 0 (eg: Hardware Watchdog for SGI IP22) or
1 (eg: Wafer 5823 WDT).

For the PowerPC Book-E watchdog driver, the firmware_version is defined as the
cpu spec's "pvr_value" [2] which can take values 0x8020, 0x8021 [3].

I am beginning to think PRINT_FIELD_X will be neater for the firmware_version.
What are your thoughts on this?

Thanks,
Sahil

[1] https://github.com/torvalds/linux/blob/master/drivers/watchdog/iTCO_wdt.c#L546
[2] https://github.com/torvalds/linux/blob/master/drivers/watchdog/booke_wdt.c#L225
[3] https://www.nxp.com/docs/en/reference-manual/E500CORERM.pdf (Section 1.2)