| Message ID | 20190731174252.18041-5-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Ziirave_wdt driver fixes | expand |
On Wed, Jul 31, 2019 at 10:42:51AM -0700, Andrey Smirnov wrote: > Reprogramming bootloader on watchdog MCU will result in reported > default timeout value of "0". That in turn will be unnecesarily unnecessarily > rejected by the driver as invalid device (-ENODEV). Simplify probe to > just read stored timeout value, clamp it to an acceptable range and > program the value unconditionally to fix the above. > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > Cc: Chris Healy <cphealy@gmail.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Rick Ramstetter <rick@anteaterllc.com> > Cc: linux-watchdog@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/watchdog/ziirave_wdt.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c > index 89ce6982ba53..33c8d2eadada 100644 > --- a/drivers/watchdog/ziirave_wdt.c > +++ b/drivers/watchdog/ziirave_wdt.c > @@ -667,22 +667,18 @@ static int ziirave_wdt_probe(struct i2c_client *client, > return val; > } > > - if (val < ZIIRAVE_TIMEOUT_MIN) > - return -ENODEV; > - > - w_priv->wdd.timeout = val; > - } else { > - ret = ziirave_wdt_set_timeout(&w_priv->wdd, > - w_priv->wdd.timeout); > - if (ret) { > - dev_err(&client->dev, "Failed to set timeout\n"); > - return ret; > - } > + w_priv->wdd.timeout = clamp(val, ZIIRAVE_TIMEOUT_MIN, > + ZIIRAVE_TIMEOUT_MAX); Are you sure ? Effectively that will set the timeout to the minimum, ie three seconds. It might be better to define and set some default. Your call, of course. Thanks, Guenter > + } > > - dev_info(&client->dev, "Timeout set to %ds\n", > - w_priv->wdd.timeout); > + ret = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout); > + if (ret) { > + dev_err(&client->dev, "Failed to set timeout\n"); > + return ret; > } > > + dev_info(&client->dev, "Timeout set to %ds\n", w_priv->wdd.timeout); > + > watchdog_set_nowayout(&w_priv->wdd, nowayout); > > i2c_set_clientdata(client, w_priv); > -- > 2.21.0 >
On Wed, Jul 31, 2019 at 10:42:51AM -0700, Andrey Smirnov wrote: > Reprogramming bootloader on watchdog MCU will result in reported > default timeout value of "0". That in turn will be unnecesarily > rejected by the driver as invalid device (-ENODEV). Simplify probe to > just read stored timeout value, clamp it to an acceptable range and > program the value unconditionally to fix the above. > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > Cc: Chris Healy <cphealy@gmail.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Rick Ramstetter <rick@anteaterllc.com> > Cc: linux-watchdog@vger.kernel.org > Cc: linux-kernel@vger.kernel.org I have not heard back on the question I had about selecting the minimum timeout and not a more reasonable default. Since the code itself is technically correct, marking the patch as Reviewed-by: Guenter Roeck <linux@roeck-us.net> Guenter > --- > drivers/watchdog/ziirave_wdt.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c > index 89ce6982ba53..33c8d2eadada 100644 > --- a/drivers/watchdog/ziirave_wdt.c > +++ b/drivers/watchdog/ziirave_wdt.c > @@ -667,22 +667,18 @@ static int ziirave_wdt_probe(struct i2c_client *client, > return val; > } > > - if (val < ZIIRAVE_TIMEOUT_MIN) > - return -ENODEV; > - > - w_priv->wdd.timeout = val; > - } else { > - ret = ziirave_wdt_set_timeout(&w_priv->wdd, > - w_priv->wdd.timeout); > - if (ret) { > - dev_err(&client->dev, "Failed to set timeout\n"); > - return ret; > - } > + w_priv->wdd.timeout = clamp(val, ZIIRAVE_TIMEOUT_MIN, > + ZIIRAVE_TIMEOUT_MAX); > + } > > - dev_info(&client->dev, "Timeout set to %ds\n", > - w_priv->wdd.timeout); > + ret = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout); > + if (ret) { > + dev_err(&client->dev, "Failed to set timeout\n"); > + return ret; > } > > + dev_info(&client->dev, "Timeout set to %ds\n", w_priv->wdd.timeout); > + > watchdog_set_nowayout(&w_priv->wdd, nowayout); > > i2c_set_clientdata(client, w_priv); > -- > 2.21.0 >
On Wed, Jul 31, 2019 at 11:09 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Jul 31, 2019 at 10:42:51AM -0700, Andrey Smirnov wrote: > > Reprogramming bootloader on watchdog MCU will result in reported > > default timeout value of "0". That in turn will be unnecesarily > > unnecessarily > > > rejected by the driver as invalid device (-ENODEV). Simplify probe to > > just read stored timeout value, clamp it to an acceptable range and > > program the value unconditionally to fix the above. > > > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > Cc: Chris Healy <cphealy@gmail.com> > > Cc: Guenter Roeck <linux@roeck-us.net> > > Cc: Rick Ramstetter <rick@anteaterllc.com> > > Cc: linux-watchdog@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > --- > > drivers/watchdog/ziirave_wdt.c | 22 +++++++++------------- > > 1 file changed, 9 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c > > index 89ce6982ba53..33c8d2eadada 100644 > > --- a/drivers/watchdog/ziirave_wdt.c > > +++ b/drivers/watchdog/ziirave_wdt.c > > @@ -667,22 +667,18 @@ static int ziirave_wdt_probe(struct i2c_client *client, > > return val; > > } > > > > - if (val < ZIIRAVE_TIMEOUT_MIN) > > - return -ENODEV; > > - > > - w_priv->wdd.timeout = val; > > - } else { > > - ret = ziirave_wdt_set_timeout(&w_priv->wdd, > > - w_priv->wdd.timeout); > > - if (ret) { > > - dev_err(&client->dev, "Failed to set timeout\n"); > > - return ret; > > - } > > + w_priv->wdd.timeout = clamp(val, ZIIRAVE_TIMEOUT_MIN, > > + ZIIRAVE_TIMEOUT_MAX); > > Are you sure ? Effectively that will set the timeout to the minimum, > ie three seconds. It might be better to define and set some default. > Your call, of course. > It doesn't really matter in my use-case (set timeout is a no-op), but it sounds like a better approach, so I'll change it in v2. Thanks, Andrey Smirnov
diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c index 89ce6982ba53..33c8d2eadada 100644 --- a/drivers/watchdog/ziirave_wdt.c +++ b/drivers/watchdog/ziirave_wdt.c @@ -667,22 +667,18 @@ static int ziirave_wdt_probe(struct i2c_client *client, return val; } - if (val < ZIIRAVE_TIMEOUT_MIN) - return -ENODEV; - - w_priv->wdd.timeout = val; - } else { - ret = ziirave_wdt_set_timeout(&w_priv->wdd, - w_priv->wdd.timeout); - if (ret) { - dev_err(&client->dev, "Failed to set timeout\n"); - return ret; - } + w_priv->wdd.timeout = clamp(val, ZIIRAVE_TIMEOUT_MIN, + ZIIRAVE_TIMEOUT_MAX); + } - dev_info(&client->dev, "Timeout set to %ds\n", - w_priv->wdd.timeout); + ret = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout); + if (ret) { + dev_err(&client->dev, "Failed to set timeout\n"); + return ret; } + dev_info(&client->dev, "Timeout set to %ds\n", w_priv->wdd.timeout); + watchdog_set_nowayout(&w_priv->wdd, nowayout); i2c_set_clientdata(client, w_priv);
Reprogramming bootloader on watchdog MCU will result in reported default timeout value of "0". That in turn will be unnecesarily rejected by the driver as invalid device (-ENODEV). Simplify probe to just read stored timeout value, clamp it to an acceptable range and program the value unconditionally to fix the above. Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: Chris Healy <cphealy@gmail.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Rick Ramstetter <rick@anteaterllc.com> Cc: linux-watchdog@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- drivers/watchdog/ziirave_wdt.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)