Skip to content

L
linux
Switch branch/tag
  1. 14 Dec, 2021 5 commits
  3. 13 Dec, 2021 23 commits
  5. 12 Dec, 2021 12 commits
    • David S. Miller's avatar
      Merge branch 'dsa-tagger-storage' · 9b5bcb19
      David S. Miller authored 
      Vladimir Oltean says:

====================
Replace DSA dp->priv with tagger-owned storage

Ansuel's recent work on qca8k register access over Ethernet:
https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
has triggered me to do something which I should've done for a longer
time:
https://patchwork.kernel.org/project/netdevbpf/patch/20211109095013.27829-7-martin.kaistra@linutronix.de/#24585521
which is to replace dp->priv with something that has less caveats.

The dp->priv was introduced when sja1105 needed to hold stateful
information in the tagging protocol driver. In that design, dp->priv
held memory allocated by the switch driver, because the tagging protocol
driver design was 100% stateless.

Some years have passed and others have started to feel the need for
stateful information kept by the tagger, as well as passing data back
and forth between the tagging protocol driver and the switch driver.
This isn't possible cleanly in DSA due to a circular dependency which
leads to broken module autoloading:
https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/

This patchset introduces a framework that resembles something normal,
which allows data to be passed from the tagging protocol driver (things
like switch management packets, which aren't intended for the network
stack) to the switch driver, while the tagging protocol still remains
more or less stateless. The overall design of the framework was
discussed with Ansuel too and it appears to be flexible enough to cover
the "register access over Ethernet" use case. Additionally, the existing
uses of dp->priv, which have mainly to do with PTP timestamping, have
also been migrated.

Changes in v2:
Fix transient build breakage in patch 5/11 due to a missing parenthesis,
https://patchwork.hopto.org/static/nipa/592567/12665213/build_clang/
and another transient build warning in patch 4/11 that for some reason
doesn't appear in my W=1 C=1 build.
https://patchwork.hopto.org/static/nipa/592567/12665209/build_clang/stderr
====================
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      9b5bcb19
    • Vladimir Oltean's avatar
      net: dsa: remove dp->priv · 4f3cb343
      Vladimir Oltean authored 
      All current in-tree uses of dp->priv have been replaced with
ds->tagger_data, which provides for a safer API especially when the
connection isn't the regular 1:1 link between one switch driver and one
tagging protocol driver, but could be either one switch to many taggers,
or many switches to one tagger.

Therefore, we can remove this unused pointer.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      4f3cb343
    • Vladimir Oltean's avatar
      net: dsa: tag_sja1105: split sja1105_tagger_data into private and public sections · 950a419d
      Vladimir Oltean authored 
      The sja1105 driver messes with the tagging protocol's state when PTP RX
timestamping is enabled/disabled. This is fundamentally necessary
because the tagger needs to know what to do when it receives a PTP
packet. If RX timestamping is enabled, then a metadata follow-up frame
is expected, and this holds the (partial) timestamp. So the tagger plays
hide-and-seek with the network stack until it also gets the metadata
frame, and then presents a single packet, the timestamped PTP packet.
But when RX timestamping isn't enabled, there is no metadata frame
expected, so the hide-and-seek game must be turned off and the packet
must be delivered right away to the network stack.

Considering this, we create a pseudo isolation by devising two tagger
methods callable by the switch: one to get the RX timestamping state,
and one to set it. Since we can't export symbols between the tagger and
the switch driver, these methods are exposed through function pointers.

After this change, the public portion of the sja1105_tagger_data
contains only function pointers.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      950a419d
    • Vladimir Oltean's avatar
      Revert "net: dsa: move sja1110_process_meta_tstamp inside the tagging protocol driver" · fcbf979a
      Vladimir Oltean authored 
      This reverts commit 6d709cad.

The above change was done to avoid calling symbols exported by the
switch driver from the tagging protocol driver.

With the tagger-owned storage model, we have a new option on our hands,
and that is for the switch driver to provide a data consumer handler in
the form of a function pointer inside the ->connect_tag_protocol()
method. Having a function pointer avoids the problems of the exported
symbols approach.

By creating a handler for metadata frames holding TX timestamps on
SJA1110, we are able to eliminate an skb queue from the tagger data, and
replace it with a simple, and stateless, function pointer. This skb
queue is now handled exclusively by sja1105_ptp.c, which makes the code
easier to follow, as it used to be before the reverted patch.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      fcbf979a
    • Vladimir Oltean's avatar
      net: dsa: tag_sja1105: convert to tagger-owned data · c79e8486
      Vladimir Oltean authored 
      Currently, struct sja1105_tagger_data is a part of struct
sja1105_private, and is used by the sja1105 driver to populate dp->priv.

With the movement towards tagger-owned storage, the sja1105 driver
should not be the owner of this memory.

This change implements the connection between the sja1105 switch driver
and its tagging protocol, which means that sja1105_tagger_data no longer
stays in dp->priv but in ds->tagger_data, and that the sja1105 driver
now only populates the sja1105_port_deferred_xmit callback pointer.
The kthread worker is now the responsibility of the tagger.

The sja1105 driver also alters the tagger's state some more, especially
with regard to the PTP RX timestamping state. This will be fixed up a
bit in further changes.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      c79e8486
    • Vladimir Oltean's avatar
      net: dsa: sja1105: move ts_id from sja1105_tagger_data · 22ee9f8e
      Vladimir Oltean authored 
      The TX timestamp ID is incremented by the SJA1110 PTP timestamping
callback (->port_tx_timestamp) for every packet, when cloning it.
It isn't used by the tagger at all, even though it sits inside the
struct sja1105_tagger_data.

Also, serialization to this structure is currently done through
tagger_data->meta_lock, which is a cheap hack because the meta_lock
isn't used for anything else on SJA1110 (sja1105_rcv_meta_state_machine
isn't called).

This change moves ts_id from sja1105_tagger_data to sja1105_private and
introduces a dedicated spinlock for it, also in sja1105_private.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      22ee9f8e
    • Vladimir Oltean's avatar
      net: dsa: sja1105: make dp->priv point directly to sja1105_tagger_data · bfcf1425
      Vladimir Oltean authored 
      The design of the sja1105 tagger dp->priv is that each port has a
separate struct sja1105_port, and the sp->data pointer points to a
common struct sja1105_tagger_data.

We have removed all per-port members accessible by the tagger, and now
only struct sja1105_tagger_data remains. Make dp->priv point directly to
this.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      bfcf1425
    • Vladimir Oltean's avatar
      net: dsa: sja1105: remove hwts_tx_en from tagger data · 6f6770ab
      Vladimir Oltean authored 
      This tagger property is in fact not used at all by the tagger, only by
the switch driver. Therefore it makes sense to be moved to
sja1105_private.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      6f6770ab
    • Vladimir Oltean's avatar
      net: dsa: sja1105: bring deferred xmit implementation in line with ocelot-8021q · d38049bb
      Vladimir Oltean authored 
      When the ocelot-8021q driver was converted to deferred xmit as part of
commit 8d5f7954 ("net: dsa: felix: break at first CPU port during
init and teardown"), the deferred implementation was deliberately made
subtly different from what sja1105 has.

The implementation differences lied on the following observations:

- There might be a race between these two lines in tag_sja1105.c:

       skb_queue_tail(&sp->xmit_queue, skb_get(skb));
       kthread_queue_work(sp->xmit_worker, &sp->xmit_work);

  and the skb dequeue logic in sja1105_port_deferred_xmit(). For
  example, the xmit_work might be already queued, however the work item
  has just finished walking through the skb queue. Because we don't
  check the return code from kthread_queue_work, we don't do anything if
  the work item is already queued.

  However, nobody will take that skb and send it, at least until the
  next timestampable skb is sent. This creates additional (and
  avoidable) TX timestamping latency.

  To close that race, what the ocelot-8021q driver does is it doesn't
  keep a single work item per port, and a skb timestamping queue, but
  rather dynamically allocates a work item per packet.

- It is also unnecessary to have more than one kthread that does the
  work. So delete the per-port kthread allocations and replace them with
  a single kthread which is global to the switch.

This change brings the two implementations in line by applying those
observations to the sja1105 driver as well.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      d38049bb
    • Vladimir Oltean's avatar
      net: dsa: sja1105: let deferred packets time out when sent to ports going down · a3d74295
      Vladimir Oltean authored 
      This code is not necessary and complicates the conversion of this driver
to tagger-owned memory. If there is a PTP packet that is sent
concurrently with the port getting disabled, the deferred xmit mechanism
is robust enough to time out when it sees that it hasn't been delivered,
and recovers.
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      a3d74295
    • Vladimir Oltean's avatar
      net: dsa: tag_ocelot: convert to tagger-owned data · 35d97680
      Vladimir Oltean authored 
      The felix driver makes very light use of dp->priv, and the tagger is
effectively stateless. dp->priv is practically only needed to set up a
callback to perform deferred xmit of PTP and STP packets using the
ocelot-8021q tagging protocol (the main ocelot tagging protocol makes no
use of dp->priv, although this driver sets up dp->priv irrespective of
actual tagging protocol in use).

struct felix_port (what used to be pointed to by dp->priv) is removed
and replaced with a two-sided structure. The public side of this
structure, visible to the switch driver, is ocelot_8021q_tagger_data.
The private side is ocelot_8021q_tagger_private, and the latter
structure physically encapsulates the former. The public half of the
tagger data structure can be accessed through a helper of the same name
(ocelot_8021q_tagger_data) which also sanity-checks the protocol
currently in use by the switch. The public/private split was requested
by Andrew Lunn.
Suggested-by: default avatarAndrew Lunn <andrew@lunn.ch>
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      35d97680
    • Vladimir Oltean's avatar
      net: dsa: introduce tagger-owned storage for private and shared data · dc452a47
      Vladimir Oltean authored 
      Ansuel is working on register access over Ethernet for the qca8k switch
family. This requires the qca8k tagging protocol driver to receive
frames which aren't intended for the network stack, but instead for the
qca8k switch driver itself.

The dp->priv is currently the prevailing method for passing data back
and forth between the tagging protocol driver and the switch driver.
However, this method is riddled with caveats.

The DSA design allows in principle for any switch driver to return any
protocol it desires in ->get_tag_protocol(). The dsa_loop driver can be
modified to do just that. But in the current design, the memory behind
dp->priv has to be allocated by the switch driver, so if the tagging
protocol is paired to an unexpected switch driver, we may end up in NULL
pointer dereferences inside the kernel, or worse (a switch driver may
allocate dp->priv according to the expectations of a different tagger).

The latter possibility is even more plausible considering that DSA
switches can dynamically change tagging protocols in certain cases
(dsa <-> edsa, ocelot <-> ocelot-8021q), and the current design lends
itself to mistakes that are all too easy to make.

This patch proposes that the tagging protocol driver should manage its
own memory, instead of relying on the switch driver to do so.
After analyzing the different in-tree needs, it can be observed that the
required tagger storage is per switch, therefore a ds->tagger_data
pointer is introduced. In principle, per-port storage could also be
introduced, although there is no need for it at the moment. Future
changes will replace the current usage of dp->priv with ds->tagger_data.

We define a "binding" event between the DSA switch tree and the tagging
protocol. During this binding event, the tagging protocol's ->connect()
method is called first, and this may allocate some memory for each
switch of the tree. Then a cross-chip notifier is emitted for the
switches within that tree, and they are given the opportunity to fix up
the tagger's memory (for example, they might set up some function
pointers that represent virtual methods for consuming packets).
Because the memory is owned by the tagger, there exists a ->disconnect()
method for the tagger (which is the place to free the resources), but
there doesn't exist a ->disconnect() method for the switch driver.
This is part of the design. The switch driver should make minimal use of
the public part of the tagger data, and only after type-checking it
using the supplied "proto" argument.

In the code there are in fact two binding events, one is the initial
event in dsa_switch_setup_tag_protocol(). At this stage, the cross chip
notifier chains aren't initialized, so we call each switch's connect()
method by hand. Then there is dsa_tree_bind_tag_proto() during
dsa_tree_change_tag_proto(), and here we have an old protocol and a new
one. We first connect to the new one before disconnecting from the old
one, to simplify error handling a bit and to ensure we remain in a valid
state at all times.
Co-developed-by: default avatarAnsuel Smith <ansuelsmth@gmail.com>
Signed-off-by: default avatarAnsuel Smith <ansuelsmth@gmail.com>
Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      dc452a47
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7