Message-ID: <20221017155536.hfetulaedgmvjoc5@skbuf>
Date:   Mon, 17 Oct 2022 18:55:36 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Sergei Antonov <saproj@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v3 net] net: ftmac100: support frames with DSA tag

On Thu, Oct 13, 2022 at 06:57:24PM +0300, Sergei Antonov wrote:
> Fixes the problem when frames coming from DSA were discarded.
> DSA tag might make frame size >1518. Such frames are discarded
> by the controller when FTMAC100_MACCR_RX_FTL is not set in the
> MAC Control Register, see datasheet [1].
> 
> Set FTMAC100_MACCR_RX_FTL in the MAC Control Register.
> For received packets marked with FTMAC100_RXDES0_FTL check if packet
> length (with FCS excluded) is within expected limits, that is not
> greater than netdev->mtu + 14 (Ethernet headers). Otherwise trigger
> an error. In the presence of DSA netdev->mtu is 1504 to accommodate
> for VLAN tag.

Which VLAN tag do you mean? Earlier you mentioned a tail tag as can be
seen and parsed by net/dsa/tag_trailer.c. Don't conflate the two, one
has nothing to do with the other.

I think this patch is at the limit of what can reasonably be considered a
bug fix, especially since inefficient use of resources does not constitute
a bug in itself. And the justification provided in the commit message
certainly does not help its cause.

DSA generally works on the assumption that netdev drivers need no change
to operate as DSA masters. However, in this particular case, it has
historically operated using the "wishful thinking" assumption that
dev_set_mtu(master, 1504) will always be enough to work, even if this
call fails (hence the reason for making its failure non-fatal).

More context (to supplement Andrew's message from Message-ID Y0gcblXFum4GsSve@...n.ch:
https://patchwork.ozlabs.org/project/netdev/patch/20200421123110.13733-2-olteanv@gmail.com/

My humble opinion is that "reasonable and noninvasive changes"
for drivers to work as DSA masters is a more desirable goal, and hence,
not every master <-> switch pair that doesn't work out of the box should
be considered a bug.

Here's how I would approach your particular issue:

1. retarget from "net" to "net-next"

2. observe the ftmac100 code. The RX_BUF_SIZE macro is set to 2044, with a
   comment that it must be smaller than 0x7ff.

3. compare with the datasheet. There it can be seen that RXBUF_SIZE is
   an 11-bit field, confirming that packets larger than 2048 bytes must
   be processed as scatter/gather (multiple BDs).

4. back to the code, the driver does not support scatter gather processing,
   but it does not push the MTU limit as far as single-buffer RX can go, either.
   It puts it to a quite random 1518, inconsistent in itself to the FTL
   field which drops frames with MTU larger than 1500 (so dev->mtu values between
   1500 and 1518 are incorrectly handled).
   It could go as far as 2047 - VLAN_ETH_HLEN, and the Frame Too Long
   bit should be unset to allow this.

5. the code currently relies on the FTL field to not trigger the BUG_ON()
   right below, if scatter/gather frames are received. But the FTL bit
   needs to go. I would completely remove the ftmac100_rxdes_frame_too_long()
   check from the fast path, instead of your approach to just make it
   more complicated. If you simply call ftmac100_rx_drop_packet() instead
   of BUG_ON() when receiving a BD which is non-final, you get a simpler
   way of protecting against multi-buffer frames, while not having to
   rely on the comparison between frame length and netdev->mtu at all.
   (side note: it isn't a problem if you receive larger frames than the
   MTU, as long as you receive the frames <= than the MTU).

6. Actually implement the ndo_change_mtu() for this driver, and even though you
   don't make any hardware change based on the precise value, you could do one
   important thing. Depending on whether the dev->mtu is <= 1500 or not, you could
   set or unset the FTL bit, and this could allow for less CPU cycles spent
   dropping S/G frames when the MTU of the interface is standard 1500.

With these changes, you would leave the ftmac100 driver in a more civilized
state, you wouldn't need to implement S/G RX unless you wanted to, and
as a side effect, it would also operate well as a DSA master.

Powered by blists - more mailing lists