| Message ID | 20210916120354.20338-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: dsa: b53: Clean up CPU/IMP ports | expand |
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!
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.
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.
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!
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.
> > 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
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.
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.
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(-)