[net-next,v7,7/9] ethtool: cmis_cdb: Add a layer for supporting CDB commands

Commit Message

Danielle Ratson June 24, 2024, 5:51 p.m. UTC

CDB (Command Data Block Message Communication) reads and writes are
performed on memory map pages 9Fh-AFh according to the CMIS standard,
section 8.20 of revision 5.2.
Page 9Fh is used to specify the CDB command to be executed and also
provides an area for a local payload (LPL).

According to the CMIS standard, the firmware update process is done using
a CDB commands sequence that will be implemented in the next patch.

The kernel interface that will implement the firmware update using CDB
command will include 2 layers that will be added under ethtool:

* The upper layer that will be triggered from the module layer, is
  cmis_fw_update.
* The lower one is cmis_cdb.

In the future there might be more operations to implement using CDB
commands. Therefore, the idea is to keep the CDB interface clean and the
cmis_fw_update specific to the CDB commands handling it.

These two layers will communicate using the API the consists of three
functions:

- struct ethtool_cmis_cdb *
  ethtool_cmis_cdb_init(struct net_device *dev,
			struct ethtool_module_fw_flash_params *params);
- void ethtool_cmis_cdb_fini(struct ethtool_cmis_cdb *cdb);
- int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
				   struct ethtool_cmis_cdb_cmd_args *args);

Add the CDB layer to support initializing, finishing and executing CDB
commands:

* The initialization process will include creating of an ethtool_cmis_cdb
  instance, querying the module CDB support, entering and validating the
  password from user space (CMD 0x0000) and querying the module features
  (CMD 0x0040).

* The finishing API will simply free the ethtool_cmis_cdb instance.

* The executing process will write the CDB command to EEPROM using
  set_module_eeprom_by_page() that was presented earlier, and will
  process the reply from EEPROM.

Signed-off-by: Danielle Ratson <[e-mail]>
Reviewed-by: Petr Machata <[e-mail]>
---

Notes:
    v6:
    	* In ethtool_cmis_cdb_init(), Use 'const' for the params
    	  parameter.
    
    v5:
    	* Drop all the inline in cmis_cdb.c.
    
    v4:
    	* Add kernel-doc for msleep_pre_rpl and err_msg.
    
    v3:
    	* Use kmemdup() instead of kmalloc+memcpy.
    
    v2:
    	* Define ethtool_cmis_cdb_request::epl_len to be __be16 instead
    	  of u16.

 net/ethtool/Makefile    |   2 +-
 net/ethtool/cmis.h      | 117 ++++++++
 net/ethtool/cmis_cdb.c  | 581 ++++++++++++++++++++++++++++++++++++++++
 net/ethtool/module_fw.h |  10 +
 4 files changed, 709 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/cmis.h
 create mode 100644 net/ethtool/cmis_cdb.c

Comments

Andrew Lunn June 24, 2024, 7:50 p.m. UTC | #1

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

Danielle Ratson June 26, 2024, 6:14 a.m. UTC | #2

Hi Andrew,

Thanks for reviewing the patches.

> From: Andrew Lunn <[e-mail]>
> Sent: Monday, 24 June 2024 22:51
> To: Danielle Ratson <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; mlxsw <[e-mail]>; Ido Schimmel
> <[e-mail]>; Petr Machata <[e-mail]>
> 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

Danielle Ratson June 26, 2024, 11:52 a.m. UTC | #3

> From: Danielle Ratson <[e-mail]>
> Sent: Wednesday, 26 June 2024 9:14
> To: Andrew Lunn <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; mlxsw <[e-mail]>; Ido Schimmel
> <[e-mail]>; Petr Machata <[e-mail]>
> 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 <[e-mail]>
> > Sent: Monday, 24 June 2024 22:51
> > To: Danielle Ratson <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > linux- [e-mail]; mlxsw <[e-mail]>; Ido Schimmel
> > <[e-mail]>; Petr Machata <[e-mail]>
> > 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

Andrew Lunn June 26, 2024, 1:40 p.m. UTC | #4

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

Please could you test it.

65535 jiffies is i think 655 seconds? That is probably too long to
loop when the module has been ejected. Maybe replace it with HZ?

Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
delay before the continue.

> > > > +		}
> > > > +
> > > > +		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,
> 
> Therefore, unfortunately in this case I'd rather stay with the origin code.

O.K. Please evaluate the condition again after the while() just so
ETIMEDOUT is not returned in error.

	Andrew

Danielle Ratson June 26, 2024, 5:26 p.m. UTC | #5

> From: Andrew Lunn <[e-mail]>
> Sent: Wednesday, 26 June 2024 16:40
> To: Danielle Ratson <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; mlxsw <[e-mail]>; Ido Schimmel
> <[e-mail]>; Petr Machata <[e-mail]>
> 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.
> 
> Please could you test it.
> 
> 65535 jiffies is i think 655 seconds? That is probably too long to loop when
> the module has been ejected. Maybe replace it with HZ?
> 

Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.

The test you are asking for is a bit complicated since I don’t have a machine physically nearby, do you find it very much important?
I mean, it is not very reasonable thing to do, burning fw on a module and in the exact same time eject it.

> Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
> delay before the continue.
> 
> > > > > +		}
> > > > > +
> > > > > +		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,
> >
> > Therefore, unfortunately in this case I'd rather stay with the origin code.
> 
> O.K. Please evaluate the condition again after the while() just so ETIMEDOUT is
> not returned in error.

Not sure I understood.
Do you want to have one more polling in the end of the loop? What could return ETIMEDOUT?

Thanks,
Danielle

> 
> 	Andrew

Andrew Lunn June 26, 2024, 5:42 p.m. UTC | #6

> > Please could you test it.
> > 
> > 65535 jiffies is i think 655 seconds? That is probably too long to loop when
> > the module has been ejected. Maybe replace it with HZ?
> > 
> 
> Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.

I _think_ it depends on CONFIG_HZ, which can be 100, 250, 300 and
1000.

> The test you are asking for is a bit complicated since I don’t have
> a machine physically nearby, do you find it very much important?

> I mean, it is not very reasonable thing to do, burning fw on a
> module and in the exact same time eject it.

Shooting yourself in the foot is not a very reasonable thing to do,
but the Unix philosophy is to all root to do it. Do we really want 60
to 600 seconds of the kernel spamming the log when somebody does do
this?

> > Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
> > delay before the continue.
> > 
> > > > > > +		}
> > > > > > +
> > > > > > +		if ((*cond_success)(rpl.state))
> > > > > > +			return 0;
> > > > > > +
> > > > > > +		if (*cond_fail && (*cond_fail)(rpl.state))
> > > > > > +			break;
> > > > > > +
> > > > > > +		msleep(20);
> > > > > > +	} while (time_before(jiffies, end));
> > > > >

> > O.K. Please evaluate the condition again after the while() just so ETIMEDOUT is
> > not returned in error.
> 
> Not sure I understood.
> Do you want to have one more polling in the end of the loop? What could return ETIMEDOUT?

Consider what happens when msleep(20) actually sleeps a lot longer.

Look at the core code which gets this correct:

#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
                                sleep_before_read, args...) \
({ \
        u64 __timeout_us = (timeout_us); \
        unsigned long __sleep_us = (sleep_us); \
        ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
        might_sleep_if((__sleep_us) != 0); \
        if (sleep_before_read && __sleep_us) \
                usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
        for (;;) { \
                (val) = op(args); \
                if (cond) \
                        break; \
                if (__timeout_us && \
                    ktime_compare(ktime_get(), __timeout) > 0) { \
                        (val) = op(args); \
                        break; \
                } \
                if (__sleep_us) \
                        usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
                cpu_relax(); \
        } \
        (cond) ? 0 : -ETIMEDOUT; \
})

So after breaking out of the for loop with a timeout, it evaluates the
condition one more time, and uses that to decide on 0 or ETIMEDOUT. So
it does not matter if usleep_range() range slept for 60 seconds, not
60ms, the exit code will be correct.

      Andrew

Danielle Ratson June 27, 2024, 1:12 p.m. UTC | #7

> From: Andrew Lunn <[e-mail]>
> Sent: Wednesday, 26 June 2024 20:43
> To: Danielle Ratson <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; mlxsw <[e-mail]>; Ido Schimmel
> <[e-mail]>; Petr Machata <[e-mail]>
> Subject: Re: [PATCH net-next v7 7/9] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
> 
> > > Please could you test it.
> > >
> > > 65535 jiffies is i think 655 seconds? That is probably too long to
> > > loop when the module has been ejected. Maybe replace it with HZ?
> > >
> >
> > Well actually it is 65535 msec which is ~65 sec and a bit over 1 minute.
> 
> I _think_ it depends on CONFIG_HZ, which can be 100, 250, 300 and 1000.
> 
> > The test you are asking for is a bit complicated since I don’t have a
> > machine physically nearby, do you find it very much important?
> 
> > I mean, it is not very reasonable thing to do, burning fw on a module
> > and in the exact same time eject it.
> 
> Shooting yourself in the foot is not a very reasonable thing to do, but the Unix
> philosophy is to all root to do it. Do we really want 60 to 600 seconds of the
> kernel spamming the log when somebody does do this?

Ok i checked it and using netdev_err_once() fulfill that issue. Thanks!

> 
> > > Maybe netdev_err() should become netdev_dbg()? And please add a 20ms
> > > delay before the continue.
> > >
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		if ((*cond_success)(rpl.state))
> > > > > > > +			return 0;
> > > > > > > +
> > > > > > > +		if (*cond_fail && (*cond_fail)(rpl.state))
> > > > > > > +			break;
> > > > > > > +
> > > > > > > +		msleep(20);
> > > > > > > +	} while (time_before(jiffies, end));
> > > > > >
> 
> > > O.K. Please evaluate the condition again after the while() just so
> > > ETIMEDOUT is not returned in error.
> >
> > Not sure I understood.
> > Do you want to have one more polling in the end of the loop? What could
> return ETIMEDOUT?
> 
> Consider what happens when msleep(20) actually sleeps a lot longer.
> 
> Look at the core code which gets this correct:
> 
> #define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
>                                 sleep_before_read, args...) \ ({ \
>         u64 __timeout_us = (timeout_us); \
>         unsigned long __sleep_us = (sleep_us); \
>         ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
>         might_sleep_if((__sleep_us) != 0); \
>         if (sleep_before_read && __sleep_us) \
>                 usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
>         for (;;) { \
>                 (val) = op(args); \
>                 if (cond) \
>                         break; \
>                 if (__timeout_us && \
>                     ktime_compare(ktime_get(), __timeout) > 0) { \
>                         (val) = op(args); \
>                         break; \
>                 } \
>                 if (__sleep_us) \
>                         usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
>                 cpu_relax(); \
>         } \
>         (cond) ? 0 : -ETIMEDOUT; \
> })
> 
> So after breaking out of the for loop with a timeout, it evaluates the condition
> one more time, and uses that to decide on 0 or ETIMEDOUT. So it does not
> matter if usleep_range() range slept for 60 seconds, not 60ms, the exit code
> will be correct.
> 
>       Andrew

Ok ill fix it, thanks.

Patch

diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 504f954a1b28..38806b3ecf83 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@  ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
 		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o mm.o \
-		   module.o pse-pd.o plca.o mm.o
+		   module.o cmis_cdb.o pse-pd.o plca.o mm.o
diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
new file mode 100644
index 000000000000..295f5d0df915
--- /dev/null
+++ b/net/ethtool/cmis.h
@@ -0,0 +1,117 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#define ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH		120
+#define ETHTOOL_CMIS_CDB_CMD_PAGE			0x9F
+#define ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR			0x50
+
+/**
+ * struct ethtool_cmis_cdb - CDB commands parameters
+ * @cmis_rev: CMIS revision major.
+ * @read_write_len_ext: Allowable additional number of byte octets to the LPL
+ *			in a READ or a WRITE CDB commands.
+ * @max_completion_time:  Maximum CDB command completion time in msec.
+ */
+struct ethtool_cmis_cdb {
+	u8	cmis_rev;
+	u8      read_write_len_ext;
+	u16     max_completion_time;
+};
+
+enum ethtool_cmis_cdb_cmd_id {
+	ETHTOOL_CMIS_CDB_CMD_QUERY_STATUS		= 0x0000,
+	ETHTOOL_CMIS_CDB_CMD_MODULE_FEATURES		= 0x0040,
+};
+
+/**
+ * struct ethtool_cmis_cdb_request - CDB commands request fields as decribed in
+ *				the CMIS standard
+ * @id: Command ID.
+ * @epl_len: EPL memory length.
+ * @lpl_len: LPL memory length.
+ * @chk_code: Check code for the previous field and the payload.
+ * @resv1: Added to match the CMIS standard request continuity.
+ * @resv2: Added to match the CMIS standard request continuity.
+ * @payload: Payload for the CDB commands.
+ */
+struct ethtool_cmis_cdb_request {
+	__be16 id;
+	struct_group(body,
+		__be16 epl_len;
+		u8 lpl_len;
+		u8 chk_code;
+		u8 resv1;
+		u8 resv2;
+		u8 payload[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH];
+	);
+};
+
+#define CDB_F_COMPLETION_VALID		BIT(0)
+#define CDB_F_STATUS_VALID		BIT(1)
+
+/**
+ * struct ethtool_cmis_cdb_cmd_args - CDB commands execution arguments
+ * @req: CDB command fields as described in the CMIS standard.
+ * @max_duration: Maximum duration time for command completion in msec.
+ * @read_write_len_ext: Allowable additional number of byte octets to the LPL
+ *			in a READ or a WRITE commands.
+ * @msleep_pre_rpl: Waiting time before checking reply in msec.
+ * @rpl_exp_len: Expected reply length in bytes.
+ * @flags: Validation flags for CDB commands.
+ * @err_msg: Error message to be sent to user space.
+ */
+struct ethtool_cmis_cdb_cmd_args {
+	struct ethtool_cmis_cdb_request req;
+	u16				max_duration;
+	u8				read_write_len_ext;
+	u8				msleep_pre_rpl;
+	u8                              rpl_exp_len;
+	u8				flags;
+	char				*err_msg;
+};
+
+/**
+ * struct ethtool_cmis_cdb_rpl_hdr - CDB commands reply header arguments
+ * @rpl_len: Reply length.
+ * @rpl_chk_code: Reply check code.
+ */
+struct ethtool_cmis_cdb_rpl_hdr {
+	u8 rpl_len;
+	u8 rpl_chk_code;
+};
+
+/**
+ * struct ethtool_cmis_cdb_rpl - CDB commands reply arguments
+ * @hdr: CDB commands reply header arguments.
+ * @payload: Payload for the CDB commands reply.
+ */
+struct ethtool_cmis_cdb_rpl {
+	struct ethtool_cmis_cdb_rpl_hdr hdr;
+	u8 payload[ETHTOOL_CMIS_CDB_LPL_MAX_PL_LENGTH];
+};
+
+u32 ethtool_cmis_get_max_payload_size(u8 num_of_byte_octs);
+
+void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
+				   enum ethtool_cmis_cdb_cmd_id cmd, u8 *pl,
+				   u8 lpl_len, u16 max_duration,
+				   u8 read_write_len_ext, u16 msleep_pre_rpl,
+				   u8 rpl_exp_len, u8 flags);
+
+void ethtool_cmis_cdb_check_completion_flag(u8 cmis_rev, u8 *flags);
+
+void ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
+			    u8 page, u32 offset, u32 length);
+void ethtool_cmis_page_fini(struct ethtool_module_eeprom *page_data);
+
+struct ethtool_cmis_cdb *
+ethtool_cmis_cdb_init(struct net_device *dev,
+		      const struct ethtool_module_fw_flash_params *params,
+		      struct ethnl_module_fw_flash_ntf_params *ntf_params);
+void ethtool_cmis_cdb_fini(struct ethtool_cmis_cdb *cdb);
+
+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);
+
+int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
+				 struct ethtool_cmis_cdb_cmd_args *args);
diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
new file mode 100644
index 000000000000..ad7ac41bf679
--- /dev/null
+++ b/net/ethtool/cmis_cdb.c
@@ -0,0 +1,581 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/ethtool.h>
+#include <linux/jiffies.h>
+
+#include "common.h"
+#include "module_fw.h"
+#include "cmis.h"
+
+/* For accessing the LPL field on page 9Fh, the allowable length extension is
+ * min(i, 15) byte octets where i specifies the allowable additional number of
+ * byte octets in a READ or a WRITE.
+ */
+u32 ethtool_cmis_get_max_payload_size(u8 num_of_byte_octs)
+{
+	return 8 * (1 + min_t(u8, num_of_byte_octs, 15));
+}
+
+void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
+				   enum ethtool_cmis_cdb_cmd_id cmd, u8 *pl,
+				   u8 lpl_len, u16 max_duration,
+				   u8 read_write_len_ext, u16 msleep_pre_rpl,
+				   u8 rpl_exp_len, u8 flags)
+{
+	args->req.id = cpu_to_be16(cmd);
+	args->req.lpl_len = lpl_len;
+	if (pl)
+		memcpy(args->req.payload, pl, args->req.lpl_len);
+
+	args->max_duration = max_duration;
+	args->read_write_len_ext =
+		ethtool_cmis_get_max_payload_size(read_write_len_ext);
+	args->msleep_pre_rpl = msleep_pre_rpl;
+	args->rpl_exp_len = rpl_exp_len;
+	args->flags = flags;
+	args->err_msg = NULL;
+}
+
+void ethtool_cmis_page_init(struct ethtool_module_eeprom *page_data,
+			    u8 page, u32 offset, u32 length)
+{
+	page_data->page = page;
+	page_data->offset = offset;
+	page_data->length = length;
+	page_data->i2c_address = ETHTOOL_CMIS_CDB_PAGE_I2C_ADDR;
+}
+
+#define CMIS_REVISION_PAGE	0x00
+#define CMIS_REVISION_OFFSET	0x01
+
+struct cmis_rev_rpl {
+	u8 rev;
+};
+
+static u8 cmis_rev_rpl_major(struct cmis_rev_rpl *rpl)
+{
+	return rpl->rev >> 4;
+}
+
+static int cmis_rev_major_get(struct net_device *dev, u8 *rev_major)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {0};
+	struct netlink_ext_ack extack = {};
+	struct cmis_rev_rpl rpl = {};
+	int err;
+
+	ethtool_cmis_page_init(&page_data, CMIS_REVISION_PAGE,
+			       CMIS_REVISION_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);
+		return err;
+	}
+
+	*rev_major = cmis_rev_rpl_major(&rpl);
+
+	return 0;
+}
+
+#define CMIS_CDB_ADVERTISEMENT_PAGE	0x01
+#define CMIS_CDB_ADVERTISEMENT_OFFSET	0xA3
+
+/* Based on section 8.4.11 "CDB Messaging Support Advertisement" in CMIS
+ * standard revision 5.2.
+ */
+struct cmis_cdb_advert_rpl {
+	u8	inst_supported;
+	u8	read_write_len_ext;
+	u8	resv1;
+	u8	resv2;
+};
+
+static u8 cmis_cdb_advert_rpl_inst_supported(struct cmis_cdb_advert_rpl *rpl)
+{
+	return rpl->inst_supported >> 6;
+}
+
+static int cmis_cdb_advertisement_get(struct ethtool_cmis_cdb *cdb,
+				      struct net_device *dev)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_module_eeprom page_data = {};
+	struct cmis_cdb_advert_rpl rpl = {};
+	struct netlink_ext_ack extack = {};
+	int err;
+
+	ethtool_cmis_page_init(&page_data, CMIS_CDB_ADVERTISEMENT_PAGE,
+			       CMIS_CDB_ADVERTISEMENT_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);
+		return err;
+	}
+
+	if (!cmis_cdb_advert_rpl_inst_supported(&rpl))
+		return -EOPNOTSUPP;
+
+	cdb->read_write_len_ext = rpl.read_write_len_ext;
+
+	return 0;
+}
+
+#define CMIS_PASSWORD_ENTRY_PAGE	0x00
+#define CMIS_PASSWORD_ENTRY_OFFSET	0x7A
+
+struct cmis_password_entry_pl {
+	__be32 password;
+};
+
+/* See section 9.3.1 "CMD 0000h: Query Status" in CMIS standard revision 5.2.
+ * struct cmis_cdb_query_status_pl and struct cmis_cdb_query_status_rpl are
+ * structured layouts of the flat arrays,
+ * struct ethtool_cmis_cdb_request::payload and
+ * struct ethtool_cmis_cdb_rpl::payload respectively.
+ */
+struct cmis_cdb_query_status_pl {
+	u16 response_delay;
+};
+
+struct cmis_cdb_query_status_rpl {
+	u8 length;
+	u8 status;
+};
+
+static int
+cmis_cdb_validate_password(struct ethtool_cmis_cdb *cdb,
+			   struct net_device *dev,
+			   const struct ethtool_module_fw_flash_params *params,
+			   struct ethnl_module_fw_flash_ntf_params *ntf_params)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct cmis_cdb_query_status_pl qs_pl = {0};
+	struct ethtool_module_eeprom page_data = {};
+	struct ethtool_cmis_cdb_cmd_args args = {};
+	struct cmis_password_entry_pl pe_pl = {};
+	struct cmis_cdb_query_status_rpl *rpl;
+	struct netlink_ext_ack extack = {};
+	int err;
+
+	ethtool_cmis_page_init(&page_data, CMIS_PASSWORD_ENTRY_PAGE,
+			       CMIS_PASSWORD_ENTRY_OFFSET, sizeof(pe_pl));
+	page_data.data = (u8 *)&pe_pl;
+
+	pe_pl = *((struct cmis_password_entry_pl *)page_data.data);
+	pe_pl.password = params->password;
+	err = ops->set_module_eeprom_by_page(dev, &page_data, &extack);
+	if (err < 0) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+		return err;
+	}
+
+	ethtool_cmis_cdb_compose_args(&args, ETHTOOL_CMIS_CDB_CMD_QUERY_STATUS,
+				      (u8 *)&qs_pl, sizeof(qs_pl), 0,
+				      cdb->read_write_len_ext, 1000,
+				      sizeof(*rpl),
+				      CDB_F_COMPLETION_VALID | CDB_F_STATUS_VALID);
+
+	err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+	if (err < 0) {
+		ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+					      "Query Status command failed",
+					      args.err_msg);
+		return err;
+	}
+
+	rpl = (struct cmis_cdb_query_status_rpl *)args.req.payload;
+	if (!rpl->length || !rpl->status) {
+		ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+					      "Password was not accepted",
+					      NULL);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* Some CDB commands asserts the CDB completion flag only from CMIS
+ * revision 5. Therefore, check the relevant validity flag only when
+ * the revision supports it.
+ */
+void ethtool_cmis_cdb_check_completion_flag(u8 cmis_rev, u8 *flags)
+{
+	*flags |= cmis_rev >= 5 ? CDB_F_COMPLETION_VALID : 0;
+}
+
+#define CMIS_CDB_MODULE_FEATURES_RESV_DATA	34
+
+/* See section 9.4.1 "CMD 0040h: Module Features" in CMIS standard revision 5.2.
+ * struct cmis_cdb_module_features_rpl is structured layout of the flat
+ * array, ethtool_cmis_cdb_rpl::payload.
+ */
+struct cmis_cdb_module_features_rpl {
+	u8	resv1[CMIS_CDB_MODULE_FEATURES_RESV_DATA];
+	__be16	max_completion_time;
+};
+
+static u16
+cmis_cdb_module_features_completion_time(struct cmis_cdb_module_features_rpl *rpl)
+{
+	return be16_to_cpu(rpl->max_completion_time);
+}
+
+static int cmis_cdb_module_features_get(struct ethtool_cmis_cdb *cdb,
+					struct net_device *dev,
+					struct ethnl_module_fw_flash_ntf_params *ntf_params)
+{
+	struct ethtool_cmis_cdb_cmd_args args = {};
+	struct cmis_cdb_module_features_rpl *rpl;
+	u8 flags = CDB_F_STATUS_VALID;
+	int err;
+
+	ethtool_cmis_cdb_check_completion_flag(cdb->cmis_rev, &flags);
+	ethtool_cmis_cdb_compose_args(&args,
+				      ETHTOOL_CMIS_CDB_CMD_MODULE_FEATURES,
+				      NULL, 0, 0, cdb->read_write_len_ext,
+				      1000, sizeof(*rpl), flags);
+
+	err = ethtool_cmis_cdb_execute_cmd(dev, &args);
+	if (err < 0) {
+		ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+					      "Module Features command failed",
+					      args.err_msg);
+		return err;
+	}
+
+	rpl = (struct cmis_cdb_module_features_rpl *)args.req.payload;
+	cdb->max_completion_time =
+		cmis_cdb_module_features_completion_time(rpl);
+
+	return 0;
+}
+
+struct ethtool_cmis_cdb *
+ethtool_cmis_cdb_init(struct net_device *dev,
+		      const struct ethtool_module_fw_flash_params *params,
+		      struct ethnl_module_fw_flash_ntf_params *ntf_params)
+{
+	struct ethtool_cmis_cdb *cdb;
+	int err;
+
+	cdb = kzalloc(sizeof(*cdb), GFP_KERNEL);
+	if (!cdb)
+		return ERR_PTR(-ENOMEM);
+
+	err = cmis_rev_major_get(dev, &cdb->cmis_rev);
+	if (err < 0)
+		goto err;
+
+	if (cdb->cmis_rev < 4) {
+		ethnl_module_fw_flash_ntf_err(dev, ntf_params,
+					      "CMIS revision doesn't support module firmware flashing",
+					      NULL);
+		err = -EOPNOTSUPP;
+		goto err;
+	}
+
+	err = cmis_cdb_advertisement_get(cdb, dev);
+	if (err < 0)
+		goto err;
+
+	if (params->password_valid) {
+		err = cmis_cdb_validate_password(cdb, dev, params, ntf_params);
+		if (err < 0)
+			goto err;
+	}
+
+	err = cmis_cdb_module_features_get(cdb, dev, ntf_params);
+	if (err < 0)
+		goto err;
+
+	return cdb;
+
+err:
+	ethtool_cmis_cdb_fini(cdb);
+	return ERR_PTR(err);
+}
+
+void ethtool_cmis_cdb_fini(struct ethtool_cmis_cdb *cdb)
+{
+	kfree(cdb);
+}
+
+static bool is_completed(u8 data)
+{
+	return !!(data & 0x40);
+}
+
+#define CMIS_CDB_STATUS_SUCCESS	0x01
+
+static bool status_success(u8 data)
+{
+	return data == CMIS_CDB_STATUS_SUCCESS;
+}
+
+#define CMIS_CDB_STATUS_FAIL	0x40
+
+static bool status_fail(u8 data)
+{
+	return data & CMIS_CDB_STATUS_FAIL;
+}
+
+struct cmis_wait_for_cond_rpl {
+	u8 state;
+};
+
+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;
+		}
+
+		if ((*cond_success)(rpl.state))
+			return 0;
+
+		if (*cond_fail && (*cond_fail)(rpl.state))
+			break;
+
+		msleep(20);
+	} while (time_before(jiffies, end));
+
+	*state = rpl.state;
+	return -EBUSY;
+}
+
+#define CMIS_CDB_COMPLETION_FLAG_OFFSET	0x08
+
+static int cmis_cdb_wait_for_completion(struct net_device *dev,
+					struct ethtool_cmis_cdb_cmd_args *args)
+{
+	u8 flag;
+	int err;
+
+	/* Some vendors demand waiting time before checking completion flag
+	 * in some CDB commands.
+	 */
+	msleep(args->msleep_pre_rpl);
+
+	err = ethtool_cmis_wait_for_cond(dev, args->flags,
+					 CDB_F_COMPLETION_VALID,
+					 args->max_duration,
+					 CMIS_CDB_COMPLETION_FLAG_OFFSET,
+					 is_completed, NULL, &flag);
+	if (err < 0)
+		args->err_msg = "Completion Flag did not set on time";
+
+	return err;
+}
+
+#define CMIS_CDB_STATUS_OFFSET	0x25
+
+static void cmis_cdb_status_fail_msg_get(u8 status, char **err_msg)
+{
+	switch (status) {
+	case 0b10000001:
+		*err_msg = "CDB Status is in progress: Busy capturing command";
+		break;
+	case 0b10000010:
+		*err_msg =
+			"CDB Status is in progress: Busy checking/validating command";
+		break;
+	case 0b10000011:
+		*err_msg = "CDB Status is in progress: Busy executing";
+		break;
+	case 0b01000000:
+		*err_msg = "CDB status failed: no specific failure";
+		break;
+	case 0b01000010:
+		*err_msg =
+			"CDB status failed: Parameter range error or parameter not supported";
+		break;
+	case 0b01000101:
+		*err_msg = "CDB status failed: CdbChkCode error";
+		break;
+	default:
+		*err_msg = "Unknown failure reason";
+	}
+};
+
+static int cmis_cdb_wait_for_status(struct net_device *dev,
+				    struct ethtool_cmis_cdb_cmd_args *args)
+{
+	u8 status;
+	int err;
+
+	/* Some vendors demand waiting time before checking status in some
+	 * CDB commands.
+	 */
+	msleep(args->msleep_pre_rpl);
+
+	err = ethtool_cmis_wait_for_cond(dev, args->flags, CDB_F_STATUS_VALID,
+					 args->max_duration,
+					 CMIS_CDB_STATUS_OFFSET,
+					 status_success, status_fail, &status);
+	if (err < 0 && !args->err_msg)
+		cmis_cdb_status_fail_msg_get(status, &args->err_msg);
+
+	return err;
+}
+
+#define CMIS_CDB_REPLY_OFFSET	0x86
+
+static int cmis_cdb_process_reply(struct net_device *dev,
+				  struct ethtool_module_eeprom *page_data,
+				  struct ethtool_cmis_cdb_cmd_args *args)
+{
+	u8 rpl_hdr_len = sizeof(struct ethtool_cmis_cdb_rpl_hdr);
+	u8 rpl_exp_len = args->rpl_exp_len + rpl_hdr_len;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct netlink_ext_ack extack = {};
+	struct ethtool_cmis_cdb_rpl *rpl;
+	int err;
+
+	if (!args->rpl_exp_len)
+		return 0;
+
+	ethtool_cmis_page_init(page_data, ETHTOOL_CMIS_CDB_CMD_PAGE,
+			       CMIS_CDB_REPLY_OFFSET, rpl_exp_len);
+	page_data->data = kmalloc(page_data->length, GFP_KERNEL);
+	if (!page_data->data)
+		return -ENOMEM;
+
+	err = ops->get_module_eeprom_by_page(dev, page_data, &extack);
+	if (err < 0) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+		goto out;
+	}
+
+	rpl = (struct ethtool_cmis_cdb_rpl *)page_data->data;
+	if ((args->rpl_exp_len > rpl->hdr.rpl_len + rpl_hdr_len) ||
+	    !rpl->hdr.rpl_chk_code) {
+		err = -EIO;
+		goto out;
+	}
+
+	args->req.lpl_len = rpl->hdr.rpl_len;
+	memcpy(args->req.payload, rpl->payload, args->req.lpl_len);
+
+out:
+	kfree(page_data->data);
+	return err;
+}
+
+static int
+__ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
+			       struct ethtool_module_eeprom *page_data,
+			       u8 page, u32 offset, u32 length, void *data)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct netlink_ext_ack extack = {};
+	int err;
+
+	ethtool_cmis_page_init(page_data, page, offset, length);
+	page_data->data = kmemdup(data, page_data->length, GFP_KERNEL);
+	if (!page_data->data)
+		return -ENOMEM;
+
+	err = ops->set_module_eeprom_by_page(dev, page_data, &extack);
+	if (err < 0) {
+		if (extack._msg)
+			netdev_err(dev, "%s\n", extack._msg);
+	}
+
+	kfree(page_data->data);
+	return err;
+}
+
+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;
+}
+
+#define CMIS_CDB_CMD_ID_OFFSET	0x80
+
+int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
+				 struct ethtool_cmis_cdb_cmd_args *args)
+{
+	struct ethtool_module_eeprom page_data = {};
+	u32 offset;
+	int err;
+
+	args->req.chk_code =
+		cmis_cdb_calc_checksum(&args->req, sizeof(args->req));
+
+	if (args->req.lpl_len > args->read_write_len_ext) {
+		args->err_msg = "LPL length is longer than CDB read write length extension allows";
+		return -EINVAL;
+	}
+
+	/* According to the CMIS standard, there are two options to trigger the
+	 * CDB commands. The default option is triggering the command by writing
+	 * the CMDID bytes. Therefore, the command will be split to 2 calls:
+	 * First, with everything except the CMDID field and then the CMDID
+	 * field.
+	 */
+	offset = CMIS_CDB_CMD_ID_OFFSET +
+		offsetof(struct ethtool_cmis_cdb_request, body);
+	err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data,
+					     ETHTOOL_CMIS_CDB_CMD_PAGE, offset,
+					     sizeof(args->req.body),
+					     &args->req.body);
+	if (err < 0)
+		return err;
+
+	offset = CMIS_CDB_CMD_ID_OFFSET +
+		offsetof(struct ethtool_cmis_cdb_request, id);
+	err = __ethtool_cmis_cdb_execute_cmd(dev, &page_data,
+					     ETHTOOL_CMIS_CDB_CMD_PAGE, offset,
+					     sizeof(args->req.id),
+					     &args->req.id);
+	if (err < 0)
+		return err;
+
+	err = cmis_cdb_wait_for_completion(dev, args);
+	if (err < 0)
+		return err;
+
+	err = cmis_cdb_wait_for_status(dev, args);
+	if (err < 0)
+		return err;
+
+	return cmis_cdb_process_reply(dev, &page_data, args);
+}
diff --git a/net/ethtool/module_fw.h b/net/ethtool/module_fw.h
index ee4a291ac1d4..6c86d05ab6cf 100644
--- a/net/ethtool/module_fw.h
+++ b/net/ethtool/module_fw.h
@@ -15,6 +15,16 @@  struct ethnl_module_fw_flash_ntf_params {
 	bool closed_sock;
 };
 
+/**
+ * struct ethtool_module_fw_flash_params - module firmware flashing parameters
+ * @password: Module password. Only valid when @pass_valid is set.
+ * @password_valid: Whether the module password is valid or not.
+ */
+struct ethtool_module_fw_flash_params {
+	__be32 password;
+	u8 password_valid:1;
+};
+
 void
 ethnl_module_fw_flash_ntf_err(struct net_device *dev,
 			      struct ethnl_module_fw_flash_ntf_params *params,