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:
 <DM6PR12MB4516907EAC007FCB05955F7CD8D62@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Wed, 26 Jun 2024 11:52:26 +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

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

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

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