netdev - Re: [PATCH] net: dsa: b53: Fix IMP port setup on BCM5301x

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: <05523f76-6846-449e-bc66-5f4d15946ad5@gmail.com>
Date:   Sun, 5 Sep 2021 14:16:00 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rafał Miłecki <zajec5@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH] net: dsa: b53: Fix IMP port setup on BCM5301x



On 9/5/2021 10:23 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@...ecki.pl>
> 
> Broadcom's b53 switches have one IMP (Inband Management Port) that needs
> to be programmed using its own designed register. IMP port may be
> different than CPU port - especially on devices with multiple CPU ports.

There are two choices: port 5 or port 8,

> 
> For that reason it's required to explicitly note IMP port index and
> check for it when choosing a register to use.
> 
> This commit fixes BCM5301x support. Those switches use CPU port 5 while
> their IMP port is 8. Before this patch b53 was trying to program port 5
> with B53_PORT_OVERRIDE_CTRL instead of B53_GMII_PORT_OVERRIDE_CTRL(5).
> 
> It may be possible to also replace "cpu_port" usages with
> dsa_is_cpu_port() but that is out of the scope of thix BCM5301x fix.

Actually this would have been well within the scope of this patch.

> 
> Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>

I really don't like the duplication of the "imp_port" and "cpu_port" 
members, first because this caused us problems before, and second 
because for all switch entries except the BCM5301X, cpu_port == 
imp_port, so this a duplication, and a waste of storage space to encode 
information.

In fact, there is no such thing as CPU port technically you chose either 
IMP0 or IMP1. IMP0 is port 8 and IMP1 is port 5.

> ---
>   drivers/net/dsa/b53/b53_common.c | 28 +++++++++++++++++++++++++---
>   drivers/net/dsa/b53/b53_priv.h   |  1 +
>   2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 5646eb8afe38..604f54112665 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1144,7 +1144,7 @@ static void b53_force_link(struct b53_device *dev, int port, int link)
>   	u8 reg, val, off;
>   
>   	/* Override the port settings */
> -	if (port == dev->cpu_port) {
> +	if (port == dev->imp_port) {

This should be port == 8

>   		off = B53_PORT_OVERRIDE_CTRL;
>   		val = PORT_OVERRIDE_EN;
>   	} else {
> @@ -1168,7 +1168,7 @@ static void b53_force_port_config(struct b53_device *dev, int port,
>   	u8 reg, val, off;
>   
>   	/* Override the port settings */
> -	if (port == dev->cpu_port) {
> +	if (port == dev->imp_port) {

Likewise

>   		off = B53_PORT_OVERRIDE_CTRL;
>   		val = PORT_OVERRIDE_EN;
>   	} else {
> @@ -1236,7 +1236,7 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
>   	b53_force_link(dev, port, phydev->link);
>   
>   	if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
> -		if (port == 8)
> +		if (port == dev->imp_port)

That use of port 8 was correct.

>   			off = B53_RGMII_CTRL_IMP;
>   		else
>   			off = B53_RGMII_CTRL_P(port);
> @@ -2280,6 +2280,7 @@ struct b53_chip_data {
>   	const char *dev_name;
>   	u16 vlans;
>   	u16 enabled_ports;
> +	u8 imp_port;
>   	u8 cpu_port;
>   	u8 vta_regs[3];
>   	u8 arl_bins;
> @@ -2304,6 +2305,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
>   		.enabled_ports = 0x1f,
>   		.arl_bins = 2,
>   		.arl_buckets = 1024,
> +		.imp_port = 5,

Could have used B53_CPU_PORT_25 here.

>   		.cpu_port = B53_CPU_PORT_25,
>   		.duplex_reg = B53_DUPLEX_STAT_FE,
>   	},
> @@ -2314,6 +2316,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
>   		.enabled_ports = 0x1f,
>   		.arl_bins = 2,
>   		.arl_buckets = 1024,
> +		.imp_port = 5,
>   		.cpu_port = B53_CPU_PORT_25,

and here.

>   		.duplex_reg = B53_DUPLEX_STAT_FE,
>   	},
> @@ -2324,6 +2327,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
>   		.enabled_ports = 0x1f,
>   		.arl_bins = 4,
>   		.arl_buckets = 1024,
> +		.imp_port = 8,
>   		.cpu_port = B53_CPU_PORT,

and B53_CPU_PORT here and for each entry below.

I will put this patch into my local test rack and see what breaks, and 
we can address this more cleanly with net-next. Another case where if we 
had more time to do a proper review we could come up with a small fix, 
and not create additional technical debt to fix in the next release 
cycle. Hope's spring is eternal, oh and I just came back from France, so 
I guess I am full of complaints, too :)
--
Florian