| Message ID | 20210213223801.1334216-1-olteanv@gmail.com |
|---|---|
| Headers | show |
| Series | PTP for DSA tag_ocelot_8021q | expand |
On 2/13/2021 14:37, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > It appears that the intention of this snippet of code is to not exit > ocelot_xtr_irq_handler() while in the middle of extracting a frame. > The problem in extracting it word by word is that future extraction > attempts are really easy to get desynchronized, since the IRQ handler > assumes that the first 16 bytes are the IFH, which give further > information about the frame, such as frame length. > > But during normal operation, "err" will not be 0, but 4, set from here: > > for (i = 0; i < OCELOT_TAG_LEN / 4; i++) { > err = ocelot_rx_frame_word(ocelot, grp, true, &ifh[i]); > if (err != 4) > break; > } > > if (err != 4) > break; > > In that case, draining the extraction queue is a no-op. So explicitly > make this code execute only on negative err. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 2/13/2021 14:37, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The ocelot_rx_frame_word() function can return a negative error code, > however this isn't being checked for consistently. Errors being ignored > have not been seen in practice though. > > Also, some constructs can be simplified by using "goto" instead of > repeated "break" statements. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 2/13/2021 14:37, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Taggers should be written to do something valid irrespective of the > switch driver that they are attached to. This is even more true now, > because since the introduction of the .change_tag_protocol method, a > certain tagger is not necessarily strictly associated with a driver any > longer, and I would like to be able to test all taggers with dsa_loop in > the future. > > In the case of ocelot, it needs to move the classified VLAN from the DSA > tag into the skb if the port is VLAN-aware. We can allow it to do that > by looking at the dp->vlan_filtering property, no need to invoke > structures which are specific to ocelot. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 2/13/2021 14:37, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > There is one place where we cannot avoid accessing driver data, and that > is 2-step PTP TX timestamping, since the switch wants us to provide a > timestamp request ID through the injection header, which naturally must > come from a sequence number kept by the driver (it is generated by the > .port_txtstamp method prior to the tagger's xmit). > > However, since other drivers like dsa_loop do not claim PTP support > anyway, the DSA_SKB_CB(skb)->clone will always be NULL anyway, so if we > move all PTP-related dereferences of struct ocelot and struct ocelot_port > into a separate function, we can effectively ensure that this is dead > code when the ocelot tagger is attached to non-ocelot switches, and the > stateful portion of the tagger is more self-contained. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 2/13/2021 14:37, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Since the felix DSA driver will need to poll the CPU port module for > extracted frames as well, let's create some common functions that read > an Extraction Frame Header, and then an skb, from a CPU extraction > group. > > We abuse the struct ocelot_ops :: port_to_netdev function a little bit, > in order to retrieve the DSA port net_device or the ocelot switchdev > net_device based on the source port information from the Extraction > Frame Header, but it's all in the benefit of code simplification - > netdev_alloc_skb needs it. Originally, the port_to_netdev method was > intended for parsing act->dev from tc flower offload code. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 2/13/2021 14:38, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Since the tag_8021q tagger is software-defined, it has no means by > itself for retrieving hardware timestamps of PTP event messages. > > Because we do want to support PTP on ocelot even with tag_8021q, we need > to use the CPU port module for that. The RX timestamp is present in the > Extraction Frame Header. And because we can't use NPI mode which redirects > the CPU queues to an "external CPU" (meaning the ARM CPU running Linux), > then we need to poll the CPU port module through the MMIO registers to > retrieve TX and RX timestamps. > > Sadly, on NXP LS1028A, the Felix switch was integrated into the SoC > without wiring the extraction IRQ line to the ARM GIC. So, if we want to > be notified of any PTP packets received on the CPU port module, we have > a problem. > > There is a possible workaround, which is to use the Ethernet CPU port as > a notification channel that packets are available on the CPU port module > as well. When a PTP packet is received by the DSA tagger (without timestamp, > of course), we go to the CPU extraction queues, poll for it there, then > we drop the original Ethernet packet and masquerade the packet retrieved > over MMIO (plus the timestamp) as the original when we inject it up the > stack. > > Create a quirk in struct felix is selected by the Felix driver (but not > by Seville, since that doesn't support PTP at all). We want to do this > such that the workaround is minimally invasive for future switches that > don't require this workaround. > > The only traffic for which we need timestamps is PTP traffic, so add a > redirection rule to the CPU port module for this. Currently we only have > the need for PTP over L2, so redirection rules for UDP ports 319 and 320 > are TBD for now. > > Note that for the workaround of matching of PTP-over-Ethernet-port with > PTP-over-MMIO queues to work properly, both channels need to be > absolutely lossless. There are two parts to achieving that: > - We keep flow control enabled on the tag_8021q CPU port > - We put the DSA master interface in promiscuous mode, so it will never > drop a PTP frame (for the profiles we are interested in, these are > sent to the multicast MAC addresses of 01-80-c2-00-00-0e and > 01-1b-19-00-00-00). > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Sun, 14 Feb 2021 00:37:49 +0200 you wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Changes in v2: > Add stub definition for ocelot_port_inject_frame when switch driver is > not compiled in. > > This is part two of the errata workaround begun here: > https://patchwork.kernel.org/project/netdevbpf/cover/20210129010009.3959398-1-olteanv@gmail.com/ > > [...] Here is the summary with links: - [v2,net-next,01/12] net: mscc: ocelot: stop returning IRQ_NONE in ocelot_xtr_irq_handler https://git.kernel.org/netdev/net-next/c/f833ca293dd1 - [v2,net-next,02/12] net: mscc: ocelot: only drain extraction queue on error https://git.kernel.org/netdev/net-next/c/d7795f8f26d9 - [v2,net-next,03/12] net: mscc: ocelot: better error handling in ocelot_xtr_irq_handler https://git.kernel.org/netdev/net-next/c/a94306cea56f - [v2,net-next,04/12] net: mscc: ocelot: use DIV_ROUND_UP helper in ocelot_port_inject_frame https://git.kernel.org/netdev/net-next/c/5f016f42d342 - [v2,net-next,05/12] net: mscc: ocelot: refactor ocelot_port_inject_frame out of ocelot_port_xmit https://git.kernel.org/netdev/net-next/c/137ffbc4bb86 - [v2,net-next,06/12] net: dsa: tag_ocelot: avoid accessing ds->priv in ocelot_rcv https://git.kernel.org/netdev/net-next/c/8a678bb29bd2 - [v2,net-next,07/12] net: mscc: ocelot: use common tag parsing code with DSA https://git.kernel.org/netdev/net-next/c/40d3f295b5fe - [v2,net-next,08/12] net: dsa: tag_ocelot: single out PTP-related transmit tag processing https://git.kernel.org/netdev/net-next/c/62bf5fde5e14 - [v2,net-next,09/12] net: dsa: tag_ocelot: create separate tagger for Seville https://git.kernel.org/netdev/net-next/c/7c4bb540e917 - [v2,net-next,10/12] net: mscc: ocelot: refactor ocelot_xtr_irq_handler into ocelot_xtr_poll https://git.kernel.org/netdev/net-next/c/924ee317f724 - [v2,net-next,11/12] net: dsa: felix: setup MMIO filtering rules for PTP when using tag_8021q https://git.kernel.org/netdev/net-next/c/c8c0ba4fe247 - [v2,net-next,12/12] net: dsa: tag_ocelot_8021q: add support for PTP timestamping https://git.kernel.org/netdev/net-next/c/0a6f17c6ae21 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
From: Vladimir Oltean <vladimir.oltean@nxp.com> Changes in v2: Add stub definition for ocelot_port_inject_frame when switch driver is not compiled in. This is part two of the errata workaround begun here: https://patchwork.kernel.org/project/netdevbpf/cover/20210129010009.3959398-1-olteanv@gmail.com/ Now that we have basic traffic support when we operate the Ocelot DSA switches without an NPI port, it would be nice to regain some of the features lost due to the lack of the NPI port functionality. An important one is PTP timestamping, which is intimately tied to the DSA frame header added by the NPI port: on TX, we put a "timestamp request ID" in the Injection Frame Header, while on RX, the Extraction Frame Header contains a partial 32-bit PTP timestamp. Get rid of the NPI port and replace it with a VLAN-based tagger, and you lose PTP, right? Well, not quite, this is what this patch series is about. The NPI port is basically a regular Ethernet port configured to service the packets in and out of the switch's CPU port module (which has other non-DSA I/O mechanisms too, such as register-based MMIO and DMA). If we disable the NPI port, we can in theory still access the packets delivered to the CPU port module by doing exactly what the ocelot switchdev driver does: extracting Ethernet packets through registers (yes, it is as icky as it sounds). However, there's a catch. The Felix switch was integrated into NXP LS1028A with the idea in mind that it will operate as DSA, i.e. using the CPU port module connected to the NPI port, not having I/O over register-based MMIO which is painfully slow and CPU intensive. So register-based packet I/O not supposed to work - those registers aren't even documented in the hardware reference manual for Felix. However they kinda do, with the exception of the fact that an RX interrupt was really not wired to the CPU cores - so we don't know when the CPU port module receives a new packet. But we can hack even around that, by replicating every packet that goes to the CPU port module and making it also go to a plain internal Ethernet port. Then drop the Ethernet packet and read the other copy of it from the CPU port module, this time annotated with the much-wanted RX timestamp. This is all fine and it works, but it does raise some questions about what DSA even is anymore, if we start having switches that inject some of their packets over Ethernet and some through registers, where do we draw the line. In principle I believe these concerns are founded, but at the same time, the way that the Felix driver uses register MMIO based packet I/O is fundamentally the same as any other DSA driver capable of PTP makes use of a side-channel for timestamps like a FIFO (just that this one is a lot more complicated, and comes with the entire actual packet, not just the timestamp). Nonetheless, I tried to keep the extra pressure added by this ERR workaround upon the DSA subsystem as small as possible, so some of the patches are just a revisit of some of Andrew's complaints w.r.t. the fact that tag_ocelot already violates any driver <-> tagger boundary, and as a consequence, is not able to be used on testbeds such as dsa_loop (which it now can). So now, the tag_ocelot and tag_ocelot_8021q drivers should be dsa_loop-clean, and have the ERR workarounds as self-contained as possible, using all the designated features for PTP timestamping and nothing more. Comments appreciated. Vladimir Oltean (12): net: mscc: ocelot: stop returning IRQ_NONE in ocelot_xtr_irq_handler net: mscc: ocelot: only drain extraction queue on error net: mscc: ocelot: better error handling in ocelot_xtr_irq_handler net: mscc: ocelot: use DIV_ROUND_UP helper in ocelot_port_inject_frame net: mscc: ocelot: refactor ocelot_port_inject_frame out of ocelot_port_xmit net: dsa: tag_ocelot: avoid accessing ds->priv in ocelot_rcv net: mscc: ocelot: use common tag parsing code with DSA net: dsa: tag_ocelot: single out PTP-related transmit tag processing net: dsa: tag_ocelot: create separate tagger for Seville net: mscc: ocelot: refactor ocelot_xtr_irq_handler into ocelot_xtr_poll net: dsa: felix: setup MMIO filtering rules for PTP when using tag_8021q net: dsa: tag_ocelot_8021q: add support for PTP timestamping drivers/net/dsa/ocelot/felix.c | 224 +++++++++++++++++-- drivers/net/dsa/ocelot/felix.h | 14 +- drivers/net/dsa/ocelot/felix_vsc9959.c | 29 +-- drivers/net/dsa/ocelot/seville_vsc9953.c | 30 +-- drivers/net/ethernet/mscc/ocelot.c | 216 ++++++++++++++++++ drivers/net/ethernet/mscc/ocelot.h | 9 - drivers/net/ethernet/mscc/ocelot_net.c | 81 +------ drivers/net/ethernet/mscc/ocelot_vsc7514.c | 178 +-------------- include/linux/dsa/ocelot.h | 218 ++++++++++++++++++ include/net/dsa.h | 2 + include/soc/mscc/ocelot.h | 41 +++- net/dsa/tag_ocelot.c | 244 +++++++-------------- net/dsa/tag_ocelot_8021q.c | 34 +++ 13 files changed, 825 insertions(+), 495 deletions(-) create mode 100644 include/linux/dsa/ocelot.h