[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <45a1b5c6-d348-cc62-681d-b2f257b578f9@nvidia.com>
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
Powered by blists - more mailing lists