Hi Andrew,

Thanks for reviewing the patches.

From: Andrew Lunn <andrew@lunn.ch>
Sent: Monday, 24 June 2024 22:51
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

+int ethtool_cmis_wait_for_cond(struct net_device *dev, u8 flags, u8 flag,
+			       u16 max_duration, u32 offset,
+			       bool (*cond_success)(u8), bool (*cond_fail)(u8),
+			       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));
+		page_data.data = (u8 *)&rpl;
+
+		err = ops->get_module_eeprom_by_page(dev, &page_data,

&extack);

quoted

+		if (err < 0) {
+			if (extack._msg)
+				netdev_err(dev, "%s\n", extack._msg);
+			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?

If the module is ejected from some reason, it might span the logs I guess.
But it is less likely than the scenario I wanted to cover.
According to SPEC 5.2:

"
7.2.5.1 Foreground Mode CDB Messaging
[...]
In foreground mode the module rejects any register ACCESS until a currently executing CDB command execution has completed.
Note: READs of the CdbStatus registers 00h:37 or 00h:38 (see Table 8-13) will also be rejected by the module.
"

So in that case the module won't be able to respond and we need to wait for it to be responsive and the status to be valid. 

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.

quoted

+static u8 cmis_cdb_calc_checksum(const void *data, size_t size) {
+	const u8 *bytes = (const u8 *)data;
+	u8 checksum = 0;
+
+	for (size_t i = 0; i < size; i++)
+		checksum += bytes[i];
+
+	return ~checksum;
+}

I expect there is already a helper do that somewhere.

    Andrew

Yes it does, but actually it is an helper that occurs in specific places (for example pci_vpd_check_csum()), that i can use from here.

---
pw-bot: cr