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

From: Rafał Miłecki <rafal@milecki.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.

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.

Fixes: 967dd82ffc52 ("net: dsa: b53: Add support for Broadcom RoboSwitch")
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, 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) {
 		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) {
 		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)
 			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,
 		.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,
 		.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,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,

@@ -2337,6 +2341,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,
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,

@@ -2350,6 +2355,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,
 		.vta_regs = B53_VTA_REGS_9798,
 		.duplex_reg = B53_DUPLEX_STAT_GE,

@@ -2363,6 +2369,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x7f,
 		.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,

@@ -2377,6 +2384,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.arl_bins = 4,
 		.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,

@@ -2389,6 +2397,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0xff,
 		.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,

@@ -2402,6 +2411,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x1ff,
 		.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,

@@ -2415,6 +2425,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0, /* pdata must provide them */
 		.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,

@@ -2428,6 +2439,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_25, /* TODO: auto detect */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,

@@ -2441,6 +2453,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x1bf,
 		.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,

@@ -2454,6 +2467,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x1bf,
 		.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,

@@ -2467,6 +2481,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_25, /* TODO: auto detect */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,

@@ -2480,6 +2495,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_25, /* TODO: auto detect */
 		.vta_regs = B53_VTA_REGS,
 		.duplex_reg = B53_DUPLEX_STAT_GE,

@@ -2493,6 +2509,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x1ff,
 		.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,

@@ -2506,6 +2523,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x103,
 		.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,

@@ -2520,6 +2538,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x1bf,
 		.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,

@@ -2533,6 +2552,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x1ff,
 		.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,

@@ -2546,6 +2566,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.enabled_ports = 0x1ff,
 		.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,

@@ -2571,6 +2592,7 @@ static int b53_switch_init(struct b53_device *dev)
 			dev->vta_regs[1] = chip->vta_regs[1];
 			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;

diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 9bf8319342b0..5d068acf7cf8 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h

@@ -123,6 +123,7 @@ struct b53_device {
 
 	/* used ports mask */
 	u16 enabled_ports;
+	unsigned int imp_port;
 	unsigned int cpu_port;
 
 	/* connect specific data */

-- 
2.26.2

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Sun,  5 Sep 2021 19:23:28 +0200 you wrote:

From: Rafał Miłecki <rafal@milecki.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.

For that reason it's required to explicitly note IMP port index and
check for it when choosing a register to use.

[...]

Here is the summary with links:
  - net: dsa: b53: Fix IMP port setup on BCM5301x
    https://git.kernel.org/netdev/net/c/63f8428b4077

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On 9/5/2021 11:10 AM, patchwork-bot+netdevbpf@kernel.org wrote:

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Sun,  5 Sep 2021 19:23:28 +0200 you wrote:

quoted

From: Rafał Miłecki <rafal@milecki.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.

For that reason it's required to explicitly note IMP port index and
check for it when choosing a register to use.

[...]

Here is the summary with links:
   - net: dsa: b53: Fix IMP port setup on BCM5301x
     https://git.kernel.org/netdev/net/c/63f8428b4077

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

David, can you please wait more than 1h 47 minutes before applying a 
patch to give a review? This is absolutely not the way this should have 
been fixed because it adds to the driver's port information burden 
rather than not.

This is not the first time this has happened, and this is really really 
starting to annoy the crap out of me. While I am appreciative of your 
responsiveness in applying patches, I am definitively not when it comes 
to not allowing a proper review to happen. So please, I am begging you, 
wait at least 12h, ideally 24h before applying a patch. You have 
patchwork, you have responsive maintainers, so nothing will get dropped 
on the floor.

Thank you

PS: for some reason Rafal's email address got turned into: "Rafał 
Miłecki [off-list ref]"@ci.codeaurora.org. You might want to look 
into that as well.
-- 
Florian

On 9/5/2021 10:23 AM, Rafał Miłecki wrote:

From: Rafał Miłecki <rafal@milecki.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@milecki.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.

This should be port == 8

Likewise

That use of port 8 was correct.

Could have used B53_CPU_PORT_25 here.

and here.

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

On 05.09.2021 23:04, Florian Fainelli wrote:

On 9/5/2021 11:10 AM, patchwork-bot+netdevbpf@kernel.org wrote:

quoted

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Sun,  5 Sep 2021 19:23:28 +0200 you wrote:

quoted

From: Rafał Miłecki <rafal@milecki.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.

For that reason it's required to explicitly note IMP port index and
check for it when choosing a register to use.

[...]

Here is the summary with links:
   - net: dsa: b53: Fix IMP port setup on BCM5301x
     https://git.kernel.org/netdev/net/c/63f8428b4077

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

David, can you please wait more than 1h 47 minutes before applying a patch to give a review? This is absolutely not the way this should have been fixed because it adds to the driver's port information burden rather than not.

This is not the first time this has happened, and this is really really starting to annoy the crap out of me. While I am appreciative of your responsiveness in applying patches, I am definitively not when it comes to not allowing a proper review to happen. So please, I am begging you, wait at least 12h, ideally 24h before applying a patch. You have patchwork, you have responsive maintainers, so nothing will get dropped on the floor.

I was also surprised a bit with that quick apply. I prefer to have my
code reviewed properly.

I'm OK with a revert and working on a better fix (or change for
net-next) if that is a valid option. I can also work on fixing that fix
as I surely don't mean to leave code as is when maintainer isn't happy
about it.

I was also surprised a bit with that quick apply. I prefer to have my
code reviewed properly.

A few people are posting with RFC in the subject, to slow down the
merge and give people a chance to actually comment. This is not how it
should be, but that does appear to at least work.

       Andrew

not allowing a proper review to happen. So please, I am begging you, wait at
least 12h, ideally 24h before applying a patch.

24 hours is too short. We all have lives outside of the kernel. I
found the older policy of 3 days worked well. Enough time for those
who had interest to do a review, but short enough to not really slow
down development. And 3 days is still probably faster than any other
subsystem.

     Andrew

On 05.09.2021 23:16, Florian Fainelli wrote:

On 9/5/2021 10:23 AM, Rafał Miłecki wrote:

quoted

From: Rafał Miłecki <rafal@milecki.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,

Yes. Depending on model I assign 5 or 8 in the b53_chip_data. What did
I miss?

quoted

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.

I guess it's a matter of taste, I prefer to remove "cpu_port" usage
piece by piece. I think it makes it easier to catch mistakes during
review and regression finding easier.

For example I'm not exactly sure how to get rid of "cpu_port" in the:

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);
}

quoted

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

Well, this isn't exactly a duplication as values differ for BCM5301X.

I guess you prefer to handle BCM5301X with extra conditions in code
while I thought it to be cleaner to store that chip data in a struct.

Let's discuss code changes and see how it could be handled differently.

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.

quoted

---
  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

What about devices that have IMP port 5? I think that change would break
b53_force_link() for them.

quoted

          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

Same question.

quoted

          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.

I tried to avoid some magic.

quoted

              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.

I didn't use B53_CPU_PORT* defines as they don't apply anymore. That _25
suffix made sense when support for first devices with CPU/IMP port 5 was
added. They were actually BCM*25 chipsets.

I think we should probably have something like B53_IMP0 and B53_IMP1.
What do you think about proposed names?

quoted

          .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.

quoted

          .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 :)

You should have visit Disneyland! ;)

On Mon, 6 Sep 2021 02:48:34 +0200 Andrew Lunn wrote:

quoted

not allowing a proper review to happen. So please, I am begging you, wait at
least 12h, ideally 24h before applying a patch.  

The fixed wait time before applying would likely require more nuance.
For example something like 0h for build fixed; 12h if reviewed by all
area experts; 24h+ for the rest? Not counting weekends?

24 hours is too short. We all have lives outside of the kernel. I
found the older policy of 3 days worked well. Enough time for those
who had interest to do a review, but short enough to not really slow
down development. And 3 days is still probably faster than any other
subsystem.

It is deeply unsatisfying tho to be waiting for reviews 3 days, ping
people and then have to apply the patch anyway based on one's own
judgment. I personally dislike the uncertainty of silently waiting. 
I have floated the idea before, perhaps it's not taken seriously given
speed of patchwork development, but would it be okay to have a strict
time bound and then require people to mark patches in patchwork as 
"I'm planning to review this"?

Right now we make some attempts to delegate to "Needs ACK" state but
with mixed result (see the two patches hanging in that state now).

Perhaps the "Plan to review" marking in pw is also putting the cart
before the horse (quite likely, knowing my project management prowess.)
Either way if we're expending brain cycles on process changes it would
be cool to think more broadly than just "how long to set a timer for".

On 07.09.2021 03:48, Jakub Kicinski wrote:

On Mon, 6 Sep 2021 02:48:34 +0200 Andrew Lunn wrote:

quoted

quoted

not allowing a proper review to happen. So please, I am begging you, wait at
least 12h, ideally 24h before applying a patch.

The fixed wait time before applying would likely require more nuance.
For example something like 0h for build fixed; 12h if reviewed by all
area experts; 24h+ for the rest? Not counting weekends?

As a maintainer I'd probably prefer 48 hours with 24 h being a minimum.

quoted

24 hours is too short. We all have lives outside of the kernel. I
found the older policy of 3 days worked well. Enough time for those
who had interest to do a review, but short enough to not really slow
down development. And 3 days is still probably faster than any other
subsystem.

It is deeply unsatisfying tho to be waiting for reviews 3 days, ping
people and then have to apply the patch anyway based on one's own
judgment. I personally dislike the uncertainty of silently waiting.
I have floated the idea before, perhaps it's not taken seriously given
speed of patchwork development, but would it be okay to have a strict
time bound and then require people to mark patches in patchwork as
"I'm planning to review this"?

Right now we make some attempts to delegate to "Needs ACK" state but
with mixed result (see the two patches hanging in that state now).

Perhaps the "Plan to review" marking in pw is also putting the cart
before the horse (quite likely, knowing my project management prowess.)
Either way if we're expending brain cycles on process changes it would
be cool to think more broadly than just "how long to set a timer for".

"Needs ACK" sounds good for a state where net maintainers need code
maintainer ack/review.

Do you already have some special meaning for the "Under Review"? That
sounds (compared to the "New") like someone actually planning to (n)ack
a patch.

On Mon, Sep 06, 2021 at 06:48:38PM -0700, Jakub Kicinski wrote:

On Mon, 6 Sep 2021 02:48:34 +0200 Andrew Lunn wrote:

quoted

quoted

not allowing a proper review to happen. So please, I am begging you, wait at
least 12h, ideally 24h before applying a patch.  

The fixed wait time before applying would likely require more nuance.
For example something like 0h for build fixed; 12h if reviewed by all
area experts; 24h+ for the rest? Not counting weekends?

quoted

24 hours is too short. We all have lives outside of the kernel. I
found the older policy of 3 days worked well. Enough time for those
who had interest to do a review, but short enough to not really slow
down development. And 3 days is still probably faster than any other
subsystem.

It is deeply unsatisfying tho to be waiting for reviews 3 days, ping
people and then have to apply the patch anyway based on one's own
judgment.

I would skip the ping bit and just apply it. Unless you really think
it needs a deep review.

Right now we make some attempts to delegate to "Needs ACK" state but
with mixed result (see the two patches hanging in that state now).

Perhaps the "Plan to review" marking in pw is also putting the cart
before the horse.

For me personally, the reason i like three days is that sometimes i'm
away in the wilderness, visiting friends, away on business, etc. I'm
not checking emails. So having to take some sort of action to say i'm
not going to take any action until later, just defeaters the point.

Either way if we're expending brain cycles on process changes it would
be cool to think more broadly than just "how long to set a timer for".

One observation is that netdev drivers are very siloed. A vendor
driver rarely gets reviewed by another vendor. I think reviews are
mostly made by vendor neutral people, and they look for specific
things. I'm interested in phy, ethtool, and anything which looks like
it could be adding a new KAPI of some sort. There are people who
trigger on the keyword XDP, or BPF, devlink etc. Some areas of netdev
do tend to get more reviews than others. Again, my areas of interest,
phys, DSA, ethtool. The core stack gets reviewed by Eric, routing by
David, and i'm sure there are others in parts i don't take an interest
and i've not noticed.

If a patch is from somebody who is not the maintainer of a siloed
driver, and the maintainer is active, then a review is more likely to
happen.

So some sort of model could be made of this, to predict if a patchset
is likely to get reviewed, if time is allowed for the reviewer to
actually do the review. I don't know if a mental model is sufficient,
for the two people who are merging patches, or some tools could be
produced to help a little. Maybe just simple things like, is the
poster of the patch series the maintain of the driver it applies
to. Maybe some keyword matching? Maybe Sasha Levin can run a machine
learning system over the last few years of the netdev archive to train
a model which will product if a patchset will get reviewed? That would
be applicable outside of netdev, it could be a useful feature in
general. Maybe it is a research project Julia Lawall at Inria could
give to a PhD student?

  Andrew

On Tue, 7 Sep 2021 07:35:09 +0200 Rafał Miłecki wrote:

Do you already have some special meaning for the "Under Review"? That
sounds (compared to the "New") like someone actually planning to (n)ack
a patch.

Under Review means patch has passed the basic triage and will be applied
to netdev trees unless instructed otherwise. Practically speaking it's 
not expected to go via any sub-tree and the patchwork build bot 
results look sane.

At least that's what it used to mean, with the advances in automatic
delegation it seems that Dave is using the state less these days, I
still follow the old protocol.

Sub-maintainers in netdev are historically asked not to change
patchwork state. The review delegation does not really work great for
netdev, since there can only be one delegate and we use it to split
user space tools vs netdev vs bpf. A more flexible scheme where
maintainer remains as a delegate but multiple reviewers can be attached
would be great, that's what I was alluding to earlier.