[PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes

[PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes

[vidya]

From: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>

This patch provides macro definitions for maximum possible
memory map size of QSFP+/QSFP28 EEPROMs as per SFF-8436/SFF-8636

According to SFF-8436/SFF-8636 specs, the common memory map for
managing external cable interfaces is arranged into a single
lower page address space of 128 bytes and multiple upper address pages
of 128 bytes each. The total size of memory map is up to 5 128 byte pages
defined by a "pages valid" register and switched via a "page select"
register for "optional pages". Memory of 256 bytes can be memory mapped
at a time and QSFP+/QSFP28  drivers can export upto 5*128 bytes of
eeprom dump.

QSFP+/QSFP28 Memory layout
           2-Wire Serial Address: 1010000x
           Lower Page 00h (128 bytes)
           ======================
           |Page Select Byte(127)|
           ======================
                    |
                    V
       ----------------------------------------
      |             |            |             |
      V             V            V             V
   ----------   ----------   ---------    ------------
  | Upper    | | Upper    | | Upper    | | Upper      |
  | Page 00h | | Page 01h | | Page 02h | | Page 03h   |
  |          | |(Optional)| |(Optional)| | (Optional) |
  |    ID    | |   AST    | |  User    | |  For       |
  |  Fields  | |  Table   | | EEPROM   | |  Cable     |
  -----------  -----------   ----------  --------------

Signed-off-by: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>
---
 include/uapi/linux/ethtool.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 5f030b4..ff6ccbd 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1584,9 +1584,15 @@ static inline int ethtool_validate_duplex(__u8 duplex)
 #define ETH_MODULE_SFF_8472		0x2
 #define ETH_MODULE_SFF_8472_LEN		512
 #define ETH_MODULE_SFF_8636		0x3
+/* min, without optional page */
 #define ETH_MODULE_SFF_8636_LEN		256
+/* max, with all optional pages */
+#define ETH_MODULE_SFF_8636_MAX_LEN		640
 #define ETH_MODULE_SFF_8436		0x4
+/* min, without optional page */
 #define ETH_MODULE_SFF_8436_LEN		256
+/* max, with all optional pages */
+#define ETH_MODULE_SFF_8436_MAX_LEN		640
 
 /* Reset flags */
 /* The reset() operation must clear the flags for the components which
-- 
2.1.4

Re: [PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes

[David Miller]

From: vidya@cumulusnetworks.com
Date: Mon,  6 Jun 2016 22:55:26 -0700

> From: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>
> 
> This patch provides macro definitions for maximum possible
> memory map size of QSFP+/QSFP28 EEPROMs as per SFF-8436/SFF-8636
> 
> According to SFF-8436/SFF-8636 specs, the common memory map for
> managing external cable interfaces is arranged into a single
> lower page address space of 128 bytes and multiple upper address pages
> of 128 bytes each. The total size of memory map is up to 5 128 byte pages
> defined by a "pages valid" register and switched via a "page select"
> register for "optional pages". Memory of 256 bytes can be memory mapped
> at a time and QSFP+/QSFP28  drivers can export upto 5*128 bytes of
> eeprom dump.
> 
> QSFP+/QSFP28 Memory layout
>            2-Wire Serial Address: 1010000x
>            Lower Page 00h (128 bytes)
>            ======================
>            |Page Select Byte(127)|
>            ======================
>                     |
>                     V
>        ----------------------------------------
>       |             |            |             |
>       V             V            V             V
>    ----------   ----------   ---------    ------------
>   | Upper    | | Upper    | | Upper    | | Upper      |
>   | Page 00h | | Page 01h | | Page 02h | | Page 03h   |
>   |          | |(Optional)| |(Optional)| | (Optional) |
>   |    ID    | |   AST    | |  User    | |  For       |
>   |  Fields  | |  Table   | | EEPROM   | |  Cable     |
>   -----------  -----------   ----------  --------------
> 
> Signed-off-by: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>

Why do we need these values in the header file at all?

The application can probe the size by repeated eeprom calls, increasing
the buffer size each time as needed until success.

Re: [PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes

[Vidya Sagar Ravipati]

On Sat, Jun 11, 2016 at 3:51 PM, David Miller <davem@davemloft.net> wrote:
> From: vidya@cumulusnetworks.com
> Date: Mon,  6 Jun 2016 22:55:26 -0700
>
>> From: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>
>>
>> This patch provides macro definitions for maximum possible
>> memory map size of QSFP+/QSFP28 EEPROMs as per SFF-8436/SFF-8636
>>
>> According to SFF-8436/SFF-8636 specs, the common memory map for
>> managing external cable interfaces is arranged into a single
>> lower page address space of 128 bytes and multiple upper address pages
>> of 128 bytes each. The total size of memory map is up to 5 128 byte pages
>> defined by a "pages valid" register and switched via a "page select"
>> register for "optional pages". Memory of 256 bytes can be memory mapped
>> at a time and QSFP+/QSFP28  drivers can export upto 5*128 bytes of
>> eeprom dump.
>>
>> QSFP+/QSFP28 Memory layout
>>            2-Wire Serial Address: 1010000x
>>            Lower Page 00h (128 bytes)
>>            ======================
>>            |Page Select Byte(127)|
>>            ======================
>>                     |
>>                     V
>>        ----------------------------------------
>>       |             |            |             |
>>       V             V            V             V
>>    ----------   ----------   ---------    ------------
>>   | Upper    | | Upper    | | Upper    | | Upper      |
>>   | Page 00h | | Page 01h | | Page 02h | | Page 03h   |
>>   |          | |(Optional)| |(Optional)| | (Optional) |
>>   |    ID    | |   AST    | |  User    | |  For       |
>>   |  Fields  | |  Table   | | EEPROM   | |  Cable     |
>>   -----------  -----------   ----------  --------------
>>
>> Signed-off-by: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>
>
> Why do we need these values in the header file at all?

As part of ethtool application, application is requesting  the drivers
to provide the supported eeprom size to allocate memory buffer for
getting complete dump.

In case of QSFP+/QSFP28, the size can be either 256 or 640 bytes(when
optional pages are supported) and it would be good to have same macros
across ethtool application and ethernet drivers for providing
eeprom_length field in  ETHTOOL_GMODULEINFO ioctl .


>
> The application can probe the size by repeated eeprom calls, increasing
> the buffer size each time as needed until success.

Re: [PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes

[David Miller]

From: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>
Date: Sat, 11 Jun 2016 16:22:38 -0700

> As part of ethtool application, application is requesting  the drivers
> to provide the supported eeprom size to allocate memory buffer for
> getting complete dump.

And the right way to do that is the driver requests the eeprom info
with a buffer size of zero, then the driver fills in the size field
for what the size actually is.

Then the application can allocate the proper buffer size and rerun
the eeprom request.

Putting endless values for each and every eeprom type a device has is
just rediculous.

I'm not going to continue promoting this broken and unscalable scheme,
we have to fix this.

Re: [PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes

[Ben Hutchings]

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

On Sat, 2016-06-11 at 15:51 -0700, David Miller wrote:
[...]
> Why do we need these values in the header file at all?

Because we don't like putting magic numbers in driver code, and these
sizes are defined by standards that are independent of a single driver.

> The application can probe the size by repeated eeprom calls, increasing
> the buffer size each time as needed until success.

But really it should use ETHTOOL_GMODULEINFO first.

Ben.

-- 
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes

[Ben Hutchings]

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

On Sat, 2016-06-11 at 19:26 -0700, David Miller wrote:
> From: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>
> Date: Sat, 11 Jun 2016 16:22:38 -0700
> 
> > As part of ethtool application, application is requesting  the drivers
> > to provide the supported eeprom size to allocate memory buffer for
> > getting complete dump.
> 
> And the right way to do that is the driver requests the eeprom info
> with a buffer size of zero, then the driver fills in the size field
> for what the size actually is.
> 
> Then the application can allocate the proper buffer size and rerun
> the eeprom request.
> 
> Putting endless values for each and every eeprom type a device has is
> just rediculous.
> 
> I'm not going to continue promoting this broken and unscalable scheme,
> we have to fix this.

I don't think there's nothing broken here.  ethtool doesn't use those
macros, the drivers do.

Ben.

-- 
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sun, 12 Jun 2016 13:34:12 +0100

> On Sat, 2016-06-11 at 19:26 -0700, David Miller wrote:
>> From: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>
>> Date: Sat, 11 Jun 2016 16:22:38 -0700
>> 
>> > As part of ethtool application, application is requesting  the drivers
>> > to provide the supported eeprom size to allocate memory buffer for
>> > getting complete dump.
>> 
>> And the right way to do that is the driver requests the eeprom info
>> with a buffer size of zero, then the driver fills in the size field
>> for what the size actually is.
>> 
>> Then the application can allocate the proper buffer size and rerun
>> the eeprom request.
>> 
>> Putting endless values for each and every eeprom type a device has is
>> just rediculous.
>> 
>> I'm not going to continue promoting this broken and unscalable scheme,
>> we have to fix this.
> 
> I don't think there's nothing broken here.  ethtool doesn't use those
> macros, the drivers do.

If it's a value only for the drivers, then why does it need to be in a
public UAPI header file and exported outside of the kernel at all?

If it's a private driver detail, it should live in the driver.