Re: [PATCH] watchdog: new option to skip ping in close

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 08/06/2012 06:25 PM, David Teigland wrote:
> On Sat, Aug 04, 2012 at 11:08:24AM +0200, Hans de Goede wrote:
>> Why on earth are you putting that if block there and not simply putting
>> the command in the switch-case where it belongs? If you were afraid watchdog_ioctl_op
>> could cause issues, then maybe you should have studied the code?
>>
>> That would have thought you that it will always return -ENOIOCTLCMD for commands it
>> does not know how to handle, so it won't interfere with adding new standard commands.
>>
>> Adding this if block there is completely unnecessary and quite ugly IMHO.
>
> It is ugly, but it's also necessary for a couple reasons that you may have
> missed at first glance.  First, this ioctl is addressing behavior that
> exists in the common framework layer, not in individual drivers, so it
> doesn't make too much sense to pass down to the drivers.

All the ioctls handled by the switch-case are handled primarily by the
core layer (doing things like range checking for the timeout value, etc),
WDIOC_NOCLOSEPING is no different!

> That being said, we could do so if ENOIOCTLCMD worked as you suggest, but
> it doesn't quite.  Drivers with an ioctl op will return ENOTTY,

No they don't, if they would do that all the standard ioctls would not work,
since watchdog_dev.c: watchdog_ioctl() has:

         err = watchdog_ioctl_op(wdd, cmd, arg);
         if (err != -ENOIOCTLCMD)
                 return err;

So if they returned ENOTTY we would never get into the switch case.

 > in which
> case the switch statement would work, but drivers with no ioctl op (e.g.
> softdog) cause watchdog_ioctl_op to return ENOIOCTLCMD, causing the switch
> statement to be skipped.

<sigH> it is the other way around not returning ENOIOCTLCMD (ie returning ENOTTY)
causes the switch case to be skipped, not the other way around. The watchdog_ioctl_op
driver callback is there to add custom driver specific ioctls, for all other ioctls
the watchdog_ioctl_op function must return ENOIOCTLCMD, this get turned into a
ENOTTY by the switch case if the ioctl is not one of the standard ones either,
so with WDIOC_NOCLOSEPING being a new standard ioctl it should, it *must* go in
the switch case just like all the other ioctls.

> In the end, we need WDIOC_NOCLOSEPING to be processed by the core,
> independent of what drivers do, since as I explained this is targeting the
> behavior of the core framework, not the drivers.

Right, and it will be processed by the core if it is in the switch-case because:

watchdog_ioctl_op will return ENOIOCTLCMD for non driver specific custom ioctls,
causing watchdog_ioctl() to continue into the switch case for standard ioctls!

Regards,

Hans