From: Andrew Lunn <andrew@lunn.ch>
Sent: Wednesday, 26 June 2024 20:43
To: Danielle Ratson <redacted>
Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com;
kuba@kernel.org; pabeni@redhat.com; corbet@lwn.net;
linux@armlinux.org.uk; sdf@google.com; kory.maincent@bootlin.com;
maxime.chevallier@bootlin.com; vladimir.oltean@nxp.com;
przemyslaw.kitszel@intel.com; ahmed.zaki@intel.com;
richardcochran@gmail.com; shayagr@amazon.com;
paul.greenwalt@intel.com; jiri@resnulli.us; linux-doc@vger.kernel.org; linux-
kernel@vger.kernel.org; mlxsw [off-list ref]; Ido Schimmel
[off-list ref]; Petr Machata [off-list ref]
Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
supporting CDB commands

quoted

quoted

Please could you test it.

65535 jiffies is i think 655 seconds? That is probably too long to
loop when the module has been ejected. Maybe replace it with HZ?

Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.

I _think_ it depends on CONFIG_HZ, which can be 100, 250, 300 and 1000.

quoted

The test you are asking for is a bit complicated since I don’t have a
machine physically nearby, do you find it very much important?

quoted

I mean, it is not very reasonable thing to do, burning fw on a module
and in the exact same time eject it.

Shooting yourself in the foot is not a very reasonable thing to do, but the Unix
philosophy is to all root to do it. Do we really want 60 to 600 seconds of the
kernel spamming the log when somebody does do this?

Ok i checked it and using netdev_err_once() fulfill that issue. Thanks!

quoted

quoted

Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
delay before the continue.

quoted

quoted

quoted

quoted

+		}
+
+		if ((*cond_success)(rpl.state))
+			return 0;
+
+		if (*cond_fail && (*cond_fail)(rpl.state))
+			break;
+
+		msleep(20);
+	} while (time_before(jiffies, end));

quoted

quoted

O.K. Please evaluate the condition again after the while() just so
ETIMEDOUT is not returned in error.

Not sure I understood.
Do you want to have one more polling in the end of the loop? What could

return ETIMEDOUT?

Consider what happens when msleep(20) actually sleeps a lot longer.

Look at the core code which gets this correct:

#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
                                sleep_before_read, args...) \ ({ \
        u64 __timeout_us = (timeout_us); \
        unsigned long __sleep_us = (sleep_us); \
        ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
        might_sleep_if((__sleep_us) != 0); \
        if (sleep_before_read && __sleep_us) \
                usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
        for (;;) { \
                (val) = op(args); \
                if (cond) \
                        break; \
                if (__timeout_us && \
                    ktime_compare(ktime_get(), __timeout) > 0) { \
                        (val) = op(args); \
                        break; \
                } \
                if (__sleep_us) \
                        usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
                cpu_relax(); \
        } \
        (cond) ? 0 : -ETIMEDOUT; \
})

So after breaking out of the for loop with a timeout, it evaluates the condition
one more time, and uses that to decide on 0 or ETIMEDOUT. So it does not
matter if usleep_range() range slept for 60 seconds, not 60ms, the exit code
will be correct.

      Andrew

Ok ill fix it, thanks.