On Wed, Oct 12, 2022 at 04:52:36PM +0000, Thomas.Kastner wrote:
> Guenter, thank you for your detailed review. I will address the issues and resubmit. Comments below.
>
> ...
> > > +#include <linux/uaccess.h>
> > > +#include <linux/spinlock.h>
> >
> > Alphabetic include file order, please.
>
> Copied from existing driver, will fix regardless.
>
That is never an good or even acceptable argument. Old drivers will always
have lots of oddities and don't typically use more recently introduced
helper functions. Case in point: both watchdog_init_timeout() and
watchdog_stop_on_unregister() were introduced after your example driver
was added to the kernel.
Also, please check if all the include files are really needed. For example,
I don't think <linux/uaccess.h> is needed.
> > > +#define DEFAULT_TIME 60
> > > +
> > > +static unsigned int timeout = DEFAULT_TIME;
> >
> > This should be set to 0. The default timeout should be set
> > in struct watchdog_device.
>
> Copied from existing driver, will fix regardless.
>
> > > + /* reset watchdog */
> > > + outb(EC_CMD_WDT_RESET, EC_ADDR_CMD);
> > > + usleep_range(10000, 11000);
> >
> > Is this really needed ? If so, write an accessor function.
> > It would be preferred to have that accessor function wait
> > before accesses (cache the time of the last access and wait
> > if not enough time has expired). This would prevent unnecessary
> > wait operations in places like this one, where only a single
> > access happens.
>
> Good point. Unfortunately the delay between individual accesses is required.
> My reasoning here was that the watchdog reset function typically is called in many-second intervals only, so the potentially unnecessary wait therefore would be negligible and not justify the overhead of an accessor function.
>
The problem is not the overhead in the code, but code pollution.
The argument should be that the code is not called often, thus
using an accessor function is cleaner and its runtime overhead
is negligible. Also, if that _was_ a concern, one could always
declare the accessor function as "static inline".
The the same time, forcing userspace to wait for 10ms for no good
reason is never a good idea, and "it is not called that often" is
not a valid argument. Add a thousand "not called that often" pieces
of code to the kernel, and userspace won't do anything but wait
for nothing.
Thanks,
Guenter
> > > + val = t*10;
> >
> > Space before and after '*'. Please run checkpatch; that should tell you.
>
> I did, but either the script missed this or I did. Will fix.
>
> > > +static struct watchdog_device adv_ec_wdt_dev = {
> > > + .info = &adv_ec_wdt_info,
> > > + .ops = &adv_ec_wdt_ops,
> > > + .min_timeout = MIN_TIME,
> > > + .max_timeout = MAX_TIME,
> >
> > .timeout should be set to the default timeout.
>
> Copied from existing driver, will fix regardless.
>
> > > +static void adv_ec_wdt_remove(struct device *dev, unsigned int id)
> > > +{
> > > + /* stop the watchdog */
> > > + adv_ec_wdt_stop(NULL);
> >
> > Call watchdog_stop_on_unregister() before registering the watchdog
> > instead. Also, again, the comment doesn't add value.
> >
> > > +
> > > + /* watchdog_unregister() and release_region() are called automatically
> > */
> >
> > Also unnecessary (implied with devm_). And you don't actually
> > need a remove function when using watchdog_stop_on_unregister().
>
> Thanks for pointing this out, I had missed that this function & feature exist.
>
> > > + pr_err("Watchdog timer: value of timeout %d (dec) "
> > > + "is out of range from %d to %d (dec)\n",
> > > + timeout, MIN_TIME, MAX_TIME);
> > > + return -EINVAL;
> >
> > This does not belong here. The probe function should call
> > watchdog_init_timeout() to set and validate the timeout module
> > parameter, and keep the default (instead of failing) if it is
> > out of range.
>
> Copied from existing driver, will fix regardless.
