Re: [PATCH] net/ibm/emac: fix size of emac dump memory areas

[Ben Hutchings]

On Thu, 2015-05-21 at 19:11 +0400, Ivan Mikhaylov wrote:
> Fix in send of emac regs dump to ethtool which
> causing in wrong data interpretation on ethtool
> layer for MII and EMAC.
> 
> Signed-off-by: Ivan Mikhaylov <ivan@xxxxxxxxxx>

Although this enables an improvement for EMAC4SYNC, this is a regression
for the EMAC and EMAC4 hardware versions.  The driver now reads
registers at undefined addresses and produces a register dump that is
incompatible with existing ethtool releases.

Although you've tried to change ethtool to match this, the kernel and
ethtool don't generally get upgraded in lockstep so the driver should
keep working with old versions of ethtool if possible (which it is).

The size of the MAC register dumps for EMAC and EMAC4 should continue to
be what ethtool assumes they are now - 112 and 116 bytes respectively.
Please fix this.

Ben.

> ---
>  drivers/net/ethernet/ibm/emac/core.c |   16 ++++++----------
>  drivers/net/ethernet/ibm/emac/core.h |    7 ++-----
>  2 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index de79193..b9df0cb 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -2084,12 +2084,8 @@ static void emac_ethtool_get_pauseparam(struct net_device *ndev,
>  
>  static int emac_get_regs_len(struct emac_instance *dev)
>  {
> -	if (emac_has_feature(dev, EMAC_FTR_EMAC4))
> -		return sizeof(struct emac_ethtool_regs_subhdr) +
> -			EMAC4_ETHTOOL_REGS_SIZE(dev);
> -	else
>  		return sizeof(struct emac_ethtool_regs_subhdr) +
> -			EMAC_ETHTOOL_REGS_SIZE(dev);
> +			sizeof(struct emac_regs);
>  }
>  
>  static int emac_ethtool_get_regs_len(struct net_device *ndev)
> @@ -2114,15 +2110,15 @@ static void *emac_dump_regs(struct emac_instance *dev, void *buf)
>  	struct emac_ethtool_regs_subhdr *hdr = buf;
>  
>  	hdr->index = dev->cell_index;
> -	if (emac_has_feature(dev, EMAC_FTR_EMAC4)) {
> +	if (emac_has_feature(dev, EMAC_FTR_EMAC4SYNC)) {
> +		hdr->version = EMAC4SYNC_ETHTOOL_REGS_VER;
> +	} else if (emac_has_feature(dev, EMAC_FTR_EMAC4)) {
>  		hdr->version = EMAC4_ETHTOOL_REGS_VER;
> -		memcpy_fromio(hdr + 1, dev->emacp, EMAC4_ETHTOOL_REGS_SIZE(dev));
> -		return (void *)(hdr + 1) + EMAC4_ETHTOOL_REGS_SIZE(dev);
>  	} else {
>  		hdr->version = EMAC_ETHTOOL_REGS_VER;
> -		memcpy_fromio(hdr + 1, dev->emacp, EMAC_ETHTOOL_REGS_SIZE(dev));
> -		return (void *)(hdr + 1) + EMAC_ETHTOOL_REGS_SIZE(dev);
>  	}
> +	memcpy_fromio(hdr + 1, dev->emacp, sizeof(struct emac_regs));
> +	return (void *)(hdr + 1) + sizeof(struct emac_regs);
>  }
>  
>  static void emac_ethtool_get_regs(struct net_device *ndev,
> diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
> index 67f342a..28df374 100644
> --- a/drivers/net/ethernet/ibm/emac/core.h
> +++ b/drivers/net/ethernet/ibm/emac/core.h
> @@ -461,10 +461,7 @@ struct emac_ethtool_regs_subhdr {
>  };
>  
>  #define EMAC_ETHTOOL_REGS_VER		0
> -#define EMAC_ETHTOOL_REGS_SIZE(dev) 	((dev)->rsrc_regs.end - \
> -					 (dev)->rsrc_regs.start + 1)
> -#define EMAC4_ETHTOOL_REGS_VER      	1
> -#define EMAC4_ETHTOOL_REGS_SIZE(dev)	((dev)->rsrc_regs.end -	\
> -					 (dev)->rsrc_regs.start + 1)
> +#define EMAC4_ETHTOOL_REGS_VER		1
> +#define EMAC4SYNC_ETHTOOL_REGS_VER	2
>  
>  #endif /* __IBM_NEWEMAC_CORE_H */

-- 
Ben Hutchings
Reality is just a crutch for people who can't handle science fiction.

Attachment: signature.asc
Description: This is a digitally signed message part