netdev - Re: [PATCH 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] [day] [month] [year] [list]

Message-ID: <20230627065331.6sxkww4x6ke572kb@soft-dev3-1>
Date: Tue, 27 Jun 2023 08:53:31 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: <Tristram.Ha@...rochip.com>
CC: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
	<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net] net: dsa: microchip: correct KSZ8795 static MAC
 table access

The 06/26/2023 19:33, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>

Hi Tristram,

Please make sure you CC all the maintainers.
You can find out the maintainers by running ./scripts/get_maintainer.pl
on your patch file.

> 
> 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 with 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 is only executed when
> KSZ8795 is detected.
> 
> Fixes: c8e04374f9e1 ("net: dsa: microchip: Make ksz8_w_sta_mac_table() static")

Don't add a new line between Fixes and SoB tags.
Also, are you sure that the blamed commit introduced the issue?
Because that commit just makes ksz8_w_sta_mac_table to be static.

> 
> Signed-off-by: Tristram Ha <Tristram.Ha@...rochip.com>
> ---
>  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 f56fca1b1a22..cc5b19a3d0df 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 a4428be5f483..a0ba2605bb62 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 8abecaf6089e..33d9a2f6af27 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -569,6 +569,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