[PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This has been tested on:

1. Luxul XBR-4500 with used CPU port 5
[    8.361438] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 0

2. Netgear R8000 with used CPU port 8
[    4.453858] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 5  

Rafał Miłecki (4):
  net: dsa: b53: Include all ports in "enabled_ports"
  net: dsa: b53: Drop BCM5301x workaround for a wrong CPU/IMP port
  net: dsa: b53: Improve flow control setup on BCM5301x
  net: dsa: b53: Drop unused "cpu_port" field

 drivers/net/dsa/b53/b53_common.c | 59 +++++++-------------------------
 drivers/net/dsa/b53/b53_priv.h   |  1 -
 2 files changed, 12 insertions(+), 48 deletions(-)

-- 
2.26.2

[PATCH net-next 1/4] net: dsa: b53: Include all ports in "enabled_ports"

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Make "enabled_ports" bitfield contain all available switch ports
including a CPU port. This way there is no need for fixup during
initialization.

For BCM53010, BCM53018 and BCM53019 include also other available ports.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/net/dsa/b53/b53_common.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 604f54112665..47a00c5364c7 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2302,7 +2302,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM5325_DEVICE_ID,
 		.dev_name = "BCM5325",
 		.vlans = 16,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x3f,
 		.arl_bins = 2,
 		.arl_buckets = 1024,
 		.imp_port = 5,
@@ -2313,7 +2313,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM5365_DEVICE_ID,
 		.dev_name = "BCM5365",
 		.vlans = 256,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x3f,
 		.arl_bins = 2,
 		.arl_buckets = 1024,
 		.imp_port = 5,
@@ -2324,7 +2324,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM5389_DEVICE_ID,
 		.dev_name = "BCM5389",
 		.vlans = 4096,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x11f,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2338,7 +2338,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM5395_DEVICE_ID,
 		.dev_name = "BCM5395",
 		.vlans = 4096,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x11f,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2352,7 +2352,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM5397_DEVICE_ID,
 		.dev_name = "BCM5397",
 		.vlans = 4096,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x11f,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2366,7 +2366,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM5398_DEVICE_ID,
 		.dev_name = "BCM5398",
 		.vlans = 4096,
-		.enabled_ports = 0x7f,
+		.enabled_ports = 0x17f,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2380,7 +2380,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM53115_DEVICE_ID,
 		.dev_name = "BCM53115",
 		.vlans = 4096,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x11f,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.vta_regs = B53_VTA_REGS,
@@ -2394,7 +2394,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM53125_DEVICE_ID,
 		.dev_name = "BCM53125",
 		.vlans = 4096,
-		.enabled_ports = 0xff,
+		.enabled_ports = 0x1ff,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2436,7 +2436,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM53010_DEVICE_ID,
 		.dev_name = "BCM53010",
 		.vlans = 4096,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x1bf,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2478,7 +2478,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM53018_DEVICE_ID,
 		.dev_name = "BCM53018",
 		.vlans = 4096,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x1bf,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2492,7 +2492,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.chip_id = BCM53019_DEVICE_ID,
 		.dev_name = "BCM53019",
 		.vlans = 4096,
-		.enabled_ports = 0x1f,
+		.enabled_ports = 0x1bf,
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2634,7 +2634,6 @@ static int b53_switch_init(struct b53_device *dev)
 			dev->cpu_port = 5;
 	}
 
-	dev->enabled_ports |= BIT(dev->cpu_port);
 	dev->num_ports = fls(dev->enabled_ports);
 
 	dev->ds->num_ports = min_t(unsigned int, dev->num_ports, DSA_MAX_PORTS);
-- 
2.26.2

[PATCH net-next 2/4] net: dsa: b53: Drop BCM5301x workaround for a wrong CPU/IMP port

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

On BCM5301x port 8 requires a fixed link when used.

Years ago when b53 was an OpenWrt downstream driver (with configuration
based on sometimes bugged NVRAM) there was a need for a fixup. In case
of forcing fixed link for (incorrectly specified) port 5 the code had to
actually setup port 8 link.

For upstream b53 driver with setup based on DT there is no need for that
workaround. In DT we have and require correct ports setup.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/net/dsa/b53/b53_common.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 47a00c5364c7..ca84e32baca0 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1291,12 +1291,6 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
 				return;
 			}
 		}
-	} else if (is5301x(dev)) {
-		if (port != dev->cpu_port) {
-			b53_force_port_config(dev, dev->cpu_port, 2000,
-					      DUPLEX_FULL, true, true);
-			b53_force_link(dev, dev->cpu_port, 1);
-		}
 	}
 
 	/* Re-negotiate EEE if it was enabled already */
-- 
2.26.2

[PATCH net-next 3/4] net: dsa: b53: Improve flow control setup on BCM5301x

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

According to the Broadcom's reference driver flow control needs to be
enabled for any CPU switch port (5, 7 or 8 - depending on which one is
used). Current code makes it work only for the port 5. Use
dsa_is_cpu_port() which solved that problem.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/net/dsa/b53/b53_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index ca84e32baca0..13f337a102ba 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1222,7 +1222,7 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
 		return;
 
 	/* Enable flow control on BCM5301x's CPU port */
-	if (is5301x(dev) && port == dev->cpu_port)
+	if (is5301x(dev) && dsa_is_cpu_port(ds, port))
 		tx_pause = rx_pause = true;
 
 	if (phydev->pause) {
-- 
2.26.2

[PATCH net-next 4/4] net: dsa: b53: Drop unused "cpu_port" field

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

It's set but never used anymore.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/net/dsa/b53/b53_common.c | 28 ----------------------------
 drivers/net/dsa/b53/b53_priv.h   |  1 -
 2 files changed, 29 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 13f337a102ba..06279ba64cc8 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2300,7 +2300,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 2,
 		.arl_buckets = 1024,
 		.imp_port = 5,
-		.cpu_port = B53_CPU_PORT_25,
 		.duplex_reg = B53_DUPLEX_STAT_FE,
 	},
 	{
@@ -2311,7 +2310,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 2,
 		.arl_buckets = 1024,
 		.imp_port = 5,
-		.cpu_port = B53_CPU_PORT_25,
 		.duplex_reg = B53_DUPLEX_STAT_FE,
 	},
 	{
@@ -2322,7 +2320,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2336,7 +2333,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2350,7 +2346,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS_9798,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2364,7 +2359,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS_9798,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2379,7 +2373,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_buckets = 1024,
 		.vta_regs = B53_VTA_REGS,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
 		.jumbo_size_reg = B53_JUMBO_MAX_SIZE,
@@ -2392,7 +2385,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2406,7 +2398,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2420,7 +2411,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS_63XX,
 		.duplex_reg = B53_DUPLEX_STAT_63XX,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK_63XX,
@@ -2434,7 +2424,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT_25, /* TODO: auto detect */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2448,7 +2437,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT_25, /* TODO: auto detect */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2462,7 +2450,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT_25, /* TODO: auto detect */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2476,7 +2463,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT_25, /* TODO: auto detect */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2490,7 +2476,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT_25, /* TODO: auto detect */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2504,7 +2489,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2518,7 +2502,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2533,7 +2516,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 256,
 		.imp_port = 8,
-		.cpu_port = 8, /* TODO: ports 4, 5, 8 */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2547,7 +2529,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2561,7 +2542,6 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.arl_buckets = 256,
 		.imp_port = 8,
-		.cpu_port = B53_CPU_PORT,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,
 		.jumbo_pm_reg = B53_JUMBO_PORT_MASK,
@@ -2587,7 +2567,6 @@ static int b53_switch_init(struct b53_device *dev)
 			dev->vta_regs[2] = chip->vta_regs[2];
 			dev->jumbo_pm_reg = chip->jumbo_pm_reg;
 			dev->imp_port = chip->imp_port;
-			dev->cpu_port = chip->cpu_port;
 			dev->num_vlans = chip->vlans;
 			dev->num_arl_bins = chip->arl_bins;
 			dev->num_arl_buckets = chip->arl_buckets;
@@ -2619,13 +2598,6 @@ static int b53_switch_init(struct b53_device *dev)
 			break;
 #endif
 		}
-	} else if (dev->chip_id == BCM53115_DEVICE_ID) {
-		u64 strap_value;
-
-		b53_read48(dev, B53_STAT_PAGE, B53_STRAP_VALUE, &strap_value);
-		/* use second IMP port if GMII is enabled */
-		if (strap_value & SV_GMII_CTRL_115)
-			dev->cpu_port = 5;
 	}
 
 	dev->num_ports = fls(dev->enabled_ports);
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 5d068acf7cf8..5be8fe000037 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -124,7 +124,6 @@ struct b53_device {
 	/* used ports mask */
 	u16 enabled_ports;
 	unsigned int imp_port;
-	unsigned int cpu_port;
 
 	/* connect specific data */
 	u8 current_page;
-- 
2.26.2

Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Florian Fainelli]

On 9/16/21 5:03 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This has been tested on:
> 
> 1. Luxul XBR-4500 with used CPU port 5
> [    8.361438] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 0
> 
> 2. Netgear R8000 with used CPU port 8
> [    4.453858] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 5  

These look good at first glance, let me give them a try on 7445 and 7278
at least before responding with Reviewed-by/Tested-by tags, thanks!
-- 
Florian

Re: [PATCH net-next 4/4] net: dsa: b53: Drop unused "cpu_port" field

[Florian Fainelli]

On 9/16/21 5:03 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> It's set but never used anymore.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 2/4] net: dsa: b53: Drop BCM5301x workaround for a wrong CPU/IMP port

[Florian Fainelli]

On 9/16/21 5:03 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> On BCM5301x port 8 requires a fixed link when used.
> 
> Years ago when b53 was an OpenWrt downstream driver (with configuration
> based on sometimes bugged NVRAM) there was a need for a fixup. In case
> of forcing fixed link for (incorrectly specified) port 5 the code had to
> actually setup port 8 link.
> 
> For upstream b53 driver with setup based on DT there is no need for that
> workaround. In DT we have and require correct ports setup.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 3/4] net: dsa: b53: Improve flow control setup on BCM5301x

[Florian Fainelli]

On 9/16/21 5:03 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> According to the Broadcom's reference driver flow control needs to be
> enabled for any CPU switch port (5, 7 or 8 - depending on which one is
> used). Current code makes it work only for the port 5. Use
> dsa_is_cpu_port() which solved that problem.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 1/4] net: dsa: b53: Include all ports in "enabled_ports"

[Florian Fainelli]

On 9/16/21 5:03 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Make "enabled_ports" bitfield contain all available switch ports
> including a CPU port. This way there is no need for fixup during
> initialization.
> 
> For BCM53010, BCM53018 and BCM53019 include also other available ports.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Florian Fainelli]

On 9/16/21 9:23 AM, Florian Fainelli wrote:
> On 9/16/21 5:03 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This has been tested on:
>>
>> 1. Luxul XBR-4500 with used CPU port 5
>> [    8.361438] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 0
>>
>> 2. Netgear R8000 with used CPU port 8
>> [    4.453858] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 5  
> 
> These look good at first glance, let me give them a try on 7445 and 7278
> at least before responding with Reviewed-by/Tested-by tags, thanks!
> 
Found some issues on 7445 and 7278 while moving to the latest net-next
which I will be addressing but this worked nicely.

What do you think about removing dev->enabled_ports and
b53_for_each_port entirely and using a DSA helper that iterates over the
switch's port list? Now that we have dev->num_ports accurately reflect
the number of ports it should be equivalent.
-- 
Florian

Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Rafał Miłecki]

On 16.09.2021 23:46, Florian Fainelli wrote:
> On 9/16/21 9:23 AM, Florian Fainelli wrote:
>> On 9/16/21 5:03 AM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> This has been tested on:
>>>
>>> 1. Luxul XBR-4500 with used CPU port 5
>>> [    8.361438] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 0
>>>
>>> 2. Netgear R8000 with used CPU port 8
>>> [    4.453858] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 5
>>
>> These look good at first glance, let me give them a try on 7445 and 7278
>> at least before responding with Reviewed-by/Tested-by tags, thanks!
>>
> Found some issues on 7445 and 7278 while moving to the latest net-next
> which I will be addressing but this worked nicely.
> 
> What do you think about removing dev->enabled_ports and
> b53_for_each_port entirely and using a DSA helper that iterates over the
> switch's port list? Now that we have dev->num_ports accurately reflect
> the number of ports it should be equivalent.

The limitation I see in DSA is skipping unavailable ports. E.g. BCM5301x
switches that don't have port 6. The closest match for such case I found
is DSA_PORT_TYPE_UNUSED but I'm not sure if it's enough to handle those
cases.

That DSA_PORT_TYPE_UNUSED would probably require investigating DSA & b53
behaviour *and* discussing it with DSA maintainer to make sure we don't
abuse that.

Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Jakub Kicinski]

On Thu, 16 Sep 2021 14:03:50 +0200 Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This has been tested on:
> 
> 1. Luxul XBR-4500 with used CPU port 5
> [    8.361438] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 0
> 
> 2. Netgear R8000 with used CPU port 8
> [    4.453858] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 5  

Applied, thanks!

Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Vladimir Oltean]

On Fri, Sep 17, 2021 at 12:19:02AM +0200, Rafał Miłecki wrote:
> On 16.09.2021 23:46, Florian Fainelli wrote:
> > On 9/16/21 9:23 AM, Florian Fainelli wrote:
> > > On 9/16/21 5:03 AM, Rafał Miłecki wrote:
> > > > From: Rafał Miłecki <rafal@milecki.pl>
> > > > 
> > > > This has been tested on:
> > > > 
> > > > 1. Luxul XBR-4500 with used CPU port 5
> > > > [    8.361438] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 0
> > > > 
> > > > 2. Netgear R8000 with used CPU port 8
> > > > [    4.453858] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 5
> > > 
> > > These look good at first glance, let me give them a try on 7445 and 7278
> > > at least before responding with Reviewed-by/Tested-by tags, thanks!
> > > 
> > Found some issues on 7445 and 7278 while moving to the latest net-next
> > which I will be addressing but this worked nicely.
> > 
> > What do you think about removing dev->enabled_ports and
> > b53_for_each_port entirely and using a DSA helper that iterates over the
> > switch's port list? Now that we have dev->num_ports accurately reflect
> > the number of ports it should be equivalent.
> 
> The limitation I see in DSA is skipping unavailable ports. E.g. BCM5301x
> switches that don't have port 6. The closest match for such case I found
> is DSA_PORT_TYPE_UNUSED but I'm not sure if it's enough to handle those
> cases.
> 
> That DSA_PORT_TYPE_UNUSED would probably require investigating DSA & b53
> behaviour *and* discussing it with DSA maintainer to make sure we don't
> abuse that.

How absent are these ports in hardware? For DSA_PORT_TYPE_UNUSED we do
register a devlink port, but if those ports are really not present in
hardware, I'm thinking maybe the easiest way would be to supply a
ds->disabled_port_mask before dsa_register_switch(), and DSA will simply
skip those ports when allocating the dp, the devlink_port etc. So you
will literally have nothing for them.

Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Andrew Lunn]

> > That DSA_PORT_TYPE_UNUSED would probably require investigating DSA & b53
> > behaviour *and* discussing it with DSA maintainer to make sure we don't
> > abuse that.
> 
> How absent are these ports in hardware? For DSA_PORT_TYPE_UNUSED we do
> register a devlink port, but if those ports are really not present in
> hardware, I'm thinking maybe the easiest way would be to supply a
> ds->disabled_port_mask before dsa_register_switch(), and DSA will simply
> skip those ports when allocating the dp, the devlink_port etc. So you
> will literally have nothing for them.

The basic idea seems O.K, we just need to be careful.

We have code like:

static inline bool dsa_is_dsa_port(struct dsa_switch *ds, int p)
{
	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_DSA;
}

static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
{
	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_USER;
}

dsa_to_port(ds, p) will return NULL, and then bad things will happen.

Maybe it would be safer to add DSA_PORT_TYPE_PHANTOM and do allocate
the dp?

    Andrew

Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Vladimir Oltean]

On Fri, Sep 17, 2021 at 02:21:16PM +0200, Andrew Lunn wrote:
> > > That DSA_PORT_TYPE_UNUSED would probably require investigating DSA & b53
> > > behaviour *and* discussing it with DSA maintainer to make sure we don't
> > > abuse that.
> >
> > How absent are these ports in hardware? For DSA_PORT_TYPE_UNUSED we do
> > register a devlink port, but if those ports are really not present in
> > hardware, I'm thinking maybe the easiest way would be to supply a
> > ds->disabled_port_mask before dsa_register_switch(), and DSA will simply
> > skip those ports when allocating the dp, the devlink_port etc. So you
> > will literally have nothing for them.
>
> The basic idea seems O.K, we just need to be careful.
>
> We have code like:
>
> static inline bool dsa_is_dsa_port(struct dsa_switch *ds, int p)
> {
> 	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_DSA;
> }
>
> static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
> {
> 	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_USER;
> }
>
> dsa_to_port(ds, p) will return NULL, and then bad things will happen.
>
> Maybe it would be safer to add DSA_PORT_TYPE_PHANTOM and do allocate
> the dp?

I think DSA is not yet large enough for us to take the defeatist
position of inserting bits of code that simply protect from other bits
of code.

Especially after these patches:
https://patchwork.kernel.org/project/netdevbpf/cover/20210810161448.1879192-1-vladimir.oltean@nxp.com/
the DSA core should basically never iterate over ports using
	for (port = 0; port < ds->num_ports; port++)
but instead using
	dsa_switch_for_each_port(dp, ds)
which basically ensures that we never dereference dsa_to_port(ds, p) for
a p with no associated dp (the "dp" from "dsa_switch_for_each_port" is
taken from a list).

Conversely, driver code is "mostly" event-driven, so DSA should only call
dsa_switch_ops methods for ports where the dp structure does exist.
What we would need to audit are just the drivers which set ds->disabled_port_mask
(b53, maybe ksz switches?). I guess that would be on the developer who
makes the change in those respective drivers to ensure that the "dp"
which is searched for does in fact exist.

My 2 cents anyway, I might be missing some trickier cases.

Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

[Florian Fainelli]

On 9/17/21 3:00 AM, Vladimir Oltean wrote:
> On Fri, Sep 17, 2021 at 12:19:02AM +0200, Rafał Miłecki wrote:
>> On 16.09.2021 23:46, Florian Fainelli wrote:
>>> On 9/16/21 9:23 AM, Florian Fainelli wrote:
>>>> On 9/16/21 5:03 AM, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> This has been tested on:
>>>>>
>>>>> 1. Luxul XBR-4500 with used CPU port 5
>>>>> [    8.361438] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 0
>>>>>
>>>>> 2. Netgear R8000 with used CPU port 8
>>>>> [    4.453858] b53-srab-switch 18007000.ethernet-switch: found switch: BCM53012, rev 5
>>>>
>>>> These look good at first glance, let me give them a try on 7445 and 7278
>>>> at least before responding with Reviewed-by/Tested-by tags, thanks!
>>>>
>>> Found some issues on 7445 and 7278 while moving to the latest net-next
>>> which I will be addressing but this worked nicely.
>>>
>>> What do you think about removing dev->enabled_ports and
>>> b53_for_each_port entirely and using a DSA helper that iterates over the
>>> switch's port list? Now that we have dev->num_ports accurately reflect
>>> the number of ports it should be equivalent.
>>
>> The limitation I see in DSA is skipping unavailable ports. E.g. BCM5301x
>> switches that don't have port 6. The closest match for such case I found
>> is DSA_PORT_TYPE_UNUSED but I'm not sure if it's enough to handle those
>> cases.
>>
>> That DSA_PORT_TYPE_UNUSED would probably require investigating DSA & b53
>> behaviour *and* discussing it with DSA maintainer to make sure we don't
>> abuse that.
> 
> How absent are these ports in hardware? For DSA_PORT_TYPE_UNUSED we do
> register a devlink port, but if those ports are really not present in
> hardware, I'm thinking maybe the easiest way would be to supply a
> ds->disabled_port_mask before dsa_register_switch(), and DSA will simply
> skip those ports when allocating the dp, the devlink_port etc. So you
> will literally have nothing for them.
> 

Port 6 on all of the newer switches where the "ideal" IMP port is 8 is
completely absent and does not exist at all as a hardware resource. The
registers are not necessarily consistent however and you typically see
two patterns:

- specifying bit 6 or port 6 as a numerical port does not nothing and no
special casing is required, this is the majority of the registers and
the maximum supported bitmask is 0x1ff and you can also set bit 8 to
address port 8 of the CPU (I would say this is intuitive)

- specifying bit 6/number 6 may alias to port 7, this is the case with
the CFP code for instance that specifically checks for port >= 7 and
subtracts one when needed (this is not intuitive)

Whether we truly consider a port being absent from a port being unused
is not probably making much difference from a semantic perspective as
long as you do not try to switch a port from unused to used (whether it
is DSA, CPU or USER). This is not an use case we support today, but if
we did in the future (say in the context of multi CPU port devices), we
would have to call back into the driver most likely and the driver could
veto changing that port from unused to used. What do you think?

NB: the enabled_port mask for the 7278 and 7445 switches is currently
incorrectly advertising the presence of port 6 (0x1ff), that needs fixing.
-- 
Florian