| Message ID | 1456850717-3670-1-git-send-email-javier@osg.samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 02.03.2016 01:45, Javier Martinez Canillas wrote: > The watchdog maximum timeout value is determined by the number of bits > for the interval timer counter, its source clock frequency, the number > of bits of the prescaler and maximum divider value. > > This can be calculated with the following equation: > > max_timeout = counter / (freq / (max_prescale + 1) / max_divider) > > Setting a maximum timeout value will allow the watchdog core to refuse > user-space calls to the WDIOC_SETTIMEOUT ioctl that sets not supported > timeout values. > > For example, systemd tries to set a timeout of 10 minutes on reboot to > ensure that the machine will be rebooted even if a reboot failed. This > leads to the following error message on an Exynos5422 Odroid XU4 board: > > [ 147.986045] s3c2410-wdt 101d0000.watchdog: timeout 600 too big > > Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> > > --- > Hello, > > I'm sending this as an RFC because I don't have access to user manuals > for all the Exynos SoCs that this driver supports. But at least in all > the ones I have here, the maximum prescaler and divider values are the > same and also all have a 16-bit interval timer counter. > > So this patch assumes that all IP blocks supported by this driver are > the same on that regard, if that's not the case then we can move these > values on a per IP block basis using struct of_device_id .data field. It looks correct at least for Exynos4210 and newer. > > Best regards, > Javier > > drivers/watchdog/s3c2410_wdt.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 0093450441fe..f289c9fc353a 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -47,6 +47,8 @@ > #define S3C2410_WTDAT 0x04 > #define S3C2410_WTCNT 0x08 > > +#define S3C2410_WTCNT_MAXCNT 0xffff > + > #define S3C2410_WTCON_RSTEN (1 << 0) > #define S3C2410_WTCON_INTEN (1 << 2) > #define S3C2410_WTCON_ENABLE (1 << 5) > @@ -56,8 +58,11 @@ > #define S3C2410_WTCON_DIV64 (2 << 3) > #define S3C2410_WTCON_DIV128 (3 << 3) > > +#define S3C2410_WTCON_MAXDIV 0x80 > + > #define S3C2410_WTCON_PRESCALE(x) ((x) << 8) > #define S3C2410_WTCON_PRESCALE_MASK (0xff << 8) > +#define S3C2410_WTCON_PRESCALE_MAX 0xff > > #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0) > #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15) > @@ -198,6 +203,14 @@ do { \ > > /* functions */ > > +static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock) > +{ > + unsigned long freq = clk_get_rate(clock); > + > + return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1) > + / S3C2410_WTCON_MAXDIV); > +} > + > static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) > { > return container_of(nb, struct s3c2410_wdt, freq_transition); > @@ -567,6 +580,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > return ret; > } > > + wdt->wdt_device.min_timeout = 1; > + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); Can the frequency of clock change? E.g. with devfreq? No problem if it goes lower but if it gets higher than initial, then the problem will appear again. Best regards, Krzysztof > + > ret = s3c2410wdt_cpufreq_register(wdt); > if (ret < 0) { > dev_err(dev, "failed to register cpufreq\n"); > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/01/2016 03:44 PM, Krzysztof Kozlowski wrote: > On 02.03.2016 01:45, Javier Martinez Canillas wrote: >> The watchdog maximum timeout value is determined by the number of bits >> for the interval timer counter, its source clock frequency, the number >> of bits of the prescaler and maximum divider value. >> >> This can be calculated with the following equation: >> >> max_timeout = counter / (freq / (max_prescale + 1) / max_divider) >> >> Setting a maximum timeout value will allow the watchdog core to refuse >> user-space calls to the WDIOC_SETTIMEOUT ioctl that sets not supported >> timeout values. >> >> For example, systemd tries to set a timeout of 10 minutes on reboot to >> ensure that the machine will be rebooted even if a reboot failed. This >> leads to the following error message on an Exynos5422 Odroid XU4 board: >> >> [ 147.986045] s3c2410-wdt 101d0000.watchdog: timeout 600 too big >> >> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> >> >> --- >> Hello, >> >> I'm sending this as an RFC because I don't have access to user manuals >> for all the Exynos SoCs that this driver supports. But at least in all >> the ones I have here, the maximum prescaler and divider values are the >> same and also all have a 16-bit interval timer counter. >> >> So this patch assumes that all IP blocks supported by this driver are >> the same on that regard, if that's not the case then we can move these >> values on a per IP block basis using struct of_device_id .data field. > > It looks correct at least for Exynos4210 and newer. > >> >> Best regards, >> Javier >> >> drivers/watchdog/s3c2410_wdt.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >> index 0093450441fe..f289c9fc353a 100644 >> --- a/drivers/watchdog/s3c2410_wdt.c >> +++ b/drivers/watchdog/s3c2410_wdt.c >> @@ -47,6 +47,8 @@ >> #define S3C2410_WTDAT 0x04 >> #define S3C2410_WTCNT 0x08 >> >> +#define S3C2410_WTCNT_MAXCNT 0xffff >> + >> #define S3C2410_WTCON_RSTEN (1 << 0) >> #define S3C2410_WTCON_INTEN (1 << 2) >> #define S3C2410_WTCON_ENABLE (1 << 5) >> @@ -56,8 +58,11 @@ >> #define S3C2410_WTCON_DIV64 (2 << 3) >> #define S3C2410_WTCON_DIV128 (3 << 3) >> >> +#define S3C2410_WTCON_MAXDIV 0x80 >> + >> #define S3C2410_WTCON_PRESCALE(x) ((x) << 8) >> #define S3C2410_WTCON_PRESCALE_MASK (0xff << 8) >> +#define S3C2410_WTCON_PRESCALE_MAX 0xff >> >> #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0) >> #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15) >> @@ -198,6 +203,14 @@ do { \ >> >> /* functions */ >> >> +static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock) >> +{ >> + unsigned long freq = clk_get_rate(clock); >> + >> + return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1) >> + / S3C2410_WTCON_MAXDIV); >> +} >> + >> static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) >> { >> return container_of(nb, struct s3c2410_wdt, freq_transition); >> @@ -567,6 +580,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >> return ret; >> } >> >> + wdt->wdt_device.min_timeout = 1; >> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); > > Can the frequency of clock change? E.g. with devfreq? No problem if it > goes lower but if it gets higher than initial, then the problem will > appear again. > If that happens, we are probably in deep trouble. If the clock frequency changes, the watchdog timeout most likely changes as well. This means the watchdog may and likely will time out unexpectedly, or it won't time out when it is expected to time out. Guenter > Best regards, > Krzysztof > >> + >> ret = s3c2410wdt_cpufreq_register(wdt); >> if (ret < 0) { >> dev_err(dev, "failed to register cpufreq\n"); >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Krzysztof, On 03/01/2016 08:44 PM, Krzysztof Kozlowski wrote: > On 02.03.2016 01:45, Javier Martinez Canillas wrote: >> The watchdog maximum timeout value is determined by the number of bits >> for the interval timer counter, its source clock frequency, the number >> of bits of the prescaler and maximum divider value. >> >> This can be calculated with the following equation: >> >> max_timeout = counter / (freq / (max_prescale + 1) / max_divider) >> >> Setting a maximum timeout value will allow the watchdog core to refuse >> user-space calls to the WDIOC_SETTIMEOUT ioctl that sets not supported >> timeout values. >> >> For example, systemd tries to set a timeout of 10 minutes on reboot to >> ensure that the machine will be rebooted even if a reboot failed. This >> leads to the following error message on an Exynos5422 Odroid XU4 board: >> >> [ 147.986045] s3c2410-wdt 101d0000.watchdog: timeout 600 too big >> >> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> >> >> --- >> Hello, >> >> I'm sending this as an RFC because I don't have access to user manuals >> for all the Exynos SoCs that this driver supports. But at least in all >> the ones I have here, the maximum prescaler and divider values are the >> same and also all have a 16-bit interval timer counter. >> >> So this patch assumes that all IP blocks supported by this driver are >> the same on that regard, if that's not the case then we can move these >> values on a per IP block basis using struct of_device_id .data field. > > It looks correct at least for Exynos4210 and newer. > Great, thanks a lot for the confirmation. >> >> Best regards, >> Javier >> >> drivers/watchdog/s3c2410_wdt.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >> index 0093450441fe..f289c9fc353a 100644 >> --- a/drivers/watchdog/s3c2410_wdt.c >> +++ b/drivers/watchdog/s3c2410_wdt.c >> @@ -47,6 +47,8 @@ >> #define S3C2410_WTDAT 0x04 >> #define S3C2410_WTCNT 0x08 >> >> +#define S3C2410_WTCNT_MAXCNT 0xffff >> + >> #define S3C2410_WTCON_RSTEN (1 << 0) >> #define S3C2410_WTCON_INTEN (1 << 2) >> #define S3C2410_WTCON_ENABLE (1 << 5) >> @@ -56,8 +58,11 @@ >> #define S3C2410_WTCON_DIV64 (2 << 3) >> #define S3C2410_WTCON_DIV128 (3 << 3) >> >> +#define S3C2410_WTCON_MAXDIV 0x80 >> + >> #define S3C2410_WTCON_PRESCALE(x) ((x) << 8) >> #define S3C2410_WTCON_PRESCALE_MASK (0xff << 8) >> +#define S3C2410_WTCON_PRESCALE_MAX 0xff >> >> #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0) >> #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15) >> @@ -198,6 +203,14 @@ do { \ >> >> /* functions */ >> >> +static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock) >> +{ >> + unsigned long freq = clk_get_rate(clock); >> + >> + return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1) >> + / S3C2410_WTCON_MAXDIV); >> +} >> + >> static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) >> { >> return container_of(nb, struct s3c2410_wdt, freq_transition); >> @@ -567,6 +580,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev) >> return ret; >> } >> >> + wdt->wdt_device.min_timeout = 1; >> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); > > Can the frequency of clock change? E.g. with devfreq? No problem if it > goes lower but if it gets higher than initial, then the problem will > appear again. > That's a very good question. As Guenter said we will be in deep troubles if that ever happens since the driver doesn't take that into account. The .set_timeout handler just sets the counter according to the current frequency and that's never updated, unless a new timeout is set of course. So in other words, I just made the same assumptions that the driver is currently doing. At least the Exynos SoCs manual don't mention frequency scaling for the watchdog timer source clock and AFAICT none of the CLK_WDT parents scale their frequencies but I don't know if that's true for all the machines using this driver (i.e: out-of-tree boards). > Best regards, > Krzysztof > Best regards,
On 03.03.2016 02:30, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 03/01/2016 08:44 PM, Krzysztof Kozlowski wrote: >> On 02.03.2016 01:45, Javier Martinez Canillas wrote: >>> The watchdog maximum timeout value is determined by the number of bits >>> for the interval timer counter, its source clock frequency, the number >>> of bits of the prescaler and maximum divider value. >>> >>> This can be calculated with the following equation: >>> >>> max_timeout = counter / (freq / (max_prescale + 1) / max_divider) >>> >>> Setting a maximum timeout value will allow the watchdog core to refuse >>> user-space calls to the WDIOC_SETTIMEOUT ioctl that sets not supported >>> timeout values. >>> >>> For example, systemd tries to set a timeout of 10 minutes on reboot to >>> ensure that the machine will be rebooted even if a reboot failed. This >>> leads to the following error message on an Exynos5422 Odroid XU4 board: >>> >>> [ 147.986045] s3c2410-wdt 101d0000.watchdog: timeout 600 too big >>> >>> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> >>> >>> --- >>> Hello, >>> >>> I'm sending this as an RFC because I don't have access to user manuals >>> for all the Exynos SoCs that this driver supports. But at least in all >>> the ones I have here, the maximum prescaler and divider values are the >>> same and also all have a 16-bit interval timer counter. >>> >>> So this patch assumes that all IP blocks supported by this driver are >>> the same on that regard, if that's not the case then we can move these >>> values on a per IP block basis using struct of_device_id .data field. >> >> It looks correct at least for Exynos4210 and newer. >> > > Great, thanks a lot for the confirmation. > >>> >>> Best regards, >>> Javier >>> >>> drivers/watchdog/s3c2410_wdt.c | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/drivers/watchdog/s3c2410_wdt.c >>> b/drivers/watchdog/s3c2410_wdt.c >>> index 0093450441fe..f289c9fc353a 100644 >>> --- a/drivers/watchdog/s3c2410_wdt.c >>> +++ b/drivers/watchdog/s3c2410_wdt.c >>> @@ -47,6 +47,8 @@ >>> #define S3C2410_WTDAT 0x04 >>> #define S3C2410_WTCNT 0x08 >>> >>> +#define S3C2410_WTCNT_MAXCNT 0xffff >>> + >>> #define S3C2410_WTCON_RSTEN (1 << 0) >>> #define S3C2410_WTCON_INTEN (1 << 2) >>> #define S3C2410_WTCON_ENABLE (1 << 5) >>> @@ -56,8 +58,11 @@ >>> #define S3C2410_WTCON_DIV64 (2 << 3) >>> #define S3C2410_WTCON_DIV128 (3 << 3) >>> >>> +#define S3C2410_WTCON_MAXDIV 0x80 >>> + >>> #define S3C2410_WTCON_PRESCALE(x) ((x) << 8) >>> #define S3C2410_WTCON_PRESCALE_MASK (0xff << 8) >>> +#define S3C2410_WTCON_PRESCALE_MAX 0xff >>> >>> #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0) >>> #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15) >>> @@ -198,6 +203,14 @@ do { \ >>> >>> /* functions */ >>> >>> +static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock) >>> +{ >>> + unsigned long freq = clk_get_rate(clock); >>> + >>> + return S3C2410_WTCNT_MAXCNT / (freq / >>> (S3C2410_WTCON_PRESCALE_MAX + 1) >>> + / S3C2410_WTCON_MAXDIV); >>> +} >>> + >>> static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block >>> *nb) >>> { >>> return container_of(nb, struct s3c2410_wdt, freq_transition); >>> @@ -567,6 +580,9 @@ static int s3c2410wdt_probe(struct >>> platform_device *pdev) >>> return ret; >>> } >>> >>> + wdt->wdt_device.min_timeout = 1; >>> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); >> >> Can the frequency of clock change? E.g. with devfreq? No problem if it >> goes lower but if it gets higher than initial, then the problem will >> appear again. >> > > That's a very good question. As Guenter said we will be in deep troubles > if that ever happens since the driver doesn't take that into account. > > The .set_timeout handler just sets the counter according to the current > frequency and that's never updated, unless a new timeout is set of course. > > So in other words, I just made the same assumptions that the driver is > currently doing. Not entirely. Change of clock frequency will affect currently set timeout. But the next timeout will be using new frequency. However you are setting the maximum timeout once. It will never change. > At least the Exynos SoCs manual don't mention frequency > scaling for the watchdog timer source clock and AFAICT none of the CLK_WDT > parents scale their frequencies but I don't know if that's true for all > the machines using this driver (i.e: out-of-tree boards). I looked at Exynos4 family because the devfreq was tested there. The WDT clock goes from ACLK100 (or ACLK66 on different socs). 1. Existing devfreq for Exynos4 does not change ACLK100 frequency. 2. New patches from Chanwoo (Cc) add scaling of ACLK100 also to 50 MHz: http://lkml.iu.edu/hypermail/linux/kernel/1512.1/04828.html The problem will be more severe if the watchdog got configured on 50 MHz and then devfreq bumps the clock to 100 MHz... Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Krzysztof, On 03/02/2016 09:21 PM, Krzysztof Kozlowski wrote: > On 03.03.2016 02:30, Javier Martinez Canillas wrote: [snip] >>>> >>>> + wdt->wdt_device.min_timeout = 1; >>>> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); >>> >>> Can the frequency of clock change? E.g. with devfreq? No problem if it >>> goes lower but if it gets higher than initial, then the problem will >>> appear again. >>> I think both cases are problematic since low scaling will meant that the watchdog will support a bigger timeout than what was set as maximum (this will be a regression) and going up will mean that the maximum timeout is bigger than what the watchdog supports (the same issue without this patch). >> >> That's a very good question. As Guenter said we will be in deep troubles >> if that ever happens since the driver doesn't take that into account. >> >> The .set_timeout handler just sets the counter according to the current >> frequency and that's never updated, unless a new timeout is set of course. >> >> So in other words, I just made the same assumptions that the driver is >> currently doing. > > Not entirely. Change of clock frequency will affect currently set > timeout. But the next timeout will be using new frequency. > > However you are setting the maximum timeout once. It will never change. Of course. I meant that the driver makes the assumption that the clock frequency never changes, no that the symptoms will be the same in both cases (maximum timeout vs current timeout). > >> At least the Exynos SoCs manual don't mention frequency >> scaling for the watchdog timer source clock and AFAICT none of the CLK_WDT >> parents scale their frequencies but I don't know if that's true for all >> the machines using this driver (i.e: out-of-tree boards). > > I looked at Exynos4 family because the devfreq was tested there. The WDT > clock goes from ACLK100 (or ACLK66 on different socs). > > 1. Existing devfreq for Exynos4 does not change ACLK100 frequency. > 2. New patches from Chanwoo (Cc) add scaling of ACLK100 also to 50 MHz: > http://lkml.iu.edu/hypermail/linux/kernel/1512.1/04828.html > Thanks for the pointer, I missed that patch from Chanwoo. > The problem will be more severe if the watchdog got configured on 50 MHz > and then devfreq bumps the clock to 100 MHz... > So, what do you propose? We could for example set a maximum timeout on probe as $SUBJECT do and also update the maximum timeout again on the .set_timeout callback in case the clock rate changed. I think that is kind of hacky but I can't think of another way to guard about the frequency being changed. > Best regards, > Krzysztof > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Best regards,
On 03.03.2016 11:14, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 03/02/2016 09:21 PM, Krzysztof Kozlowski wrote: >> On 03.03.2016 02:30, Javier Martinez Canillas wrote: > > [snip] > >>>>> >>>>> + wdt->wdt_device.min_timeout = 1; >>>>> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); >>>> >>>> Can the frequency of clock change? E.g. with devfreq? No problem if it >>>> goes lower but if it gets higher than initial, then the problem will >>>> appear again. >>>> > > I think both cases are problematic since low scaling will meant that the > watchdog will support a bigger timeout than what was set as maximum (this > will be a regression) and going up will mean that the maximum timeout is > bigger than what the watchdog supports (the same issue without this patch). Yes, both cases are bad. >> The problem will be more severe if the watchdog got configured on 50 MHz >> and then devfreq bumps the clock to 100 MHz... >> > > So, what do you propose? We could for example set a maximum timeout on > probe > as $SUBJECT do and also update the maximum timeout again on the > .set_timeout > callback in case the clock rate changed. Or print warning and don't care... :) > I think that is kind of hacky > but I > can't think of another way to guard about the frequency being changed. First of all your patch does not introduce any kind of regression on its own. Since we are at the topic of watchdog I am just looking for doing this properly. We can merge the patches now and fix stuff later. Second, I think there won't be any issues with current mainline code (and devfreq). I don't care about out of tree code. Third, it would be good to confirm that watchdog clock really changes frequency with devfreq... BR, KRzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/02/2016 06:14 PM, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 03/02/2016 09:21 PM, Krzysztof Kozlowski wrote: >> On 03.03.2016 02:30, Javier Martinez Canillas wrote: > > [snip] > >>>>> >>>>> + wdt->wdt_device.min_timeout = 1; >>>>> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); >>>> >>>> Can the frequency of clock change? E.g. with devfreq? No problem if it >>>> goes lower but if it gets higher than initial, then the problem will >>>> appear again. >>>> > > I think both cases are problematic since low scaling will meant that the > watchdog will support a bigger timeout than what was set as maximum (this > will be a regression) and going up will mean that the maximum timeout is > bigger than what the watchdog supports (the same issue without this patch). > >>> >>> That's a very good question. As Guenter said we will be in deep troubles >>> if that ever happens since the driver doesn't take that into account. >>> >>> The .set_timeout handler just sets the counter according to the current >>> frequency and that's never updated, unless a new timeout is set of course. >>> >>> So in other words, I just made the same assumptions that the driver is >>> currently doing. >> >> Not entirely. Change of clock frequency will affect currently set >> timeout. But the next timeout will be using new frequency. >> >> However you are setting the maximum timeout once. It will never change. > > Of course. I meant that the driver makes the assumption that the clock > frequency never changes, no that the symptoms will be the same in both > cases (maximum timeout vs current timeout). > >> >>> At least the Exynos SoCs manual don't mention frequency >>> scaling for the watchdog timer source clock and AFAICT none of the CLK_WDT >>> parents scale their frequencies but I don't know if that's true for all >>> the machines using this driver (i.e: out-of-tree boards). >> >> I looked at Exynos4 family because the devfreq was tested there. The WDT >> clock goes from ACLK100 (or ACLK66 on different socs). >> >> 1. Existing devfreq for Exynos4 does not change ACLK100 frequency. >> 2. New patches from Chanwoo (Cc) add scaling of ACLK100 also to 50 MHz: >> http://lkml.iu.edu/hypermail/linux/kernel/1512.1/04828.html >> > > Thanks for the pointer, I missed that patch from Chanwoo. > >> The problem will be more severe if the watchdog got configured on 50 MHz >> and then devfreq bumps the clock to 100 MHz... >> > > So, what do you propose? We could for example set a maximum timeout on probe > as $SUBJECT do and also update the maximum timeout again on the .set_timeout > callback in case the clock rate changed. I think that is kind of hacky but I > can't think of another way to guard about the frequency being changed. > People will likely get random watchdog timeouts if the frequency increases. Typical example for shot-yourself-into-the-foot. A watchdog driver using a non-static clock must register a clock change notifier to handle the clock rate change and update its settings accordingly. I would also argue that the maximum timeout should be set to the minimum possible value (probably associated with the highest possible frequency). All other cases might end up causing trouble if a clock frequency chance results in an enforced timeout change, since there is currently no mechanism to inform user space about such a change. Example: maximum possible timeout changes from 1 minute to 30 seconds. The timeout was set to 1 minute, and has to be reduced to 30 seconds. Very likely result is that the watchdog will reset the system because user space still believes that the timeout is 60 seconds and doesn't ping the watchdog often enough to prevent it. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Guenter, On 03/03/2016 01:50 AM, Guenter Roeck wrote: > On 03/02/2016 06:14 PM, Javier Martinez Canillas wrote: >> Hello Krzysztof, >> >> On 03/02/2016 09:21 PM, Krzysztof Kozlowski wrote: >>> On 03.03.2016 02:30, Javier Martinez Canillas wrote: >> >> [snip] >> >>>>>> >>>>>> + wdt->wdt_device.min_timeout = 1; >>>>>> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); >>>>> >>>>> Can the frequency of clock change? E.g. with devfreq? No problem if it >>>>> goes lower but if it gets higher than initial, then the problem will >>>>> appear again. >>>>> >> >> I think both cases are problematic since low scaling will meant that the >> watchdog will support a bigger timeout than what was set as maximum (this >> will be a regression) and going up will mean that the maximum timeout is >> bigger than what the watchdog supports (the same issue without this patch). >> >>>> >>>> That's a very good question. As Guenter said we will be in deep troubles >>>> if that ever happens since the driver doesn't take that into account. >>>> >>>> The .set_timeout handler just sets the counter according to the current >>>> frequency and that's never updated, unless a new timeout is set of course. >>>> >>>> So in other words, I just made the same assumptions that the driver is >>>> currently doing. >>> >>> Not entirely. Change of clock frequency will affect currently set >>> timeout. But the next timeout will be using new frequency. >>> >>> However you are setting the maximum timeout once. It will never change. >> >> Of course. I meant that the driver makes the assumption that the clock >> frequency never changes, no that the symptoms will be the same in both >> cases (maximum timeout vs current timeout). >> >>> >>>> At least the Exynos SoCs manual don't mention frequency >>>> scaling for the watchdog timer source clock and AFAICT none of the CLK_WDT >>>> parents scale their frequencies but I don't know if that's true for all >>>> the machines using this driver (i.e: out-of-tree boards). >>> >>> I looked at Exynos4 family because the devfreq was tested there. The WDT >>> clock goes from ACLK100 (or ACLK66 on different socs). >>> >>> 1. Existing devfreq for Exynos4 does not change ACLK100 frequency. >>> 2. New patches from Chanwoo (Cc) add scaling of ACLK100 also to 50 MHz: >>> http://lkml.iu.edu/hypermail/linux/kernel/1512.1/04828.html >>> >> >> Thanks for the pointer, I missed that patch from Chanwoo. >> >>> The problem will be more severe if the watchdog got configured on 50 MHz >>> and then devfreq bumps the clock to 100 MHz... >>> >> >> So, what do you propose? We could for example set a maximum timeout on probe >> as $SUBJECT do and also update the maximum timeout again on the .set_timeout >> callback in case the clock rate changed. I think that is kind of hacky but I >> can't think of another way to guard about the frequency being changed. >> > > People will likely get random watchdog timeouts if the frequency increases. > Typical example for shot-yourself-into-the-foot. > > A watchdog driver using a non-static clock must register a clock change notifier > to handle the clock rate change and update its settings accordingly. > > I would also argue that the maximum timeout should be set to the minimum > possible value (probably associated with the highest possible frequency). > All other cases might end up causing trouble if a clock frequency > chance results in an enforced timeout change, since there is currently > no mechanism to inform user space about such a change. > > Example: maximum possible timeout changes from 1 minute to 30 seconds. > The timeout was set to 1 minute, and has to be reduced to 30 seconds. > Very likely result is that the watchdog will reset the system because > user space still believes that the timeout is 60 seconds and doesn't > ping the watchdog often enough to prevent it. > Agreed. In any case this discussion is not related to this patch since currently in mainline the watchdog source clock is fixed and does not change. So, $SUBJECT solves the issue of not having the fixed .{min,max}_timeout defined to allow the watchdog_timeout_invalid() function to check values set by WDIOC_SETTIMEOUT and avoid calling the .set_timeout callback. If later someone tries to scale a parent clock used by many drivers, then the submitter should make sure that no regressions are added by the patch. > Guenter > Best regards,
2016-03-03 20:55 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>: > Hello Guenter, > > > On 03/03/2016 01:50 AM, Guenter Roeck wrote: >> A watchdog driver using a non-static clock must register a clock change >> notifier >> to handle the clock rate change and update its settings accordingly. >> >> I would also argue that the maximum timeout should be set to the minimum >> possible value (probably associated with the highest possible frequency). >> All other cases might end up causing trouble if a clock frequency >> chance results in an enforced timeout change, since there is currently >> no mechanism to inform user space about such a change. >> >> Example: maximum possible timeout changes from 1 minute to 30 seconds. >> The timeout was set to 1 minute, and has to be reduced to 30 seconds. >> Very likely result is that the watchdog will reset the system because >> user space still believes that the timeout is 60 seconds and doesn't >> ping the watchdog often enough to prevent it. >> > > Agreed. > > In any case this discussion is not related to this patch since currently > in mainline the watchdog source clock is fixed and does not change. > > So, $SUBJECT solves the issue of not having the fixed .{min,max}_timeout > defined to allow the watchdog_timeout_invalid() function to check values > set by WDIOC_SETTIMEOUT and avoid calling the .set_timeout callback. > > If later someone tries to scale a parent clock used by many drivers, then > the submitter should make sure that no regressions are added by the patch. Sounds good. For this patch then: Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/03/2016 04:26 AM, Krzysztof Kozlowski wrote: > 2016-03-03 20:55 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>: >> Hello Guenter, >> >> >> On 03/03/2016 01:50 AM, Guenter Roeck wrote: >>> A watchdog driver using a non-static clock must register a clock change >>> notifier >>> to handle the clock rate change and update its settings accordingly. >>> >>> I would also argue that the maximum timeout should be set to the minimum >>> possible value (probably associated with the highest possible frequency). >>> All other cases might end up causing trouble if a clock frequency >>> chance results in an enforced timeout change, since there is currently >>> no mechanism to inform user space about such a change. >>> >>> Example: maximum possible timeout changes from 1 minute to 30 seconds. >>> The timeout was set to 1 minute, and has to be reduced to 30 seconds. >>> Very likely result is that the watchdog will reset the system because >>> user space still believes that the timeout is 60 seconds and doesn't >>> ping the watchdog often enough to prevent it. >>> >> >> Agreed. >> >> In any case this discussion is not related to this patch since currently >> in mainline the watchdog source clock is fixed and does not change. >> >> So, $SUBJECT solves the issue of not having the fixed .{min,max}_timeout >> defined to allow the watchdog_timeout_invalid() function to check values >> set by WDIOC_SETTIMEOUT and avoid calling the .set_timeout callback. >> >> If later someone tries to scale a parent clock used by many drivers, then >> the submitter should make sure that no regressions are added by the patch. > > Sounds good. For this patch then: > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Agreed. Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thanks, Guenter > > Best regards, > Krzysztof > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Javier, > The watchdog maximum timeout value is determined by the number of bits > for the interval timer counter, its source clock frequency, the number > of bits of the prescaler and maximum divider value. > > This can be calculated with the following equation: > > max_timeout = counter / (freq / (max_prescale + 1) / max_divider) > > Setting a maximum timeout value will allow the watchdog core to refuse > user-space calls to the WDIOC_SETTIMEOUT ioctl that sets not supported > timeout values. > > For example, systemd tries to set a timeout of 10 minutes on reboot to > ensure that the machine will be rebooted even if a reboot failed. This > leads to the following error message on an Exynos5422 Odroid XU4 board: > > [ 147.986045] s3c2410-wdt 101d0000.watchdog: timeout 600 too big > > Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> > > --- > Hello, > > I'm sending this as an RFC because I don't have access to user manuals > for all the Exynos SoCs that this driver supports. But at least in all > the ones I have here, the maximum prescaler and divider values are the > same and also all have a 16-bit interval timer counter. > > So this patch assumes that all IP blocks supported by this driver are > the same on that regard, if that's not the case then we can move these > values on a per IP block basis using struct of_device_id .data field. > > Best regards, > Javier > > drivers/watchdog/s3c2410_wdt.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 0093450441fe..f289c9fc353a 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -47,6 +47,8 @@ > #define S3C2410_WTDAT 0x04 > #define S3C2410_WTCNT 0x08 > > +#define S3C2410_WTCNT_MAXCNT 0xffff > + > #define S3C2410_WTCON_RSTEN (1 << 0) > #define S3C2410_WTCON_INTEN (1 << 2) > #define S3C2410_WTCON_ENABLE (1 << 5) > @@ -56,8 +58,11 @@ > #define S3C2410_WTCON_DIV64 (2 << 3) > #define S3C2410_WTCON_DIV128 (3 << 3) > > +#define S3C2410_WTCON_MAXDIV 0x80 > + > #define S3C2410_WTCON_PRESCALE(x) ((x) << 8) > #define S3C2410_WTCON_PRESCALE_MASK (0xff << 8) > +#define S3C2410_WTCON_PRESCALE_MAX 0xff > > #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0) > #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15) > @@ -198,6 +203,14 @@ do { \ > > /* functions */ > > +static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock) > +{ > + unsigned long freq = clk_get_rate(clock); > + > + return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1) > + / S3C2410_WTCON_MAXDIV); > +} > + > static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) > { > return container_of(nb, struct s3c2410_wdt, freq_transition); > @@ -567,6 +580,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > return ret; > } > > + wdt->wdt_device.min_timeout = 1; > + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); > + > ret = s3c2410wdt_cpufreq_register(wdt); > if (ret < 0) { > dev_err(dev, "failed to register cpufreq\n"); > -- > 2.5.0 > This patch has been added to linux-watchdog-next. Kind regards, Wim. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index 0093450441fe..f289c9fc353a 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -47,6 +47,8 @@ #define S3C2410_WTDAT 0x04 #define S3C2410_WTCNT 0x08 +#define S3C2410_WTCNT_MAXCNT 0xffff + #define S3C2410_WTCON_RSTEN (1 << 0) #define S3C2410_WTCON_INTEN (1 << 2) #define S3C2410_WTCON_ENABLE (1 << 5) @@ -56,8 +58,11 @@ #define S3C2410_WTCON_DIV64 (2 << 3) #define S3C2410_WTCON_DIV128 (3 << 3) +#define S3C2410_WTCON_MAXDIV 0x80 + #define S3C2410_WTCON_PRESCALE(x) ((x) << 8) #define S3C2410_WTCON_PRESCALE_MASK (0xff << 8) +#define S3C2410_WTCON_PRESCALE_MAX 0xff #define CONFIG_S3C2410_WATCHDOG_ATBOOT (0) #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15) @@ -198,6 +203,14 @@ do { \ /* functions */ +static inline unsigned int s3c2410wdt_max_timeout(struct clk *clock) +{ + unsigned long freq = clk_get_rate(clock); + + return S3C2410_WTCNT_MAXCNT / (freq / (S3C2410_WTCON_PRESCALE_MAX + 1) + / S3C2410_WTCON_MAXDIV); +} + static inline struct s3c2410_wdt *freq_to_wdt(struct notifier_block *nb) { return container_of(nb, struct s3c2410_wdt, freq_transition); @@ -567,6 +580,9 @@ static int s3c2410wdt_probe(struct platform_device *pdev) return ret; } + wdt->wdt_device.min_timeout = 1; + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); + ret = s3c2410wdt_cpufreq_register(wdt); if (ret < 0) { dev_err(dev, "failed to register cpufreq\n");
The watchdog maximum timeout value is determined by the number of bits for the interval timer counter, its source clock frequency, the number of bits of the prescaler and maximum divider value. This can be calculated with the following equation: max_timeout = counter / (freq / (max_prescale + 1) / max_divider) Setting a maximum timeout value will allow the watchdog core to refuse user-space calls to the WDIOC_SETTIMEOUT ioctl that sets not supported timeout values. For example, systemd tries to set a timeout of 10 minutes on reboot to ensure that the machine will be rebooted even if a reboot failed. This leads to the following error message on an Exynos5422 Odroid XU4 board: [ 147.986045] s3c2410-wdt 101d0000.watchdog: timeout 600 too big Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com> --- Hello, I'm sending this as an RFC because I don't have access to user manuals for all the Exynos SoCs that this driver supports. But at least in all the ones I have here, the maximum prescaler and divider values are the same and also all have a 16-bit interval timer counter. So this patch assumes that all IP blocks supported by this driver are the same on that regard, if that's not the case then we can move these values on a per IP block basis using struct of_device_id .data field. Best regards, Javier drivers/watchdog/s3c2410_wdt.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)