| Message ID | 20220819062933.1155112-1-tomasz.mon@camlingroup.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool] ethtool: fix EEPROM byte write | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Aug 19, 2022 at 08:29:33AM +0200, Tomasz Moń wrote: > ethtool since version 1.8 supports EEPROM byte write: > # ethtool -E DEVNAME [ magic N ] [ offset N ] [ value N ] > > ethtool 2.6.33 added EEPROM block write: > # ethtool -E ethX [ magic N ] [ offset N ] [ length N ] [ value N ] > > EEPROM block write introduced in 2.6.33 is backwards compatible, i.e. > when value is specified the length is forced to 1 (commandline length > value is ignored). > > The byte write behaviour changed in ethtool 5.9 where the value write > only works when value parameter is specified together with length 1. > While byte writes to any offset other than 0, without length 1, simply > fail with "offset & length out of bounds" error message, writing value > to offset 0 basically erased whole EEPROM. That is, the provided byte > value was written at offset 0, but the rest of the EEPROM was set to 0. > > Fix the issue by forcing length to 1 when value is provided. > > Fixes: 923c3f51c444 ("ioctl: check presence of eeprom length argument properly") > Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com> > --- > ethtool.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/ethtool.c b/ethtool.c > index 89613ca..b9602ce 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -3531,8 +3531,7 @@ static int do_seeprom(struct cmd_context *ctx) > > if (seeprom_value_seen) > seeprom_length = 1; > - > - if (!seeprom_length_seen) > + else if (!seeprom_length_seen) > seeprom_length = drvinfo.eedump_len; > > if (drvinfo.eedump_len < seeprom_offset + seeprom_length) { I don't like the idea of silently ignoring the length parameter if value is used. We should rather issue an error for invalid combination of parameters, i.e. value present and length different from 1. Michal
On Fri, 2022-08-19 at 10:27 +0200, Michal Kubecek wrote: > On Fri, Aug 19, 2022 at 08:29:33AM +0200, Tomasz Moń wrote: > > @@ -3531,8 +3531,7 @@ static int do_seeprom(struct cmd_context *ctx) > > > > if (seeprom_value_seen) > > seeprom_length = 1; > > - > > - if (!seeprom_length_seen) > > + else if (!seeprom_length_seen) > > seeprom_length = drvinfo.eedump_len; > > > > if (drvinfo.eedump_len < seeprom_offset + seeprom_length) { > > I don't like the idea of silently ignoring the length parameter if value > is used. We should rather issue an error for invalid combination of > parameters, i.e. value present and length different from 1. This patch simply restores the way ethtool 2.6.33 - 5.8 worked. Setting length to 1 matches ethtool man page description: > If value is specified, changes EEPROM byte for the specified network device. > offset and value specify which byte and it's new value. What about changing the code to default to length 1 if value is provided without length (so scripts relying on the way ethtool 1.8 up to 5.8 works without any change), but report error if user provides both value and length other than 1? Best Regards, Tomasz Moń
On Fri, Aug 19, 2022 at 10:58:19AM +0200, Tomasz Moń wrote: > On Fri, 2022-08-19 at 10:27 +0200, Michal Kubecek wrote: > > On Fri, Aug 19, 2022 at 08:29:33AM +0200, Tomasz Moń wrote: > > > @@ -3531,8 +3531,7 @@ static int do_seeprom(struct cmd_context *ctx) > > > > > > if (seeprom_value_seen) > > > seeprom_length = 1; > > > - > > > - if (!seeprom_length_seen) > > > + else if (!seeprom_length_seen) > > > seeprom_length = drvinfo.eedump_len; > > > > > > if (drvinfo.eedump_len < seeprom_offset + seeprom_length) { > > > > I don't like the idea of silently ignoring the length parameter if value > > is used. We should rather issue an error for invalid combination of > > parameters, i.e. value present and length different from 1. > > This patch simply restores the way ethtool 2.6.33 - 5.8 worked. Right but when have to touch the code anyway, let's improve the behaviour. > Setting length to 1 matches ethtool man page description: > > If value is specified, changes EEPROM byte for the specified network device. > > offset and value specify which byte and it's new value. > > What about changing the code to default to length 1 if value is > provided without length (so scripts relying on the way ethtool 1.8 up > to 5.8 works without any change), but report error if user provides > both value and length other than 1? Yes, that's exactly what I meant. Using length 1 as default if length is omitted and value present is perfectly fine, what I did not like was the idea of silently adjusting invalid combinations of parameters (value present and length different from 1). Michal
diff --git a/ethtool.c b/ethtool.c index 89613ca..b9602ce 100644 --- a/ethtool.c +++ b/ethtool.c @@ -3531,8 +3531,7 @@ static int do_seeprom(struct cmd_context *ctx) if (seeprom_value_seen) seeprom_length = 1; - - if (!seeprom_length_seen) + else if (!seeprom_length_seen) seeprom_length = drvinfo.eedump_len; if (drvinfo.eedump_len < seeprom_offset + seeprom_length) {
ethtool since version 1.8 supports EEPROM byte write: # ethtool -E DEVNAME [ magic N ] [ offset N ] [ value N ] ethtool 2.6.33 added EEPROM block write: # ethtool -E ethX [ magic N ] [ offset N ] [ length N ] [ value N ] EEPROM block write introduced in 2.6.33 is backwards compatible, i.e. when value is specified the length is forced to 1 (commandline length value is ignored). The byte write behaviour changed in ethtool 5.9 where the value write only works when value parameter is specified together with length 1. While byte writes to any offset other than 0, without length 1, simply fail with "offset & length out of bounds" error message, writing value to offset 0 basically erased whole EEPROM. That is, the provided byte value was written at offset 0, but the rest of the EEPROM was set to 0. Fix the issue by forcing length to 1 when value is provided. Fixes: 923c3f51c444 ("ioctl: check presence of eeprom length argument properly") Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com> --- ethtool.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)