From: Ido Schimmel <idosch@nvidia.com> This patchset contains small patches for various issues I noticed while working on the module EEPROM parsing code. Ido Schimmel (7): cmis: Fix CLEI code parsing cmis: Fix wrong define name cmis: Correct comment sff-8636: Remove incorrect comment sff-8636: Fix incorrect function name sff-8636: Convert if statement to switch-case sff-8636: Remove extra blank lines cmis.c | 12 +++++++----- cmis.h | 5 ++--- qsfp.c | 18 +++++++++--------- 3 files changed, 18 insertions(+), 17 deletions(-) -- 2.31.1
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) 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
From: Ido Schimmel <idosch@nvidia.com> Offset 0x10 in the Lower Memory stores the "VccMonVoltage". Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- cmis.c | 2 +- cmis.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmis.c b/cmis.c index 2a48c1a1d56a..7fa7da87a92a 100644 --- a/cmis.c +++ b/cmis.c @@ -271,7 +271,7 @@ static void cmis_show_mod_lvl_monitors(const __u8 *id) PRINT_TEMP("Module temperature", OFFSET_TO_TEMP(CMIS_CURR_TEMP_OFFSET)); PRINT_VCC("Module voltage", - OFFSET_TO_U16(CMIS_CURR_CURR_OFFSET)); + OFFSET_TO_U16(CMIS_CURR_VCC_OFFSET)); } static void cmis_show_link_len_from_page(const __u8 *page_one_data) diff --git a/cmis.h b/cmis.h index d365252baa48..5fb2efe08265 100644 --- a/cmis.h +++ b/cmis.h @@ -11,7 +11,7 @@ /* Module-Level Monitors (Page 0) */ #define CMIS_CURR_TEMP_OFFSET 0x0E -#define CMIS_CURR_CURR_OFFSET 0x10 +#define CMIS_CURR_VCC_OFFSET 0x10 #define CMIS_CTOR_OFFSET 0xCB -- 2.31.1
From: Ido Schimmel <idosch@nvidia.com> The file is concerned with CMIS support, not QSFP-DD which is the physical form factor. Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- cmis.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmis.c b/cmis.c index 7fa7da87a92a..b85250a96a95 100644 --- a/cmis.c +++ b/cmis.c @@ -1,7 +1,7 @@ /** * Description: * - * This module adds QSFP-DD support to ethtool. The changes are similar to + * This module adds CMIS support to ethtool. The changes are similar to * the ones already existing in qsfp.c, but customized to use the memory * addresses and logic as defined in the specification's document. * -- 2.31.1
From: Ido Schimmel <idosch@nvidia.com> The comment was copied from SFF-8472 (i.e., sfpdiag.c) where the diagnostic page is at I2C address 0x51. SFF-8636 only uses I2C address 0x50. Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- qsfp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qsfp.c b/qsfp.c index e84226bc1554..263cf188377d 100644 --- a/qsfp.c +++ b/qsfp.c @@ -64,7 +64,7 @@ static struct sff8636_aw_flags { const char *str; /* Human-readable string, null at the end */ - int offset; /* A2-relative address offset */ + int offset; __u8 value; /* Alarm is on if (offset & value) != 0. */ } sff8636_aw_flags[] = { { "Laser bias current high alarm (Chan 1)", -- 2.31.1
From: Ido Schimmel <idosch@nvidia.com> The specification is called SFF-8636, not SFF-6836. Rename the function accordingly. Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- qsfp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qsfp.c b/qsfp.c index 263cf188377d..3401db84352d 100644 --- a/qsfp.c +++ b/qsfp.c @@ -820,7 +820,7 @@ static void sff8636_show_dom(const __u8 *id, const __u8 *page_three, __u32 eepro } -static void sff6836_show_page_zero(const __u8 *id) +static void sff8636_show_page_zero(const __u8 *id) { sff8636_show_ext_identifier(id); sff8636_show_connector(id); @@ -866,7 +866,7 @@ void sff8636_show_all(const __u8 *id, __u32 eeprom_len) if ((id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP) || (id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP_PLUS) || (id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP28)) { - sff6836_show_page_zero(id); + sff8636_show_page_zero(id); sff8636_show_dom(id, id + 3 * 0x80, eeprom_len); } } @@ -875,7 +875,7 @@ void sff8636_show_all_paged(const struct ethtool_module_eeprom *page_zero, const struct ethtool_module_eeprom *page_three) { sff8636_show_identifier(page_zero->data); - sff6836_show_page_zero(page_zero->data); + sff8636_show_page_zero(page_zero->data); if (page_three) sff8636_show_dom(page_zero->data, page_three->data - 0x80, ETH_MODULE_SFF_8636_MAX_LEN); -- 2.31.1
From: Ido Schimmel <idosch@nvidia.com> The indentation is wrong and the statement can be more clearly represented using a switch-case statement. Convert it. Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- qsfp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/qsfp.c b/qsfp.c index 3401db84352d..d1464cb50fdc 100644 --- a/qsfp.c +++ b/qsfp.c @@ -863,11 +863,13 @@ void sff8636_show_all(const __u8 *id, __u32 eeprom_len) } sff8636_show_identifier(id); - if ((id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP) || - (id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP_PLUS) || - (id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP28)) { + switch (id[SFF8636_ID_OFFSET]) { + case SFF8024_ID_QSFP: + case SFF8024_ID_QSFP_PLUS: + case SFF8024_ID_QSFP28: sff8636_show_page_zero(id); sff8636_show_dom(id, id + 3 * 0x80, eeprom_len); + break; } } -- 2.31.1
From: Ido Schimmel <idosch@nvidia.com> Not needed, so remove them. Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- qsfp.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/qsfp.c b/qsfp.c index d1464cb50fdc..3f37f1036e96 100644 --- a/qsfp.c +++ b/qsfp.c @@ -738,7 +738,6 @@ static void sff8636_dom_parse(const __u8 *id, const __u8 *page_three, struct sff sd->scd[i].rx_power = OFFSET_TO_U16(rx_power_offset); sd->scd[i].tx_power = OFFSET_TO_U16(tx_power_offset); } - } static void sff8636_show_dom(const __u8 *id, const __u8 *page_three, __u32 eeprom_len) @@ -819,7 +818,6 @@ static void sff8636_show_dom(const __u8 *id, const __u8 *page_three, __u32 eepro } } - static void sff8636_show_page_zero(const __u8 *id) { sff8636_show_ext_identifier(id); -- 2.31.1
[-- Attachment #1: Type: text/plain, Size: 2610 bytes --] 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 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
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 > >
[-- Attachment #1: Type: text/plain, Size: 2656 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]