Re: [PATCH net-next 2/8] net/fungible: Add service module for Fungible drivers

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Dec 30, 2021 at 10:24:10AM -0800, Dimitris Michailidis wrote:
> On Thu, Dec 30, 2021 at 9:28 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > +/* Wait for the CSTS.RDY bit to match @enabled. */
> > > +static int fun_wait_ready(struct fun_dev *fdev, bool enabled)
> > > +{
> > > +     unsigned int cap_to = NVME_CAP_TIMEOUT(fdev->cap_reg);
> > > +     unsigned long timeout = ((cap_to + 1) * HZ / 2) + jiffies;
> > > +     u32 bit = enabled ? NVME_CSTS_RDY : 0;
> >
> > Reverse Christmas tree, since this is a network driver.
> 
> The longer line in the middle depends on the previous line, I'd need to
> remove the initializers to sort these by length.

Yes.


> > Please also consider using include/linux/iopoll.h. The signal handling
> > might make that not possible, but signal handling in driver code is in
> > itself very unusual.
> 
> This initialization is based on NVMe, hence the use of NVMe registers,
> and this function is based on nvme_wait_ready(). The check sequence
> including signal handling comes from there.
> 
> iopoll is possible with the signal check removed, though I see I'd need a
> shorter delay than the 100ms used here and it doesn't check for reads of
> all 1s, which happen occasionally. My preference though would be to keep
> this close to the NVMe version. Let me know.

I knew it would be hard to directly use iopoll, which is why i only
said 'consider'. The problem is, this implementation has the same bug
nearly everybody makes when writing their own implementation of what
iopoll does, which is why i always point people at iopoll.

msleep(100) guarantees that it will not return within 100ms. That is
all. Consider what happens when msleep(100) actually sleeps for
1000.

	Andrew