| Message ID | 20220517194015.1081632-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | lantiq_gswip: Two small fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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?
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
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/
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;
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(-)