| Message ID | 20240819101238.1570176-2-vtpieter@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/2] dt-bindings: net: dsa: add none to dsa-tag-protocol enum | expand |
Hi Pieter, On Mon, Aug 19, 2024 at 12:12:35PM +0200, vtpieter@gmail.com wrote: > From: Pieter Van Trappen <pieter.van.trappen@cern.ch> > > Add support for changing the KSZ8 switches tag protocol. In fact > these devices can only enable or disable the tail tag, so there's > really only three supported protocols: > - DSA_TAG_PROTO_KSZ8795 for KSZ87xx > - DSA_TAG_PROTO_KSZ9893 for KSZ88x3 > - DSA_TAG_PROTO_NONE > > When disabled, this can be used as a workaround for the 'Common > pitfalls using DSA setups' [1] to use the conduit network interface as > a regular one, admittedly forgoing most DSA functionality and using > the device as an unmanaged switch whilst allowing control > operations (ethtool, PHY management, WoL). Concretely, what is it that you wish to accomplish? I see you chose to ignore my previous NACK due to the lack of a strong justification for disabling the tagging protocol. https://lore.kernel.org/netdev/20240801134401.h24ikzuoiakwg4i4@skbuf/ > Implementing the new software-defined DSA tagging protocol tag_8021q > [2] for these devices seems overkill for this use case at the time > being. I think there's a misunderstanding about tag_8021q. It does not disable the tagging protocol. But rather, it helps you implement a tagging protocol when the hardware does not want to cooperate. So I don't see how it would have helped you in your goal (whatever that is), and why mention it. tag_8021q exists because it is my goal for DSA_TAG_PROTO_NONE to eventually disappear. The trend is for drivers to be converted from DSA_TAG_PROTO_NONE to something else (like DSA_TAG_PROTO_VSC73XX_8021Q), not the other way around. It's a strong usability concern to not be able to ping through the port net devices. At the very least we need consensus among the current DSA maintainers that accepting 'none' as an alternative tagging protocol is acceptable.
Hi Vladimir, > Hi Pieter, > > > - DSA_TAG_PROTO_NONE > > > > When disabled, this can be used as a workaround for the 'Common > > pitfalls using DSA setups' [1] to use the conduit network interface as > > a regular one, admittedly forgoing most DSA functionality and using > > the device as an unmanaged switch whilst allowing control > > operations (ethtool, PHY management, WoL). > > Concretely, what is it that you wish to accomplish? I see you chose to > ignore my previous NACK due to the lack of a strong justification for > disabling the tagging protocol. > https://lore.kernel.org/netdev/20240801134401.h24ikzuoiakwg4i4@skbuf/ Sorry I definitely did not try to ignore your previous NACK but here the motivation and solution are both different, which is why I did not consider it a patch iteration of the previous one. Previously I could not use DSA because of the macb driver limitation, now fixed (max_mtu increase, submitted here). Once I got that working, I notice that full DSA was not a compatible use case for my board because of requiring the conduit interface to behave as a regular ethernet interface. So it's really the unmanaged switch case, which I though I motivated well in the patch description here (PHY library, ethtool and switch WoL management). The solution is now the one you proposed earlier. > > Implementing the new software-defined DSA tagging protocol tag_8021q > > [2] for these devices seems overkill for this use case at the time > > being. > > I think there's a misunderstanding about tag_8021q. It does not disable > the tagging protocol. But rather, it helps you implement a tagging > protocol when the hardware does not want to cooperate. So I don't see > how it would have helped you in your goal (whatever that is), and why > mention it. Right I understand, indeed a misunderstanding. Will remove this part. > tag_8021q exists because it is my goal for DSA_TAG_PROTO_NONE to > eventually disappear. The trend is for drivers to be converted from > DSA_TAG_PROTO_NONE to something else (like DSA_TAG_PROTO_VSC73XX_8021Q), > not the other way around. It's a strong usability concern to not be able > to ping through the port net devices. > > At the very least we need consensus among the current DSA maintainers > that accepting 'none' as an alternative tagging protocol is acceptable. This of course I understand as well. Cheers, Pieter
> Previously I could not use DSA because of the macb driver limitation, now > fixed (max_mtu increase, submitted here). Once I got that working, I notice > that full DSA was not a compatible use case for my board because of > requiring the conduit interface to behave as a regular ethernet interface. > So it's really the unmanaged switch case, which I though I motivated well in > the patch description here (PHY library, ethtool and switch WoL management). If its an unmanaged switch, you don't need DSA, or anything at all other than MACB. Linux is just a plain host connected to a switch. It is a little unusual that the switch is integrated into the same box, rather than being a patch cable away, bit linux does not really see this difference compared to any other unmanaged switch. Andrew
Hi Andrew, > > Previously I could not use DSA because of the macb driver limitation, now > > fixed (max_mtu increase, submitted here). Once I got that working, I notice > > that full DSA was not a compatible use case for my board because of > > requiring the conduit interface to behave as a regular ethernet interface. > > So it's really the unmanaged switch case, which I though I motivated well in > > the patch description here (PHY library, ethtool and switch WoL management). > > If its an unmanaged switch, you don't need DSA, or anything at all > other than MACB. Linux is just a plain host connected to a switch. It > is a little unusual that the switch is integrated into the same box, > rather than being a patch cable away, bit linux does not really see > this difference compared to any other unmanaged switch. That's true in theory but not in practice because without DSA I can't use the ksz_spi.c driver which gives me access to the full register set. I need this for the KSZ8794 I'm using to: - apply the EEE link drop erratum from ksz8795.c - active port WoL which is connected through its PME_N pin - use iproute2 for PHY and connection debugging (link up/down, packets statistics etc.) If there's another way to accomplish the above without DSA, I'd be happy to learn about it. Cheers, Pieter
On Mon, Aug 19, 2024 at 03:21:51PM +0200, Pieter wrote: > Hi Andrew, > > > > Previously I could not use DSA because of the macb driver limitation, now > > > fixed (max_mtu increase, submitted here). Once I got that working, I notice > > > that full DSA was not a compatible use case for my board because of > > > requiring the conduit interface to behave as a regular ethernet interface. > > > So it's really the unmanaged switch case, which I though I motivated well in > > > the patch description here (PHY library, ethtool and switch WoL management). > > > > If its an unmanaged switch, you don't need DSA, or anything at all > > other than MACB. Linux is just a plain host connected to a switch. It > > is a little unusual that the switch is integrated into the same box, > > rather than being a patch cable away, bit linux does not really see > > this difference compared to any other unmanaged switch. > > That's true in theory but not in practice because without DSA I can't use > the ksz_spi.c driver which gives me access to the full register set. I need > this for the KSZ8794 I'm using to: > - apply the EEE link drop erratum from ksz8795.c > - active port WoL which is connected through its PME_N pin > - use iproute2 for PHY and connection debugging (link up/down, > packets statistics etc.) Then it is not an unmanaged switch. You are managing it. > If there's another way to accomplish the above without DSA, I'd be > happy to learn about it. Its go back to the beginning. Why cannot use you DSA, and use it as a manage switch? None of your use-cases above are prevented by DSA. Andrew
Hi Andrew, > > > > Previously I could not use DSA because of the macb driver limitation, now > > > > fixed (max_mtu increase, submitted here). Once I got that working, I notice > > > > that full DSA was not a compatible use case for my board because of > > > > requiring the conduit interface to behave as a regular ethernet interface. > > > > So it's really the unmanaged switch case, which I though I motivated well in > > > > the patch description here (PHY library, ethtool and switch WoL management). > > > > > > If its an unmanaged switch, you don't need DSA, or anything at all > > > other than MACB. Linux is just a plain host connected to a switch. It > > > is a little unusual that the switch is integrated into the same box, > > > rather than being a patch cable away, bit linux does not really see > > > this difference compared to any other unmanaged switch. > > > > That's true in theory but not in practice because without DSA I can't use > > the ksz_spi.c driver which gives me access to the full register set. I need > > this for the KSZ8794 I'm using to: > > - apply the EEE link drop erratum from ksz8795.c > > - active port WoL which is connected through its PME_N pin > > - use iproute2 for PHY and connection debugging (link up/down, > > packets statistics etc.) > > Then it is not an unmanaged switch. You are managing it. > > > If there's another way to accomplish the above without DSA, I'd be > > happy to learn about it. > > Its go back to the beginning. Why cannot use you DSA, and use it as a > manage switch? None of your use-cases above are prevented by DSA. Right so I'm managing it but I don't care from which port the packets originate, so I could disable the tagging in my case. My problem is that with tagging enabled, I cannot use the DSA conduit interface as a regular one to open sockets etc. I don't fully understand why I have to admit but it's documented here [1] and with wireshark I can see only control packets going through, the ingress data ones are not tagged and subsequently dropped by the switch with tagging enabled. PS here are my iproute2 commands: ip link set lan1 up ip link set lan2 up ip link add br0 type bridge ip link set lan1 master br0 ip link set lan2 master br0 ip link set br0 up Cheers, Pieter [1] https://www.kernel.org/doc/html/latest/networking/dsa/dsa.html#common-pitfalls-using-dsa-setups
On Mon, Aug 19, 2024 at 03:43:42PM +0200, Pieter wrote: > Right so I'm managing it but I don't care from which port the packets > originate, so I could disable the tagging in my case. > > My problem is that with tagging enabled, I cannot use the DSA conduit > interface as a regular one to open sockets etc. Open the socket on the bridge interface then?
> Right so I'm managing it but I don't care from which port the packets > originate, so I could disable the tagging in my case. > My problem is that with tagging enabled, I cannot use the DSA conduit > interface as a regular one to open sockets etc. I don't fully understand > why I have to admit but it's documented here [1] and with wireshark I > can see only control packets going through, the ingress data ones are > not tagged and subsequently dropped by the switch with tagging enabled. > > PS here are my iproute2 commands: > ip link set lan1 up > ip link set lan2 up > ip link add br0 type bridge > ip link set lan1 master br0 > ip link set lan2 master br0 > ip link set br0 up So forget about the switch for the moment. Just think about having two network interfaces. They could be intel e1000e, macb, whatever. You would use the exact same commands above to setup a bridge so packets would flow between them. This is all standard Linux networking, nothing special. You would typically add an IPv4/IPv6 address on br0, or run a DHCP client on it. lan1 and lan2 don't have IP addresses, only the br0. Any sockets you open and don't bind to a specific interface will make use of the IP addresses on the br0, since that is the only interface with an IP address. The Linux software bridge will determine which interface packets go out of, or perform flooding if the destination MAC address is not know etc. DSA does not change any of this. DSA just allows Linux to offload what it is doing in software to the hardware. You use the same commands as above. If you want to bind your socket to a local interface, you bind it to br0, nothing different. The software bridge will still determine the egress interface, and pass the frame to the hardware. The hardware itself should be able to handle frames which ingress one port and egress another, i.e. that bit of bridging has been offloaded to the hardware. The name 'conduit' is supposed to give you a clue. It is just a conduit, nothing more. It is part of the plumbing to make DSA work. But apart from ensuring it is up, you don't do anything with it. You operate on lan1, lan2, and anything you build on top of those, e.g. a bridge, bonds, vlan interfaces. Andrew
On 8/19/2024 7:05 AM, Vladimir Oltean wrote: > On Mon, Aug 19, 2024 at 03:43:42PM +0200, Pieter wrote: >> Right so I'm managing it but I don't care from which port the packets >> originate, so I could disable the tagging in my case. >> >> My problem is that with tagging enabled, I cannot use the DSA conduit >> interface as a regular one to open sockets etc. > > Open the socket on the bridge interface then? +1
Hi Vladimir, > On Mon, Aug 19, 2024 at 03:43:42PM +0200, Pieter wrote: > > Right so I'm managing it but I don't care from which port the packets > > originate, so I could disable the tagging in my case. > > > > My problem is that with tagging enabled, I cannot use the DSA conduit > > interface as a regular one to open sockets etc. > > Open the socket on the bridge interface then? Assuming this works, how to tell all user space programs to use br0 instead of eth0? Both interfaces are up and I can't do `ifdown eth0` without losing all connectivity. I'm using busybox's ifup BTW and it says: $ ifup br0 ifup: ignoring unknown interface br0 Thanks, Pieter
On Mon, Aug 19, 2024 at 04:20:31PM +0200, Pieter wrote: > Hi Vladimir, > > > On Mon, Aug 19, 2024 at 03:43:42PM +0200, Pieter wrote: > > > Right so I'm managing it but I don't care from which port the packets > > > originate, so I could disable the tagging in my case. > > > > > > My problem is that with tagging enabled, I cannot use the DSA conduit > > > interface as a regular one to open sockets etc. > > > > Open the socket on the bridge interface then? > > Assuming this works, how to tell all user space programs to use br0 instead > of eth0? How did you tell userspace to use eth0? In general, you don't tell userspace anything about interfaces. You open a client socket to a destination IP address, and the kernel routing tables are used to determine the egress interface. In general, it will use a public scope IP address from that interface as the source address. The conduit interface should not have an IP address, its just plumbing, but not otherwise used. Your IP address is on br0, so by default the kernel will use the IP address from it. Andrew
On Mon, Aug 19, 2024 at 04:20:31PM +0200, Pieter wrote: > Hi Vladimir, > > > On Mon, Aug 19, 2024 at 03:43:42PM +0200, Pieter wrote: > > > Right so I'm managing it but I don't care from which port the packets > > > originate, so I could disable the tagging in my case. > > > > > > My problem is that with tagging enabled, I cannot use the DSA conduit > > > interface as a regular one to open sockets etc. > > > > Open the socket on the bridge interface then? > > Assuming this works, You don't have to "assume" it works. You can test and verify that it works. We have a selftest for receiving all kinds of packets on standalone and bridged interfaces. https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/tools/testing/selftests/net/forwarding/local_termination.sh > how to tell all user space programs to use br0 instead of eth0? Question does not compute, sorry. Is this answer what you're looking for? "Just like you tell them to use eth0, just that instead of eth0 you type br0". Or just like Andrew says. You don't explicitly bind IP sockets to interfaces, you let the routing layer pick the interface based on the routing table and the IP addresses on each interface. Ergo, for IP sockets you just need to put your IP address on the bridge interface. > Both interfaces are up and I can't do `ifdown eth0` without losing > all connectivity. I'm using busybox's ifup BTW and it says: > $ ifup br0 > ifup: ignoring unknown interface br0 busybox ifupdown reads the /etc/network/interfaces, it's saying that interface isn't there. Which it really isn't, maybe? I haven't really used busybox ifupdown and I don't know what it can do with bridges. The basic command to bring a network interface up is "ip link set dev $NAME up". This has no state/configuration file and just constructs netlink messages to pass through the rtnetlink socket to the kernel.
> > > On Mon, Aug 19, 2024 at 03:43:42PM +0200, Pieter wrote: > > > > Right so I'm managing it but I don't care from which port the packets > > > > originate, so I could disable the tagging in my case. > > > > > > > > My problem is that with tagging enabled, I cannot use the DSA conduit > > > > interface as a regular one to open sockets etc. > > > > > > Open the socket on the bridge interface then? > > > > Assuming this works, how to tell all user space programs to use br0 instead > > of eth0? > > How did you tell userspace to use eth0? > > In general, you don't tell userspace anything about interfaces. You > open a client socket to a destination IP address, and the kernel > routing tables are used to determine the egress interface. In general, > it will use a public scope IP address from that interface as the > source address. Hi Andrew, Vladimir, thanks for your explanation and patience! It works as you said, I will have to do some changes to userspace to ensure the DHCP client uses br0 instead of eth0 but that's it. I just tried and br0 obtains the IP address and all is good, with DSA tagging enabled. This patch can be dropped, sorry for the hassle. > The conduit interface should not have an IP address, its just > plumbing, but not otherwise used. Your IP address is on br0, so by > default the kernel will use the IP address from it. > > Andrew >
On Mon, Aug 19, 2024 at 04:41:29PM +0200, Pieter wrote: > Hi Andrew, Vladimir, > thanks for your explanation and patience! > > It works as you said, I will have to do some changes to userspace to > ensure the DHCP client uses br0 instead of eth0 but that's it. > I just tried and br0 obtains the IP address and all is good, with > DSA tagging enabled. > > This patch can be dropped, sorry for the hassle. I'm pretty baffled. Was this unclear from the user documentation at: https://www.kernel.org/doc/html/next/networking/dsa/configuration.html ? Maybe we should change something to make it more clear that this is what is expected.
Hi Vladimir, > > It works as you said, I will have to do some changes to userspace to > > ensure the DHCP client uses br0 instead of eth0 but that's it. > > I just tried and br0 obtains the IP address and all is good, with > > DSA tagging enabled. > > > > This patch can be dropped, sorry for the hassle. > > I'm pretty baffled. Was this unclear from the user documentation at: > https://www.kernel.org/doc/html/next/networking/dsa/configuration.html > ? Maybe we should change something to make it more clear that this is > what is expected. It was to me but that's rather due to my limited knowledge of the network stack and routing than to the quality of that documentation per se. Cheers, Pieter
diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h index e1c79ff97123..14c7912b854e 100644 --- a/drivers/net/dsa/microchip/ksz8.h +++ b/drivers/net/dsa/microchip/ksz8.h @@ -57,6 +57,8 @@ int ksz8_change_mtu(struct ksz_device *dev, int port, int mtu); int ksz8_pme_write8(struct ksz_device *dev, u32 reg, u8 value); int ksz8_pme_pread8(struct ksz_device *dev, int port, int offset, u8 *data); int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 data); +int ksz8_change_tag_protocol(struct ksz_device *dev, + enum dsa_tag_protocol proto); void ksz8_phylink_mac_link_up(struct phylink_config *config, struct phy_device *phydev, unsigned int mode, phy_interface_t interface, int speed, int duplex, diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index a01079297a8c..41d163e88f03 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -194,6 +194,33 @@ int ksz8_change_mtu(struct ksz_device *dev, int port, int mtu) return -EOPNOTSUPP; } +/** + * ksz8_change_tag_protocol - Change tag protocol + * @dev: The device structure. + * @proto: The requested protocol. + * + * This function allows changing the tag protocol. In fact the ksz8 + * devices can only enable or disable the tail tag. + * + * Return: 0 on success, -EPROTONOSUPPORT in case protocol not supported. + */ +int ksz8_change_tag_protocol(struct ksz_device *dev, + enum dsa_tag_protocol proto) +{ + const u32 *masks = dev->info->masks; + const u16 *regs = dev->info->regs; + + if ((proto == DSA_TAG_PROTO_KSZ8795 && ksz_is_ksz87xx(dev)) || + (proto == DSA_TAG_PROTO_KSZ9893 && ksz_is_ksz88x3(dev))) + ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true); + else if (proto == DSA_TAG_PROTO_NONE) + ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], false); + else + return -EPROTONOSUPPORT; + + return 0; +} + static int ksz8_port_queue_split(struct ksz_device *dev, int port, int queues) { u8 mask_4q, mask_2q; diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index cd3991792b69..e5194660ed99 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -310,6 +310,7 @@ static const struct ksz_dev_ops ksz88x3_dev_ops = { .pme_write8 = ksz8_pme_write8, .pme_pread8 = ksz8_pme_pread8, .pme_pwrite8 = ksz8_pme_pwrite8, + .change_tag_protocol = ksz8_change_tag_protocol, }; static const struct ksz_dev_ops ksz87xx_dev_ops = { @@ -345,6 +346,7 @@ static const struct ksz_dev_ops ksz87xx_dev_ops = { .pme_write8 = ksz8_pme_write8, .pme_pread8 = ksz8_pme_pread8, .pme_pwrite8 = ksz8_pme_pwrite8, + .change_tag_protocol = ksz8_change_tag_protocol, }; static void ksz9477_phylink_mac_link_up(struct phylink_config *config, @@ -2937,9 +2939,7 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds, struct ksz_device *dev = ds->priv; enum dsa_tag_protocol proto = DSA_TAG_PROTO_NONE; - if (dev->chip_id == KSZ8795_CHIP_ID || - dev->chip_id == KSZ8794_CHIP_ID || - dev->chip_id == KSZ8765_CHIP_ID) + if (ksz_is_ksz87xx(dev)) proto = DSA_TAG_PROTO_KSZ8795; if (dev->chip_id == KSZ8830_CHIP_ID || @@ -2961,12 +2961,25 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds, return proto; } +static int ksz_change_tag_protocol(struct dsa_switch *ds, + enum dsa_tag_protocol proto) +{ + struct ksz_device *dev = ds->priv; + + if (dev->dev_ops->change_tag_protocol) + return dev->dev_ops->change_tag_protocol(dev, proto); + else + return -EPROTONOSUPPORT; +} + static int ksz_connect_tag_protocol(struct dsa_switch *ds, enum dsa_tag_protocol proto) { struct ksz_tagger_data *tagger_data; switch (proto) { + case DSA_TAG_PROTO_NONE: + return 0; case DSA_TAG_PROTO_KSZ8795: return 0; case DSA_TAG_PROTO_KSZ9893: @@ -4208,6 +4221,7 @@ static int ksz_hsr_leave(struct dsa_switch *ds, int port, static const struct dsa_switch_ops ksz_switch_ops = { .get_tag_protocol = ksz_get_tag_protocol, + .change_tag_protocol = ksz_change_tag_protocol, .connect_tag_protocol = ksz_connect_tag_protocol, .get_phy_flags = ksz_get_phy_flags, .setup = ksz_setup, @@ -4443,9 +4457,7 @@ static int ksz9477_drive_strength_write(struct ksz_device *dev, dev_warn(dev->dev, "%s is not supported by this chip variant\n", props[KSZ_DRIVER_STRENGTH_IO].name); - if (dev->chip_id == KSZ8795_CHIP_ID || - dev->chip_id == KSZ8794_CHIP_ID || - dev->chip_id == KSZ8765_CHIP_ID) + if (ksz_is_ksz87xx(dev)) reg = KSZ8795_REG_SW_CTRL_20; else reg = KSZ9477_REG_SW_IO_STRENGTH; diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index 8094d90d6ca4..e1178063e6e4 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -363,6 +363,8 @@ struct ksz_dev_ops { u8 *data); int (*pme_pwrite8)(struct ksz_device *dev, int port, int offset, u8 data); + int (*change_tag_protocol)(struct ksz_device *dev, + enum dsa_tag_protocol proto); void (*freeze_mib)(struct ksz_device *dev, int port, bool freeze); void (*port_init_cnt)(struct ksz_device *dev, int port); void (*phylink_mac_link_up)(struct ksz_device *dev, int port,