[PATCH net v1 0/2] lantiq_gswip: Two small fixes

[PATCH net v1 0/2] lantiq_gswip: Two small fixes

[Martin Blumenstingl]

While updating the Lantiq target in OpenWrt to Linux 5.15 I came across
an FDB related error message. While that still needs to be solved I
found two other small issues on the way.

This series fixes the two minor issues found while revisiting the FDB
code in the lantiq_gswip driver:
- The first patch fixes the start index used in gswip_port_fdb() to
  find the entry with the matching bridge. The updated logic is now
  consistent with the rest of the driver.
- The second patch fixes a typo in a dev_err() message.

Hauke gave his Acked-by off-list to me before I sent the patches.


Martin Blumenstingl (2):
  net: dsa: lantiq_gswip: Fix start index in gswip_port_fdb()
  net: dsa: lantiq_gswip: Fix typo in gswip_port_fdb_dump() error print

 drivers/net/dsa/lantiq_gswip.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.36.1

[PATCH net v1 1/2] net: dsa: lantiq_gswip: Fix start index in gswip_port_fdb()

[Martin Blumenstingl]

The first N entries in priv->vlans are reserved for managing ports which
are not part of a bridge. Use priv->hw_info->max_ports to consistently
access per-bridge entries at index 7. Starting at
priv->hw_info->cpu_port (6) is harmless in this case because
priv->vlan[6].bridge is always NULL so the comparison result is always
false (which results in this entry being skipped).

Fixes: 58c59ef9e930c4 ("net: dsa: lantiq: Add Forwarding Database access")
Acked-by: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 12c15da55664..0c313db23451 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1360,7 +1360,7 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
 	struct net_device *bridge = dsa_port_bridge_dev_get(dsa_to_port(ds, port));
 	struct gswip_priv *priv = ds->priv;
 	struct gswip_pce_table_entry mac_bridge = {0,};
-	unsigned int cpu_port = priv->hw_info->cpu_port;
+	unsigned int max_ports = priv->hw_info->max_ports;
 	int fid = -1;
 	int i;
 	int err;
@@ -1368,7 +1368,7 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
 	if (!bridge)
 		return -EINVAL;
 
-	for (i = cpu_port; i < ARRAY_SIZE(priv->vlans); i++) {
+	for (i = max_ports; i < ARRAY_SIZE(priv->vlans); i++) {
 		if (priv->vlans[i].bridge == bridge) {
 			fid = priv->vlans[i].fid;
 			break;
-- 
2.36.1

[PATCH net v1 2/2] net: dsa: lantiq_gswip: Fix typo in gswip_port_fdb_dump() error print

[Martin Blumenstingl]

gswip_port_fdb_dump() reads the MAC bridge entries. The error message
should say "failed to read mac bridge entry". While here, also add the
index to the error print so humans can get to the cause of the problem
easier.

Fixes: 58c59ef9e930c4 ("net: dsa: lantiq: Add Forwarding Database access")
Acked-by: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/dsa/lantiq_gswip.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 0c313db23451..8af4def38a98 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1426,8 +1426,9 @@ static int gswip_port_fdb_dump(struct dsa_switch *ds, int port,
 
 		err = gswip_pce_table_entry_read(priv, &mac_bridge);
 		if (err) {
-			dev_err(priv->dev, "failed to write mac bridge: %d\n",
-				err);
+			dev_err(priv->dev,
+				"failed to read mac bridge entry %d: %d\n",
+				i, err);
 			return err;
 		}
 
-- 
2.36.1

Re: [PATCH net v1 2/2] net: dsa: lantiq_gswip: Fix typo in gswip_port_fdb_dump() error print

[Vladimir Oltean]

On Tue, May 17, 2022 at 09:40:15PM +0200, Martin Blumenstingl wrote:
> gswip_port_fdb_dump() reads the MAC bridge entries. The error message
> should say "failed to read mac bridge entry". While here, also add the
> index to the error print so humans can get to the cause of the problem
> easier.
> 
> Fixes: 58c59ef9e930c4 ("net: dsa: lantiq: Add Forwarding Database access")
> Acked-by: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/dsa/lantiq_gswip.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 0c313db23451..8af4def38a98 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -1426,8 +1426,9 @@ static int gswip_port_fdb_dump(struct dsa_switch *ds, int port,
>  
>  		err = gswip_pce_table_entry_read(priv, &mac_bridge);
>  		if (err) {
> -			dev_err(priv->dev, "failed to write mac bridge: %d\n",
> -				err);
> +			dev_err(priv->dev,
> +				"failed to read mac bridge entry %d: %d\n",
> +				i, err);
>  			return err;
>  		}
>  
> -- 
> 2.36.1
> 

Re: [PATCH net v1 1/2] net: dsa: lantiq_gswip: Fix start index in gswip_port_fdb()

[Vladimir Oltean]

Hi Martin,

On Tue, May 17, 2022 at 09:40:14PM +0200, Martin Blumenstingl wrote:
> The first N entries in priv->vlans are reserved for managing ports which
> are not part of a bridge. Use priv->hw_info->max_ports to consistently
> access per-bridge entries at index 7. Starting at
> priv->hw_info->cpu_port (6) is harmless in this case because
> priv->vlan[6].bridge is always NULL so the comparison result is always
> false (which results in this entry being skipped).
> 
> Fixes: 58c59ef9e930c4 ("net: dsa: lantiq: Add Forwarding Database access")
> Acked-by: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

The patch, as well as other improvements you might want to bring to the gswip driver
(we have more streamlined support in DSA now for FDB isolation, see ds->fdb_isolation)
is appreciated.

But I don't think that a code cleanup patch that makes no functional
difference, and isn't otherwise needed by other backported patches,
should be sent to the "net" tree, and be backported to "stable" later?

Re: [PATCH net v1 1/2] net: dsa: lantiq_gswip: Fix start index in gswip_port_fdb()

[Martin Blumenstingl]

Hi Vladimir,

On Wed, May 18, 2022 at 1:45 PM Vladimir Oltean <olteanv@gmail.com> wrote:
[...]
> The patch, as well as other improvements you might want to bring to the gswip driver
> (we have more streamlined support in DSA now for FDB isolation, see ds->fdb_isolation)
> is appreciated.
Thank you very much for this hint! I was not aware of the
ds->fdb_isolation flag and additionally I have some other questions
regarding FDB - I'll send these in a separate email though.
Also thank you for being quick to review my patches and on top of that
providing extra hints!

> But I don't think that a code cleanup patch that makes no functional
> difference, and isn't otherwise needed by other backported patches,
> should be sent to the "net" tree, and be backported to "stable" later?
Sure, I will actually re-send the whole series based on net-next.
When I initially wrote this patch I thought that it would fix a more
severe issue. Only later on I found that the bug is harmless (as
mentioned in the patch description). When I then finally sent the
patch I just stuck with my original plan to send it for the net.git
tree - instead of re-thinking whether that's still needed after my
latest findings.


Best regards,
Martin

Re: [PATCH net v1 1/2] net: dsa: lantiq_gswip: Fix start index in gswip_port_fdb()

[Vladimir Oltean]

On Wed, May 18, 2022 at 07:58:58PM +0200, Martin Blumenstingl wrote:
> Hi Vladimir,
> 
> On Wed, May 18, 2022 at 1:45 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> [...]
> > The patch, as well as other improvements you might want to bring to the gswip driver
> > (we have more streamlined support in DSA now for FDB isolation, see ds->fdb_isolation)
> > is appreciated.
> Thank you very much for this hint! I was not aware of the
> ds->fdb_isolation flag and additionally I have some other questions
> regarding FDB - I'll send these in a separate email though.
> Also thank you for being quick to review my patches and on top of that
> providing extra hints!

Ok, feel free to ask.
Please note that there's also this discussion with Alvin about FDB
isolation and host filtering which hopefully helped to clear some more
concepts.
https://lore.kernel.org/all/87wnhbdyee.fsf@bang-olufsen.dk/