netdev - Re: [PATCH v1 net] net: dsa: microchip: correct KSZ8795 static MAC table access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230711063020.kmipc2wxsfuwpypz@soft-dev3-1>
Date: Tue, 11 Jul 2023 08:30:20 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: <Tristram.Ha@...rochip.com>
CC: Woojung Huh <woojung.huh@...rochip.com>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>, Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
	<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH v1 net] net: dsa: microchip: correct KSZ8795 static MAC
 table access

The 07/10/2023 17:10, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>

Hi Tristram,

It looks like you forgot again to add all the maintainers to the email
thread. Was it something wrong with the command
./scripts/get_maintainer.pl?

> 
> The KSZ8795 driver code was modified to use on KSZ8863/73, which has
> different register definitions.  Some of the new KSZ8795 register
> information are wrong compared to previous code.
> 
> KSZ8795 also behaves differently in that the STATIC_MAC_TABLE_USE_FID
> and STATIC_MAC_TABLE_FID bits are off by 1 when doing MAC table reading
> than writing.  To compensate that a special code was added to shift the
> register value by 1 before applying those bits.  This is wrong when the
> code is running on KSZ8863, so this special code is only executed when
> KSZ8795 is detected.
> 
> Fixes: 4b20a07e103f ("net: dsa: microchip: ksz8795: add support for ksz88xx chips")
> Signed-off-by: Tristram Ha <Tristram.Ha@...rochip.com>

Other than what I mentioned aboved, it looks OK.
Reviewed-by: Horatiu Vultur <horatiu.vultur@...rochip.com>

> ---
> v1
> - Use correct commit for fixes
> 
>  drivers/net/dsa/microchip/ksz8795.c    | 8 +++++++-
>  drivers/net/dsa/microchip/ksz_common.c | 8 ++++----
>  drivers/net/dsa/microchip/ksz_common.h | 7 +++++++
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 84d502589f8e..91aba470fb2f 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -506,7 +506,13 @@ static int ksz8_r_sta_mac_table(struct ksz_device *dev, u16 addr,
>  		(data_hi & masks[STATIC_MAC_TABLE_FWD_PORTS]) >>
>  			shifts[STATIC_MAC_FWD_PORTS];
>  	alu->is_override = (data_hi & masks[STATIC_MAC_TABLE_OVERRIDE]) ? 1 : 0;
> -	data_hi >>= 1;
> +
> +	/* KSZ8795 family switches have STATIC_MAC_TABLE_USE_FID and
> +	 * STATIC_MAC_TABLE_FID definitions off by 1 when doing read on the
> +	 * static MAC table compared to doing write.
> +	 */
> +	if (ksz_is_ksz87xx(dev))
> +		data_hi >>= 1;
>  	alu->is_static = true;
>  	alu->is_use_fid = (data_hi & masks[STATIC_MAC_TABLE_USE_FID]) ? 1 : 0;
>  	alu->fid = (data_hi & masks[STATIC_MAC_TABLE_FID]) >>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 813b91a816bb..b18cd170ec06 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -331,13 +331,13 @@ static const u32 ksz8795_masks[] = {
>  	[STATIC_MAC_TABLE_VALID]	= BIT(21),
>  	[STATIC_MAC_TABLE_USE_FID]	= BIT(23),
>  	[STATIC_MAC_TABLE_FID]		= GENMASK(30, 24),
> -	[STATIC_MAC_TABLE_OVERRIDE]	= BIT(26),
> -	[STATIC_MAC_TABLE_FWD_PORTS]	= GENMASK(24, 20),
> +	[STATIC_MAC_TABLE_OVERRIDE]	= BIT(22),
> +	[STATIC_MAC_TABLE_FWD_PORTS]	= GENMASK(20, 16),
>  	[DYNAMIC_MAC_TABLE_ENTRIES_H]	= GENMASK(6, 0),
> -	[DYNAMIC_MAC_TABLE_MAC_EMPTY]	= BIT(8),
> +	[DYNAMIC_MAC_TABLE_MAC_EMPTY]	= BIT(7),
>  	[DYNAMIC_MAC_TABLE_NOT_READY]	= BIT(7),
>  	[DYNAMIC_MAC_TABLE_ENTRIES]	= GENMASK(31, 29),
> -	[DYNAMIC_MAC_TABLE_FID]		= GENMASK(26, 20),
> +	[DYNAMIC_MAC_TABLE_FID]		= GENMASK(22, 16),
>  	[DYNAMIC_MAC_TABLE_SRC_PORT]	= GENMASK(26, 24),
>  	[DYNAMIC_MAC_TABLE_TIMESTAMP]	= GENMASK(28, 27),
>  	[P_MII_TX_FLOW_CTRL]		= BIT(5),
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 28444e5924f9..a4de58847dea 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -601,6 +601,13 @@ static inline void ksz_regmap_unlock(void *__mtx)
>  	mutex_unlock(mtx);
>  }
>  
> +static inline bool ksz_is_ksz87xx(struct ksz_device *dev)
> +{
> +	return dev->chip_id == KSZ8795_CHIP_ID ||
> +	       dev->chip_id == KSZ8794_CHIP_ID ||
> +	       dev->chip_id == KSZ8765_CHIP_ID;
> +}
> +
>  static inline bool ksz_is_ksz88x3(struct ksz_device *dev)
>  {
>  	return dev->chip_id == KSZ8830_CHIP_ID;
> -- 
> 2.17.1
> 

-- 
/Horatiu