From: Andrew Lunn <andrew@lunn.ch>
Sent: Wednesday, 26 June 2024 16:40
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

quoted

quoted

+int ethtool_cmis_wait_for_cond(struct net_device *dev, u8 flags, u8

flag,

quoted

quoted

quoted

quoted

+			       u16 max_duration, u32 offset,
+			       bool (*cond_success)(u8), bool

(*cond_fail)(u8),

quoted

quoted

quoted

quoted

+			       u8 *state)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {0};
+	struct cmis_wait_for_cond_rpl rpl = {};
+	struct netlink_ext_ack extack = {};
+	unsigned long end;
+	int err;
+
+	if (!(flags & flag))
+		return 0;
+
+	if (max_duration == 0)
+		max_duration = U16_MAX;
+
+	end = jiffies + msecs_to_jiffies(max_duration);
+	do {
+		ethtool_cmis_page_init(&page_data, 0, offset,

sizeof(rpl));

quoted

quoted

quoted

quoted

+		page_data.data = (u8 *)&rpl;
+
+		err = ops->get_module_eeprom_by_page(dev,

&page_data,

quoted

quoted

quoted

&extack);

quoted

+		if (err < 0) {
+			if (extack._msg)
+				netdev_err(dev, "%s\n",

extack._msg);

quoted

quoted

quoted

quoted

+			continue;

continue here is interested. Say you get -EIO because the module
has been ejected. I would say that is fatal. Won't this spam the
logs, as fast as the I2C bus can fail, without the 20ms sleep, for 65535

jiffies?

quoted

quoted

If the module is ejected from some reason, it might span the logs I guess.

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.

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?
I mean, it is not very reasonable thing to do, burning fw on a module and in the exact same time eject it.

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));

Please could you implement this using iopoll.h. This appears to
have the usual problem. Say msleep(20) actually sleeps a lot
longer, because the system is busy doing other things.
time_before(jiffies,
end)) is false, because of the long delay, but in fact the
operation has completed without error. Yet you return EBUSY.
iopoll.h gets this correct, it does one more evaluation of the
condition after exiting the loop to handle this issue.

OK.

Hi Andrew,

Therefore, unfortunately in this case I'd rather stay with the origin code.

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?

Thanks,
Danielle

	Andrew