Date:   Tue, 23 Mar 2021 21:03:02 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mv88e6xxx: Allow dynamic
 reconfiguration of tag protocol

On Tue, Mar 23, 2021 at 03:48:51PM +0100, Tobias Waldekranz wrote:
> On Tue, Mar 23, 2021 at 13:35, Vladimir Oltean <olteanv@...il.com> wrote:
> > The netdev_uses_dsa thing is a bit trashy, I think that a more polished
> > version should rather set NETIF_F_RXALL for the DSA master, and have the
> > dpaa driver act upon that. But first I'm curious if it works.
> 
> It does work. Thank you!

Happy to hear that.

> Does setting RXALL mean that the master would accept frames with a bad
> FCS as well?

Do you mean from the perspective of the network stack, or of the hardware?

As far as the hardware is concerned, here is what the manual has to say:

Frame reception from the network may encounter certain error conditions.
Such errors are reported by the Ethernet MAC when the frame is transferred
to the Buffer Manager Interface (BMI). The action taken per error case
is described below. Besides the interrupts, the BMI is capable of
recognizing several conditions and setting a corresponding flag in FD
status field for Host usage. These conditions are as follows:

* Physical Error. One of the following events were detected by the
  Ethernet MAC: Rx FIFO overflow, FCS error, code error, running
  disparity error (in applicable modes), FIFO parity error, PHY Sequence
  error, PHY error control character detected, CRC error. The BMI
  discards the frame, or enqueue directly to EFQID if FMBM_RCFG[FDOVR]
  is set [ editor's note: this is what my patch does ]. FPE bit is set
  in the FD status.
* Frame size error. The Ethernet MAC detected a frame that its length
  exceeds the maximum allowed as configured in the MAC registers. The
  frame is truncated by the MAC to the maximum allowed, and it is marked
  as truncated. The BMI sets FSE in the FD status and forwards the frame
  to next module in the FMan as usual.
* Some other network error may result in the frame being discarded by
  the MAC and not shown to the BMI. However, the MAC is responsible for
  counting such errors in its own statistics counters.

So yes, packets with bad FCS are accepted with FMBM_RCFG[FDOVR] set.
But it would be interesting to see what is the value of "fd_status" in
rx_default_dqrr() for bad packets. You know, in the DPAA world, the
correct approach to solve this problem would be to create a
configuration to describe a "soft examination sequence" for the
programmable hardware "soft parser", which identifies the DSA tag and
skips over a programmable number of octets. This allows you to be able
to continue to do things such as flow steering based on IP headers
located after the DSA tag, etc. This is not supported in the upstream
FMan driver however, neither the soft parser itself nor an abstraction
for making DSA masters DSA-aware. I think it would also require more
work than it took me to hack up this patch. But at least, if I
understand correctly, with a soft parser in place, the MAC error
counters should at least stop incrementing, if that is of any importance
to you.

> If so, would that mean that we would have to verify it in software?

I don't see any place in the network stack that recalculates the FCS if
NETIF_F_RXALL is set. Additionally, without NETIF_F_RXFCS, I don't even
know how could the stack even tell a packet with bad FCS apart from one
with good FCS. If NETIF_F_RXALL is set, then once a packet is received,
it's taken for granted as good.

There is a separate hardware bit to include the FCS in the RX buffer, I
don't think this is what you want/need.

> >> 
> >> As a workaround, switching to EDSA (thereby always having a proper
> >> EtherType in the frame) solves the issue.
> >
> > So basically every user needs to change the tag protocol manually to be
> > able to receive from port 8? Not sure if that's too friendly.
> 
> No it is not friendly at all. My goal was to add it as a device-tree
> property, but for reasons I will detail in my answer to Andrew, I did
> not manage to figure out a good way to do that. Happy to take
> suggestions.

My two cents here are that you should think for the long term. If you
need it due to a limitation which you have today but might no longer
have tomorrow, don't put it in the device tree unless you want to
support it even when you don't need it anymore.

Powered by blists - more mailing lists