Re: [RFC PATCH V2 net-next 5/5] ethtool: Add fallback to get_module_eeprom from netlink command

On 3/5/2021 2:50 AM, Don Bollinger wrote:

On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote:

quoted

From: Vladyslav Tarasiuk <redacted>

In case netlink get_module_eeprom_data_by_page() callback is not
implemented by the driver, try to call old get_module_info() and
get_module_eeprom() pair. Recalculate parameters to
get_module_eeprom() offset and len using page number and their sizes.
Return error if this can't be done.

Signed-off-by: Vladyslav Tarasiuk <redacted>
---
  net/ethtool/eeprom.c | 84
+++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/eeprom.c b/net/ethtool/eeprom.c index

2618a55b9a40..72c7714a9d37 100644

--- a/net/ethtool/eeprom.c
+++ b/net/ethtool/eeprom.c

@@ -26,6 +26,88 @@ struct eeprom_data_reply_data {  #define

EEPROM_DATA_REPDATA(__reply_base) \
       container_of(__reply_base, struct eeprom_data_reply_data, base)

+static int fallback_set_params(struct eeprom_data_req_info *request,
+                            struct ethtool_modinfo *modinfo,
+                            struct ethtool_eeprom *eeprom) {

This is translating the new data structure into the old.  Hence, I assume we
have i2c_addr, page, bank, offset, len to work with, and we should use
all of them.  We shouldn't be applying the legacy data structure's rules
to how we interpret the *request data.  Therefore...

quoted

+     u32 offset = request->offset;
+     u32 length = request->length;
+
+     if (request->page)
+             offset = 128 + request->page * 128 + offset;

This is tricky to map to old behavior.  The new data structure should give
lower
memory for offsets less than 128, and paged upper memory for offsets of 128
and higher.  There is no way to describe that request as {offset, length} in
the
old ethtool format with a fake linear memory.

         if (request->page) {
                 if (offset < 128) && (offset + length > 128)
                        return -EINVAL;
                 if (offset > 127) offset = request->page * 128 + offset;

Yes, if we got page, that's the new API.

quoted

+
+     if (!length)
+             length = modinfo->eeprom_len;
+
+     if (offset >= modinfo->eeprom_len)
+             return -EINVAL;
+
+     if (modinfo->eeprom_len < offset + length)
+             length = modinfo->eeprom_len - offset;
+
+     eeprom->cmd = ETHTOOL_GMODULEEEPROM;
+     eeprom->len = length;
+     eeprom->offset = offset;
+
+     switch (modinfo->type) {
+     case ETH_MODULE_SFF_8079:
+             if (request->page > 1)
+                     return -EINVAL;
+             break;
+     case ETH_MODULE_SFF_8472:
+             if (request->page > 3)

Not sure this is needed, there can be pages higher than 3.

quoted

+                     return -EINVAL;

I *think* the linear memory on SFP puts 0x50 in the first
256 bytes, 0x51 after that, including pages after that.  So,
the old fashioned linear memory offset needs to be adjusted
for accesses to 0x51.  Thus add:

         if (request->i2c_address == 0x51)
                 offset += 256;

Will check that. In the old KAPI the i2c address is not a parameter, so 
it depends on driver implementation.

quoted

+             break;
+     case ETH_MODULE_SFF_8436:
+     case ETH_MODULE_SFF_8636:

Not sure this is needed, there can be pages higher than 3.

quoted

+             if (request->page > 3)
+                     return -EINVAL;
+             break;
+     }
+     return 0;
+}
+
+static int eeprom_data_fallback(struct eeprom_data_req_info *request,
+                             struct eeprom_data_reply_data *reply,
+                             struct genl_info *info)
+{
+     struct net_device *dev = reply->base.dev;
+     struct ethtool_modinfo modinfo = {0};
+     struct ethtool_eeprom eeprom = {0};
+     u8 *data;
+     int err;
+
+     if ((!dev->ethtool_ops->get_module_info &&
+          !dev->ethtool_ops->get_module_eeprom) ||
+         request->bank || request->i2c_address) {

We don't need to reject if there is an i2c_address.  Indeed, we need that
to determine the correct offset for the legacy linear memory offset.

Will check that. As Andrew said, there might be usage of other i2c 
addresses with old KAPI.

Note my comment on an earlier patch in this series, I would have rejected
any request that didn't have either 0x50 or 0x51 here.

quoted

+             return -EOPNOTSUPP;
+     }
+     modinfo.cmd = ETHTOOL_GMODULEINFO;
+     err = dev->ethtool_ops->get_module_info(dev, &modinfo);
+     if (err < 0)
+             return err;
+
+     err = fallback_set_params(request, &modinfo, &eeprom);
+     if (err < 0)
+             return err;
+
+     data = kmalloc(eeprom.len, GFP_KERNEL);
+     if (!data)
+             return -ENOMEM;
+     err = dev->ethtool_ops->get_module_eeprom(dev, &eeprom,
data);
+     if (err < 0)
+             goto err_out;
+
+     reply->data = data;
+     reply->length = eeprom.len;
+
+     return 0;
+
+err_out:
+     kfree(data);
+     return err;
+}
+
  static int eeprom_data_prepare_data(const struct ethnl_req_info
*req_base,
                                   struct ethnl_reply_data *reply_base,
                                   struct genl_info *info)

@@ -37,7 +119,7 @@ static int eeprom_data_prepare_data(const struct

ethnl_req_info *req_base,
       int err;

       if (!dev->ethtool_ops->get_module_eeprom_data_by_page)
-             return -EOPNOTSUPP;
+             return eeprom_data_fallback(request, reply, info);

       page_data.offset = request->offset;
       page_data.length = request->length;
--
2.18.2