[PATCH ethtool] ethtool: fix EEPROM byte write

[PATCH ethtool] ethtool: fix EEPROM byte write

[Tomasz Moń]

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) {
-- 
2.25.1

Re: [PATCH ethtool] ethtool: fix EEPROM byte write

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ethtool] ethtool: fix EEPROM byte write

[Tomasz Moń]

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ń

Re: [PATCH ethtool] ethtool: fix EEPROM byte write

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]