| Message ID | 20190416102515.12269-7-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | watchdog: refactor init_timeout and update users | expand |
On Tue, Apr 16, 2019 at 12:25:05PM +0200, Wolfram Sang wrote: > The core will print out details now. > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/watchdog/hpwdt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > index ef30c7e9728d..db1bf6f546ae 100644 > --- a/drivers/watchdog/hpwdt.c > +++ b/drivers/watchdog/hpwdt.c > @@ -311,8 +311,7 @@ static int hpwdt_init_one(struct pci_dev *dev, > goto error_init_nmi_decoding; > > watchdog_set_nowayout(&hpwdt_dev, nowayout); > - if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL)) > - dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin); > + watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL); I applied patches 1,2 & 6 in testing. Note, that hpwdt is passing NULL as the third parameter to watchdog_init_timeout(). The second patch in this series is using "dev" as input to dev_err and dev_warn. This results in the following in dmesg when trying to load hpwdt w/ an invalid soft_margin: [ 80.848160] (NULL device *): driver supplied timeout (4294967295) out of range [ 80.855429] (NULL device *): falling back to default timeout (30) if the call in hpwdt driver is changed to: if (watchdog_init_timeout(&hpwdt_dev, soft_margin, &dev->dev)) We see the message like we'd desire: [ 2061.167100] hpwdt 0000:01:00.0: driver supplied timeout (4294967295) out of range [ 2061.174633] hpwdt 0000:01:00.0: falling back to default timeout (30) watchdog_init_timeout() uses dev to "try to get the timeout_sec property" I am not familiar with this part of the core and what effect having the hpwdt driver pass in dev to watchdog_init_timeout would have. (I.e. is the change safe?) Guenter, can you help on this question? Note, hpwdt isn't the only watch dog device that is passing NULL to watchdog_init_timeout. Thanks, Jerry > > if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) { > dev_warn(&dev->dev, "timeout <= pretimeout. Setting pretimeout to zero\n"); > -- > 2.11.0
On Tue, Apr 16, 2019 at 02:34:31PM -0600, Jerry Hoemann wrote: > On Tue, Apr 16, 2019 at 12:25:05PM +0200, Wolfram Sang wrote: > > The core will print out details now. > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > drivers/watchdog/hpwdt.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > > index ef30c7e9728d..db1bf6f546ae 100644 > > --- a/drivers/watchdog/hpwdt.c > > +++ b/drivers/watchdog/hpwdt.c > > @@ -311,8 +311,7 @@ static int hpwdt_init_one(struct pci_dev *dev, > > goto error_init_nmi_decoding; > > > > watchdog_set_nowayout(&hpwdt_dev, nowayout); > > - if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL)) > > - dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin); > > + watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL); > > I applied patches 1,2 & 6 in testing. > > Note, that hpwdt is passing NULL as the third parameter to watchdog_init_timeout(). > > The second patch in this series is using "dev" as input to dev_err and dev_warn. > > This results in the following in dmesg when trying to load hpwdt w/ an invalid soft_margin: > > > [ 80.848160] (NULL device *): driver supplied timeout (4294967295) out of range > [ 80.855429] (NULL device *): falling back to default timeout (30) > Good find. Thanks a lot for testing! We'll have to address this. Wolfram - it looks like we'll need separate error message handling for situations where dev is NULL. We may have to leave it up to the parent after all to display a message in that case (since we do want to see the driver name). > > if the call in hpwdt driver is changed to: > > if (watchdog_init_timeout(&hpwdt_dev, soft_margin, &dev->dev)) > > > We see the message like we'd desire: > > [ 2061.167100] hpwdt 0000:01:00.0: driver supplied timeout (4294967295) out of range > [ 2061.174633] hpwdt 0000:01:00.0: falling back to default timeout (30) > > > > watchdog_init_timeout() uses dev to "try to get the timeout_sec property" > > I am not familiar with this part of the core and what effect having the hpwdt > driver pass in dev to watchdog_init_timeout would have. (I.e. is the > change safe?) > Yes, in general it is safe. watchdog_init_timeout() only uses dev to extract a timeout value from devicetree (and now to display error messages). > Guenter, can you help on this question? > > Note, hpwdt isn't the only watch dog device that is passing NULL to > watchdog_init_timeout. > That is indeed a problem: the pointer will be NULL if there is no parent device (such as in softdog.c). Otherwise it should never be NULL. Thanks, Guenter
Hi Jerry, > I applied patches 1,2 & 6 in testing. > > Note, that hpwdt is passing NULL as the third parameter to watchdog_init_timeout(). > > The second patch in this series is using "dev" as input to dev_err and dev_warn. > > This results in the following in dmesg when trying to load hpwdt w/ an invalid soft_margin: > > > [ 80.848160] (NULL device *): driver supplied timeout (4294967295) out of range > [ 80.855429] (NULL device *): falling back to default timeout (30) Thank you for this report. Yes, using 'dev' blindly is a bug. > if the call in hpwdt driver is changed to: > > if (watchdog_init_timeout(&hpwdt_dev, soft_margin, &dev->dev)) > > > We see the message like we'd desire: > > [ 2061.167100] hpwdt 0000:01:00.0: driver supplied timeout (4294967295) out of range > [ 2061.174633] hpwdt 0000:01:00.0: falling back to default timeout (30) The above observation makes sense, but I think we should fix the core code and not the hpwdt driver. My suggestion would be to add something like this to watchdog_init_timeout(): struct device *err_dev = dev ?: wdd->parent; And then use err_dev for all the printing. Guenter? Regards, Wolfram
> That is indeed a problem: the pointer will be NULL if there is no parent > device (such as in softdog.c). Otherwise it should never be NULL. Okay, this spoils my err_dev solution. So, we probably go this route then: pr_<errlvl>("watchdog%d: <err_msg>\n", wdd->id); ?
On Tue, Apr 16, 2019 at 10:50:03PM +0200, Wolfram Sang wrote: > Hi Jerry, > > > I applied patches 1,2 & 6 in testing. > > > > Note, that hpwdt is passing NULL as the third parameter to watchdog_init_timeout(). > > > > The second patch in this series is using "dev" as input to dev_err and dev_warn. > > > > This results in the following in dmesg when trying to load hpwdt w/ an invalid soft_margin: > > > > > > [ 80.848160] (NULL device *): driver supplied timeout (4294967295) out of range > > [ 80.855429] (NULL device *): falling back to default timeout (30) > > Thank you for this report. Yes, using 'dev' blindly is a bug. > > > if the call in hpwdt driver is changed to: > > > > if (watchdog_init_timeout(&hpwdt_dev, soft_margin, &dev->dev)) > > > > > > We see the message like we'd desire: > > > > [ 2061.167100] hpwdt 0000:01:00.0: driver supplied timeout (4294967295) out of range > > [ 2061.174633] hpwdt 0000:01:00.0: falling back to default timeout (30) > > The above observation makes sense, but I think we should fix the core > code and not the hpwdt driver. My suggestion would be to add something > like this to watchdog_init_timeout(): > > struct device *err_dev = dev ?: wdd->parent; > > And then use err_dev for all the printing. Guenter? > That is a good idea, and we should do that. Unfortunately, wdd->parent can also be NULL, either because there is no parent device (e.g. softdog.c), or because the driver author forgot to set ->parent. So we still need a fallback. Does it make sense to print watchdog_info->identity if both dev and wdd->parent are NULL ? Guenter
On Tue, Apr 16, 2019 at 10:55:33PM +0200, Wolfram Sang wrote: > > > That is indeed a problem: the pointer will be NULL if there is no parent > > device (such as in softdog.c). Otherwise it should never be NULL. > > Okay, this spoils my err_dev solution. So, we probably go this route > then: > > pr_<errlvl>("watchdog%d: <err_msg>\n", wdd->id); > I don't like it because it doesn't show the driver name, and watchdog%d can change with each reboot. How about something like this ? static void pr_wdt_err(struct watchdog_device *wdd, char *text, int err) { if (wdd->parent) dev_err(wdd->parent, "%s: %d\n", text, err); else pr_err("%s: %s: %d\n", wdd->info->identity, text, err); } We could then use the same mechanism to generate error messages for watchdog_register_device(). Guenter
On Tue, Apr 16, 2019 at 02:20:46PM -0700, Guenter Roeck wrote: > On Tue, Apr 16, 2019 at 10:55:33PM +0200, Wolfram Sang wrote: > > > > > That is indeed a problem: the pointer will be NULL if there is no parent > > > device (such as in softdog.c). Otherwise it should never be NULL. > > > > Okay, this spoils my err_dev solution. So, we probably go this route > > then: > > > > pr_<errlvl>("watchdog%d: <err_msg>\n", wdd->id); > > > > I don't like it because it doesn't show the driver name, and watchdog%d > can change with each reboot. How about something like this ? > > static void pr_wdt_err(struct watchdog_device *wdd, char *text, int err) > { > if (wdd->parent) > dev_err(wdd->parent, "%s: %d\n", text, err); > else > pr_err("%s: %s: %d\n", wdd->info->identity, text, err); > } > > We could then use the same mechanism to generate error messages for > watchdog_register_device(). 'text' is a constant string then. Supporting a format string will make this much more complicated. Yet, printing out the wrong timeout is useful, I think. What about: dev_str = wdd->parent ? dev_name(wdd->parent) : wdd->info->identity; pr_<errlvl>("%s: <errstr>\n", dev_str, ...); ? This can be easily copied for watchdog_register_device, not much overhead there.
On Wed, Apr 17, 2019 at 12:17:02AM +0200, Wolfram Sang wrote: > On Tue, Apr 16, 2019 at 02:20:46PM -0700, Guenter Roeck wrote: > > On Tue, Apr 16, 2019 at 10:55:33PM +0200, Wolfram Sang wrote: > > > > > > > That is indeed a problem: the pointer will be NULL if there is no parent > > > > device (such as in softdog.c). Otherwise it should never be NULL. > > > > > > Okay, this spoils my err_dev solution. So, we probably go this route > > > then: > > > > > > pr_<errlvl>("watchdog%d: <err_msg>\n", wdd->id); > > > > > > > I don't like it because it doesn't show the driver name, and watchdog%d > > can change with each reboot. How about something like this ? > > > > static void pr_wdt_err(struct watchdog_device *wdd, char *text, int err) > > { > > if (wdd->parent) > > dev_err(wdd->parent, "%s: %d\n", text, err); > > else > > pr_err("%s: %s: %d\n", wdd->info->identity, text, err); > > } > > > > We could then use the same mechanism to generate error messages for > > watchdog_register_device(). > > 'text' is a constant string then. Supporting a format string will make > this much more complicated. Yet, printing out the wrong timeout is > useful, I think. > > What about: > > dev_str = wdd->parent ? dev_name(wdd->parent) : wdd->info->identity; > pr_<errlvl>("%s: <errstr>\n", dev_str, ...); > Yes, that works as well. Note that it will actually print something like "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c. I guess that should be ok. Guenter
> Yes, that works as well. Note that it will actually print something like > "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c. > I guess that should be ok. I have the following diff applied on top of patch 2. Works with and without a parent device. I am not super happy casting 'identity' but since its u8-type is exported to userspace, I think we can't avoid it. Guenter, is this cast safe? Here is the diff: diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index cd3ca6b366ef..62be9e52a4de 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -115,6 +115,8 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) int watchdog_init_timeout(struct watchdog_device *wdd, unsigned int timeout_parm, struct device *dev) { + const char *dev_str = wdd->parent ? dev_name(wdd->parent) : + (const char *)wdd->info->identity; unsigned int t = 0; int ret = 0; @@ -126,8 +128,8 @@ int watchdog_init_timeout(struct watchdog_device *wdd, wdd->timeout = timeout_parm; return 0; } - dev_err(dev, "driver supplied timeout (%u) out of range\n", - timeout_parm); + pr_err("%s: driver supplied timeout (%u) out of range\n", + dev_str, timeout_parm); ret = -EINVAL; } @@ -138,12 +140,13 @@ int watchdog_init_timeout(struct watchdog_device *wdd, wdd->timeout = t; return 0; } - dev_err(dev, "DT supplied timeout (%u) out of range\n", t); + pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str, t); ret = -EINVAL; } if (ret < 0 && wdd->timeout) - dev_warn(dev, "falling back to default timeout (%u)\n", wdd->timeout); + pr_warn("%s: falling back to default timeout (%u)\n", dev_str, + wdd->timeout); return ret; }
On Wed, Apr 17, 2019 at 09:45:28PM +0200, Wolfram Sang wrote: > > > Yes, that works as well. Note that it will actually print something like > > "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c. > > I guess that should be ok. > > I have the following diff applied on top of patch 2. Works with and > without a parent device. I am not super happy casting 'identity' but > since its u8-type is exported to userspace, I think we can't avoid it. > Guenter, is this cast safe? Here is the diff: > I would think it is safe, but question is if it is it needed - ie do you see a warning if it isn't there ? Presumably yes; if so, let's just do it. Thanks, Guenter > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index cd3ca6b366ef..62be9e52a4de 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -115,6 +115,8 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) > int watchdog_init_timeout(struct watchdog_device *wdd, > unsigned int timeout_parm, struct device *dev) > { > + const char *dev_str = wdd->parent ? dev_name(wdd->parent) : > + (const char *)wdd->info->identity; > unsigned int t = 0; > int ret = 0; > > @@ -126,8 +128,8 @@ int watchdog_init_timeout(struct watchdog_device *wdd, > wdd->timeout = timeout_parm; > return 0; > } > - dev_err(dev, "driver supplied timeout (%u) out of range\n", > - timeout_parm); > + pr_err("%s: driver supplied timeout (%u) out of range\n", > + dev_str, timeout_parm); > ret = -EINVAL; > } > > @@ -138,12 +140,13 @@ int watchdog_init_timeout(struct watchdog_device *wdd, > wdd->timeout = t; > return 0; > } > - dev_err(dev, "DT supplied timeout (%u) out of range\n", t); > + pr_err("%s: DT supplied timeout (%u) out of range\n", dev_str, t); > ret = -EINVAL; > } > > if (ret < 0 && wdd->timeout) > - dev_warn(dev, "falling back to default timeout (%u)\n", wdd->timeout); > + pr_warn("%s: falling back to default timeout (%u)\n", dev_str, > + wdd->timeout); > > return ret; > } >
> > I have the following diff applied on top of patch 2. Works with and > > without a parent device. I am not super happy casting 'identity' but > > since its u8-type is exported to userspace, I think we can't avoid it. > > Guenter, is this cast safe? Here is the diff: > > > I would think it is safe, but question is if it is it needed - ie do you see > a warning if it isn't there ? Presumably yes; if so, let's just do it. Yes, the compiler rightfully complains otherwise. Do you prefer resending only patch 2 or the whole series?
On Wed, Apr 17, 2019 at 11:18:32PM +0200, Wolfram Sang wrote: > > > > I have the following diff applied on top of patch 2. Works with and > > > without a parent device. I am not super happy casting 'identity' but > > > since its u8-type is exported to userspace, I think we can't avoid it. > > > Guenter, is this cast safe? Here is the diff: > > > > > I would think it is safe, but question is if it is it needed - ie do you see > > a warning if it isn't there ? Presumably yes; if so, let's just do it. > > Yes, the compiler rightfully complains otherwise. > > Do you prefer resending only patch 2 or the whole series? > Please resend the whole series; that simplifies tracking. Thanks, Guenter
Hi Wolfram, On Wed, Apr 17, 2019 at 9:46 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > Yes, that works as well. Note that it will actually print something like > > "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c. > > I guess that should be ok. > > I have the following diff applied on top of patch 2. Works with and > without a parent device. I am not super happy casting 'identity' but > since its u8-type is exported to userspace, I think we can't avoid it. > Guenter, is this cast safe? Here is the diff: > > diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c > index cd3ca6b366ef..62be9e52a4de 100644 > --- a/drivers/watchdog/watchdog_core.c > +++ b/drivers/watchdog/watchdog_core.c > @@ -115,6 +115,8 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) > int watchdog_init_timeout(struct watchdog_device *wdd, > unsigned int timeout_parm, struct device *dev) > { > + const char *dev_str = wdd->parent ? dev_name(wdd->parent) : > + (const char *)wdd->info->identity; struct watchdog_info { ... __u8 identity[32]; /* Identity of the board */ }; Is identity[] guaranteed to be NUL-terminated? Gr{oetje,eeting}s, Geert
On 4/24/19 1:38 AM, Geert Uytterhoeven wrote: > Hi Wolfram, > > On Wed, Apr 17, 2019 at 9:46 PM Wolfram Sang <wsa@the-dreams.de> wrote: >>> Yes, that works as well. Note that it will actually print something like >>> "watchdog: <device>: ..." due to the pr_fmt() at the top of watchdog_core.c. >>> I guess that should be ok. >> >> I have the following diff applied on top of patch 2. Works with and >> without a parent device. I am not super happy casting 'identity' but >> since its u8-type is exported to userspace, I think we can't avoid it. >> Guenter, is this cast safe? Here is the diff: >> >> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c >> index cd3ca6b366ef..62be9e52a4de 100644 >> --- a/drivers/watchdog/watchdog_core.c >> +++ b/drivers/watchdog/watchdog_core.c >> @@ -115,6 +115,8 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) >> int watchdog_init_timeout(struct watchdog_device *wdd, >> unsigned int timeout_parm, struct device *dev) >> { >> + const char *dev_str = wdd->parent ? dev_name(wdd->parent) : >> + (const char *)wdd->info->identity; > > struct watchdog_info { > ... > __u8 identity[32]; /* Identity of the board */ > }; > > Is identity[] guaranteed to be NUL-terminated? > I would hope so, because we export its contents via sysfs to userspace with return sprintf(buf, "%s\n", wdd->info->identity); Also, there are already several pr_err() assuming that it is a string, Guenter
diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index ef30c7e9728d..db1bf6f546ae 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -311,8 +311,7 @@ static int hpwdt_init_one(struct pci_dev *dev, goto error_init_nmi_decoding; watchdog_set_nowayout(&hpwdt_dev, nowayout); - if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL)) - dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin); + watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL); if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) { dev_warn(&dev->dev, "timeout <= pretimeout. Setting pretimeout to zero\n");