diff mbox series

[2/2,RFC] rtc: pcf2127: only use watchdog when explicitly available

Message ID 20200924105256.18162-3-u.kleine-koenig@pengutronix.de
State New
Headers show
Series rtc: pcf2127: only use watchdog when explicitly available | expand

Commit Message

Uwe Kleine-König Sept. 24, 2020, 10:52 a.m. UTC
Most boards using the pcf2127 chip (in my bubble) don't make use of the
watchdog functionality and the respective output is not connected. The
effect on such a board is that there is a watchdog device provided that
doesn't work.

So only register the watchdog if the device tree has a "has-watchdog"
property.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/rtc/rtc-pcf2127.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Bruno Thomsen Sept. 27, 2020, 8:09 a.m. UTC | #1
Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
<u.kleine-koenig@pengutronix.de>:
>
> Most boards using the pcf2127 chip (in my bubble) don't make use of the
> watchdog functionality and the respective output is not connected. The
> effect on such a board is that there is a watchdog device provided that
> doesn't work.
>
> So only register the watchdog if the device tree has a "has-watchdog"
> property.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/rtc/rtc-pcf2127.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 5b1f1949b5e5..8bd89d641578 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
>         u32 wdd_timeout;
>         int ret;
>
> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
> +           !device_property_read_bool(dev, "has-watchdog"))
>                 return 0;

I don't think the compiler can remove the function if
CONFIG_WATCHDOG is disabled due to the device tree
value check. Maybe it can if split into 2 conditions.

if (!IS_ENABLED(CONFIG_WATCHDOG))
        return 0;
if (!device_property_read_bool(dev, "has-watchdog"))
        return 0;

/Bruno

>
>         pcf2127->wdd.parent = dev;
> --
> 2.28.0
>
Guenter Roeck Sept. 27, 2020, 3:54 p.m. UTC | #2
On 9/27/20 1:09 AM, Bruno Thomsen wrote:
> Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de>:
>>
>> Most boards using the pcf2127 chip (in my bubble) don't make use of the
>> watchdog functionality and the respective output is not connected. The
>> effect on such a board is that there is a watchdog device provided that
>> doesn't work.
>>
>> So only register the watchdog if the device tree has a "has-watchdog"
>> property.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>>  drivers/rtc/rtc-pcf2127.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index 5b1f1949b5e5..8bd89d641578 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
>>         u32 wdd_timeout;
>>         int ret;
>>
>> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
>> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
>> +           !device_property_read_bool(dev, "has-watchdog"))
>>                 return 0;
> 
> I don't think the compiler can remove the function if
> CONFIG_WATCHDOG is disabled due to the device tree
> value check. Maybe it can if split into 2 conditions.
> 

If the first part of the expression is always false, the second
part should not even be evaluated. Either case, the code now
hard depends on the compiler optimizing the code away.
It calls devm_watchdog_register_device() which doesn't exist
if CONFIG_WATCHDOG is not enabled. I didn't know that this is safe,
and I would personally not want to rely on it, but we live and
learn.

Guenter

> if (!IS_ENABLED(CONFIG_WATCHDOG))
>         return 0;
> if (!device_property_read_bool(dev, "has-watchdog"))
>         return 0;
> 
> /Bruno
> 
>>
>>         pcf2127->wdd.parent = dev;
>> --
>> 2.28.0
>>
Uwe Kleine-König Sept. 28, 2020, 8:43 a.m. UTC | #3
On Sun, Sep 27, 2020 at 08:54:47AM -0700, Guenter Roeck wrote:
> On 9/27/20 1:09 AM, Bruno Thomsen wrote:
> > Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de>:
> >>
> >> Most boards using the pcf2127 chip (in my bubble) don't make use of the
> >> watchdog functionality and the respective output is not connected. The
> >> effect on such a board is that there is a watchdog device provided that
> >> doesn't work.
> >>
> >> So only register the watchdog if the device tree has a "has-watchdog"
> >> property.
> >>
> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> ---
> >>  drivers/rtc/rtc-pcf2127.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> >> index 5b1f1949b5e5..8bd89d641578 100644
> >> --- a/drivers/rtc/rtc-pcf2127.c
> >> +++ b/drivers/rtc/rtc-pcf2127.c
> >> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
> >>         u32 wdd_timeout;
> >>         int ret;
> >>
> >> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
> >> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
> >> +           !device_property_read_bool(dev, "has-watchdog"))
> >>                 return 0;
> > 
> > I don't think the compiler can remove the function if
> > CONFIG_WATCHDOG is disabled due to the device tree
> > value check. Maybe it can if split into 2 conditions.
> > 
> 
> If the first part of the expression is always false, the second
> part should not even be evaluated.

This is wrong. For || the second expression isn't evaluated if the first
evaluates to true (and the whole expression becomes true). This is the
intended behaviour: If CONFIG_WATCHDOG is off, we don't need to check
for the dt property and just skip the watchdog part.

> Either case, the code now hard depends on the compiler optimizing the
> code away.
> 
> It calls devm_watchdog_register_device() which doesn't exist
> if CONFIG_WATCHDOG is not enabled. I didn't know that this is safe,
> and I would personally not want to rely on it, but we live and
> learn.

AFAICT this is save and used in other places in the kernel, too.  This
is one of the reasons why you cannot compile the kernel with -O0.

Best regards
Uwe
Guenter Roeck Sept. 28, 2020, 4:26 p.m. UTC | #4
On Mon, Sep 28, 2020 at 10:43:43AM +0200, Uwe Kleine-König wrote:
> On Sun, Sep 27, 2020 at 08:54:47AM -0700, Guenter Roeck wrote:
> > On 9/27/20 1:09 AM, Bruno Thomsen wrote:
> > > Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de>:
> > >>
> > >> Most boards using the pcf2127 chip (in my bubble) don't make use of the
> > >> watchdog functionality and the respective output is not connected. The
> > >> effect on such a board is that there is a watchdog device provided that
> > >> doesn't work.
> > >>
> > >> So only register the watchdog if the device tree has a "has-watchdog"
> > >> property.
> > >>
> > >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > >> ---
> > >>  drivers/rtc/rtc-pcf2127.c | 3 ++-
> > >>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > >> index 5b1f1949b5e5..8bd89d641578 100644
> > >> --- a/drivers/rtc/rtc-pcf2127.c
> > >> +++ b/drivers/rtc/rtc-pcf2127.c
> > >> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
> > >>         u32 wdd_timeout;
> > >>         int ret;
> > >>
> > >> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
> > >> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
> > >> +           !device_property_read_bool(dev, "has-watchdog"))
> > >>                 return 0;
> > > 
> > > I don't think the compiler can remove the function if
> > > CONFIG_WATCHDOG is disabled due to the device tree
> > > value check. Maybe it can if split into 2 conditions.
> > > 
> > 
> > If the first part of the expression is always false, the second
> > part should not even be evaluated.
> 
> This is wrong. For || the second expression isn't evaluated if the first
> evaluates to true (and the whole expression becomes true). This is the
> intended behaviour: If CONFIG_WATCHDOG is off, we don't need to check
> for the dt property and just skip the watchdog part.
> 
Sorry, I meant to say "If the first part of the expression is always true".

Guenter

> > Either case, the code now hard depends on the compiler optimizing the
> > code away.
> > 
> > It calls devm_watchdog_register_device() which doesn't exist
> > if CONFIG_WATCHDOG is not enabled. I didn't know that this is safe,
> > and I would personally not want to rely on it, but we live and
> > learn.
> 
> AFAICT this is save and used in other places in the kernel, too.  This
> is one of the reasons why you cannot compile the kernel with -O0.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 5b1f1949b5e5..8bd89d641578 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -340,7 +340,8 @@  static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
 	u32 wdd_timeout;
 	int ret;
 
-	if (!IS_ENABLED(CONFIG_WATCHDOG))
+	if (!IS_ENABLED(CONFIG_WATCHDOG) ||
+	    !device_property_read_bool(dev, "has-watchdog"))
 		return 0;
 
 	pcf2127->wdd.parent = dev;