| Message ID | 20240131-qcom-wdt-start-probe-v1-1-bee0a86e2bba@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: qcom: Start the watchdog in probe | expand |
On 1/30/24 20:15, Pavankumar Kondeti wrote: > When CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is enabled, kernel can pet the > watchdog until user space takes over. Make use of this feature and > start the watchdog in probe. > > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com> > --- > drivers/watchdog/qcom-wdt.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > index 9e790f0c2096..4fb5dbf5faee 100644 > --- a/drivers/watchdog/qcom-wdt.c > +++ b/drivers/watchdog/qcom-wdt.c > @@ -276,12 +276,16 @@ static int qcom_wdt_probe(struct platform_device *pdev) > watchdog_init_timeout(&wdt->wdd, 0, dev); > > /* > + * Kernel can pet the watchdog until user space takes over. > + * Start the watchdog here to make use of this feature. > + No, that is not what CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is about. Please see its description. NACK. Guenter > * If WDT is already running, call WDT start which > * will stop the WDT, set timeouts as bootloader > * might use different ones and set running bit > * to inform the WDT subsystem to ping the WDT > */ > - if (qcom_wdt_is_running(&wdt->wdd)) { > + if (IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED) || > + qcom_wdt_is_running(&wdt->wdd)) { > qcom_wdt_start(&wdt->wdd); > set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); > } > > --- > base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3 > change-id: 20240131-qcom-wdt-start-probe-b8e0560aef7d > > Best regards,
On Tue, Jan 30, 2024 at 10:01:15PM -0800, Guenter Roeck wrote: > On 1/30/24 20:15, Pavankumar Kondeti wrote: > > When CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is enabled, kernel can pet the > > watchdog until user space takes over. Make use of this feature and > > start the watchdog in probe. > > > > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com> > > --- > > drivers/watchdog/qcom-wdt.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > > index 9e790f0c2096..4fb5dbf5faee 100644 > > --- a/drivers/watchdog/qcom-wdt.c > > +++ b/drivers/watchdog/qcom-wdt.c > > @@ -276,12 +276,16 @@ static int qcom_wdt_probe(struct platform_device *pdev) > > watchdog_init_timeout(&wdt->wdd, 0, dev); > > /* > > + * Kernel can pet the watchdog until user space takes over. > > + * Start the watchdog here to make use of this feature. > > + > > No, that is not what CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is about. > Please see its description. > > NACK. > Thanks for taking a look Guenter. I thought of using WATCHDOG_HANDLE_BOOT_ENABLED and infiniite open timeout as a hint to start the watchdog in probe. After seeing your NACK for this patch, I guess that would also would have been rejected. Do you think we can revive this [1] to add support for watchdog pet from the kernel? It would be helpful in cases where the user space has no support for watchdog pet (say minimal ramdisk). [1] https://lore.kernel.org/linux-watchdog/20210924133509.3454834-1-f.suligoi@asem.it/#t
On 1/30/24 22:16, Pavan Kondeti wrote: > On Tue, Jan 30, 2024 at 10:01:15PM -0800, Guenter Roeck wrote: >> On 1/30/24 20:15, Pavankumar Kondeti wrote: >>> When CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is enabled, kernel can pet the >>> watchdog until user space takes over. Make use of this feature and >>> start the watchdog in probe. >>> >>> Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com> >>> --- >>> drivers/watchdog/qcom-wdt.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c >>> index 9e790f0c2096..4fb5dbf5faee 100644 >>> --- a/drivers/watchdog/qcom-wdt.c >>> +++ b/drivers/watchdog/qcom-wdt.c >>> @@ -276,12 +276,16 @@ static int qcom_wdt_probe(struct platform_device *pdev) >>> watchdog_init_timeout(&wdt->wdd, 0, dev); >>> /* >>> + * Kernel can pet the watchdog until user space takes over. >>> + * Start the watchdog here to make use of this feature. >>> + >> >> No, that is not what CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is about. >> Please see its description. >> >> NACK. >> > Thanks for taking a look Guenter. I thought of using > WATCHDOG_HANDLE_BOOT_ENABLED and infiniite open timeout as a hint to start > the watchdog in probe. After seeing your NACK for this patch, I guess > that would also would have been rejected. > WATCHDOG_HANDLE_BOOT_ENABLED is not supposed to be used in drivers. It is a flag for the watchdog core. Before you bring it up, stm32_iwdg.c is a corner case because (presumably) the driver can not determine if the watchdog is running. > Do you think we can revive this [1] to add support for watchdog pet from > the kernel? It would be helpful in cases where the user space has no > support for watchdog pet (say minimal ramdisk). > If done properly, sure. Looking at the exchange, the patch still had issues which I don't think were ever resolved. Personally I would not want to rely on this, though. It won't address situations where userspace hangs but low level kernel interrupts still work. I think it is mostly useful to cover the time from loading the watchdog driver to starting the watchdog daemon, but even that would better be solved by starting the watchdog in the ROM monitor or BIOS. A minimal watchdog daemon would not consume that much memory, so I don't think "user space has no support for watchdog pet (say minimal ramdisk)" would ever be a real problem. Such a minimal system would probably (hopefully) be based on busybox which does support a watchdog. Guenter
On Tue, Jan 30, 2024 at 10:37:50PM -0800, Guenter Roeck wrote: > On 1/30/24 22:16, Pavan Kondeti wrote: > > On Tue, Jan 30, 2024 at 10:01:15PM -0800, Guenter Roeck wrote: > > > On 1/30/24 20:15, Pavankumar Kondeti wrote: > > > > When CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is enabled, kernel can pet the > > > > watchdog until user space takes over. Make use of this feature and > > > > start the watchdog in probe. > > > > > > > > Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com> > > > > --- > > > > drivers/watchdog/qcom-wdt.c | 6 +++++- > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > > > > index 9e790f0c2096..4fb5dbf5faee 100644 > > > > --- a/drivers/watchdog/qcom-wdt.c > > > > +++ b/drivers/watchdog/qcom-wdt.c > > > > @@ -276,12 +276,16 @@ static int qcom_wdt_probe(struct platform_device *pdev) > > > > watchdog_init_timeout(&wdt->wdd, 0, dev); > > > > /* > > > > + * Kernel can pet the watchdog until user space takes over. > > > > + * Start the watchdog here to make use of this feature. > > > > + > > > > > > No, that is not what CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is about. > > > Please see its description. > > > > > > NACK. > > > > > Thanks for taking a look Guenter. I thought of using > > WATCHDOG_HANDLE_BOOT_ENABLED and infiniite open timeout as a hint to start > > the watchdog in probe. After seeing your NACK for this patch, I guess > > that would also would have been rejected. > > > WATCHDOG_HANDLE_BOOT_ENABLED is not supposed to be used in drivers. > It is a flag for the watchdog core. Before you bring it up, stm32_iwdg.c > is a corner case because (presumably) the driver can not determine > if the watchdog is running. > > > Do you think we can revive this [1] to add support for watchdog pet from > > the kernel? It would be helpful in cases where the user space has no > > support for watchdog pet (say minimal ramdisk). > > > > If done properly, sure. Looking at the exchange, the patch still had issues > which I don't think were ever resolved. Thanks. I will take a look at your review feedback on the series and address them before sending the next revision. > > Personally I would not want to rely on this, though. It won't address situations > where userspace hangs but low level kernel interrupts still work. I think > it is mostly useful to cover the time from loading the watchdog driver > to starting the watchdog daemon, but even that would better be solved by > starting the watchdog in the ROM monitor or BIOS. A minimal watchdog daemon > would not consume that much memory, so I don't think "user space has no > support for watchdog pet (say minimal ramdisk)" would ever be a real problem. > Such a minimal system would probably (hopefully) be based on busybox which > does support a watchdog. > Got it. I will find ways to start the watchdog in firmware so that we don't need anything special handling. Thanks, Pavan
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c index 9e790f0c2096..4fb5dbf5faee 100644 --- a/drivers/watchdog/qcom-wdt.c +++ b/drivers/watchdog/qcom-wdt.c @@ -276,12 +276,16 @@ static int qcom_wdt_probe(struct platform_device *pdev) watchdog_init_timeout(&wdt->wdd, 0, dev); /* + * Kernel can pet the watchdog until user space takes over. + * Start the watchdog here to make use of this feature. + * * If WDT is already running, call WDT start which * will stop the WDT, set timeouts as bootloader * might use different ones and set running bit * to inform the WDT subsystem to ping the WDT */ - if (qcom_wdt_is_running(&wdt->wdd)) { + if (IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED) || + qcom_wdt_is_running(&wdt->wdd)) { qcom_wdt_start(&wdt->wdd); set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); }
When CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED is enabled, kernel can pet the watchdog until user space takes over. Make use of this feature and start the watchdog in probe. Signed-off-by: Pavankumar Kondeti <quic_pkondeti@quicinc.com> --- drivers/watchdog/qcom-wdt.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) --- base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3 change-id: 20240131-qcom-wdt-start-probe-b8e0560aef7d Best regards,