Date:   Tue, 29 Dec 2020 12:23:04 +0200
From:   Vladyslav Tarasiuk <vladyslavt@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>, Moshe Shemesh <moshe@...dia.com>
CC:     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 27-Nov-20 17:56, Andrew Lunn wrote:
>> OK, but if the caching system is checking one time netlink and one time
>> ioctl, it means this cache should be in user space, or did you mean to have
>> this cache in kernel ?
> This is all in userspace, in the ethtool code.
>
>>>> What about the global offset that we currently got when user doesn't specify
>>>> a page, do you mean that this global offset goes through the optional and
>>>> non optional pages that exist and skip the ones that are missing according
>>>> to the specific EEPROM ?
>>> ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off] [hex on|off] [offset N] [length N]
>>>
>>> So you mean [offset N] [length N].
>>
>> Yes, that's the current options and we can either try coding new
>> implementation for that or just call the current ioctl implementation. The
>> new code can be triggered once options [bank N] and [Page N] are used.
> You cannot rely on the ioctl being available. New drivers won't
> implement it, if they have the netlink code. Drivers will convert from
> get_module_info to whatever new ndo call you add for netlink.
>
>> OK, if I got it right on current API [offset N] [length N] just call ioctl
>> current implementation, while using the option [raw on] will call new
>> implementation for new SFPs (CMIS 4). Also using [bank N] and [page N] will
>> call new implementation for new SFPs.
> Not just CMIS. All SFPs.
>
>      Andrew

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;
}

Check every requested page, offset and length availability in userspace,
depending on module standard and EEPROM data.
================================================================================

cache_fetch_add(page_number, bank_number, offset, length)
{
         ethnla_put_u8 (msgbuff, ETHTOOL_A_EEPROM_PAGE_NUMBER, page_number);
         ethnla_put_u8 (msgbuff, ETHTOOL_A_EEPROM_BANK_NUMBER, bank_number);
         ethnla_put_u16(msgbuff, ETHTOOL_A_EEPROM_PAGE_OFFSET, offset);
         ethnla_put_u16(msgbuff, ETHTOOL_A_EEPROM_PAGE_LENGTH, length);
         err = nlsock_send_get_request(eeprom_page_reply_cb) // caches a 
page
         if (err < 0)
                 return err;
}

// Any call should be a pair of cache_fetch_add() with error check and only
// then a cache_get(), but for pseudocode this will do
cache_get_page(page_number, bank_number, offset, length)
{
         if (!cache_contains(page_number, bank_number, offset, length))
                 cache_fetch_add(page_number, bank_number, offset, length);
         return cache_get(page_number, bank_number);
}

print_cmis_y()
{
         page_data = cache_get_page(0, 0, 0, 256)->data;
         print_page_zero(page_data);
         page_data = cache_get_page(1, 0, 128, 128)->data;
         print_page_one(page_data);
}

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();
         }
}

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();
}

eeprom_page_reply_cb()
{
         cache_add(new_page);
}

page_available(page_number)
{
         spec_id = cache_get_page(0, 0, 0, 128)->data[0];

         // check bits indicating page availability
         switch (spec_id) {
         case sff_xxxx:
                 return is_page_available_sff_xxxx(page_number);
         case cmis_y:
                 return is_page_available_cmis_y(page_number, bank_number);
         default:
                 return -EINVAL;
         }
}

nl_getmodule()
{
         msg_init();

         // request first low page
         ret = cache_fetch_add(0, 0, 0, 128);

         if (ret < 0) // netlink unsupported, fall back to ioctl
                 return ret;

         netlink_cmd_check();
         nl_parser()
         // check bits indicating page availability according to used spec
         if (page_available(page_number, bank_number)) {
                 if (cache_contains(page_number, bank_number, offset, 
length))
                         print_page(page_number, bank_number, offset, 
length)
                 else {
                         ret = cache_fetch_add();
                         if (ret < 0)
                                 return ret;
                         print_page(page_number, bank_number, offset, 
length);
                 }
         } else {
                 return -EINVAL;
         }
}


Or just proceed to request any page and depend on kernel parameter 
validation
================================================================================

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();
}

nl_getmodule()
{
         // request page straight away
         netlink_cmd_check();
         nl_parser();
         /*
          * nl_parser() sets page_number, page_bank, offset and length and
          * prepares to pass them to the kernel. See 
eeprom_page_parse_request()
          */
         ret = nlsock_sendmsg(getmodule_reply_cb);

         if (ret < 0)
                 retrun ret;
}



Kernel
This part is the same for both userspace variants
================================================================================

kernel_fallback(request, data)
{
         struct ethtool_eeprom eeprom;

         if (request->bank_number)
                 return -EINVAL;
         // also take into account page 00 size
         eeprom.offset = request->offset + page_size * request->page_number;
         eeprom.len = request->length;
         eeprom.cmd = ETHTOOL_GMODULEEEPROM;
         eeprom.data = data;

         err = ops->get_module_eeprom(dev, &eeprom, eeprom.data)
         if (err < 0)
                 return err;

         // prepare data for response via netlink like for supported ndo
}

eeprom_page_parse_request()
{
         struct eeprom_page_req_info *request;

         request->offset = nla_get_u16(tb[ETHTOOL_A_EEPROM_PAGE_OFFSET]);
         request->length = nla_get_u16(tb[ETHTOOL_A_EEPROM_PAGE_LENGTH]);
         request->page_number = 
nla_get_u8(tb[ETHTOOL_A_EEPROM_PAGE_NUMBER]);
         request->bank_number = 
nla_get_u8(tb[ETHTOOL_A_EEPROM_PAGE_BANK_NUMBER]);
}

eeprom_page_prepare_data(request)
{
         u8 data[128];
         u8 page_zero_data[128];

         if (!ops->get_module_eeprom_page) {
                 if (ops->get_module_info && ops->get_module_eeprom)
                         return kernel_fallback(request, &data);
                 else
                         return -EOPNOTSUPP;
         }
         // fetch page 00 for further checks
         err = ops->get_module_eeprom_page(dev, 0, 128, 0, 0, 
&page_zero_data);
         if (err < 0)
                 return err;

         // Checks according to spec, similar to page_available() in 
userspace.
         // May be ommited and passed directly to the driver for it to 
validate
         if (is_request_valid(request, &page_zero_data))
                 err = ops->get_module_eeprom_page(dev, request->offset,
request->length,
request->page_number,
request->bank_number, &data);
         else
                 return -EINVAL;

}

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);

The driver may test whatever it may need in this ndo implementation.
Its parameters may also be passed using a special struct.
===============================================================================

driver_get_module_eeprom_page()
{
         vendor_query_module_eeprom(page_number, bank_number, offset, 
length, data);
}


Thanks,
Vlad

Powered by blists - more mailing lists