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));
+    }
+
     switch (IPMI_BMC_WATCHDOG_GET_ACTION(ibs)) {
     case IPMI_BMC_WATCHDOG_ACTION_NONE:
         sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 0, 1,
-- 
2.47.1

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".

>      switch (IPMI_BMC_WATCHDOG_GET_ACTION(ibs)) {
>      case IPMI_BMC_WATCHDOG_ACTION_NONE:
>          sensor_set_discrete_bit(ibs, IPMI_WATCHDOG_SENSOR, 0, 1,
> -- 
> 2.47.1
>

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.

Thanks,
Nick

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.

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

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