On Mon, Dec 07, 2020 at 10:54:26PM -0600, Scott Cheloha wrote:
> On Wed, Dec 02, 2020 at 11:43:32PM +0100, Mark Kettenis wrote:
> > > From: "Constantine A. Murenin" <muren...@gmail.com>
> > > Date: Wed, 2 Dec 2020 14:04:52 -0800
> > >
> > > Not sure if you've seen it, but ipmi(4) has been disabled for over 12
> > > years, because it's broken on some machines, so, this code is not
> > > necessarily guaranteed to be correct as-is.
> > >
> > > http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/i386/conf/GENERIC#rev1.632
> > > http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/amd64/conf/GENERIC#rev1.238
> >
> > The driver is actually enabled on arm64. And I'll probably enable it
> > on powerpc64 at some point.
>
> kettenis@/sthen@:
>
> In that case, could one or both of you test this diff?
>
> I doubt anyone remembers why we spin until tsleep(9) returns
> EWOULDBLOCK. If we can confirm that the driver still works with a 1ms
> block in this spot then that's good enough for me.
>
> So, does this still work?
>
> Index: ipmi.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ipmi.c,v
> retrieving revision 1.112
> diff -u -p -r1.112 ipmi.c
> --- ipmi.c 29 Mar 2020 09:31:10 -0000 1.112
> +++ ipmi.c 2 Dec 2020 20:31:57 -0000
> @@ -1497,7 +1497,8 @@ ipmi_poll_thread(void *arg)
> printf("%s: no SDRs IPMI disabled\n", DEVNAME(sc));
> goto done;
> }
> - while (tsleep(sc, PWAIT, "ipmirun", 1) != EWOULDBLOCK)
> + while (tsleep_nsec(sc, PWAIT, "ipmirun",
> + MSEC_TO_NSEC(1)) != EWOULDBLOCK)
> continue;
> }
>
This idiom of a quick sleep is a bit strange and I would prefer if this is
rewritten to be a simple tsleep_nsec call without the while loop.
Since there is no corresponding wakeup call this tsleep can only return
EWOULDBLOCK there is no way to return any other value (PCATCH is not set
and nothing will do a wakeup).
So this could be simply written as:
tsleep_nsec(sc, PWAIT, "ipmirun", MSEC_TO_NSEC(1));
This whole poll thread is just way more complicated then it needs to be.
Neither current_sensor nor thread->running are needed. I'm not even sure
if the tsleep itself is needed in that discovery loop. get_sdr() calls
ipmi_cmd() which does another tsleep to wait for the command.
This driver seems to just use all the concepts without much thought. I bet
ipmi_cmd() calls can race against each other.
--
:wq Claudio
