On Thu, Dec 10, 2020 at 10:00:46AM +0100, Claudio Jeker wrote:
> On Mon, Dec 07, 2020 at 10:54:26PM -0600, Scott Cheloha wrote:
> > 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.
One thing at a time.
First, remove the loop. It is unnecessary, as there is no other
thread calling wakeup(9), i.e. tsleep_nsec(9) will always return
EWOULDBLOCK here.
ok?
Index: ipmi.c
===================================================================
RCS file: /cvs/src/sys/dev/ipmi.c,v
retrieving revision 1.113
diff -u -p -r1.113 ipmi.c
--- ipmi.c 11 Dec 2020 04:00:33 -0000 1.113
+++ ipmi.c 11 Dec 2020 04:05:31 -0000
@@ -1497,9 +1497,7 @@ ipmi_poll_thread(void *arg)
printf("%s: no SDRs IPMI disabled\n", DEVNAME(sc));
goto done;
}
- while (tsleep_nsec(sc, PWAIT, "ipmirun",
- MSEC_TO_NSEC(1)) != EWOULDBLOCK)
- continue;
+ tsleep_nsec(sc, PWAIT, "ipmirun", MSEC_TO_NSEC(1));
}
/* initialize sensor list for thread */
