| Message ID | 20200924105256.18162-3-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: pcf2127: only use watchdog when explicitly available | expand |
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 >
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 >>
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
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 --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;
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(-)