The core will print out details now.
Reviewed-by: Guenter Roeck <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
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);
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 12:25:05PM +0200, Wolfram Sang wrote:
> The core will print out details now.
>
> Reviewed-by: Guenter Roeck <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> 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
--
-----------------------------------------------------------------------------
Jerry Hoemann Software Engineer Hewlett Packard Enterprise
-----------------------------------------------------------------------------
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 <[email protected]>
> > Signed-off-by: Wolfram Sang <[email protected]>
> > ---
> > 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 <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 4/24/19 1:38 AM, Geert Uytterhoeven wrote:
> Hi Wolfram,
>
> On Wed, Apr 17, 2019 at 9:46 PM Wolfram Sang <[email protected]> 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