[PATCH net-next 1/2] dt-bindings: net: dsa: add none to dsa-tag-protocol enum

[PATCH net-next 1/2] dt-bindings: net: dsa: add none to dsa-tag-protocol enum

[vtpieter]

From: Pieter Van Trappen <pieter.van.trappen@cern.ch>

This allows the switch to disable tagging all together, for the use
case of an unmanaged switch for example.

Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
---
 Documentation/devicetree/bindings/net/dsa/dsa-port.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
index 480120469953..ded8019b6ba6 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/dsa-port.yaml
@@ -53,6 +53,7 @@ properties:
     enum:
       - dsa
       - edsa
+      - none
       - ocelot
       - ocelot-8021q
       - rtl8_4

base-commit: 1bf8e07c382bd4f04ede81ecc05267a8ffd60999
-- 
2.43.0

[PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[vtpieter]

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). Implementing the new
software-defined DSA tagging protocol tag_8021q [2] for these devices
seems overkill for this use case at the time being.

In addition, shorten certain dev->chip_id checks by using the existing
ksz_is_ksz87xx instead.

Link: https://www.kernel.org/doc/html/latest/networking/dsa/dsa.html [1]
Link: https://lpc.events/event/11/contributions/949/attachments/823/1555/paper.pdf [2]
Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
---
 drivers/net/dsa/microchip/ksz8.h       |  2 ++
 drivers/net/dsa/microchip/ksz8795.c    | 27 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.c | 24 +++++++++++++++++------
 drivers/net/dsa/microchip/ksz_common.h |  2 ++
 4 files changed, 49 insertions(+), 6 deletions(-)

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,
-- 
2.43.0

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Vladimir Oltean]

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.

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Pieter]

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

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Andrew Lunn]

> 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

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Pieter]

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

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Andrew Lunn]

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

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Pieter]

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

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Vladimir Oltean]

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?

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Florian Fainelli]

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
-- 
Florian

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Pieter]

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

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Andrew Lunn]

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

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Pieter]

> > > 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
>

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Vladimir Oltean]

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.

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Pieter]

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

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Vladimir Oltean]

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.

Re: [PATCH net-next 2/2] net: dsa: microchip: add KSZ8 change_tag_protocol support

[Andrew Lunn]

> 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

Re: [PATCH net-next 1/2] dt-bindings: net: dsa: add none to dsa-tag-protocol enum

[Krzysztof Kozlowski]

On 19/08/2024 12:12, vtpieter@gmail.com wrote:
> From: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> 
> This allows the switch to disable tagging all together, for the use
> case of an unmanaged switch for example.
> 
> Signed-off-by: Pieter Van Trappen <pieter.van.trappen@cern.ch>
> ---

I think this is v2 or v3. Please provide changelog under --- (or in
cover letter but there is no cover letter here).


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


---

<form letter>
This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
</form letter>

Best regards,
Krzysztof