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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 30 Dec 2020 15:55:02 +0200
From:   Vladyslav Tarasiuk <vladyslavt@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Moshe Shemesh <moshe@...dia.com>, Jakub Kicinski <kuba@...nel.org>,
        "Moshe Shemesh" <moshe@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Adrian Pop <pop.adrian61@...il.com>,
        Michal Kubecek <mkubecek@...e.cz>, <netdev@...r.kernel.org>,
        Maxim Mikityanskiy <maximmi@...dia.com>,
        <vladyslavt@...dia.com>
Subject: Re: [PATCH net-next v2 0/2] Add support for DSFP transceiver type


On 29-Dec-20 18:25, Andrew Lunn wrote:
>> 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.
As far as I know, there may be bytes, which may change on read.
For example, clear on read values in CMIS 4.0.
Then, retrieving whole page every time may be incorrect.
So I kept these for cases, when user asks for specific few bytes.
> But maybe some indicator of low/high is
> needed, since many pages are actually 1/2 pages?
I was planning to use offset and data_length fields to indicate the
available page. For example, high page will have offset 128 and
data_length 128.
> 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.

I wasn't aware of that. It complicates things a bit, should we add a
parameter of i2c address? So in this case page 0 will be with i2c
address A0h. And if user needs page 0 from i2c address A2h, he will
specify it in command line. And for other specs, this parameter will
not be supported.

>> 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.
Yes, under print_sff_xxxx(), for example, I meant using existing functions.
Either as is, or refactoring according to cache requirements.
>> 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.
I'm thinking about a standard-specific function, which will prefetch pages
needed by print_human_readable(). It will check the standard ID, and go 
request
pages and add them to the cache. Then, decoder kicks with already cached
pages. This will eliminate recursive netlink calls.
>> 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.

OK, I will add extack.

Thanks,
Vlad