Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 8 Mar 2022 13:21:56 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Woojung Huh <woojung.huh@microchip.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Vivien Didelot <vivien.didelot@gmail.com>,
        kernel@pengutronix.de, Jakub Kicinski <kuba@kernel.org>,
        UNGLinuxDriver@microchip.com,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH net-next v2 1/1] net: dsa: microchip: ksz9477: implement
 MTU configuration
Message-ID: <20220308112156.6s2ssnkhifxuuw2w@skbuf>
References: <20220223233833.mjknw5ko7hpxj3go@skbuf>
 <20220224045936.GB4594@pengutronix.de>
 <20220224093329.hssghouq7hmgxvwb@skbuf>
 <20220224093827.GC4594@pengutronix.de>
 <20220224094657.jzhvi67ryhuipor4@skbuf>
 <20220225114740.GA27407@pengutronix.de>
 <20220225115802.bvjd54cwwk6hjyfa@skbuf>
 <20220225125430.GB27407@pengutronix.de>
 <20220225163543.vnqlkltgmwf4vlmm@skbuf>
 <20220308100644.GA5189@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220308100644.GA5189@pengutronix.de>
Precedence: bulk

On Tue, Mar 08, 2022 at 11:06:44AM +0100, Oleksij Rempel wrote:
> > > > I was saying:
> > > > 
> > > > ip link set lan1 up
> > > > ip link add link lan1 name lan1.5 type vlan id 5
> > > > ip addr add 172.17.0.2/24 dev lan1.5 && ip link set lan1.5 up
> > > > iperf3 -c 172.17.0.10
> > > 
> > > It works.
> > 
> > This is akin to saying that without any calls to ksz9477_change_mtu(),
> > just writing VLAN_ETH_FRAME_LEN + ETH_FCS_LEN into REG_SW_MTU__2 is
> > sufficient to get VLAN-tagged MTU-sized packets to pass through the CPU
> > port and the lan1 user port.
> > 
> > So my question is: is this necessary?
> > 
> > 	if (dsa_is_cpu_port(ds, port))
> > 		new_mtu += KSZ9477_INGRESS_TAG_LEN;
> > 
> 
> No.
> 
> I did some extra tests with following results: REG_SW_MTU__2 should be
> configured to 1518 to pass 1514 frame. Independent if the frame is
> passed between external ports or external to CPU port. So, I assume,
> ETH_FRAME_LEN + ETH_FCS_LEN should be used instead of VLAN_ETH_FRAME_LEN
> + ETH_FCS_LEN. Correct?

Oleksij, to be clear, I only had an issue with consistency.
You were adding KSZ9477_INGRESS_TAG_LEN during ksz9477_change_mtu() but
not during initial setup. That prompted the question: is that particular
member of the sum needed or not? Either it's needed in both places, or
in none.

Then, apart from removing KSZ9477_INGRESS_TAG_LEN, you've also made an
unsolicited change (subtracted VLAN_HLEN from the value programmed to
hardware) without a clear confirmation that you understand what this
does, and without explicitly saying that the iperf3 test above still
works with this formula applied.

Since the VLAN header is part of L2, it means that a port configured for
MTU 1500 must also support VLAN-tagged packets with an L2 payload of
1500 octets. 1500 + ETH_HLEN + VLAN_HLEN == 1518 octets.
And since you need to add ETH_HLEN + ETH_FCS_LEN, I have an unconfirmed
hunch that VLAN_HLEN is also needed for the case above.

So, I'm sorry for being paranoid, but you aren't really giving me a
choice but to ask again, and again.