| Message ID | 20240801123143.622037-1-vtpieter@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | implement microchip,no-tag-protocol flag | expand |
Hi Pieter, On Thu, Aug 01, 2024 at 02:31:41PM +0200, vtpieter@gmail.com wrote: > From: Pieter Van Trappen <pieter.van.trappen@cern.ch> > > Add and implement microchip,no-tag-protocol flag to allow disabling > the switch' tagging protocol. For cases where the CPU MAC does not > support MTU size > 1500 such as the Zynq GEM. > > This code was tested with a KSZ8794 chip. > > Pieter Van Trappen (2): > dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag > net: dsa: microchip: implement microchip,no-tag-protocol flag > > .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 5 +++++ > drivers/net/dsa/microchip/ksz8795.c | 2 +- > drivers/net/dsa/microchip/ksz9477.c | 2 +- > drivers/net/dsa/microchip/ksz_common.c | 11 ++++++++--- > drivers/net/dsa/microchip/ksz_common.h | 1 + > drivers/net/dsa/microchip/lan937x_main.c | 2 +- > 6 files changed, 17 insertions(+), 6 deletions(-) > > > base-commit: 0a658d088cc63745528cf0ec8a2c2df0f37742d9 > -- > 2.43.0 Please use ./scripts/get_maintainer.pl when generating the To: and Cc: fields. Not to say that they don't exist, but I have never seen a NIC where MTU=1500 is the absolute hard upper limit. How seriously did you study this before determining that it is impossible to raise that? We're talking about one byte for the tail tag, FWIW. There are also alternative paths to explore, like reducing the DSA user ports MTU to 1499. This is currently not done when dev_set_mtu() fails on the conduit, because Andrew said in the past it's likelier that the conduit is coded to accept up to 1500 but will still work for small oversized packets. Disabling DSA tagging is a very heavy hammer, because it cuts off a whole lot of functionality (the driver should no longer accept PTP hwtimestamping ioctls, etc), so the patch set gets this tag from me currently, due to very shallow justification: Nacked-by: Vladimir Oltean <olteanv@gmail.com> Please carry it forward if you choose to resubmit. Even assuming that a strong justification does exists, there already exists a mechanism for disabling the tagging protocol from the device tree. It is the same as for specifying any other alternative tagging protocol (applied in this case to DSA_TAG_PROTO_NONE). ethernet-switch@N { dsa-tag-protocol = "none"; }; it just needs implementing in the driver. The fact that you chose to add a custom device tree property suggests to me that you did not investigate the problem space very seriously.
On Thu, Aug 01, 2024 at 04:44:01PM +0300, Vladimir Oltean wrote: > ethernet-switch@N { > dsa-tag-protocol = "none"; > }; My mistake - this was supposed to be: ethernet-switch@N { ethernet-ports { /* This is the CPU port */ ethernet-port@0 { ethernet = <&conduit>; dsa-tag-protocol = "none"; }; }; };
On Thu, Aug 01, 2024 at 15:44, Vladimir Oltean <olteanv@gmail.com> wrote: > > Hi Pieter, > > On Thu, Aug 01, 2024 at 02:31:41PM +0200, vtpieter@gmail.com wrote: > > From: Pieter Van Trappen <pieter.van.trappen@cern.ch> > > > > Add and implement microchip,no-tag-protocol flag to allow disabling > > the switch' tagging protocol. For cases where the CPU MAC does not > > support MTU size > 1500 such as the Zynq GEM. > > > > This code was tested with a KSZ8794 chip. > > > > Pieter Van Trappen (2): > > dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag > > net: dsa: microchip: implement microchip,no-tag-protocol flag > > > > .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 5 +++++ > > drivers/net/dsa/microchip/ksz8795.c | 2 +- > > drivers/net/dsa/microchip/ksz9477.c | 2 +- > > drivers/net/dsa/microchip/ksz_common.c | 11 ++++++++--- > > drivers/net/dsa/microchip/ksz_common.h | 1 + > > drivers/net/dsa/microchip/lan937x_main.c | 2 +- > > 6 files changed, 17 insertions(+), 6 deletions(-) > > > > > > base-commit: 0a658d088cc63745528cf0ec8a2c2df0f37742d9 > > -- > > 2.43.0 > > Please use ./scripts/get_maintainer.pl when generating the To: and Cc: fields. > > Not to say that they don't exist, but I have never seen a NIC where MTU=1500 > is the absolute hard upper limit. How seriously did you study this before > determining that it is impossible to raise that? We're talking about one > byte for the tail tag, FWIW. > > There are also alternative paths to explore, like reducing the DSA user ports > MTU to 1499. This is currently not done when dev_set_mtu() fails on the conduit, > because Andrew said in the past it's likelier that the conduit is coded > to accept up to 1500 but will still work for small oversized packets. > > Disabling DSA tagging is a very heavy hammer, because it cuts off a whole lot > of functionality (the driver should no longer accept PTP hwtimestamping ioctls, > etc), so the patch set gets this tag from me currently, due to very shallow > justification: > > Nacked-by: Vladimir Oltean <olteanv@gmail.com> > > Please carry it forward if you choose to resubmit. Hi Vladimir, I do understand your reservation for this path, it defeats most of the advantages of DSA but I still do need the driver to handle the PHYs over SPI and for the WoL functionality; using the switch in a bridge configuration. My reason for mainlining is that I though there might be more people like me in a similar situation. This is actually an older issue and solution of mine so inspired by your comment I revisited the documentation and indeed hardware-wise I can't find back the MTU limitation. I see now that it's actually a limitation of the macb driver [1] so I will try to rework this one instead. Or implement `dsa-tag-protocol` as you propose. Or event better, both! Patch can be thus be cancelled, sorry for the spam. Cheers, Pieter [1]: https://elixir.bootlin.com/linux/v6.11-rc1/source/drivers/net/ethernet/cadence/macb_main.c#L5127
From: Pieter Van Trappen <pieter.van.trappen@cern.ch> Add and implement microchip,no-tag-protocol flag to allow disabling the switch' tagging protocol. For cases where the CPU MAC does not support MTU size > 1500 such as the Zynq GEM. This code was tested with a KSZ8794 chip. Pieter Van Trappen (2): dt-bindings: net: dsa: microchip: add microchip,no-tag-protocol flag net: dsa: microchip: implement microchip,no-tag-protocol flag .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 5 +++++ drivers/net/dsa/microchip/ksz8795.c | 2 +- drivers/net/dsa/microchip/ksz9477.c | 2 +- drivers/net/dsa/microchip/ksz_common.c | 11 ++++++++--- drivers/net/dsa/microchip/ksz_common.h | 1 + drivers/net/dsa/microchip/lan937x_main.c | 2 +- 6 files changed, 17 insertions(+), 6 deletions(-) base-commit: 0a658d088cc63745528cf0ec8a2c2df0f37742d9