Re: [PATCH net-next 0/2] implement microchip,no-tag-protocol flag

[Vladimir Oltean <olteanv@gmail.com>]

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.