| Message ID | 1614884228-8542-6-git-send-email-moshe@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethtool: Extend module EEPROM dump API | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 96 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote: > From: Vladyslav Tarasiuk <vladyslavt@nvidia.com> > > 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 <vladyslavt@nvidia.com> > --- > 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... > + 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; > + > + 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. > + 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; > + break; > + case ETH_MODULE_SFF_8436: > + case ETH_MODULE_SFF_8636: Not sure this is needed, there can be pages higher than 3. > + 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. 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. > + 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
> > +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. Nope. We actually have none of them. The old API just asked the driver to give me the data in the SFP. And the driver gets to decide what it returns, following a well known layout. The driver can decide to give just the first 1/2 page, or any number of multiple 1/2 pages in a well known linear way, which ethtool knows how to decode. So when mapping the new KAPI onto the old driver API, you need to call the old API, and see if what is returned can be used to fulfil the KAPI request. If the bytes are there, great, return them, otherwise EOPNOTSUPP. And we also need to consider the other way around. The old KAPI is used, and the MAC driver only supports the new driver API. Since the linear layout is well know, you need to make a number of calls into the driver to read the 1/2 pages, and them glue them together and return them. I've not reviewed this code in detail yet, so i've no idea how it actually works. But i would like to see as much compatibility as possible. That has been the approach with moving from IOCTL to netlink with ethool. Everything the old KAPI can do, netlink should also be able to, plus there can be additional features. > > + 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. Not with the old KAPI call. As far as i remember, it stops at three pages. But i need to check the ethtool(1) sources to be sure. Andrew
> > > +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. > > Nope. We actually have none of them. The old API just asked the driver to > give me the data in the SFP. And the driver gets to decide what it returns, > following a well known layout. The driver can decide to give just the first 1/2 > page, or any number of multiple 1/2 pages in a well known linear way, which > ethtool knows how to decode. This code is to take a new KAPI request (a struct eeprom_data_req_info), and create an old driver API request (a struct ethtool_eeprom) that will get the same data. It isn't actually fetching the data, it is just forming the data structure to create the request. So, we do indeed have all of the new KAPI parameters, and need to use all of them to precisely create the matching old KAPI request. > So when mapping the new KAPI onto the old driver API, you need to call the > old API, and see if what is returned can be used to fulfil the KAPI request. If > the bytes are there, great, return them, otherwise EOPNOTSUPP. Actually, this code has to figure out in advance whether the old API can return the data to fulfill the request, then form a request to accomplish that. > And we also need to consider the other way around. The old KAPI is used, > and the MAC driver only supports the new driver API. Since the linear layout > is well know, you need to make a number of calls into the driver to read the > 1/2 pages, and them glue them together and return them. That is a great idea, probably not difficult. It is not in this patch set. > I've not reviewed this code in detail yet, so i've no idea how it actually works. > But i would like to see as much compatibility as possible. That has been the > approach with moving from IOCTL to netlink with ethool. Everything the old > KAPI can do, netlink should also be able to, plus there can be additional > features. > > > > + 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. > > Not with the old KAPI call. As far as i remember, it stops at three pages. But i > need to check the ethtool(1) sources to be sure. > > Andrew
> > --- 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... > > > + 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; Actually, reflecting on Andrew's response, it occurs to me this does not have to be an error. The routine eeprom_data_fallback() (below) could detect this case (a request crossing the 128 byte offset boundary) and create two requests, one for lower memory and one for the paged upper memory. That can't be done as a single request with the linear memory model, but the two pieces can be read separately and glued together. Don
On 3/5/2021 2:50 AM, Don Bollinger wrote: > > On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote: >> From: Vladyslav Tarasiuk <vladyslavt@nvidia.com> >> >> 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 <vladyslavt@nvidia.com> >> --- >> 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... > >> + 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. >> + >> + 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. > >> + 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. >> + break; >> + case ETH_MODULE_SFF_8436: >> + case ETH_MODULE_SFF_8636: > Not sure this is needed, there can be pages higher than 3. > >> + 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. > >> + 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 >
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) +{ + u32 offset = request->offset; + u32 length = request->length; + + if (request->page) + offset = 128 + request->page * 128 + offset; + + 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) + return -EINVAL; + break; + case ETH_MODULE_SFF_8436: + case ETH_MODULE_SFF_8636: + 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) { + 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;