Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Sun, 29 Oct 2023 01:04:02 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Linus Walleij <linus.walleij@linaro.org>,
        DENG Qingfang <dqfext@gmail.com>,
        Mauri Sandberg <sandberg@mailfence.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] dsa: tag_rtl4_a: Bump min packet size
Message-ID: <20231028220402.gdsynephzfkpvk4m@skbuf>
References: <20231027-fix-rtl8366rb-v1-1-d565d905535a@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231027-fix-rtl8366rb-v1-1-d565d905535a@linaro.org>
Precedence: bulk

On Fri, Oct 27, 2023 at 10:21:39PM +0200, Linus Walleij wrote:
> It was reported that the "LuCI" web UI was not working properly
> with a device using the RTL8366RB switch. Disabling the egress
> port tagging code made the switch work again, but this is not
> a good solution as we want to be able to direct traffic to a
> certain port.
> 
> It turns out that sometimes, but not always, small packets are
> dropped by the switch for no reason.

"For no reason" is a technical statement which means "an unspecific/inconclusive
drop reason in the ethtool -S output on the conduit interface (which also
shows the hardware counters of the CPU port", or is it just a figure of
speech? If just a figure of speech, could you please determine which
counter gets incremented when the switch drops packets?

What user port is being targeted when the switch drops packets? Any user
port, or just specific ones?

What protocol headers do those packets that are dropped have? Is it size
that they have in common, I wonder (given that you say that small
packets are not always dropped), or is it something else?

> If we pad the ethernet frames to a minimum of ETH_FRAME_LEN + FCS
> (1518 bytes) everything starts working fine.
> 
> As we completely lack datasheet or any insight into how this
> switch works, this trial-and-error solution is the best we
> can do.
> 
> I have tested smaller sizes, ETH_FRAME_LEN doesn't work for
> example.
> 
> Fixes: 0e90dfa7a8d8 ("net: dsa: tag_rtl4_a: Fix egress tags")

Have you actually checked out this sha1sum and confirmed that the packet
drop can be reproduced there? Ideally you could also go back to a bit
earlier, to commit 9eb8bc593a5e ("net: dsa: tag_rtl4_a: fix egress tags")
(this is a different commit from Qingfang with the same description) and
test on user port 0 only?

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  net/dsa/tag_rtl4_a.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
> index c327314b95e3..8b1e81a6377b 100644
> --- a/net/dsa/tag_rtl4_a.c
> +++ b/net/dsa/tag_rtl4_a.c
> @@ -41,8 +41,11 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
>  	u8 *tag;
>  	u16 out;
>  
> -	/* Pad out to at least 60 bytes */
> -	if (unlikely(__skb_put_padto(skb, ETH_ZLEN, false)))
> +	/* We need to pad out to at least ETH_FRAME_LEN + FCS bytes. This was
> +	 * found by trial-and-error. Sometimes smaller frames work, but sadly
> +	 * not always.
> +	 */
> +	if (unlikely(__skb_put_padto(skb, ETH_FRAME_LEN + ETH_FCS_LEN, false)))
>  		return NULL;
>  
>  	netdev_dbg(dev, "add realtek tag to package to port %d\n",
> 
> ---
> base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
> change-id: 20231027-fix-rtl8366rb-e752bd5901ca
> 
> Best regards,
> -- 
> Linus Walleij <linus.walleij@linaro.org>
> 

Until more is known:

pw-bot: cr