On Thu, Dec 10, 2020 at 10:07:29PM -0600, Scott Cheloha wrote:
> 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 */
OK claudio@
--
:wq Claudio
