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

On 9/17/21 3:00 AM, Vladimir Oltean wrote:

On Fri, Sep 17, 2021 at 12:19:02AM +0200, Rafał Miłecki wrote:

quoted

On 16.09.2021 23:46, Florian Fainelli wrote:

quoted

On 9/16/21 9:23 AM, Florian Fainelli wrote:

quoted

On 9/16/21 5:03 AM, Rafał Miłecki wrote:

quoted

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