[PATCH net 0/2] ethtool: cmis fixes

[PATCH net 0/2] ethtool: cmis fixes

[Michael Chan]

Two bug fixes from Damodharam Ammepalli to fix ethtool cmis module
firmware flashing.

Damodharam Ammepalli (2):
  ethtool: cmis_cdb: use correct rpl size in ethtool_cmis_module_poll()
  ethtool: cmis: use u16 for calculated read_write_len_ext

 net/ethtool/cmis.h     |  7 ++++---
 net/ethtool/cmis_cdb.c | 10 +++++-----
 2 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.30.1

[PATCH net 1/2] ethtool: cmis_cdb: use correct rpl size in ethtool_cmis_module_poll()

[Michael Chan]

From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>

rpl is passed as a pointer to ethtool_cmis_module_poll(), so the correct
size of rpl is sizeof(*rpl) which should be just 1 byte.  Using the
pointer size instead can cause stack corruption:

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ethtool_cmis_wait_for_cond+0xf4/0x100
CPU: 72 UID: 0 PID: 4440 Comm: kworker/72:2 Kdump: loaded Tainted: G           OE      6.11.0 #24
Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Hardware name: Dell Inc. PowerEdge R760/04GWWM, BIOS 1.6.6 09/20/2023
Workqueue: events module_flash_fw_work
Call Trace:
 <TASK>
 panic+0x339/0x360
 ? ethtool_cmis_wait_for_cond+0xf4/0x100
 ? __pfx_status_success+0x10/0x10
 ? __pfx_status_fail+0x10/0x10
 __stack_chk_fail+0x10/0x10
 ethtool_cmis_wait_for_cond+0xf4/0x100
 ethtool_cmis_cdb_execute_cmd+0x1fc/0x330
 ? __pfx_status_fail+0x10/0x10
 cmis_cdb_module_features_get+0x6d/0xd0
 ethtool_cmis_cdb_init+0x8a/0xd0
 ethtool_cmis_fw_update+0x46/0x1d0
 module_flash_fw_work+0x17/0xa0
 process_one_work+0x179/0x390
 worker_thread+0x239/0x340
 ? __pfx_worker_thread+0x10/0x10
 kthread+0xcc/0x100
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x2d/0x50
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1a/0x30
 </TASK>

Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 net/ethtool/cmis_cdb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
index d159dc121bde..dba3aa909a95 100644
--- a/net/ethtool/cmis_cdb.c
+++ b/net/ethtool/cmis_cdb.c
@@ -363,7 +363,7 @@ ethtool_cmis_module_poll(struct net_device *dev,
 	struct netlink_ext_ack extack = {};
 	int err;
 
-	ethtool_cmis_page_init(&page_data, 0, offset, sizeof(rpl));
+	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);
-- 
2.30.1

Re: [PATCH net 1/2] ethtool: cmis_cdb: use correct rpl size in ethtool_cmis_module_poll()

[Simon Horman]

On Wed, Apr 02, 2025 at 11:31:22AM -0700, Michael Chan wrote:
> From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> 
> rpl is passed as a pointer to ethtool_cmis_module_poll(), so the correct
> size of rpl is sizeof(*rpl) which should be just 1 byte.  Using the
> pointer size instead can cause stack corruption:
> 
> Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ethtool_cmis_wait_for_cond+0xf4/0x100
> CPU: 72 UID: 0 PID: 4440 Comm: kworker/72:2 Kdump: loaded Tainted: G           OE      6.11.0 #24
> Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> Hardware name: Dell Inc. PowerEdge R760/04GWWM, BIOS 1.6.6 09/20/2023
> Workqueue: events module_flash_fw_work
> Call Trace:
>  <TASK>
>  panic+0x339/0x360
>  ? ethtool_cmis_wait_for_cond+0xf4/0x100
>  ? __pfx_status_success+0x10/0x10
>  ? __pfx_status_fail+0x10/0x10
>  __stack_chk_fail+0x10/0x10
>  ethtool_cmis_wait_for_cond+0xf4/0x100
>  ethtool_cmis_cdb_execute_cmd+0x1fc/0x330
>  ? __pfx_status_fail+0x10/0x10
>  cmis_cdb_module_features_get+0x6d/0xd0
>  ethtool_cmis_cdb_init+0x8a/0xd0
>  ethtool_cmis_fw_update+0x46/0x1d0
>  module_flash_fw_work+0x17/0xa0
>  process_one_work+0x179/0x390
>  worker_thread+0x239/0x340
>  ? __pfx_worker_thread+0x10/0x10
>  kthread+0xcc/0x100
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork+0x2d/0x50
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork_asm+0x1a/0x30
>  </TASK>
> 
> Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Hi Damodharam, all,

I don't think there is any need to resend for this, but I think
there is a '"' missing towards the end of the Fixes tag above.
That is, I think it should look like this:

Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands")

Other than the nit above this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 1/2] ethtool: cmis_cdb: use correct rpl size in ethtool_cmis_module_poll()

[Ido Schimmel]

On Wed, Apr 02, 2025 at 11:31:22AM -0700, Michael Chan wrote:
> From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> 
> rpl is passed as a pointer to ethtool_cmis_module_poll(), so the correct
> size of rpl is sizeof(*rpl) which should be just 1 byte.  Using the
> pointer size instead can cause stack corruption:
> 
> Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ethtool_cmis_wait_for_cond+0xf4/0x100
> CPU: 72 UID: 0 PID: 4440 Comm: kworker/72:2 Kdump: loaded Tainted: G           OE      6.11.0 #24
> Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> Hardware name: Dell Inc. PowerEdge R760/04GWWM, BIOS 1.6.6 09/20/2023
> Workqueue: events module_flash_fw_work
> Call Trace:
>  <TASK>
>  panic+0x339/0x360
>  ? ethtool_cmis_wait_for_cond+0xf4/0x100
>  ? __pfx_status_success+0x10/0x10
>  ? __pfx_status_fail+0x10/0x10
>  __stack_chk_fail+0x10/0x10
>  ethtool_cmis_wait_for_cond+0xf4/0x100
>  ethtool_cmis_cdb_execute_cmd+0x1fc/0x330
>  ? __pfx_status_fail+0x10/0x10
>  cmis_cdb_module_features_get+0x6d/0xd0
>  ethtool_cmis_cdb_init+0x8a/0xd0
>  ethtool_cmis_fw_update+0x46/0x1d0
>  module_flash_fw_work+0x17/0xa0
>  process_one_work+0x179/0x390
>  worker_thread+0x239/0x340
>  ? __pfx_worker_thread+0x10/0x10
>  kthread+0xcc/0x100
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork+0x2d/0x50
>  ? __pfx_kthread+0x10/0x10
>  ret_from_fork_asm+0x1a/0x30
>  </TASK>
> 
> Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

[PATCH net 2/2] ethtool: cmis: use u16 for calculated read_write_len_ext

[Michael Chan]

From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>

For EPL (Extended Payload), the maximum calculated size returned by
ethtool_cmis_get_max_epl_size() is 2048, so the read_write_len_ext
field in struct ethtool_cmis_cdb_cmd_args needs to be changed to u16
to hold the value.

To avoid confusion with other u8 read_write_len_ext fields defined
by the CMIS spec, change the field name to calc_read_write_len_ext.

Without this change, module flashing can fail:

Transceiver module firmware flashing started for device enp177s0np0
Transceiver module firmware flashing in progress for device enp177s0np0
Progress: 0%
Transceiver module firmware flashing encountered an error for device enp177s0np0
Status message: Write FW block EPL command failed, LPL length is longer
	than CDB read write length extension allows.

Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 net/ethtool/cmis.h     | 7 ++++---
 net/ethtool/cmis_cdb.c | 8 ++++----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
index 1e790413db0e..51f5d5439e2a 100644
--- a/net/ethtool/cmis.h
+++ b/net/ethtool/cmis.h
@@ -63,8 +63,9 @@ struct ethtool_cmis_cdb_request {
  * 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.
+ * @calc_read_write_len_ext: Calculated allowable additional number of byte
+ *			     octets to the LPL or EPL in a READ or WRITE CDB
+ *			     command.
  * @msleep_pre_rpl: Waiting time before checking reply in msec.
  * @rpl_exp_len: Expected reply length in bytes.
  * @flags: Validation flags for CDB commands.
@@ -73,7 +74,7 @@ struct ethtool_cmis_cdb_request {
 struct ethtool_cmis_cdb_cmd_args {
 	struct ethtool_cmis_cdb_request req;
 	u16				max_duration;
-	u8				read_write_len_ext;
+	u16				calc_read_write_len_ext;
 	u8				msleep_pre_rpl;
 	u8                              rpl_exp_len;
 	u8				flags;
diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
index dba3aa909a95..1f487e1a6347 100644
--- a/net/ethtool/cmis_cdb.c
+++ b/net/ethtool/cmis_cdb.c
@@ -35,13 +35,13 @@ void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
 	args->req.lpl_len = lpl_len;
 	if (lpl) {
 		memcpy(args->req.payload, lpl, args->req.lpl_len);
-		args->read_write_len_ext =
+		args->calc_read_write_len_ext =
 			ethtool_cmis_get_max_lpl_size(read_write_len_ext);
 	}
 	if (epl) {
 		args->req.epl_len = cpu_to_be16(epl_len);
 		args->req.epl = epl;
-		args->read_write_len_ext =
+		args->calc_read_write_len_ext =
 			ethtool_cmis_get_max_epl_size(read_write_len_ext);
 	}
 
@@ -590,7 +590,7 @@ ethtool_cmis_cdb_execute_epl_cmd(struct net_device *dev,
 			space_left = CMIS_CDB_EPL_FW_BLOCK_OFFSET_END - offset + 1;
 			bytes_to_write = min_t(u16, bytes_left,
 					       min_t(u16, space_left,
-						     args->read_write_len_ext));
+						     args->calc_read_write_len_ext));
 
 			err = __ethtool_cmis_cdb_execute_cmd(dev, page_data,
 							     page, offset,
@@ -631,7 +631,7 @@ int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
 				       offsetof(struct ethtool_cmis_cdb_request,
 						epl));
 
-	if (args->req.lpl_len > args->read_write_len_ext) {
+	if (args->req.lpl_len > args->calc_read_write_len_ext) {
 		args->err_msg = "LPL length is longer than CDB read write length extension allows";
 		return -EINVAL;
 	}
-- 
2.30.1

Re: [PATCH net 2/2] ethtool: cmis: use u16 for calculated read_write_len_ext

[Simon Horman]

On Wed, Apr 02, 2025 at 11:31:23AM -0700, Michael Chan wrote:
> From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> 
> For EPL (Extended Payload), the maximum calculated size returned by
> ethtool_cmis_get_max_epl_size() is 2048, so the read_write_len_ext
> field in struct ethtool_cmis_cdb_cmd_args needs to be changed to u16
> to hold the value.
> 
> To avoid confusion with other u8 read_write_len_ext fields defined
> by the CMIS spec, change the field name to calc_read_write_len_ext.
> 
> Without this change, module flashing can fail:
> 
> Transceiver module firmware flashing started for device enp177s0np0
> Transceiver module firmware flashing in progress for device enp177s0np0
> Progress: 0%
> Transceiver module firmware flashing encountered an error for device enp177s0np0
> Status message: Write FW block EPL command failed, LPL length is longer
> 	than CDB read write length extension allows.
> 
> Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)

Hi Damodharam, all,

As per my comment on patch 1/2: I don't think there is any need to resend
for this, but I think there is a '"' missing towards the end of the Fixes
tag above. That is, I think it should look like this.

Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands")

> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Other than the nit above this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 2/2] ethtool: cmis: use u16 for calculated read_write_len_ext

[Ido Schimmel]

Adding Petr given Danielle is away

On Wed, Apr 02, 2025 at 11:31:23AM -0700, Michael Chan wrote:
> From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> 
> For EPL (Extended Payload), the maximum calculated size returned by
> ethtool_cmis_get_max_epl_size() is 2048, so the read_write_len_ext
> field in struct ethtool_cmis_cdb_cmd_args needs to be changed to u16
> to hold the value.
> 
> To avoid confusion with other u8 read_write_len_ext fields defined
> by the CMIS spec, change the field name to calc_read_write_len_ext.
> 
> Without this change, module flashing can fail:
> 
> Transceiver module firmware flashing started for device enp177s0np0
> Transceiver module firmware flashing in progress for device enp177s0np0
> Progress: 0%
> Transceiver module firmware flashing encountered an error for device enp177s0np0
> Status message: Write FW block EPL command failed, LPL length is longer
> 	than CDB read write length extension allows.
> 
> Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  net/ethtool/cmis.h     | 7 ++++---
>  net/ethtool/cmis_cdb.c | 8 ++++----
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
> index 1e790413db0e..51f5d5439e2a 100644
> --- a/net/ethtool/cmis.h
> +++ b/net/ethtool/cmis.h
> @@ -63,8 +63,9 @@ struct ethtool_cmis_cdb_request {
>   * 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.
> + * @calc_read_write_len_ext: Calculated allowable additional number of byte
> + *			     octets to the LPL or EPL in a READ or WRITE CDB
> + *			     command.
>   * @msleep_pre_rpl: Waiting time before checking reply in msec.
>   * @rpl_exp_len: Expected reply length in bytes.
>   * @flags: Validation flags for CDB commands.
> @@ -73,7 +74,7 @@ struct ethtool_cmis_cdb_request {
>  struct ethtool_cmis_cdb_cmd_args {
>  	struct ethtool_cmis_cdb_request req;
>  	u16				max_duration;
> -	u8				read_write_len_ext;
> +	u16				calc_read_write_len_ext;
>  	u8				msleep_pre_rpl;
>  	u8                              rpl_exp_len;
>  	u8				flags;
> diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
> index dba3aa909a95..1f487e1a6347 100644
> --- a/net/ethtool/cmis_cdb.c
> +++ b/net/ethtool/cmis_cdb.c
> @@ -35,13 +35,13 @@ void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
>  	args->req.lpl_len = lpl_len;
>  	if (lpl) {
>  		memcpy(args->req.payload, lpl, args->req.lpl_len);
> -		args->read_write_len_ext =
> +		args->calc_read_write_len_ext =
>  			ethtool_cmis_get_max_lpl_size(read_write_len_ext);
>  	}
>  	if (epl) {
>  		args->req.epl_len = cpu_to_be16(epl_len);
>  		args->req.epl = epl;
> -		args->read_write_len_ext =
> +		args->calc_read_write_len_ext =
>  			ethtool_cmis_get_max_epl_size(read_write_len_ext);

AFAIU, a size larger than a page (128 bytes) is only useful when auto
paging is supported which is something the kernel doesn't currently
support. Therefore, I think it's misleading to initialize this field to
a value larger than 128.

How about deleting ethtool_cmis_get_max_epl_size() and moving the
initialization of 'args->read_write_len_ext' outside of the if block as
it was before 9a3b0d078bd82?

>  	}
>  
> @@ -590,7 +590,7 @@ ethtool_cmis_cdb_execute_epl_cmd(struct net_device *dev,
>  			space_left = CMIS_CDB_EPL_FW_BLOCK_OFFSET_END - offset + 1;
>  			bytes_to_write = min_t(u16, bytes_left,
>  					       min_t(u16, space_left,
> -						     args->read_write_len_ext));
> +						     args->calc_read_write_len_ext));
>  
>  			err = __ethtool_cmis_cdb_execute_cmd(dev, page_data,
>  							     page, offset,
> @@ -631,7 +631,7 @@ int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
>  				       offsetof(struct ethtool_cmis_cdb_request,
>  						epl));
>  
> -	if (args->req.lpl_len > args->read_write_len_ext) {
> +	if (args->req.lpl_len > args->calc_read_write_len_ext) {
>  		args->err_msg = "LPL length is longer than CDB read write length extension allows";
>  		return -EINVAL;
>  	}
> -- 
> 2.30.1
> 
> 

Re: [PATCH net 2/2] ethtool: cmis: use u16 for calculated read_write_len_ext

[Damodharam Ammepalli]

From: Ido Schimmel <idosch@idosch.org>

Adding Petr given Danielle is away

On Wed, Apr 02, 2025 at 11:31:23AM -0700, Michael Chan wrote:
> From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> 
> For EPL (Extended Payload), the maximum calculated size returned by
> ethtool_cmis_get_max_epl_size() is 2048, so the read_write_len_ext
> field in struct ethtool_cmis_cdb_cmd_args needs to be changed to u16
> to hold the value.
> 
> To avoid confusion with other u8 read_write_len_ext fields defined
> by the CMIS spec, change the field name to calc_read_write_len_ext.
> 
> Without this change, module flashing can fail:
> 
> Transceiver module firmware flashing started for device enp177s0np0
> Transceiver module firmware flashing in progress for device enp177s0np0
> Progress: 0%
> Transceiver module firmware flashing encountered an error for device enp177s0np0
> Status message: Write FW block EPL command failed, LPL length is longer
> 	than CDB read write length extension allows.
> 
> Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  net/ethtool/cmis.h     | 7 ++++---
>  net/ethtool/cmis_cdb.c | 8 ++++----
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
> index 1e790413db0e..51f5d5439e2a 100644
> --- a/net/ethtool/cmis.h
> +++ b/net/ethtool/cmis.h
> @@ -63,8 +63,9 @@ struct ethtool_cmis_cdb_request {
>   * 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.
> + * @calc_read_write_len_ext: Calculated allowable additional number of byte
> + *			     octets to the LPL or EPL in a READ or WRITE CDB
> + *			     command.
>   * @msleep_pre_rpl: Waiting time before checking reply in msec.
>   * @rpl_exp_len: Expected reply length in bytes.
>   * @flags: Validation flags for CDB commands.
> @@ -73,7 +74,7 @@ struct ethtool_cmis_cdb_request {
>  struct ethtool_cmis_cdb_cmd_args {
>  	struct ethtool_cmis_cdb_request req;
>  	u16				max_duration;
> -	u8				read_write_len_ext;
> +	u16				calc_read_write_len_ext;
>  	u8				msleep_pre_rpl;
>  	u8                              rpl_exp_len;
>  	u8				flags;
> diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
> index dba3aa909a95..1f487e1a6347 100644
> --- a/net/ethtool/cmis_cdb.c
> +++ b/net/ethtool/cmis_cdb.c
> @@ -35,13 +35,13 @@ void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
>  	args->req.lpl_len = lpl_len;
>  	if (lpl) {
>  		memcpy(args->req.payload, lpl, args->req.lpl_len);
> -		args->read_write_len_ext =
> +		args->calc_read_write_len_ext =
>  			ethtool_cmis_get_max_lpl_size(read_write_len_ext);
>  	}
>  	if (epl) {
>  		args->req.epl_len = cpu_to_be16(epl_len);
>  		args->req.epl = epl;
> -		args->read_write_len_ext =
> +		args->calc_read_write_len_ext =
>  			ethtool_cmis_get_max_epl_size(read_write_len_ext);

AFAIU, a size larger than a page (128 bytes) is only useful when auto
paging is supported which is something the kernel doesn't currently
support. Therefore, I think it's misleading to initialize this field to
a value larger than 128.

How about deleting ethtool_cmis_get_max_epl_size() and moving the
initialization of 'args->read_write_len_ext' outside of the if block as
it was before 9a3b0d078bd82?

>  	}
>  
> @@ -590,7 +590,7 @@ ethtool_cmis_cdb_execute_epl_cmd(struct net_device *dev,
>  			space_left = CMIS_CDB_EPL_FW_BLOCK_OFFSET_END - offset + 1;
>  			bytes_to_write = min_t(u16, bytes_left,
>  					       min_t(u16, space_left,
> -						     args->read_write_len_ext));
> +						     args->calc_read_write_len_ext));
>  
>  			err = __ethtool_cmis_cdb_execute_cmd(dev, page_data,
>  							     page, offset,
> @@ -631,7 +631,7 @@ int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
>  				       offsetof(struct ethtool_cmis_cdb_request,
>  						epl));
>  
> -	if (args->req.lpl_len > args->read_write_len_ext) {
> +	if (args->req.lpl_len > args->calc_read_write_len_ext) {
>  		args->err_msg = "LPL length is longer than CDB read write length extension allows";
>  		return -EINVAL;
>  	}
> -- 
> 2.30.1
> 
> 

This module supports AutoPaging, 255 read_write_len_ext and EPL write mechanism.
This advertised 0xff (byte2) calculates the args->read_write_len_ext
to 2048B, which needs u16.
Hexdump: cmis_cdb_advert_rpl
Off 0x00 :77 ff 1f 80

With your suggested change, ethtool_cmis_cdb_execute_epl_cmd() is skipped
since args->req.epl_len is set to zero and download fails.


-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Re: [PATCH net 2/2] ethtool: cmis: use u16 for calculated read_write_len_ext

[Ido Schimmel]

On Mon, Apr 07, 2025 at 08:09:56AM -0700, Damodharam Ammepalli wrote:
> From: Ido Schimmel <idosch@idosch.org>
> 
> Adding Petr given Danielle is away
> 
> On Wed, Apr 02, 2025 at 11:31:23AM -0700, Michael Chan wrote:
> > From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> > 
> > For EPL (Extended Payload), the maximum calculated size returned by
> > ethtool_cmis_get_max_epl_size() is 2048, so the read_write_len_ext
> > field in struct ethtool_cmis_cdb_cmd_args needs to be changed to u16
> > to hold the value.
> > 
> > To avoid confusion with other u8 read_write_len_ext fields defined
> > by the CMIS spec, change the field name to calc_read_write_len_ext.
> > 
> > Without this change, module flashing can fail:
> > 
> > Transceiver module firmware flashing started for device enp177s0np0
> > Transceiver module firmware flashing in progress for device enp177s0np0
> > Progress: 0%
> > Transceiver module firmware flashing encountered an error for device enp177s0np0
> > Status message: Write FW block EPL command failed, LPL length is longer
> > 	than CDB read write length extension allows.
> > 
> > Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)
> > Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> > Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > ---
> >  net/ethtool/cmis.h     | 7 ++++---
> >  net/ethtool/cmis_cdb.c | 8 ++++----
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
> > index 1e790413db0e..51f5d5439e2a 100644
> > --- a/net/ethtool/cmis.h
> > +++ b/net/ethtool/cmis.h
> > @@ -63,8 +63,9 @@ struct ethtool_cmis_cdb_request {
> >   * 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.
> > + * @calc_read_write_len_ext: Calculated allowable additional number of byte
> > + *			     octets to the LPL or EPL in a READ or WRITE CDB
> > + *			     command.
> >   * @msleep_pre_rpl: Waiting time before checking reply in msec.
> >   * @rpl_exp_len: Expected reply length in bytes.
> >   * @flags: Validation flags for CDB commands.
> > @@ -73,7 +74,7 @@ struct ethtool_cmis_cdb_request {
> >  struct ethtool_cmis_cdb_cmd_args {
> >  	struct ethtool_cmis_cdb_request req;
> >  	u16				max_duration;
> > -	u8				read_write_len_ext;
> > +	u16				calc_read_write_len_ext;
> >  	u8				msleep_pre_rpl;
> >  	u8                              rpl_exp_len;
> >  	u8				flags;
> > diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
> > index dba3aa909a95..1f487e1a6347 100644
> > --- a/net/ethtool/cmis_cdb.c
> > +++ b/net/ethtool/cmis_cdb.c
> > @@ -35,13 +35,13 @@ void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
> >  	args->req.lpl_len = lpl_len;
> >  	if (lpl) {
> >  		memcpy(args->req.payload, lpl, args->req.lpl_len);
> > -		args->read_write_len_ext =
> > +		args->calc_read_write_len_ext =
> >  			ethtool_cmis_get_max_lpl_size(read_write_len_ext);
> >  	}
> >  	if (epl) {
> >  		args->req.epl_len = cpu_to_be16(epl_len);
> >  		args->req.epl = epl;
> > -		args->read_write_len_ext =
> > +		args->calc_read_write_len_ext =
> >  			ethtool_cmis_get_max_epl_size(read_write_len_ext);
> 
> AFAIU, a size larger than a page (128 bytes) is only useful when auto
> paging is supported which is something the kernel doesn't currently
> support. Therefore, I think it's misleading to initialize this field to
> a value larger than 128.
> 
> How about deleting ethtool_cmis_get_max_epl_size() and moving the
> initialization of 'args->read_write_len_ext' outside of the if block as
> it was before 9a3b0d078bd82?
> 
> >  	}
> >  
> > @@ -590,7 +590,7 @@ ethtool_cmis_cdb_execute_epl_cmd(struct net_device *dev,
> >  			space_left = CMIS_CDB_EPL_FW_BLOCK_OFFSET_END - offset + 1;
> >  			bytes_to_write = min_t(u16, bytes_left,
> >  					       min_t(u16, space_left,
> > -						     args->read_write_len_ext));
> > +						     args->calc_read_write_len_ext));
> >  
> >  			err = __ethtool_cmis_cdb_execute_cmd(dev, page_data,
> >  							     page, offset,
> > @@ -631,7 +631,7 @@ int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
> >  				       offsetof(struct ethtool_cmis_cdb_request,
> >  						epl));
> >  
> > -	if (args->req.lpl_len > args->read_write_len_ext) {
> > +	if (args->req.lpl_len > args->calc_read_write_len_ext) {
> >  		args->err_msg = "LPL length is longer than CDB read write length extension allows";
> >  		return -EINVAL;
> >  	}
> > -- 
> > 2.30.1
> > 
> > 
> 
> This module supports AutoPaging, 255 read_write_len_ext and EPL write mechanism.
> This advertised 0xff (byte2) calculates the args->read_write_len_ext
> to 2048B, which needs u16.
> Hexdump: cmis_cdb_advert_rpl
> Off 0x00 :77 ff 1f 80
> 
> With your suggested change, ethtool_cmis_cdb_execute_epl_cmd() is skipped
> since args->req.epl_len is set to zero and download fails.

To be clear, this is what I'm suggesting [1] and it doesn't involve
setting args->req.epl_len to zero, so I'm not sure what was tested.

Basically, setting maximum length of read or write to 128 bytes as the
kernel does not currently support auto paging (even if the transceiver
module does) and will not try to perform cross-page reads or writes.

[1]
diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
index 1e790413db0e..4a9a946cabf0 100644
--- a/net/ethtool/cmis.h
+++ b/net/ethtool/cmis.h
@@ -101,7 +101,6 @@ struct ethtool_cmis_cdb_rpl {
 };
 
 u32 ethtool_cmis_get_max_lpl_size(u8 num_of_byte_octs);
-u32 ethtool_cmis_get_max_epl_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 *lpl,
diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
index d159dc121bde..0e2691ccb0df 100644
--- a/net/ethtool/cmis_cdb.c
+++ b/net/ethtool/cmis_cdb.c
@@ -16,15 +16,6 @@ u32 ethtool_cmis_get_max_lpl_size(u8 num_of_byte_octs)
 	return 8 * (1 + min_t(u8, num_of_byte_octs, 15));
 }
 
-/* For accessing the EPL field on page 9Fh, the allowable length extension is
- * min(i, 255) byte octets where i specifies the allowable additional number of
- * byte octets in a READ or a WRITE.
- */
-u32 ethtool_cmis_get_max_epl_size(u8 num_of_byte_octs)
-{
-	return 8 * (1 + min_t(u8, num_of_byte_octs, 255));
-}
-
 void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
 				   enum ethtool_cmis_cdb_cmd_id cmd, u8 *lpl,
 				   u8 lpl_len, u8 *epl, u16 epl_len,
@@ -33,19 +24,16 @@ void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
 {
 	args->req.id = cpu_to_be16(cmd);
 	args->req.lpl_len = lpl_len;
-	if (lpl) {
+	if (lpl)
 		memcpy(args->req.payload, lpl, args->req.lpl_len);
-		args->read_write_len_ext =
-			ethtool_cmis_get_max_lpl_size(read_write_len_ext);
-	}
 	if (epl) {
 		args->req.epl_len = cpu_to_be16(epl_len);
 		args->req.epl = epl;
-		args->read_write_len_ext =
-			ethtool_cmis_get_max_epl_size(read_write_len_ext);
 	}
 
 	args->max_duration = max_duration;
+	args->read_write_len_ext =
+		ethtool_cmis_get_max_lpl_size(read_write_len_ext);
 	args->msleep_pre_rpl = msleep_pre_rpl;
 	args->rpl_exp_len = rpl_exp_len;
 	args->flags = flags;

Re: [PATCH net 2/2] ethtool: cmis: use u16 for calculated read_write_len_ext

[Damodharam Ammepalli]

[-- Attachment #1: Type: text/plain, Size: 13381 bytes --]

On Mon, Apr 7, 2025 at 9:23 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> On Mon, Apr 07, 2025 at 08:09:56AM -0700, Damodharam Ammepalli wrote:
> > From: Ido Schimmel <idosch@idosch.org>
> >
> > Adding Petr given Danielle is away
> >
> > On Wed, Apr 02, 2025 at 11:31:23AM -0700, Michael Chan wrote:
> > > From: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> > >
> > > For EPL (Extended Payload), the maximum calculated size returned by
> > > ethtool_cmis_get_max_epl_size() is 2048, so the read_write_len_ext
> > > field in struct ethtool_cmis_cdb_cmd_args needs to be changed to u16
> > > to hold the value.
> > >
> > > To avoid confusion with other u8 read_write_len_ext fields defined
> > > by the CMIS spec, change the field name to calc_read_write_len_ext.
> > >
> > > Without this change, module flashing can fail:
> > >
> > > Transceiver module firmware flashing started for device enp177s0np0
> > > Transceiver module firmware flashing in progress for device enp177s0np0
> > > Progress: 0%
> > > Transceiver module firmware flashing encountered an error for device enp177s0np0
> > > Status message: Write FW block EPL command failed, LPL length is longer
> > >     than CDB read write length extension allows.
> > >
> > > Fixes: a39c84d79625 ("ethtool: cmis_cdb: Add a layer for supporting CDB commands)
> > > Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> > > Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> > > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > > ---
> > >  net/ethtool/cmis.h     | 7 ++++---
> > >  net/ethtool/cmis_cdb.c | 8 ++++----
> > >  2 files changed, 8 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
> > > index 1e790413db0e..51f5d5439e2a 100644
> > > --- a/net/ethtool/cmis.h
> > > +++ b/net/ethtool/cmis.h
> > > @@ -63,8 +63,9 @@ struct ethtool_cmis_cdb_request {
> > >   * 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.
> > > + * @calc_read_write_len_ext: Calculated allowable additional number of byte
> > > + *                      octets to the LPL or EPL in a READ or WRITE CDB
> > > + *                      command.
> > >   * @msleep_pre_rpl: Waiting time before checking reply in msec.
> > >   * @rpl_exp_len: Expected reply length in bytes.
> > >   * @flags: Validation flags for CDB commands.
> > > @@ -73,7 +74,7 @@ struct ethtool_cmis_cdb_request {
> > >  struct ethtool_cmis_cdb_cmd_args {
> > >     struct ethtool_cmis_cdb_request req;
> > >     u16                             max_duration;
> > > -   u8                              read_write_len_ext;
> > > +   u16                             calc_read_write_len_ext;
> > >     u8                              msleep_pre_rpl;
> > >     u8                              rpl_exp_len;
> > >     u8                              flags;
> > > diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
> > > index dba3aa909a95..1f487e1a6347 100644
> > > --- a/net/ethtool/cmis_cdb.c
> > > +++ b/net/ethtool/cmis_cdb.c
> > > @@ -35,13 +35,13 @@ void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
> > >     args->req.lpl_len = lpl_len;
> > >     if (lpl) {
> > >             memcpy(args->req.payload, lpl, args->req.lpl_len);
> > > -           args->read_write_len_ext =
> > > +           args->calc_read_write_len_ext =
> > >                     ethtool_cmis_get_max_lpl_size(read_write_len_ext);
> > >     }
> > >     if (epl) {
> > >             args->req.epl_len = cpu_to_be16(epl_len);
> > >             args->req.epl = epl;
> > > -           args->read_write_len_ext =
> > > +           args->calc_read_write_len_ext =
> > >                     ethtool_cmis_get_max_epl_size(read_write_len_ext);
> >
> > AFAIU, a size larger than a page (128 bytes) is only useful when auto
> > paging is supported which is something the kernel doesn't currently
> > support. Therefore, I think it's misleading to initialize this field to
> > a value larger than 128.
> >
> > How about deleting ethtool_cmis_get_max_epl_size() and moving the
> > initialization of 'args->read_write_len_ext' outside of the if block as
> > it was before 9a3b0d078bd82?
> >
> > >     }
> > >
> > > @@ -590,7 +590,7 @@ ethtool_cmis_cdb_execute_epl_cmd(struct net_device *dev,
> > >                     space_left = CMIS_CDB_EPL_FW_BLOCK_OFFSET_END - offset + 1;
> > >                     bytes_to_write = min_t(u16, bytes_left,
> > >                                            min_t(u16, space_left,
> > > -                                                args->read_write_len_ext));
> > > +                                                args->calc_read_write_len_ext));
> > >
> > >                     err = __ethtool_cmis_cdb_execute_cmd(dev, page_data,
> > >                                                          page, offset,
> > > @@ -631,7 +631,7 @@ int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
> > >                                    offsetof(struct ethtool_cmis_cdb_request,
> > >                                             epl));
> > >
> > > -   if (args->req.lpl_len > args->read_write_len_ext) {
> > > +   if (args->req.lpl_len > args->calc_read_write_len_ext) {
> > >             args->err_msg = "LPL length is longer than CDB read write length extension allows";
> > >             return -EINVAL;
> > >     }
> > > --
> > > 2.30.1
> > >
> > >
> >
> > This module supports AutoPaging, 255 read_write_len_ext and EPL write mechanism.
> > This advertised 0xff (byte2) calculates the args->read_write_len_ext
> > to 2048B, which needs u16.
> > Hexdump: cmis_cdb_advert_rpl
> > Off 0x00 :77 ff 1f 80
> >
> > With your suggested change, ethtool_cmis_cdb_execute_epl_cmd() is skipped
> > since args->req.epl_len is set to zero and download fails.
>
> To be clear, this is what I'm suggesting [1] and it doesn't involve
> setting args->req.epl_len to zero, so I'm not sure what was tested.
>
> Basically, setting maximum length of read or write to 128 bytes as the
> kernel does not currently support auto paging (even if the transceiver
> module does) and will not try to perform cross-page reads or writes.
>
> [1]
> diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
> index 1e790413db0e..4a9a946cabf0 100644
> --- a/net/ethtool/cmis.h
> +++ b/net/ethtool/cmis.h
> @@ -101,7 +101,6 @@ struct ethtool_cmis_cdb_rpl {
>  };
>
>  u32 ethtool_cmis_get_max_lpl_size(u8 num_of_byte_octs);
> -u32 ethtool_cmis_get_max_epl_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 *lpl,
> diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
> index d159dc121bde..0e2691ccb0df 100644
> --- a/net/ethtool/cmis_cdb.c
> +++ b/net/ethtool/cmis_cdb.c
> @@ -16,15 +16,6 @@ u32 ethtool_cmis_get_max_lpl_size(u8 num_of_byte_octs)
>         return 8 * (1 + min_t(u8, num_of_byte_octs, 15));
>  }
>
> -/* For accessing the EPL field on page 9Fh, the allowable length extension is
> - * min(i, 255) byte octets where i specifies the allowable additional number of
> - * byte octets in a READ or a WRITE.
> - */
> -u32 ethtool_cmis_get_max_epl_size(u8 num_of_byte_octs)
> -{
> -       return 8 * (1 + min_t(u8, num_of_byte_octs, 255));
> -}
> -
>  void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
>                                    enum ethtool_cmis_cdb_cmd_id cmd, u8 *lpl,
>                                    u8 lpl_len, u8 *epl, u16 epl_len,
> @@ -33,19 +24,16 @@ void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
>  {
>         args->req.id = cpu_to_be16(cmd);
>         args->req.lpl_len = lpl_len;
> -       if (lpl) {
> +       if (lpl)
>                 memcpy(args->req.payload, lpl, args->req.lpl_len);
> -               args->read_write_len_ext =
> -                       ethtool_cmis_get_max_lpl_size(read_write_len_ext);
> -       }
>         if (epl) {
>                 args->req.epl_len = cpu_to_be16(epl_len);
>                 args->req.epl = epl;
> -               args->read_write_len_ext =
> -                       ethtool_cmis_get_max_epl_size(read_write_len_ext);
>         }
>
>         args->max_duration = max_duration;
> +       args->read_write_len_ext =
> +               ethtool_cmis_get_max_lpl_size(read_write_len_ext);
>         args->msleep_pre_rpl = msleep_pre_rpl;
>         args->rpl_exp_len = rpl_exp_len;
>         args->flags = flags;

Yes, your change works and here is the diff.
From the comment "before 9a3b0d078bd82", I tried
updating your comments on top of edc344568922. I should
have asked you for clarification.


diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
index 54916c16d13f..4f5ad34af3ea 100644
--- a/net/ethtool/cmis.h
+++ b/net/ethtool/cmis.h
@@ -75,7 +75,7 @@ struct ethtool_cmis_cdb_request {
 struct ethtool_cmis_cdb_cmd_args {
        struct ethtool_cmis_cdb_request req;
        u16                             max_duration;
-       u16                             calc_read_write_len_ext;
+       u8                              read_write_len_ext;
        u8                              msleep_pre_rpl;
        u8                              rpl_exp_len;
        u8                              flags;
diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
index 1f487e1a6347..56c6130fe010 100644
--- a/net/ethtool/cmis_cdb.c
+++ b/net/ethtool/cmis_cdb.c
@@ -16,15 +16,6 @@ u32 ethtool_cmis_get_max_lpl_size(u8 num_of_byte_octs)
        return 8 * (1 + min_t(u8, num_of_byte_octs, 15));
 }

-/* For accessing the EPL field on page 9Fh, the allowable length extension is
- * min(i, 255) byte octets where i specifies the allowable additional number of
- * byte octets in a READ or a WRITE.
- */
-u32 ethtool_cmis_get_max_epl_size(u8 num_of_byte_octs)
-{
-       return 8 * (1 + min_t(u8, num_of_byte_octs, 255));
-}
-
 void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
                                   enum ethtool_cmis_cdb_cmd_id cmd, u8 *lpl,
                                   u8 lpl_len, u8 *epl, u16 epl_len,
@@ -35,16 +26,13 @@ void ethtool_cmis_cdb_compose_args(struct
ethtool_cmis_cdb_cmd_args *args,
        args->req.lpl_len = lpl_len;
        if (lpl) {
                memcpy(args->req.payload, lpl, args->req.lpl_len);
-               args->calc_read_write_len_ext =
-                       ethtool_cmis_get_max_lpl_size(read_write_len_ext);
        }
        if (epl) {
                args->req.epl_len = cpu_to_be16(epl_len);
                args->req.epl = epl;
-               args->calc_read_write_len_ext =
-                       ethtool_cmis_get_max_epl_size(read_write_len_ext);
        }
-
+       args->read_write_len_ext =
+                       ethtool_cmis_get_max_lpl_size(read_write_len_ext);
        args->max_duration = max_duration;
        args->msleep_pre_rpl = msleep_pre_rpl;
        args->rpl_exp_len = rpl_exp_len;
@@ -590,7 +578,7 @@ ethtool_cmis_cdb_execute_epl_cmd(struct net_device *dev,
                        space_left = CMIS_CDB_EPL_FW_BLOCK_OFFSET_END
- offset + 1;
                        bytes_to_write = min_t(u16, bytes_left,
                                               min_t(u16, space_left,
-
args->calc_read_write_len_ext));
+                                                    args->read_write_len_ext));

                        err = __ethtool_cmis_cdb_execute_cmd(dev, page_data,
                                                             page, offset,
@@ -631,7 +619,7 @@ int ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
                                       offsetof(struct ethtool_cmis_cdb_request,
                                                epl));

-       if (args->req.lpl_len > args->calc_read_write_len_ext) {
+       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;
        }

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5444 bytes --]

Re: [PATCH net 2/2] ethtool: cmis: use u16 for calculated read_write_len_ext

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 3069 bytes --]

On Mon, Apr 7, 2025 at 9:23 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> To be clear, this is what I'm suggesting [1] and it doesn't involve
> setting args->req.epl_len to zero, so I'm not sure what was tested.
>
> Basically, setting maximum length of read or write to 128 bytes as the
> kernel does not currently support auto paging (even if the transceiver
> module does) and will not try to perform cross-page reads or writes.

Ido, do you want to post your patch formally?  Damodharam has tested
it and he is providing his:

Tested-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>

I'll drop this patch and repost patch #1 only.  Thanks.

>
> [1]
> diff --git a/net/ethtool/cmis.h b/net/ethtool/cmis.h
> index 1e790413db0e..4a9a946cabf0 100644
> --- a/net/ethtool/cmis.h
> +++ b/net/ethtool/cmis.h
> @@ -101,7 +101,6 @@ struct ethtool_cmis_cdb_rpl {
>  };
>
>  u32 ethtool_cmis_get_max_lpl_size(u8 num_of_byte_octs);
> -u32 ethtool_cmis_get_max_epl_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 *lpl,
> diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
> index d159dc121bde..0e2691ccb0df 100644
> --- a/net/ethtool/cmis_cdb.c
> +++ b/net/ethtool/cmis_cdb.c
> @@ -16,15 +16,6 @@ u32 ethtool_cmis_get_max_lpl_size(u8 num_of_byte_octs)
>         return 8 * (1 + min_t(u8, num_of_byte_octs, 15));
>  }
>
> -/* For accessing the EPL field on page 9Fh, the allowable length extension is
> - * min(i, 255) byte octets where i specifies the allowable additional number of
> - * byte octets in a READ or a WRITE.
> - */
> -u32 ethtool_cmis_get_max_epl_size(u8 num_of_byte_octs)
> -{
> -       return 8 * (1 + min_t(u8, num_of_byte_octs, 255));
> -}
> -
>  void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
>                                    enum ethtool_cmis_cdb_cmd_id cmd, u8 *lpl,
>                                    u8 lpl_len, u8 *epl, u16 epl_len,
> @@ -33,19 +24,16 @@ void ethtool_cmis_cdb_compose_args(struct ethtool_cmis_cdb_cmd_args *args,
>  {
>         args->req.id = cpu_to_be16(cmd);
>         args->req.lpl_len = lpl_len;
> -       if (lpl) {
> +       if (lpl)
>                 memcpy(args->req.payload, lpl, args->req.lpl_len);
> -               args->read_write_len_ext =
> -                       ethtool_cmis_get_max_lpl_size(read_write_len_ext);
> -       }
>         if (epl) {
>                 args->req.epl_len = cpu_to_be16(epl_len);
>                 args->req.epl = epl;
> -               args->read_write_len_ext =
> -                       ethtool_cmis_get_max_epl_size(read_write_len_ext);
>         }
>
>         args->max_duration = max_duration;
> +       args->read_write_len_ext =
> +               ethtool_cmis_get_max_lpl_size(read_write_len_ext);
>         args->msleep_pre_rpl = msleep_pre_rpl;
>         args->rpl_exp_len = rpl_exp_len;
>         args->flags = flags;

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4196 bytes --]

Re: [PATCH net 2/2] ethtool: cmis: use u16 for calculated read_write_len_ext

[Ido Schimmel]

On Tue, Apr 08, 2025 at 11:25:44AM -0700, Michael Chan wrote:
> On Mon, Apr 7, 2025 at 9:23 AM Ido Schimmel <idosch@idosch.org> wrote:
> >
> > To be clear, this is what I'm suggesting [1] and it doesn't involve
> > setting args->req.epl_len to zero, so I'm not sure what was tested.
> >
> > Basically, setting maximum length of read or write to 128 bytes as the
> > kernel does not currently support auto paging (even if the transceiver
> > module does) and will not try to perform cross-page reads or writes.
> 
> Ido, do you want to post your patch formally?  Damodharam has tested
> it and he is providing his:
> 
> Tested-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> 
> I'll drop this patch and repost patch #1 only.  Thanks.

OK, as you wish. I figured you would just incorporate the change into
v2, but I can post the patch myself.