From: Danielle Ratson <redacted>
Sent: Wednesday, 26 June 2024 9:14
To: Andrew Lunn <andrew@lunn.ch>
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

Hi Andrew,

Thanks for reviewing the patches.

quoted

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;

quoted

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

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,

I implemented the above as you asked, but it seems to have a problem.
The iopoll functions have a sleeping parameter "sleep_us" that supposed to be equivalent to the msleep(20) if I put 20000 there.
However, this parameter is defined as 'Maximum time to sleep between reads in us', so it will not always sleep 20msec as it should.
This is problematic since there are modules that needs this 20msec sleep in order to be able to poll again from the module.
Otherwise, these modules fail during the write FW command iterations, while polling the flag or status.
Therefore, unfortunately in this case I'd rather stay with the origin code.

Thank you for all your comments,
Danielle

quoted

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.

quoted

---
pw-bot: cr