Re: [PATCH net-next v2 0/2] Add support for DSFP transceiver type

[Andrew Lunn <andrew@lunn.ch>]

> Hi Andrew,
> 
> Following this conversation, I wrote some pseudocode checking if I'm on
> right path here.
> Please review:
> 
> struct eeprom_page {
>         u8 page_number;
>         u8 bank_number;
>         u16 offset;
>         u16 data_length;
>         u8 *data;
> }

I'm wondering about offset and data_length, in this context. I would
expect you always ask the kernel for the full page, not part of
it. Even when user space asks for just part of a page. That keeps you
cache management simpler. But maybe some indicator of low/high is
needed, since many pages are actually 1/2 pages?

The other thing to consider is SFF-8472 and its use of two different
i2c addresses, A0h and A2h. These are different to pages and banks.

> print_human_readable()
> {
>         spec_id = cache_get_page(0, 0, 0, 128)->data[0];
>         switch (spec_id) {
>         case sff_xxxx:
>                 print_sff_xxxx();
>         case cmis_y:
>                 print_cmis_y();
>         default:
>                 print_hex();
>         }
> }

You want to keep as much of the existing ethtool code as you can, but
the basic idea looks O.K.

> getmodule_reply_cb()
> {
>         if (offset || hex || bank_number || page number)
>                 print_hex();
>         else
>                 // if _human_readable() decoder needs more than page 00, it
> will
>                 // fetch it on demand
>                 print_human_readable();
> }

Things get interesting here. Say this is page 0, and
print_human_readable() finds a bit indicating page 1 is valid. So it
requests page 1. We go recursive. While deep down in
print_human_readable(), we send the next netlink message and call
getmodule_reply_cb() when the answer appears. I've had problems with
some of the netlink code not liking recursive calls.

So i suggest you try to find a different structure for the code. Try
to complete the netlink call before doing the decoding. So add the
page to the cache and then return. Do the decode after
nlsock_sendmsg() has returned.

> Driver
> It is required to implement get_module_eeprom_page() ndo, where it queries
> its EEPROM and copies to u8 *data array allocated by the kernel previously.
> The ndo has the following prototype:
> int get_module_eeprom_page(struct net_device *, u16 offset, u16 length,
>                            u8 page_number, u8 bank_number, u8 *data);


I would include extack here, so we can get better error messages.

  Andrew