netdev - 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:
 <DM6PR12MB4516DD74CA5F4D52D5290E26D8D62@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Wed, 26 Jun 2024 06:14:22 +0000
From: Danielle Ratson <danieller@...dia.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"corbet@....net" <corbet@....net>, "linux@...linux.org.uk"
	<linux@...linux.org.uk>, "sdf@...gle.com" <sdf@...gle.com>,
	"kory.maincent@...tlin.com" <kory.maincent@...tlin.com>,
	"maxime.chevallier@...tlin.com" <maxime.chevallier@...tlin.com>,
	"vladimir.oltean@....com" <vladimir.oltean@....com>,
	"przemyslaw.kitszel@...el.com" <przemyslaw.kitszel@...el.com>,
	"ahmed.zaki@...el.com" <ahmed.zaki@...el.com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, "shayagr@...zon.com" <shayagr@...zon.com>,
	"paul.greenwalt@...el.com" <paul.greenwalt@...el.com>, "jiri@...nulli.us"
	<jiri@...nulli.us>, "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, mlxsw
	<mlxsw@...dia.com>, Ido Schimmel <idosch@...dia.com>, Petr Machata
	<petrm@...dia.com>
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.

> From: Andrew Lunn <andrew@...n.ch>
> Sent: Monday, 24 June 2024 22:51
> 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 <mlxsw@...dia.com>; Ido Schimmel
> <idosch@...dia.com>; Petr Machata <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 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. 

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

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