diff mbox series

[ethtool-next,1/7] cmis: Fix CLEI code parsing

Message ID 20210917144043.566049-2-idosch@idosch.org (mailing list archive)
State Changes Requested, archived
Delegated to: Michal Kubecek
Headers show
Series ethtool: Small EEPROM changes | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Ido Schimmel Sept. 17, 2021, 2:40 p.m. UTC 
From: Ido Schimmel <idosch@nvidia.com>

In CMIS, unlike SFF-8636, there is no presence indication for the CLEI
code (Common Language Equipment Identification) field. The field is
always present, but might not be supported. In which case, "a value of
all ASCII 20h (spaces) shall be entered".

Therefore, remove the erroneous check which seems to be influenced from
SFF-8636 and only print the string if it is supported and has a non-zero
length.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 cmis.c | 8 +++++---
 cmis.h | 3 +--
 2 files changed, 6 insertions(+), 5 deletions(-)

Comments

Michal Kubecek Sept. 30, 2021, 8:21 p.m. UTC | #1 
On Fri, Sep 17, 2021 at 05:40:37PM +0300, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> In CMIS, unlike SFF-8636, there is no presence indication for the CLEI
> code (Common Language Equipment Identification) field. The field is
> always present, but might not be supported. In which case, "a value of
> all ASCII 20h (spaces) shall be entered".
> 
> Therefore, remove the erroneous check which seems to be influenced from
> SFF-8636 and only print the string if it is supported and has a non-zero
> length.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  cmis.c | 8 +++++---
>  cmis.h | 3 +--
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/cmis.c b/cmis.c
> index 1a91e798e4b8..2a48c1a1d56a 100644
> --- a/cmis.c
> +++ b/cmis.c
> @@ -307,6 +307,8 @@ static void cmis_show_link_len(const __u8 *id)
>   */
>  static void cmis_show_vendor_info(const __u8 *id)
>  {
> +	const char *clei = (const char *)(id + CMIS_CLEI_START_OFFSET);
> +
>  	sff_show_ascii(id, CMIS_VENDOR_NAME_START_OFFSET,
>  		       CMIS_VENDOR_NAME_END_OFFSET, "Vendor name");
>  	cmis_show_oui(id);
> @@ -319,9 +321,9 @@ static void cmis_show_vendor_info(const __u8 *id)
>  	sff_show_ascii(id, CMIS_DATE_YEAR_OFFSET,
>  		       CMIS_DATE_VENDOR_LOT_OFFSET + 1, "Date code");
>  
> -	if (id[CMIS_CLEI_PRESENT_BYTE] & CMIS_CLEI_PRESENT_MASK)
> -		sff_show_ascii(id, CMIS_CLEI_START_OFFSET,
> -			       CMIS_CLEI_END_OFFSET, "CLEI code");
> +	if (strlen(clei) && strcmp(clei, CMIS_CLEI_BLANK))
> +		sff_show_ascii(id, CMIS_CLEI_START_OFFSET, CMIS_CLEI_END_OFFSET,
> +			       "CLEI code");
>  }
>  
>  void qsfp_dd_show_all(const __u8 *id)

Is it safe to assume that the string will be always null terminated?
Looking at the code below, CMIS_CLEI_BLANK consists of 10 spaces which
would fill the whole block at offsets 0xBE through 0xC7 with spaces and
offset 0xC8 is used as CMIS_PWR_CLASS_OFFSET. Also, sff_show_ascii()
doesn't seem to expect a null terminated string, rather a space padded
one.

Michal


> diff --git a/cmis.h b/cmis.h
> index 78ee1495bc33..d365252baa48 100644
> --- a/cmis.h
> +++ b/cmis.h
> @@ -34,10 +34,9 @@
>  #define CMIS_DATE_VENDOR_LOT_OFFSET		0xBC
>  
>  /* CLEI Code (Page 0) */
> -#define CMIS_CLEI_PRESENT_BYTE			0x02
> -#define CMIS_CLEI_PRESENT_MASK			0x20
>  #define CMIS_CLEI_START_OFFSET			0xBE
>  #define CMIS_CLEI_END_OFFSET			0xC7
> +#define CMIS_CLEI_BLANK				"          "
>  
>  /* Cable assembly length */
>  #define CMIS_CBL_ASM_LEN_OFFSET			0xCA
> -- 
> 2.31.1
>
Ido Schimmel Sept. 30, 2021, 8:41 p.m. UTC | #2 
On Thu, Sep 30, 2021 at 10:21:33PM +0200, Michal Kubecek wrote:
> On Fri, Sep 17, 2021 at 05:40:37PM +0300, Ido Schimmel wrote:
> > From: Ido Schimmel <idosch@nvidia.com>
> > 
> > In CMIS, unlike SFF-8636, there is no presence indication for the CLEI
> > code (Common Language Equipment Identification) field. The field is
> > always present, but might not be supported. In which case, "a value of
> > all ASCII 20h (spaces) shall be entered".
> > 
> > Therefore, remove the erroneous check which seems to be influenced from
> > SFF-8636 and only print the string if it is supported and has a non-zero
> > length.
> > 
> > Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> > ---
> >  cmis.c | 8 +++++---
> >  cmis.h | 3 +--
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/cmis.c b/cmis.c
> > index 1a91e798e4b8..2a48c1a1d56a 100644
> > --- a/cmis.c
> > +++ b/cmis.c
> > @@ -307,6 +307,8 @@ static void cmis_show_link_len(const __u8 *id)
> >   */
> >  static void cmis_show_vendor_info(const __u8 *id)
> >  {
> > +	const char *clei = (const char *)(id + CMIS_CLEI_START_OFFSET);
> > +
> >  	sff_show_ascii(id, CMIS_VENDOR_NAME_START_OFFSET,
> >  		       CMIS_VENDOR_NAME_END_OFFSET, "Vendor name");
> >  	cmis_show_oui(id);
> > @@ -319,9 +321,9 @@ static void cmis_show_vendor_info(const __u8 *id)
> >  	sff_show_ascii(id, CMIS_DATE_YEAR_OFFSET,
> >  		       CMIS_DATE_VENDOR_LOT_OFFSET + 1, "Date code");
> >  
> > -	if (id[CMIS_CLEI_PRESENT_BYTE] & CMIS_CLEI_PRESENT_MASK)
> > -		sff_show_ascii(id, CMIS_CLEI_START_OFFSET,
> > -			       CMIS_CLEI_END_OFFSET, "CLEI code");
> > +	if (strlen(clei) && strcmp(clei, CMIS_CLEI_BLANK))
> > +		sff_show_ascii(id, CMIS_CLEI_START_OFFSET, CMIS_CLEI_END_OFFSET,
> > +			       "CLEI code");
> >  }
> >  
> >  void qsfp_dd_show_all(const __u8 *id)
> 
> Is it safe to assume that the string will be always null terminated?

No. You want to see strnlen() and strncmp() instead?

> Looking at the code below, CMIS_CLEI_BLANK consists of 10 spaces which
> would fill the whole block at offsets 0xBE through 0xC7 with spaces and
> offset 0xC8 is used as CMIS_PWR_CLASS_OFFSET. Also, sff_show_ascii()
> doesn't seem to expect a null terminated string, rather a space padded
> one.
> 
> Michal
> 
> 
> > diff --git a/cmis.h b/cmis.h
> > index 78ee1495bc33..d365252baa48 100644
> > --- a/cmis.h
> > +++ b/cmis.h
> > @@ -34,10 +34,9 @@
> >  #define CMIS_DATE_VENDOR_LOT_OFFSET		0xBC
> >  
> >  /* CLEI Code (Page 0) */
> > -#define CMIS_CLEI_PRESENT_BYTE			0x02
> > -#define CMIS_CLEI_PRESENT_MASK			0x20
> >  #define CMIS_CLEI_START_OFFSET			0xBE
> >  #define CMIS_CLEI_END_OFFSET			0xC7
> > +#define CMIS_CLEI_BLANK				"          "
> >  
> >  /* Cable assembly length */
> >  #define CMIS_CBL_ASM_LEN_OFFSET			0xCA
> > -- 
> > 2.31.1
> >
Michal Kubecek Sept. 30, 2021, 9:12 p.m. UTC | #3 
On Thu, Sep 30, 2021 at 11:41:02PM +0300, Ido Schimmel wrote:
> On Thu, Sep 30, 2021 at 10:21:33PM +0200, Michal Kubecek wrote:
> > On Fri, Sep 17, 2021 at 05:40:37PM +0300, Ido Schimmel wrote:
> > > From: Ido Schimmel <idosch@nvidia.com>
> > > 
> > > In CMIS, unlike SFF-8636, there is no presence indication for the CLEI
> > > code (Common Language Equipment Identification) field. The field is
> > > always present, but might not be supported. In which case, "a value of
> > > all ASCII 20h (spaces) shall be entered".
> > > 
> > > Therefore, remove the erroneous check which seems to be influenced from
> > > SFF-8636 and only print the string if it is supported and has a non-zero
> > > length.
> > > 
> > > Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> > > ---
> > >  cmis.c | 8 +++++---
> > >  cmis.h | 3 +--
> > >  2 files changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/cmis.c b/cmis.c
> > > index 1a91e798e4b8..2a48c1a1d56a 100644
> > > --- a/cmis.c
> > > +++ b/cmis.c
> > > @@ -307,6 +307,8 @@ static void cmis_show_link_len(const __u8 *id)
> > >   */
> > >  static void cmis_show_vendor_info(const __u8 *id)
> > >  {
> > > +	const char *clei = (const char *)(id + CMIS_CLEI_START_OFFSET);
> > > +
> > >  	sff_show_ascii(id, CMIS_VENDOR_NAME_START_OFFSET,
> > >  		       CMIS_VENDOR_NAME_END_OFFSET, "Vendor name");
> > >  	cmis_show_oui(id);
> > > @@ -319,9 +321,9 @@ static void cmis_show_vendor_info(const __u8 *id)
> > >  	sff_show_ascii(id, CMIS_DATE_YEAR_OFFSET,
> > >  		       CMIS_DATE_VENDOR_LOT_OFFSET + 1, "Date code");
> > >  
> > > -	if (id[CMIS_CLEI_PRESENT_BYTE] & CMIS_CLEI_PRESENT_MASK)
> > > -		sff_show_ascii(id, CMIS_CLEI_START_OFFSET,
> > > -			       CMIS_CLEI_END_OFFSET, "CLEI code");
> > > +	if (strlen(clei) && strcmp(clei, CMIS_CLEI_BLANK))
> > > +		sff_show_ascii(id, CMIS_CLEI_START_OFFSET, CMIS_CLEI_END_OFFSET,
> > > +			       "CLEI code");
> > >  }
> > >  
> > >  void qsfp_dd_show_all(const __u8 *id)
> > 
> > Is it safe to assume that the string will be always null terminated?
> 
> No. You want to see strnlen() and strncmp() instead?

Yes, that would solve the problem. Actually, "strlen(clei)" could be
also replaced with "*clei" or "clei[0]" as a string is empty if and only
if its first byte is null.

Michal

> > Looking at the code below, CMIS_CLEI_BLANK consists of 10 spaces which
> > would fill the whole block at offsets 0xBE through 0xC7 with spaces and
> > offset 0xC8 is used as CMIS_PWR_CLASS_OFFSET. Also, sff_show_ascii()
> > doesn't seem to expect a null terminated string, rather a space padded
> > one.
diff mbox series

Patch

diff --git a/cmis.c b/cmis.c
index 1a91e798e4b8..2a48c1a1d56a 100644
--- a/cmis.c
+++ b/cmis.c
@@ -307,6 +307,8 @@  static void cmis_show_link_len(const __u8 *id)
  */
 static void cmis_show_vendor_info(const __u8 *id)
 {
+	const char *clei = (const char *)(id + CMIS_CLEI_START_OFFSET);
+
 	sff_show_ascii(id, CMIS_VENDOR_NAME_START_OFFSET,
 		       CMIS_VENDOR_NAME_END_OFFSET, "Vendor name");
 	cmis_show_oui(id);
@@ -319,9 +321,9 @@  static void cmis_show_vendor_info(const __u8 *id)
 	sff_show_ascii(id, CMIS_DATE_YEAR_OFFSET,
 		       CMIS_DATE_VENDOR_LOT_OFFSET + 1, "Date code");
 
-	if (id[CMIS_CLEI_PRESENT_BYTE] & CMIS_CLEI_PRESENT_MASK)
-		sff_show_ascii(id, CMIS_CLEI_START_OFFSET,
-			       CMIS_CLEI_END_OFFSET, "CLEI code");
+	if (strlen(clei) && strcmp(clei, CMIS_CLEI_BLANK))
+		sff_show_ascii(id, CMIS_CLEI_START_OFFSET, CMIS_CLEI_END_OFFSET,
+			       "CLEI code");
 }
 
 void qsfp_dd_show_all(const __u8 *id)
diff --git a/cmis.h b/cmis.h
index 78ee1495bc33..d365252baa48 100644
--- a/cmis.h
+++ b/cmis.h
@@ -34,10 +34,9 @@ 
 #define CMIS_DATE_VENDOR_LOT_OFFSET		0xBC
 
 /* CLEI Code (Page 0) */
-#define CMIS_CLEI_PRESENT_BYTE			0x02
-#define CMIS_CLEI_PRESENT_MASK			0x20
 #define CMIS_CLEI_START_OFFSET			0xBE
 #define CMIS_CLEI_END_OFFSET			0xC7
+#define CMIS_CLEI_BLANK				"          "
 
 /* Cable assembly length */
 #define CMIS_CBL_ASM_LEN_OFFSET			0xCA