Message-ID: <CAKSYD4y5cTMdRmo97naYX=xE4k3jLBOBzptmyXi-YEimK4VmPw@mail.gmail.com>
Date: Wed, 9 Apr 2025 11:02:47 -0700
From: Damodharam Ammepalli <damodharam.ammepalli@...adcom.com>
To: Ido Schimmel <idosch@...dia.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, edumazet@...gle.com, horms@...nel.org, 
	danieller@...dia.com, petrm@...dia.com, andrew@...n.ch, 
	michael.chan@...adcom.com, andrew.gospodarek@...adcom.com
Subject: Re: [PATCH net] ethtool: cmis_cdb: Fix incorrect read / write length extension

On Wed, Apr 9, 2025 at 4:25 AM Ido Schimmel <idosch@...dia.com> wrote:
>
> The 'read_write_len_ext' field in 'struct ethtool_cmis_cdb_cmd_args'
> stores the maximum number of bytes that can be read from or written to
> the Local Payload (LPL) page in a single multi-byte access.
>
> Cited commit started overwriting this field with the maximum number of
> bytes that can be read from or written to the Extended Payload (LPL)
> pages in a single multi-byte access. Transceiver modules that support
> auto paging can advertise a number larger than 255 which is problematic
> as 'read_write_len_ext' is a 'u8', resulting in the number getting
> truncated and firmware flashing failing [1].
>
> Fix by ignoring the maximum EPL access size as the kernel does not
> currently support auto paging (even if the transceiver module does) and
> will not try to read / write more than 128 bytes at once.
>
> [1]
> 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: 9a3b0d078bd8 ("net: ethtool: Add support for writing firmware blocks using EPL payload")
> Reported-by: Damodharam Ammepalli <damodharam.ammepalli@...adcom.com>
> Closes: https://lore.kernel.org/netdev/20250402183123.321036-3-michael.chan@broadcom.com/
> Tested-by: Damodharam Ammepalli <damodharam.ammepalli@...adcom.com>
> Signed-off-by: Ido Schimmel <idosch@...dia.com>
Reviewed-by: Damodharam Ammepalli <damodharam.ammepalli@...adcom.com>
> ---
>  net/ethtool/cmis.h     |  1 -
>  net/ethtool/cmis_cdb.c | 18 +++---------------
>  2 files changed, 3 insertions(+), 16 deletions(-)
>
> 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;
> --
> 2.49.0
>

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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5444 bytes)

Powered by blists - more mailing lists