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

[Michal Kubecek <mkubecek@suse.cz>]

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

On Fri, Aug 19, 2022 at 12:10:49PM +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 setting length to 1 when value is specified and length
> is omitted. Exit with error if length is specified to value other than 1
> and value is specified.
> 
> Fixes: 923c3f51c444 ("ioctl: check presence of eeprom length argument properly")
> Signed-off-by: Tomasz Moń <tomasz.mon@camlingroup.com>
> ---
> changes in v2:
>   - set the length to 1 only if not specified by user
>   - exit with error if length is not 1
> 
> v1: https://lore.kernel.org/netdev/20220819062933.1155112-1-tomasz.mon@camlingroup.com/
> ---
>  ethtool.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 89613ca..7b400da 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3529,12 +3529,16 @@ static int do_seeprom(struct cmd_context *ctx)
>  		return 74;
>  	}
>  
> -	if (seeprom_value_seen)
> +	if (seeprom_value_seen && !seeprom_length_seen)
>  		seeprom_length = 1;
> -
> -	if (!seeprom_length_seen)
> +	else if (!seeprom_length_seen)
>  		seeprom_length = drvinfo.eedump_len;

It would probably look a bit nicer like this:

	if (!seeprom_length_seen)
		seeprom_length = seeprom_value_seen ? 1 : drvinfo.eedump_len;

but that's just matter of taste so let's take it as it is.

Michal

>  
> +	if (seeprom_value_seen && (seeprom_length != 1)) {
> +		fprintf(stderr, "value requires length 1\n");
> +		return 1;
> +	}
> +
>  	if (drvinfo.eedump_len < seeprom_offset + seeprom_length) {
>  		fprintf(stderr, "offset & length out of bounds\n");
>  		return 1;
> -- 
> 2.25.1
> 

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