Re: [PATCH 1/1] ipmi/watchdog: Add WDIOC_GETTIMELEFT ioctl

[Corey Minyard <minyard@acm.org>]

On Tue, May 04, 2021 at 01:59:49PM -0700, Ryan O'Leary wrote:
> Hi, please let me know if there's anything missing for review.

Only my negligence.

Unfortunately I can't add anything to my next tree at the moment, I have
to wait until the merge window closes.

Anyway, on the patch, if you look at the definition of msg_wait, it
says:

/*
 * We use a mutex to make sure that only one thing can send a set a
 * message at one time.  The mutex is claimed when a message is sent
 * and freed when both the send and receive messages are free.
 */

So if you use msg_wait, you need to grab that mutex, or bad things can
happen.

However, that means that a non-critical operation, grabbing the time
left, can result in blocking a more critical operations, pinging the
watchdog.  So it would be better if you could use another set of
messages for doing this.

-corey

> 
> On Tue, Mar 16, 2021 at 5:11 PM Ryan O'Leary <ryanoleary@google.com> wrote:
> 
> > This is the same ioctl the rest of the watchdogs support. GETTIMELEFT
> > returns the number of seconds in the countdown -- useful for testing
> > whether the watchdog is functioning.
> >
> > Signed-off-by: Ryan O'Leary <ryanoleary@google.com>
> > ---
> >  drivers/char/ipmi/ipmi_watchdog.c | 75 +++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >
> > diff --git a/drivers/char/ipmi/ipmi_watchdog.c
> > b/drivers/char/ipmi/ipmi_watchdog.c
> > index 32c334e34d55..f253c8667395 100644
> > --- a/drivers/char/ipmi/ipmi_watchdog.c
> > +++ b/drivers/char/ipmi/ipmi_watchdog.c
> > @@ -456,6 +456,71 @@ static int ipmi_set_timeout(int do_heartbeat)
> >         return rv;
> >  }
> >
> > +static unsigned int __ipmi_get_timeout(struct ipmi_smi_msg  *smi_msg,
> > +                                      struct ipmi_recv_msg *recv_msg,
> > +                                      int                  *countdown)
> > +{
> > +       struct kernel_ipmi_msg            msg;
> > +       int                               rv = 0;
> > +       struct ipmi_system_interface_addr addr;
> > +
> > +
> > +       addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
> > +       addr.channel = IPMI_BMC_CHANNEL;
> > +       addr.lun = 0;
> > +
> > +       msg.netfn = 0x06;
> > +       msg.cmd = IPMI_WDOG_GET_TIMER;
> > +       msg.data = NULL;
> > +       msg.data_len = 0;
> > +       rv = ipmi_request_supply_msgs(watchdog_user,
> > +                                     (struct ipmi_addr *) &addr,
> > +                                     0,
> > +                                     &msg,
> > +                                     NULL,
> > +                                     smi_msg,
> > +                                     recv_msg,
> > +                                     1);
> > +       if (rv) {
> > +               pr_warn("get timeout error: %d\n", rv);
> > +               return rv;
> > +       }
> > +
> > +       wait_for_completion(&msg_wait);
> > +
> > +       if (recv_msg->msg.data_len < 9) {
> > +               pr_warn("get timeout response size: %d (expected 9)\n",
> > +                       recv_msg->msg.data_len);
> > +               return -EIO;
> > +       }
> > +
> > +       if (recv_msg->msg.data[0] != 0)  {
> > +               pr_warn("get timeout completion code error: %d\n",
> > +                       recv_msg->msg.data[0]);
> > +               return -EIO;
> > +       }
> > +
> > +       *countdown = WDOG_GET_TIMEOUT(recv_msg->msg.data[7],
> > recv_msg->msg.data[8]);
> > +
> > +       return rv;
> > +}
> > +
> > +static int _ipmi_get_timeout(int *countdown)
> > +{
> > +       int rv;
> > +
> > +       if (!watchdog_user)
> > +               return -ENODEV;
> > +
> > +       atomic_set(&msg_tofree, 2);
> > +
> > +       rv = __ipmi_get_timeout(&smi_msg,
> > +                               &recv_msg,
> > +                               countdown);
> > +
> > +       return rv;
> > +}
> > +
> >  static atomic_t panic_done_count = ATOMIC_INIT(0);
> >
> >  static void panic_smi_free(struct ipmi_smi_msg *msg)
> > @@ -729,6 +794,16 @@ static int ipmi_ioctl(struct file *file,
> >                         return -EFAULT;
> >                 return 0;
> >
> > +       case WDIOC_GETTIMELEFT:
> > +               val = 0;
> > +               i = _ipmi_get_timeout(&val);
> > +               if (i)
> > +                       return i;
> > +               i = copy_to_user(argp, &val, sizeof(val));
> > +               if (i)
> > +                       return -EFAULT;
> > +               return 0;
> > +
> >         default:
> >                 return -ENOIOCTLCMD;
> >         }
> > --
> > 2.31.0.rc2.261.g7f71774620-goog
> >
> >