| Message ID | 20180524001335.15457-4-wak@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ipmi-watchdog: Fixes for error handling and general cleanups | expand |
On Wednesday, 23 May 2018 5:13:29 PM AEST William A. Kennington III wrote: > There is no clarification for why this change was needed, but presumably > this is due to a buggy BMC implementation where the Watchdog Set command That is correct, although I'm not sure if the buggy BMC implementation was ever fixed so we should probably test this on a P8 box as well. - Alistair > was processed concurrently or after the initial Watchdog Reset. This > inversion would cause the watchdog to stop since the DONT_STOP bit was > not set. Since we are now using the DONT_STOP bit during initialization, > the watchdog should not be stopped even if an inversion occurs. > > Signed-off-by: William A. Kennington III <wak@google.com> > --- > hw/ipmi/ipmi-watchdog.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/hw/ipmi/ipmi-watchdog.c b/hw/ipmi/ipmi-watchdog.c > index ee1608443..e8efba2b4 100644 > --- a/hw/ipmi/ipmi-watchdog.c > +++ b/hw/ipmi/ipmi-watchdog.c > @@ -143,9 +143,5 @@ void ipmi_wdt_init(void) > * could crash before the wdt has actually been started. */ > sync_reset_wdt(); > > - /* For some reason we have to reset it twice to get it to > - * actually start the first time. */ > - sync_reset_wdt(); > - > return; > } >
That sounds like a good plan. Do you have access to any of those? We only have our custom P8 boxes here which don't have a BMC. On Thu, May 24, 2018 at 5:28 PM Alistair Popple <alistair@popple.id.au> wrote: > On Wednesday, 23 May 2018 5:13:29 PM AEST William A. Kennington III wrote: > > There is no clarification for why this change was needed, but presumably > > this is due to a buggy BMC implementation where the Watchdog Set command > That is correct, although I'm not sure if the buggy BMC implementation > was ever fixed so we should probably test this on a P8 box as well. > - Alistair > > was processed concurrently or after the initial Watchdog Reset. This > > inversion would cause the watchdog to stop since the DONT_STOP bit was > > not set. Since we are now using the DONT_STOP bit during initialization, > > the watchdog should not be stopped even if an inversion occurs. > > > > Signed-off-by: William A. Kennington III <wak@google.com> > > --- > > hw/ipmi/ipmi-watchdog.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/hw/ipmi/ipmi-watchdog.c b/hw/ipmi/ipmi-watchdog.c > > index ee1608443..e8efba2b4 100644 > > --- a/hw/ipmi/ipmi-watchdog.c > > +++ b/hw/ipmi/ipmi-watchdog.c > > @@ -143,9 +143,5 @@ void ipmi_wdt_init(void) > > * could crash before the wdt has actually been started. */ > > sync_reset_wdt(); > > > > - /* For some reason we have to reset it twice to get it to > > - * actually start the first time. */ > > - sync_reset_wdt(); > > - > > return; > > } > >
William Kennington <wak@google.com> writes: > That sounds like a good plan. Do you have access to any of those? We only > have our custom P8 boxes here which don't have a BMC. Yeah, we do. P8 with buggy BMCs we have.
Stewart Smith <stewart@linux.ibm.com> writes: > William Kennington <wak@google.com> writes: >> That sounds like a good plan. Do you have access to any of those? We only >> have our custom P8 boxes here which don't have a BMC. > > Yeah, we do. P8 with buggy BMCs we have. Oh how I'd forgotten how much I dislike these BMCs. The bugs in serial over LAN alone are worth the entire effort that is OpenBMC. Anyway, things appeared to work, so it *must* be fine :)
diff --git a/hw/ipmi/ipmi-watchdog.c b/hw/ipmi/ipmi-watchdog.c index ee1608443..e8efba2b4 100644 --- a/hw/ipmi/ipmi-watchdog.c +++ b/hw/ipmi/ipmi-watchdog.c @@ -143,9 +143,5 @@ void ipmi_wdt_init(void) * could crash before the wdt has actually been started. */ sync_reset_wdt(); - /* For some reason we have to reset it twice to get it to - * actually start the first time. */ - sync_reset_wdt(); - return; }
There is no clarification for why this change was needed, but presumably this is due to a buggy BMC implementation where the Watchdog Set command was processed concurrently or after the initial Watchdog Reset. This inversion would cause the watchdog to stop since the DONT_STOP bit was not set. Since we are now using the DONT_STOP bit during initialization, the watchdog should not be stopped even if an inversion occurs. Signed-off-by: William A. Kennington III <wak@google.com> --- hw/ipmi/ipmi-watchdog.c | 4 ---- 1 file changed, 4 deletions(-)