On 7/22/20 1:53 PM, Vladimir Oltean wrote:
> Prior to the commit below, dsa_master_ndo_setup() used to avoid
> overriding .ndo_get_phys_port_name() unless the callback was empty.
>
> https://elixir.bootlin.com/linux/v5.7.7/source/net/dsa/master.c#L269
>
> Now, it overrides it unconditionally.
>
> This matters for boards where DSA switches are hanging off of other DSA
> switches, or switchdev interfaces.
> Say a user has these udev rules for the top-level switch:
>
> ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5",
> DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p0", NAME="swp0"
> ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5",
> DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p1", NAME="swp1"
> ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5",
> DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p2", NAME="swp2"
> ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5",
> DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p3", NAME="swp3"
> ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5",
> DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p4", NAME="swp4"
> ACTION=="add", SUBSYSTEM=="net", KERNELS=="0000:00:00.5",
> DRIVERS=="mscc_felix", ATTR{phys_port_name}=="p5", NAME="swp5"
>
> If the DSA switches below start randomly overriding
> ndo_get_phys_port_name with their own CPU port, bad things can happen.
> Not only may the CPU port number be not unique among different
> downstream DSA switches, but one of the upstream switchdev interfaces
> may also happen to have a port with the same number. So, we may even end
> up in a situation where all interfaces of the top-level switch end up
> having a phys_port_name attribute of "p0". Clearly not ok if the purpose
> of the udev rules is to assign unique names.
>
> Fix this by restoring the old behavior, which did not overlay this
> operation on top of the DSA master logic, if there was one in place
> already.
>
> Fixes: 3369afba1e46 ("net: Call into DSA netdevice_ops wrappers")
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> This is brain-dead, please consider killing this and retrieving the CPU
> port number from "devlink port"...
That is fair enough. Do you want to submit such a change while you are
at it?
>
> pci/0000:00:00.5/0: type eth netdev swp0 flavour physical port 0
> pci/0000:00:00.5/2: type eth netdev swp2 flavour physical port 2
> pci/0000:00:00.5/4: type notset flavour cpu port 4
> spi/spi2.0/0: type eth netdev sw0p0 flavour physical port 0
> spi/spi2.0/1: type eth netdev sw0p1 flavour physical port 1
> spi/spi2.0/2: type eth netdev sw0p2 flavour physical port 2
> spi/spi2.0/4: type notset flavour cpu port 4
> spi/spi2.1/0: type eth netdev sw1p0 flavour physical port 0
> spi/spi2.1/1: type eth netdev sw1p1 flavour physical port 1
> spi/spi2.1/2: type eth netdev sw1p2 flavour physical port 2
> spi/spi2.1/3: type eth netdev sw1p3 flavour physical port 3
> spi/spi2.1/4: type notset flavour cpu port 4
>
> net/core/dev.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 19f1abc26fcd..60778bd8c3b1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8603,15 +8603,20 @@ int dev_get_phys_port_name(struct net_device *dev,
> const struct net_device_ops *ops = dev->netdev_ops;
> int err;
>
> - err = dsa_ndo_get_phys_port_name(dev, name, len);
> - if (err == 0 || err != -EOPNOTSUPP)
> - return err;
> -
> if (ops->ndo_get_phys_port_name) {
> err = ops->ndo_get_phys_port_name(dev, name, len);
> if (err != -EOPNOTSUPP)
> return err;
> + } else {
> + /* DSA may override this operation, but only if the master
> + * isn't a switchdev or another DSA, in that case it breaks
> + * their port numbering.
> + */
> + err = dsa_ndo_get_phys_port_name(dev, name, len);
Extraneous space here.
Acked-by: Florian Fainelli <[email protected]>
--
Florian
