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
