Re: [PATCH 1/3] ipmi/bmc-sim: implement watchdog dont log flag

[Corey Minyard <corey@minyard.net>]

On Mon, Mar 31, 2025 at 06:03:11PM -0500, Corey Minyard wrote:
> On Tue, Apr 01, 2025 at 08:37:19AM +1000, Nicholas Piggin wrote:
> > On Mon Mar 31, 2025 at 11:13 PM AEST, Corey Minyard wrote:
> > > On Mon, Mar 31, 2025 at 10:57:22PM +1000, Nicholas Piggin wrote:
> > >> If the dont-log flag is set in the 'timer use' field for the
> > >> 'set watchdog' command, a watchdog timeout will not get logged as
> > >> a timer use expiration.
> > >> 
> > >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > >> ---
> > >>  hw/ipmi/ipmi_bmc_sim.c | 7 ++++++-
> > >>  1 file changed, 6 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> > >> index 6157ac71201..32161044c0b 100644
> > >> --- a/hw/ipmi/ipmi_bmc_sim.c
> > >> +++ b/hw/ipmi/ipmi_bmc_sim.c
> > >> @@ -733,7 +733,12 @@ static void ipmi_sim_handle_timeout(IPMIBmcSim *ibs)
> > >>  
> > >>   do_full_expiry:
> > >>      ibs->watchdog_running = 0; /* Stop the watchdog on a timeout */
> > >> -    ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
> > >> +
> > >> +    /* Log the expiry if the don't log bit is clear */
> > >> +    if (!IPMI_BMC_WATCHDOG_GET_DONT_LOG(ibs)) {
> > >> +        ibs->watchdog_expired |= (1 << IPMI_BMC_WATCHDOG_GET_USE(ibs));
> > >> +    }
> > >> +
> > >
> > > Are you sure this is correct?  The spec doesn't say what this means, but
> > > I would assume this means "Don't add a system log" not "Don't set the
> > > expiry happened bit".
> > 
> > From IPMI spec, Set Watchdog Timer command timer use field of byte 1
> > says "timer use (logged on expiration when “don’t log” bit = 0b)".
> > But it also says it should disable the timeout sensor event logging.
> > I missed that part, I will see if I can make that work.
> 
> It doesn't currently add an event to the log, I don't think.  If you
> want to add that, it's fine.

Actually, I'm wrong here.  It does generate an event through the sensor
handling, the sensor_set_discrete_bit() function will cause an event to
be generated.  However, you can't just not call that function, you need
the event set.  I think the best way would be to add a "do_log"
parameter to that function to suppress the log in this case.

-corey

> 
> However, as it is, your change will cause the Get Watchdog Timer command
> to return the wrong value for Timer Use Expiration flags.  It's not what
> you want to do.
> 
> What bug are you trying to solve?
> 
> -corey