On Thu, Dec 30, 2021 at 10:24:10AM -0800, Dimitris Michailidis wrote:

On Thu, Dec 30, 2021 at 9:28 AM Andrew Lunn [off-list ref] wrote:

quoted

quoted

+/* 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.

quoted

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