linux-kernel - Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <003ca0dd-ea1c-4721-8c3f-d4a578662057@lunn.ch>
Date: Mon, 24 Jun 2024 21:50:42 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Danielle Ratson <danieller@...dia.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, corbet@....net,
	linux@...linux.org.uk, sdf@...gle.com, kory.maincent@...tlin.com,
	maxime.chevallier@...tlin.com, vladimir.oltean@....com,
	przemyslaw.kitszel@...el.com, ahmed.zaki@...el.com,
	richardcochran@...il.com, shayagr@...zon.com,
	paul.greenwalt@...el.com, jiri@...nulli.us,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	mlxsw@...dia.com, idosch@...dia.com, petrm@...dia.com
Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
 supporting CDB commands

> +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);
> +		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 ((*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.

> +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

---
pw-bot: cr