* Re: [PATCH 1/1] ipmi/watchdog: Add WDIOC_GETTIMELEFT ioctl
[not found] ` <CAKVu-U=Om6CQF7DVkzp487=R_a77Qbi_grKQ7J-HPVZ_QR7DfA@mail.gmail.com>
@ 2021-05-05 21:36 ` Corey Minyard
0 siblings, 0 replies; 2+ messages in thread
From: Corey Minyard @ 2021-05-05 21:36 UTC (permalink / raw)
To: Ryan O'Leary; +Cc: openipmi-developer, linux-kernel
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
> >
> >
^ permalink raw reply [flat|nested] 2+ messages in thread