* [PATCH net-next 01/11] net: dsa: introduce tagger-owned storage for private and shared data
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
@ 2021-12-08 20:04 ` Vladimir Oltean
2021-12-08 20:04 ` [PATCH net-next 02/11] net: dsa: tag_ocelot: convert to tagger-owned data Vladimir Oltean
` (9 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:04 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Ansuel Smith <ansuelsmth@gmail.com>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/net/dsa.h | 12 ++++++++
net/dsa/dsa2.c | 73 +++++++++++++++++++++++++++++++++++++++++++---
net/dsa/dsa_priv.h | 1 +
net/dsa/switch.c | 14 +++++++++
4 files changed, 96 insertions(+), 4 deletions(-)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index bdf308a5c55e..8b496c7e62ef 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -82,12 +82,15 @@ enum dsa_tag_protocol {
};
struct dsa_switch;
+struct dsa_switch_tree;
struct dsa_device_ops {
struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
void (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
int *offset);
+ int (*connect)(struct dsa_switch_tree *dst);
+ void (*disconnect)(struct dsa_switch_tree *dst);
unsigned int needed_headroom;
unsigned int needed_tailroom;
const char *name;
@@ -337,6 +340,8 @@ struct dsa_switch {
*/
void *priv;
+ void *tagger_data;
+
/*
* Configuration data for this switch.
*/
@@ -689,6 +694,13 @@ struct dsa_switch_ops {
enum dsa_tag_protocol mprot);
int (*change_tag_protocol)(struct dsa_switch *ds, int port,
enum dsa_tag_protocol proto);
+ /*
+ * Method for switch drivers to connect to the tagging protocol driver
+ * in current use. The switch driver can provide handlers for certain
+ * types of packets for switch management.
+ */
+ int (*connect_tag_protocol)(struct dsa_switch *ds,
+ enum dsa_tag_protocol proto);
/* Optional switch-wide initialization and destruction methods */
int (*setup)(struct dsa_switch *ds);
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 8814fa0e44c8..cf6566168620 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -248,8 +248,12 @@ static struct dsa_switch_tree *dsa_tree_alloc(int index)
static void dsa_tree_free(struct dsa_switch_tree *dst)
{
- if (dst->tag_ops)
+ if (dst->tag_ops) {
+ if (dst->tag_ops->disconnect)
+ dst->tag_ops->disconnect(dst);
+
dsa_tag_driver_put(dst->tag_ops);
+ }
list_del(&dst->list);
kfree(dst);
}
@@ -822,7 +826,7 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
int err;
if (tag_ops->proto == dst->default_proto)
- return 0;
+ goto connect;
dsa_switch_for_each_cpu_port(cpu_dp, ds) {
rtnl_lock();
@@ -836,6 +840,17 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
}
}
+connect:
+ if (ds->ops->connect_tag_protocol) {
+ err = ds->ops->connect_tag_protocol(ds, tag_ops->proto);
+ if (err) {
+ dev_err(ds->dev,
+ "Unable to connect to tag protocol \"%s\": %pe\n",
+ tag_ops->name, ERR_PTR(err));
+ return err;
+ }
+ }
+
return 0;
}
@@ -1136,6 +1151,46 @@ static void dsa_tree_teardown(struct dsa_switch_tree *dst)
dst->setup = false;
}
+static int dsa_tree_bind_tag_proto(struct dsa_switch_tree *dst,
+ const struct dsa_device_ops *tag_ops)
+{
+ const struct dsa_device_ops *old_tag_ops = dst->tag_ops;
+ struct dsa_notifier_tag_proto_info info;
+ int err;
+
+ dst->tag_ops = tag_ops;
+
+ /* Notify the new tagger about the connection to this tree */
+ if (tag_ops->connect) {
+ err = tag_ops->connect(dst);
+ if (err)
+ goto out_revert;
+ }
+
+ /* Notify the switches from this tree about the connection
+ * to the new tagger
+ */
+ info.tag_ops = tag_ops;
+ err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_CONNECT, &info);
+ if (err && err != -EOPNOTSUPP)
+ goto out_disconnect;
+
+ /* Notify the old tagger about the disconnection from this tree */
+ if (old_tag_ops->disconnect)
+ old_tag_ops->disconnect(dst);
+
+ return 0;
+
+out_disconnect:
+ /* Revert the new tagger's connection to this tree */
+ if (tag_ops->disconnect)
+ tag_ops->disconnect(dst);
+out_revert:
+ dst->tag_ops = old_tag_ops;
+
+ return err;
+}
+
/* Since the dsa/tagging sysfs device attribute is per master, the assumption
* is that all DSA switches within a tree share the same tagger, otherwise
* they would have formed disjoint trees (different "dsa,member" values).
@@ -1168,12 +1223,15 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
goto out_unlock;
}
+ /* Notify the tag protocol change */
info.tag_ops = tag_ops;
err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO, &info);
if (err)
- goto out_unwind_tagger;
+ return err;
- dst->tag_ops = tag_ops;
+ err = dsa_tree_bind_tag_proto(dst, tag_ops);
+ if (err)
+ goto out_unwind_tagger;
rtnl_unlock();
@@ -1260,6 +1318,7 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
struct dsa_switch_tree *dst = ds->dst;
const struct dsa_device_ops *tag_ops;
enum dsa_tag_protocol default_proto;
+ int err;
/* Find out which protocol the switch would prefer. */
default_proto = dsa_get_tag_protocol(dp, master);
@@ -1307,6 +1366,12 @@ static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
*/
dsa_tag_driver_put(tag_ops);
} else {
+ if (tag_ops->connect) {
+ err = tag_ops->connect(dst);
+ if (err)
+ return err;
+ }
+
dst->tag_ops = tag_ops;
}
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 38ce5129a33d..0db2b26b0c83 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -37,6 +37,7 @@ enum {
DSA_NOTIFIER_VLAN_DEL,
DSA_NOTIFIER_MTU,
DSA_NOTIFIER_TAG_PROTO,
+ DSA_NOTIFIER_TAG_PROTO_CONNECT,
DSA_NOTIFIER_MRP_ADD,
DSA_NOTIFIER_MRP_DEL,
DSA_NOTIFIER_MRP_ADD_RING_ROLE,
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 9c92edd96961..06948f536829 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -647,6 +647,17 @@ static int dsa_switch_change_tag_proto(struct dsa_switch *ds,
return 0;
}
+static int dsa_switch_connect_tag_proto(struct dsa_switch *ds,
+ struct dsa_notifier_tag_proto_info *info)
+{
+ const struct dsa_device_ops *tag_ops = info->tag_ops;
+
+ if (!ds->ops->connect_tag_protocol)
+ return -EOPNOTSUPP;
+
+ return ds->ops->connect_tag_protocol(ds, tag_ops->proto);
+}
+
static int dsa_switch_mrp_add(struct dsa_switch *ds,
struct dsa_notifier_mrp_info *info)
{
@@ -766,6 +777,9 @@ static int dsa_switch_event(struct notifier_block *nb,
case DSA_NOTIFIER_TAG_PROTO:
err = dsa_switch_change_tag_proto(ds, info);
break;
+ case DSA_NOTIFIER_TAG_PROTO_CONNECT:
+ err = dsa_switch_connect_tag_proto(ds, info);
+ break;
case DSA_NOTIFIER_MRP_ADD:
err = dsa_switch_mrp_add(ds, info);
break;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 02/11] net: dsa: tag_ocelot: convert to tagger-owned data
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
2021-12-08 20:04 ` [PATCH net-next 01/11] net: dsa: introduce tagger-owned storage for private and shared data Vladimir Oltean
@ 2021-12-08 20:04 ` Vladimir Oltean
2021-12-08 20:04 ` [PATCH net-next 03/11] net: dsa: sja1105: let deferred packets time out when sent to ports going down Vladimir Oltean
` (8 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:04 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/ocelot/felix.c | 64 +++++++----------------------
include/linux/dsa/ocelot.h | 12 +++++-
net/dsa/tag_ocelot_8021q.c | 73 ++++++++++++++++++++++++++++++++--
3 files changed, 94 insertions(+), 55 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index f76dcf0d369f..a52d0809619e 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1152,38 +1152,22 @@ static void felix_port_deferred_xmit(struct kthread_work *work)
kfree(xmit_work);
}
-static int felix_port_setup_tagger_data(struct dsa_switch *ds, int port)
+static int felix_connect_tag_protocol(struct dsa_switch *ds,
+ enum dsa_tag_protocol proto)
{
- struct dsa_port *dp = dsa_to_port(ds, port);
- struct ocelot *ocelot = ds->priv;
- struct felix *felix = ocelot_to_felix(ocelot);
- struct felix_port *felix_port;
+ struct ocelot_8021q_tagger_data *tagger_data;
- if (!dsa_port_is_user(dp))
+ switch (proto) {
+ case DSA_TAG_PROTO_OCELOT_8021Q:
+ tagger_data = ocelot_8021q_tagger_data(ds);
+ tagger_data->xmit_work_fn = felix_port_deferred_xmit;
return 0;
-
- felix_port = kzalloc(sizeof(*felix_port), GFP_KERNEL);
- if (!felix_port)
- return -ENOMEM;
-
- felix_port->xmit_worker = felix->xmit_worker;
- felix_port->xmit_work_fn = felix_port_deferred_xmit;
-
- dp->priv = felix_port;
-
- return 0;
-}
-
-static void felix_port_teardown_tagger_data(struct dsa_switch *ds, int port)
-{
- struct dsa_port *dp = dsa_to_port(ds, port);
- struct felix_port *felix_port = dp->priv;
-
- if (!felix_port)
- return;
-
- dp->priv = NULL;
- kfree(felix_port);
+ case DSA_TAG_PROTO_OCELOT:
+ case DSA_TAG_PROTO_SEVILLE:
+ return 0;
+ default:
+ return -EPROTONOSUPPORT;
+ }
}
/* Hardware initialization done here so that we can allocate structures with
@@ -1214,12 +1198,6 @@ static int felix_setup(struct dsa_switch *ds)
}
}
- felix->xmit_worker = kthread_create_worker(0, "felix_xmit");
- if (IS_ERR(felix->xmit_worker)) {
- err = PTR_ERR(felix->xmit_worker);
- goto out_deinit_timestamp;
- }
-
for (port = 0; port < ds->num_ports; port++) {
if (dsa_is_unused_port(ds, port))
continue;
@@ -1230,14 +1208,6 @@ static int felix_setup(struct dsa_switch *ds)
* bits of vlan tag.
*/
felix_port_qos_map_init(ocelot, port);
-
- err = felix_port_setup_tagger_data(ds, port);
- if (err) {
- dev_err(ds->dev,
- "port %d failed to set up tagger data: %pe\n",
- port, ERR_PTR(err));
- goto out_deinit_ports;
- }
}
err = ocelot_devlink_sb_register(ocelot);
@@ -1265,13 +1235,9 @@ static int felix_setup(struct dsa_switch *ds)
if (dsa_is_unused_port(ds, port))
continue;
- felix_port_teardown_tagger_data(ds, port);
ocelot_deinit_port(ocelot, port);
}
- kthread_destroy_worker(felix->xmit_worker);
-
-out_deinit_timestamp:
ocelot_deinit_timestamp(ocelot);
ocelot_deinit(ocelot);
@@ -1300,12 +1266,9 @@ static void felix_teardown(struct dsa_switch *ds)
if (dsa_is_unused_port(ds, port))
continue;
- felix_port_teardown_tagger_data(ds, port);
ocelot_deinit_port(ocelot, port);
}
- kthread_destroy_worker(felix->xmit_worker);
-
ocelot_devlink_sb_unregister(ocelot);
ocelot_deinit_timestamp(ocelot);
ocelot_deinit(ocelot);
@@ -1645,6 +1608,7 @@ felix_mrp_del_ring_role(struct dsa_switch *ds, int port,
const struct dsa_switch_ops felix_switch_ops = {
.get_tag_protocol = felix_get_tag_protocol,
.change_tag_protocol = felix_change_tag_protocol,
+ .connect_tag_protocol = felix_connect_tag_protocol,
.setup = felix_setup,
.teardown = felix_teardown,
.set_ageing_time = felix_set_ageing_time,
diff --git a/include/linux/dsa/ocelot.h b/include/linux/dsa/ocelot.h
index 7ee708ad7df2..dca2969015d8 100644
--- a/include/linux/dsa/ocelot.h
+++ b/include/linux/dsa/ocelot.h
@@ -8,6 +8,7 @@
#include <linux/kthread.h>
#include <linux/packing.h>
#include <linux/skbuff.h>
+#include <net/dsa.h>
struct ocelot_skb_cb {
struct sk_buff *clone;
@@ -168,11 +169,18 @@ struct felix_deferred_xmit_work {
struct kthread_work work;
};
-struct felix_port {
+struct ocelot_8021q_tagger_data {
void (*xmit_work_fn)(struct kthread_work *work);
- struct kthread_worker *xmit_worker;
};
+static inline struct ocelot_8021q_tagger_data *
+ocelot_8021q_tagger_data(struct dsa_switch *ds)
+{
+ BUG_ON(ds->dst->tag_ops->proto != DSA_TAG_PROTO_OCELOT_8021Q);
+
+ return ds->tagger_data;
+}
+
static inline void ocelot_xfh_get_rew_val(void *extraction, u64 *rew_val)
{
packing(extraction, rew_val, 116, 85, OCELOT_TAG_LEN, UNPACK, 0);
diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c
index a1919ea5e828..fe451f4de7ba 100644
--- a/net/dsa/tag_ocelot_8021q.c
+++ b/net/dsa/tag_ocelot_8021q.c
@@ -12,25 +12,39 @@
#include <linux/dsa/ocelot.h>
#include "dsa_priv.h"
+struct ocelot_8021q_tagger_private {
+ struct ocelot_8021q_tagger_data data; /* Must be first */
+ struct kthread_worker *xmit_worker;
+};
+
static struct sk_buff *ocelot_defer_xmit(struct dsa_port *dp,
struct sk_buff *skb)
{
+ struct ocelot_8021q_tagger_private *priv = dp->ds->tagger_data;
+ struct ocelot_8021q_tagger_data *data = &priv->data;
+ void (*xmit_work_fn)(struct kthread_work *work);
struct felix_deferred_xmit_work *xmit_work;
- struct felix_port *felix_port = dp->priv;
+ struct kthread_worker *xmit_worker;
+
+ xmit_work_fn = data->xmit_work_fn;
+ xmit_worker = priv->xmit_worker;
+
+ if (!xmit_work_fn || !xmit_worker)
+ return NULL;
xmit_work = kzalloc(sizeof(*xmit_work), GFP_ATOMIC);
if (!xmit_work)
return NULL;
/* Calls felix_port_deferred_xmit in felix.c */
- kthread_init_work(&xmit_work->work, felix_port->xmit_work_fn);
+ kthread_init_work(&xmit_work->work, xmit_work_fn);
/* Increase refcount so the kfree_skb in dsa_slave_xmit
* won't really free the packet.
*/
xmit_work->dp = dp;
xmit_work->skb = skb_get(skb);
- kthread_queue_work(felix_port->xmit_worker, &xmit_work->work);
+ kthread_queue_work(xmit_worker, &xmit_work->work);
return NULL;
}
@@ -67,11 +81,64 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
return skb;
}
+static void ocelot_disconnect(struct dsa_switch_tree *dst)
+{
+ struct ocelot_8021q_tagger_private *priv;
+ struct dsa_port *dp;
+
+ list_for_each_entry(dp, &dst->ports, list) {
+ priv = dp->ds->tagger_data;
+
+ if (!priv)
+ continue;
+
+ if (priv->xmit_worker)
+ kthread_destroy_worker(priv->xmit_worker);
+
+ kfree(priv);
+ dp->ds->tagger_data = NULL;
+ }
+}
+
+static int ocelot_connect(struct dsa_switch_tree *dst)
+{
+ struct ocelot_8021q_tagger_private *priv;
+ struct dsa_port *dp;
+ int err;
+
+ list_for_each_entry(dp, &dst->ports, list) {
+ if (dp->ds->tagger_data)
+ continue;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ priv->xmit_worker = kthread_create_worker(0, "felix_xmit");
+ if (IS_ERR(priv->xmit_worker)) {
+ err = PTR_ERR(priv->xmit_worker);
+ goto out;
+ }
+
+ dp->ds->tagger_data = priv;
+ }
+
+ return 0;
+
+out:
+ ocelot_disconnect(dst);
+ return err;
+}
+
static const struct dsa_device_ops ocelot_8021q_netdev_ops = {
.name = "ocelot-8021q",
.proto = DSA_TAG_PROTO_OCELOT_8021Q,
.xmit = ocelot_xmit,
.rcv = ocelot_rcv,
+ .connect = ocelot_connect,
+ .disconnect = ocelot_disconnect,
.needed_headroom = VLAN_HLEN,
.promisc_on_master = true,
};
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 03/11] net: dsa: sja1105: let deferred packets time out when sent to ports going down
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
2021-12-08 20:04 ` [PATCH net-next 01/11] net: dsa: introduce tagger-owned storage for private and shared data Vladimir Oltean
2021-12-08 20:04 ` [PATCH net-next 02/11] net: dsa: tag_ocelot: convert to tagger-owned data Vladimir Oltean
@ 2021-12-08 20:04 ` Vladimir Oltean
2021-12-08 20:04 ` [PATCH net-next 04/11] net: dsa: sja1105: bring in line deferred xmit implementation with ocelot-8021q Vladimir Oltean
` (7 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:04 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/sja1105_main.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index cefde41ce8d6..f7c88da377e4 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2617,18 +2617,6 @@ static int sja1105_prechangeupper(struct dsa_switch *ds, int port,
return 0;
}
-static void sja1105_port_disable(struct dsa_switch *ds, int port)
-{
- struct sja1105_private *priv = ds->priv;
- struct sja1105_port *sp = &priv->ports[port];
-
- if (!dsa_is_user_port(ds, port))
- return;
-
- kthread_cancel_work_sync(&sp->xmit_work);
- skb_queue_purge(&sp->xmit_queue);
-}
-
static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
struct sk_buff *skb, bool takets)
{
@@ -3215,7 +3203,6 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
.get_ethtool_stats = sja1105_get_ethtool_stats,
.get_sset_count = sja1105_get_sset_count,
.get_ts_info = sja1105_get_ts_info,
- .port_disable = sja1105_port_disable,
.port_fdb_dump = sja1105_fdb_dump,
.port_fdb_add = sja1105_fdb_add,
.port_fdb_del = sja1105_fdb_del,
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 04/11] net: dsa: sja1105: bring in line deferred xmit implementation with ocelot-8021q
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
` (2 preceding siblings ...)
2021-12-08 20:04 ` [PATCH net-next 03/11] net: dsa: sja1105: let deferred packets time out when sent to ports going down Vladimir Oltean
@ 2021-12-08 20:04 ` Vladimir Oltean
2021-12-08 20:04 ` [PATCH net-next 05/11] net: dsa: sja1105: remove hwts_tx_en from tagger data Vladimir Oltean
` (6 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:04 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
When the ocelot-8021q driver was converted to deferred xmit as part of
commit 8d5f7954b7c8 ("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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/sja1105_main.c | 75 +++++++++++---------------
include/linux/dsa/sja1105.h | 11 ++--
net/dsa/tag_sja1105.c | 21 ++++++--
3 files changed, 56 insertions(+), 51 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index f7c88da377e4..5c486bd2bc61 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2675,10 +2675,8 @@ static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
return NETDEV_TX_OK;
}
-#define work_to_port(work) \
- container_of((work), struct sja1105_port, xmit_work)
-#define tagger_to_sja1105(t) \
- container_of((t), struct sja1105_private, tagger_data)
+#define work_to_xmit_work(w) \
+ container_of((w), struct sja1105_deferred_xmit_work, work)
/* Deferred work is unfortunately necessary because setting up the management
* route cannot be done from atomit context (SPI transfer takes a sleepable
@@ -2686,25 +2684,25 @@ static int sja1105_mgmt_xmit(struct dsa_switch *ds, int port, int slot,
*/
static void sja1105_port_deferred_xmit(struct kthread_work *work)
{
- struct sja1105_port *sp = work_to_port(work);
- struct sja1105_tagger_data *tagger_data = sp->data;
- struct sja1105_private *priv = tagger_to_sja1105(tagger_data);
- int port = sp - priv->ports;
- struct sk_buff *skb;
+ struct sja1105_deferred_xmit_work *xmit_work = work_to_xmit_work(work);
+ struct sk_buff *clone, *skb = xmit_work->skb;
+ struct dsa_switch *ds = xmit_work->dp->ds;
+ struct sja1105_private *priv = ds->priv;
+ int port = xmit_work->dp->index;
- while ((skb = skb_dequeue(&sp->xmit_queue)) != NULL) {
- struct sk_buff *clone = SJA1105_SKB_CB(skb)->clone;
+ clone = SJA1105_SKB_CB(skb)->clone;
- mutex_lock(&priv->mgmt_lock);
+ mutex_lock(&priv->mgmt_lock);
- sja1105_mgmt_xmit(priv->ds, port, 0, skb, !!clone);
+ sja1105_mgmt_xmit(ds, port, 0, skb, !!clone);
- /* The clone, if there, was made by dsa_skb_tx_timestamp */
- if (clone)
- sja1105_ptp_txtstamp_skb(priv->ds, port, clone);
+ /* The clone, if there, was made by dsa_skb_tx_timestamp */
+ if (clone)
+ sja1105_ptp_txtstamp_skb(ds, port, clone);
- mutex_unlock(&priv->mgmt_lock);
- }
+ mutex_unlock(&priv->mgmt_lock);
+
+ kfree(xmit_work);
}
/* The MAXAGE setting belongs to the L2 Forwarding Parameters table,
@@ -3009,54 +3007,43 @@ static int sja1105_port_bridge_flags(struct dsa_switch *ds, int port,
static void sja1105_teardown_ports(struct sja1105_private *priv)
{
- struct dsa_switch *ds = priv->ds;
- int port;
-
- for (port = 0; port < ds->num_ports; port++) {
- struct sja1105_port *sp = &priv->ports[port];
+ struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
- if (sp->xmit_worker)
- kthread_destroy_worker(sp->xmit_worker);
- }
+ kthread_destroy_worker(tagger_data->xmit_worker);
}
static int sja1105_setup_ports(struct sja1105_private *priv)
{
struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
struct dsa_switch *ds = priv->ds;
+ struct kthread_worker *worker;
int port, rc;
+ worker = kthread_create_worker(0, "dsa%d:%d_xmit", ds->dst->index,
+ ds->index);
+ if (IS_ERR(worker)) {
+ dev_err(ds->dev,
+ "failed to create deferred xmit thread: %d\n",
+ rc);
+ return PTR_ERR(worker);
+ }
+
+ tagger_data->xmit_worker = worker;
+ tagger_data->xmit_work_fn = sja1105_port_deferred_xmit;
+
/* Connections between dsa_port and sja1105_port */
for (port = 0; port < ds->num_ports; port++) {
struct sja1105_port *sp = &priv->ports[port];
struct dsa_port *dp = dsa_to_port(ds, port);
- struct kthread_worker *worker;
- struct net_device *slave;
if (!dsa_port_is_user(dp))
continue;
dp->priv = sp;
sp->data = tagger_data;
- slave = dp->slave;
- kthread_init_work(&sp->xmit_work, sja1105_port_deferred_xmit);
- worker = kthread_create_worker(0, "%s_xmit", slave->name);
- if (IS_ERR(worker)) {
- rc = PTR_ERR(worker);
- dev_err(ds->dev,
- "failed to create deferred xmit thread: %d\n",
- rc);
- goto out_destroy_workers;
- }
- sp->xmit_worker = worker;
- skb_queue_head_init(&sp->xmit_queue);
}
return 0;
-
-out_destroy_workers:
- sja1105_teardown_ports(priv);
- return rc;
}
/* The programming model for the SJA1105 switch is "all-at-once" via static
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index e6c78be40bde..acd9d2afccab 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -37,6 +37,12 @@
#define SJA1105_HWTS_RX_EN 0
+struct sja1105_deferred_xmit_work {
+ struct dsa_port *dp;
+ struct sk_buff *skb;
+ struct kthread_work work;
+};
+
/* Global tagger data: each struct sja1105_port has a reference to
* the structure defined in struct sja1105_private.
*/
@@ -52,6 +58,8 @@ struct sja1105_tagger_data {
* 2-step TX timestamps
*/
struct sk_buff_head skb_txtstamp_queue;
+ struct kthread_worker *xmit_worker;
+ void (*xmit_work_fn)(struct kthread_work *work);
};
struct sja1105_skb_cb {
@@ -65,9 +73,6 @@ struct sja1105_skb_cb {
((struct sja1105_skb_cb *)((skb)->cb))
struct sja1105_port {
- struct kthread_worker *xmit_worker;
- struct kthread_work xmit_work;
- struct sk_buff_head xmit_queue;
struct sja1105_tagger_data *data;
bool hwts_tx_en;
};
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 6c293c2a3008..7008952b6c1d 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -125,16 +125,29 @@ static inline bool sja1105_is_meta_frame(const struct sk_buff *skb)
static struct sk_buff *sja1105_defer_xmit(struct dsa_port *dp,
struct sk_buff *skb)
{
+ void (*xmit_work_fn)(struct kthread_work *work);
+ struct sja1105_deferred_xmit_work *xmit_work;
struct sja1105_port *sp = dp->priv;
+ struct kthread_worker *xmit_worker;
- if (!dsa_port_is_sja1105(dp))
- return skb;
+ xmit_work_fn = sp->data->xmit_work_fn;
+ xmit_worker = sp->data->xmit_worker;
+
+ if (!xmit_work_fn || !xmit_worker)
+ return NULL;
+ xmit_work = kzalloc(sizeof(*xmit_work), GFP_ATOMIC);
+ if (!xmit_work)
+ return NULL;
+
+ kthread_init_work(&xmit_work->work, xmit_work_fn);
/* Increase refcount so the kfree_skb in dsa_slave_xmit
* won't really free the packet.
*/
- skb_queue_tail(&sp->xmit_queue, skb_get(skb));
- kthread_queue_work(sp->xmit_worker, &sp->xmit_work);
+ xmit_work->dp = dp;
+ xmit_work->skb = skb_get(skb);
+
+ kthread_queue_work(xmit_worker, &xmit_work->work);
return NULL;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 05/11] net: dsa: sja1105: remove hwts_tx_en from tagger data
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
` (3 preceding siblings ...)
2021-12-08 20:04 ` [PATCH net-next 04/11] net: dsa: sja1105: bring in line deferred xmit implementation with ocelot-8021q Vladimir Oltean
@ 2021-12-08 20:04 ` Vladimir Oltean
2021-12-09 0:13 ` Jakub Kicinski
2021-12-08 20:04 ` [PATCH net-next 06/11] net: dsa: sja1105: make dp->priv point directly to sja1105_tagger_data Vladimir Oltean
` (5 subsequent siblings)
10 siblings, 1 reply; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:04 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/sja1105.h | 1 +
drivers/net/dsa/sja1105/sja1105_ptp.c | 9 ++++-----
include/linux/dsa/sja1105.h | 1 -
3 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 21dba16af097..b0612c763ec0 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -249,6 +249,7 @@ struct sja1105_private {
bool fixed_link[SJA1105_MAX_NUM_PORTS];
unsigned long ucast_egress_floods;
unsigned long bcast_egress_floods;
+ unsigned long hwts_tx_en;
const struct sja1105_info *info;
size_t max_xfer_len;
struct spi_device *spidev;
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 54396992a919..ea41cee805b0 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -98,10 +98,10 @@ int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
switch (config.tx_type) {
case HWTSTAMP_TX_OFF:
- priv->ports[port].hwts_tx_en = false;
+ priv->hwts_tx_en &= ~BIT(port);
break;
case HWTSTAMP_TX_ON:
- priv->ports[port].hwts_tx_en = true;
+ priv->hwts_tx_en |= BIT(port);
break;
default:
return -ERANGE;
@@ -140,7 +140,7 @@ int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
struct hwtstamp_config config;
config.flags = 0;
- if (priv->ports[port].hwts_tx_en)
+ if (priv->hwts_tx_en & BIT(port))
config.tx_type = HWTSTAMP_TX_ON;
else
config.tx_type = HWTSTAMP_TX_OFF;
@@ -486,10 +486,9 @@ void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
void sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
{
struct sja1105_private *priv = ds->priv;
- struct sja1105_port *sp = &priv->ports[port];
struct sk_buff *clone;
- if (!sp->hwts_tx_en)
+ if (!(priv->hwts_tx_en & BIT(port))
return;
clone = skb_clone_sk(skb);
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index acd9d2afccab..32a8a1344cf6 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -74,7 +74,6 @@ struct sja1105_skb_cb {
struct sja1105_port {
struct sja1105_tagger_data *data;
- bool hwts_tx_en;
};
/* Timestamps are in units of 8 ns clock ticks (equivalent to
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH net-next 05/11] net: dsa: sja1105: remove hwts_tx_en from tagger data
2021-12-08 20:04 ` [PATCH net-next 05/11] net: dsa: sja1105: remove hwts_tx_en from tagger data Vladimir Oltean
@ 2021-12-09 0:13 ` Jakub Kicinski
2021-12-09 0:14 ` Vladimir Oltean
0 siblings, 1 reply; 14+ messages in thread
From: Jakub Kicinski @ 2021-12-09 0:13 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, David S. Miller, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
On Wed, 8 Dec 2021 22:04:58 +0200 Vladimir Oltean wrote:
> 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.
This one transiently breaks build.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 05/11] net: dsa: sja1105: remove hwts_tx_en from tagger data
2021-12-09 0:13 ` Jakub Kicinski
@ 2021-12-09 0:14 ` Vladimir Oltean
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-09 0:14 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, David S. Miller, Andrew Lunn, Vivien Didelot,
Florian Fainelli, martin.kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
On Wed, Dec 08, 2021 at 04:13:28PM -0800, Jakub Kicinski wrote:
> On Wed, 8 Dec 2021 22:04:58 +0200 Vladimir Oltean wrote:
> > 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.
>
> This one transiently breaks build.
I was just looking at it how it does exactly that and was wondering
whether I should resend or not.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 06/11] net: dsa: sja1105: make dp->priv point directly to sja1105_tagger_data
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
` (4 preceding siblings ...)
2021-12-08 20:04 ` [PATCH net-next 05/11] net: dsa: sja1105: remove hwts_tx_en from tagger data Vladimir Oltean
@ 2021-12-08 20:04 ` Vladimir Oltean
2021-12-08 20:05 ` [PATCH net-next 07/11] net: dsa: sja1105: move ts_id from sja1105_tagger_data Vladimir Oltean
` (4 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:04 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/sja1105.h | 1 -
drivers/net/dsa/sja1105/sja1105_main.c | 16 +++------
drivers/net/dsa/sja1105/sja1105_ptp.c | 16 +++++----
include/linux/dsa/sja1105.h | 8 +----
net/dsa/tag_sja1105.c | 48 ++++++++++++++------------
5 files changed, 39 insertions(+), 50 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index b0612c763ec0..6ef6fb4f30e6 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -257,7 +257,6 @@ struct sja1105_private {
u16 bridge_pvid[SJA1105_MAX_NUM_PORTS];
u16 tag_8021q_pvid[SJA1105_MAX_NUM_PORTS];
struct sja1105_flow_block flow_block;
- struct sja1105_port ports[SJA1105_MAX_NUM_PORTS];
/* Serializes transmission of management frames so that
* the switch doesn't confuse them with one another.
*/
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 5c486bd2bc61..4f3350df7f4d 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3017,7 +3017,8 @@ static int sja1105_setup_ports(struct sja1105_private *priv)
struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
struct dsa_switch *ds = priv->ds;
struct kthread_worker *worker;
- int port, rc;
+ struct dsa_port *dp;
+ int rc;
worker = kthread_create_worker(0, "dsa%d:%d_xmit", ds->dst->index,
ds->index);
@@ -3031,17 +3032,8 @@ static int sja1105_setup_ports(struct sja1105_private *priv)
tagger_data->xmit_worker = worker;
tagger_data->xmit_work_fn = sja1105_port_deferred_xmit;
- /* Connections between dsa_port and sja1105_port */
- for (port = 0; port < ds->num_ports; port++) {
- struct sja1105_port *sp = &priv->ports[port];
- struct dsa_port *dp = dsa_to_port(ds, port);
-
- if (!dsa_port_is_user(dp))
- continue;
-
- dp->priv = sp;
- sp->data = tagger_data;
- }
+ dsa_switch_for_each_user_port(dp, ds)
+ dp->priv = tagger_data;
return 0;
}
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index ea41cee805b0..9077067328c2 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -461,22 +461,24 @@ void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
{
struct sk_buff *clone = SJA1105_SKB_CB(skb)->clone;
struct sja1105_private *priv = ds->priv;
- struct sja1105_port *sp = &priv->ports[port];
+ struct sja1105_tagger_data *tagger_data;
u8 ts_id;
+ tagger_data = &priv->tagger_data;
+
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
- spin_lock(&sp->data->meta_lock);
+ spin_lock(&tagger_data->meta_lock);
- ts_id = sp->data->ts_id;
+ ts_id = tagger_data->ts_id;
/* Deal automatically with 8-bit wraparound */
- sp->data->ts_id++;
+ tagger_data->ts_id++;
SJA1105_SKB_CB(clone)->ts_id = ts_id;
- spin_unlock(&sp->data->meta_lock);
+ spin_unlock(&tagger_data->meta_lock);
- skb_queue_tail(&sp->data->skb_txtstamp_queue, clone);
+ skb_queue_tail(&tagger_data->skb_txtstamp_queue, clone);
}
/* Called from dsa_skb_tx_timestamp. This callback is just to clone
@@ -488,7 +490,7 @@ void sja1105_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
struct sja1105_private *priv = ds->priv;
struct sk_buff *clone;
- if (!(priv->hwts_tx_en & BIT(port))
+ if (!(priv->hwts_tx_en & BIT(port)))
return;
clone = skb_clone_sk(skb);
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index 32a8a1344cf6..1dda9cce85d9 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -43,9 +43,7 @@ struct sja1105_deferred_xmit_work {
struct kthread_work work;
};
-/* Global tagger data: each struct sja1105_port has a reference to
- * the structure defined in struct sja1105_private.
- */
+/* Global tagger data */
struct sja1105_tagger_data {
struct sk_buff *stampable_skb;
/* Protects concurrent access to the meta state machine
@@ -72,10 +70,6 @@ struct sja1105_skb_cb {
#define SJA1105_SKB_CB(skb) \
((struct sja1105_skb_cb *)((skb)->cb))
-struct sja1105_port {
- struct sja1105_tagger_data *data;
-};
-
/* Timestamps are in units of 8 ns clock ticks (equivalent to
* a fixed 125 MHz clock).
*/
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index 7008952b6c1d..fc2af71b965c 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -125,13 +125,13 @@ static inline bool sja1105_is_meta_frame(const struct sk_buff *skb)
static struct sk_buff *sja1105_defer_xmit(struct dsa_port *dp,
struct sk_buff *skb)
{
+ struct sja1105_tagger_data *tagger_data = dp->priv;
void (*xmit_work_fn)(struct kthread_work *work);
struct sja1105_deferred_xmit_work *xmit_work;
- struct sja1105_port *sp = dp->priv;
struct kthread_worker *xmit_worker;
- xmit_work_fn = sp->data->xmit_work_fn;
- xmit_worker = sp->data->xmit_worker;
+ xmit_work_fn = tagger_data->xmit_work_fn;
+ xmit_worker = tagger_data->xmit_worker;
if (!xmit_work_fn || !xmit_worker)
return NULL;
@@ -368,32 +368,32 @@ static struct sk_buff
*/
if (is_link_local) {
struct dsa_port *dp = dsa_slave_to_port(skb->dev);
- struct sja1105_port *sp = dp->priv;
+ struct sja1105_tagger_data *tagger_data = dp->priv;
if (unlikely(!dsa_port_is_sja1105(dp)))
return skb;
- if (!test_bit(SJA1105_HWTS_RX_EN, &sp->data->state))
+ if (!test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
/* Do normal processing. */
return skb;
- spin_lock(&sp->data->meta_lock);
+ spin_lock(&tagger_data->meta_lock);
/* Was this a link-local frame instead of the meta
* that we were expecting?
*/
- if (sp->data->stampable_skb) {
+ if (tagger_data->stampable_skb) {
dev_err_ratelimited(dp->ds->dev,
"Expected meta frame, is %12llx "
"in the DSA master multicast filter?\n",
SJA1105_META_DMAC);
- kfree_skb(sp->data->stampable_skb);
+ kfree_skb(tagger_data->stampable_skb);
}
/* Hold a reference to avoid dsa_switch_rcv
* from freeing the skb.
*/
- sp->data->stampable_skb = skb_get(skb);
- spin_unlock(&sp->data->meta_lock);
+ tagger_data->stampable_skb = skb_get(skb);
+ spin_unlock(&tagger_data->meta_lock);
/* Tell DSA we got nothing */
return NULL;
@@ -406,7 +406,7 @@ static struct sk_buff
*/
} else if (is_meta) {
struct dsa_port *dp = dsa_slave_to_port(skb->dev);
- struct sja1105_port *sp = dp->priv;
+ struct sja1105_tagger_data *tagger_data = dp->priv;
struct sk_buff *stampable_skb;
if (unlikely(!dsa_port_is_sja1105(dp)))
@@ -415,13 +415,13 @@ static struct sk_buff
/* Drop the meta frame if we're not in the right state
* to process it.
*/
- if (!test_bit(SJA1105_HWTS_RX_EN, &sp->data->state))
+ if (!test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
return NULL;
- spin_lock(&sp->data->meta_lock);
+ spin_lock(&tagger_data->meta_lock);
- stampable_skb = sp->data->stampable_skb;
- sp->data->stampable_skb = NULL;
+ stampable_skb = tagger_data->stampable_skb;
+ tagger_data->stampable_skb = NULL;
/* Was this a meta frame instead of the link-local
* that we were expecting?
@@ -429,14 +429,14 @@ static struct sk_buff
if (!stampable_skb) {
dev_err_ratelimited(dp->ds->dev,
"Unexpected meta frame\n");
- spin_unlock(&sp->data->meta_lock);
+ spin_unlock(&tagger_data->meta_lock);
return NULL;
}
if (stampable_skb->dev != skb->dev) {
dev_err_ratelimited(dp->ds->dev,
"Meta frame on wrong port\n");
- spin_unlock(&sp->data->meta_lock);
+ spin_unlock(&tagger_data->meta_lock);
return NULL;
}
@@ -447,7 +447,7 @@ static struct sk_buff
skb = stampable_skb;
sja1105_transfer_meta(skb, meta);
- spin_unlock(&sp->data->meta_lock);
+ spin_unlock(&tagger_data->meta_lock);
}
return skb;
@@ -545,8 +545,8 @@ static void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port,
{
struct sk_buff *skb, *skb_tmp, *skb_match = NULL;
struct dsa_port *dp = dsa_to_port(ds, port);
+ struct sja1105_tagger_data *tagger_data;
struct skb_shared_hwtstamps shwt = {0};
- struct sja1105_port *sp = dp->priv;
if (!dsa_port_is_sja1105(dp))
return;
@@ -555,19 +555,21 @@ static void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port,
if (dir == SJA1110_META_TSTAMP_RX)
return;
- spin_lock(&sp->data->skb_txtstamp_queue.lock);
+ tagger_data = dp->priv;
- skb_queue_walk_safe(&sp->data->skb_txtstamp_queue, skb, skb_tmp) {
+ spin_lock(&tagger_data->skb_txtstamp_queue.lock);
+
+ skb_queue_walk_safe(&tagger_data->skb_txtstamp_queue, skb, skb_tmp) {
if (SJA1105_SKB_CB(skb)->ts_id != ts_id)
continue;
- __skb_unlink(skb, &sp->data->skb_txtstamp_queue);
+ __skb_unlink(skb, &tagger_data->skb_txtstamp_queue);
skb_match = skb;
break;
}
- spin_unlock(&sp->data->skb_txtstamp_queue.lock);
+ spin_unlock(&tagger_data->skb_txtstamp_queue.lock);
if (WARN_ON(!skb_match))
return;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 07/11] net: dsa: sja1105: move ts_id from sja1105_tagger_data
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
` (5 preceding siblings ...)
2021-12-08 20:04 ` [PATCH net-next 06/11] net: dsa: sja1105: make dp->priv point directly to sja1105_tagger_data Vladimir Oltean
@ 2021-12-08 20:05 ` Vladimir Oltean
2021-12-08 20:05 ` [PATCH net-next 08/11] net: dsa: tag_sja1105: convert to tagger-owned data Vladimir Oltean
` (3 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:05 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/sja1105.h | 3 +++
drivers/net/dsa/sja1105/sja1105_main.c | 1 +
drivers/net/dsa/sja1105/sja1105_ptp.c | 8 ++++----
include/linux/dsa/sja1105.h | 1 -
4 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 6ef6fb4f30e6..850a7d3e69bb 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -261,6 +261,9 @@ struct sja1105_private {
* the switch doesn't confuse them with one another.
*/
struct mutex mgmt_lock;
+ /* PTP two-step TX timestamp ID, and its serialization lock */
+ spinlock_t ts_id_lock;
+ u8 ts_id;
/* Serializes access to the dynamic config interface */
struct mutex dynamic_config_lock;
struct devlink_region **regions;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 4f3350df7f4d..6468a8e963e8 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3349,6 +3349,7 @@ static int sja1105_probe(struct spi_device *spi)
mutex_init(&priv->ptp_data.lock);
mutex_init(&priv->dynamic_config_lock);
mutex_init(&priv->mgmt_lock);
+ spin_lock_init(&priv->ts_id_lock);
rc = sja1105_parse_dt(priv);
if (rc < 0) {
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 9077067328c2..0904ab10bd2f 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -468,15 +468,15 @@ void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
- spin_lock(&tagger_data->meta_lock);
+ spin_lock(&priv->ts_id_lock);
- ts_id = tagger_data->ts_id;
+ ts_id = priv->ts_id;
/* Deal automatically with 8-bit wraparound */
- tagger_data->ts_id++;
+ priv->ts_id++;
SJA1105_SKB_CB(clone)->ts_id = ts_id;
- spin_unlock(&tagger_data->meta_lock);
+ spin_unlock(&priv->ts_id_lock);
skb_queue_tail(&tagger_data->skb_txtstamp_queue, clone);
}
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index 1dda9cce85d9..d8ee53085c09 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -51,7 +51,6 @@ struct sja1105_tagger_data {
*/
spinlock_t meta_lock;
unsigned long state;
- u8 ts_id;
/* Used on SJA1110 where meta frames are generated only for
* 2-step TX timestamps
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 08/11] net: dsa: tag_sja1105: convert to tagger-owned data
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
` (6 preceding siblings ...)
2021-12-08 20:05 ` [PATCH net-next 07/11] net: dsa: sja1105: move ts_id from sja1105_tagger_data Vladimir Oltean
@ 2021-12-08 20:05 ` Vladimir Oltean
2021-12-08 20:05 ` [PATCH net-next 09/11] Revert "net: dsa: move sja1110_process_meta_tstamp inside the tagging protocol driver" Vladimir Oltean
` (2 subsequent siblings)
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:05 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/sja1105.h | 1 -
drivers/net/dsa/sja1105/sja1105_main.c | 55 +++++-----------
drivers/net/dsa/sja1105/sja1105_ptp.c | 35 +++++-----
include/linux/dsa/sja1105.h | 7 +-
net/dsa/tag_sja1105.c | 90 +++++++++++++++++++++-----
5 files changed, 110 insertions(+), 78 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 850a7d3e69bb..9ba2ec2b966d 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -272,7 +272,6 @@ struct sja1105_private {
struct mii_bus *mdio_base_tx;
struct mii_bus *mdio_pcs;
struct dw_xpcs *xpcs[SJA1105_MAX_NUM_PORTS];
- struct sja1105_tagger_data tagger_data;
struct sja1105_ptp_data ptp_data;
struct sja1105_tas_data tas_data;
};
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 6468a8e963e8..4f5ea5d6a623 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2705,6 +2705,21 @@ static void sja1105_port_deferred_xmit(struct kthread_work *work)
kfree(xmit_work);
}
+static int sja1105_connect_tag_protocol(struct dsa_switch *ds,
+ enum dsa_tag_protocol proto)
+{
+ struct sja1105_tagger_data *tagger_data;
+
+ switch (proto) {
+ case DSA_TAG_PROTO_SJA1105:
+ tagger_data = sja1105_tagger_data(ds);
+ tagger_data->xmit_work_fn = sja1105_port_deferred_xmit;
+ return 0;
+ default:
+ return -EPROTONOSUPPORT;
+ }
+}
+
/* The MAXAGE setting belongs to the L2 Forwarding Parameters table,
* which cannot be reconfigured at runtime. So a switch reset is required.
*/
@@ -3005,39 +3020,6 @@ static int sja1105_port_bridge_flags(struct dsa_switch *ds, int port,
return 0;
}
-static void sja1105_teardown_ports(struct sja1105_private *priv)
-{
- struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
-
- kthread_destroy_worker(tagger_data->xmit_worker);
-}
-
-static int sja1105_setup_ports(struct sja1105_private *priv)
-{
- struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
- struct dsa_switch *ds = priv->ds;
- struct kthread_worker *worker;
- struct dsa_port *dp;
- int rc;
-
- worker = kthread_create_worker(0, "dsa%d:%d_xmit", ds->dst->index,
- ds->index);
- if (IS_ERR(worker)) {
- dev_err(ds->dev,
- "failed to create deferred xmit thread: %d\n",
- rc);
- return PTR_ERR(worker);
- }
-
- tagger_data->xmit_worker = worker;
- tagger_data->xmit_work_fn = sja1105_port_deferred_xmit;
-
- dsa_switch_for_each_user_port(dp, ds)
- dp->priv = tagger_data;
-
- return 0;
-}
-
/* The programming model for the SJA1105 switch is "all-at-once" via static
* configuration tables. Some of these can be dynamically modified at runtime,
* but not the xMII mode parameters table.
@@ -3083,10 +3065,6 @@ static int sja1105_setup(struct dsa_switch *ds)
}
}
- rc = sja1105_setup_ports(priv);
- if (rc)
- goto out_static_config_free;
-
sja1105_tas_setup(ds);
sja1105_flower_setup(ds);
@@ -3143,7 +3121,6 @@ static int sja1105_setup(struct dsa_switch *ds)
out_flower_teardown:
sja1105_flower_teardown(ds);
sja1105_tas_teardown(ds);
- sja1105_teardown_ports(priv);
out_static_config_free:
sja1105_static_config_free(&priv->static_config);
@@ -3163,12 +3140,12 @@ static void sja1105_teardown(struct dsa_switch *ds)
sja1105_ptp_clock_unregister(ds);
sja1105_flower_teardown(ds);
sja1105_tas_teardown(ds);
- sja1105_teardown_ports(priv);
sja1105_static_config_free(&priv->static_config);
}
static const struct dsa_switch_ops sja1105_switch_ops = {
.get_tag_protocol = sja1105_get_tag_protocol,
+ .connect_tag_protocol = sja1105_connect_tag_protocol,
.setup = sja1105_setup,
.teardown = sja1105_teardown,
.set_ageing_time = sja1105_set_ageing_time,
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 0904ab10bd2f..b34e4674e217 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -58,13 +58,13 @@ enum sja1105_ptp_clk_mode {
#define ptp_data_to_sja1105(d) \
container_of((d), struct sja1105_private, ptp_data)
-/* Must be called only with priv->tagger_data.state bit
+/* Must be called only with the tagger_data->state bit
* SJA1105_HWTS_RX_EN cleared
*/
static int sja1105_change_rxtstamping(struct sja1105_private *priv,
+ struct sja1105_tagger_data *tagger_data,
bool on)
{
- struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
struct sja1105_general_params_entry *general_params;
struct sja1105_table *table;
@@ -75,9 +75,9 @@ static int sja1105_change_rxtstamping(struct sja1105_private *priv,
general_params->send_meta0 = on;
/* Initialize the meta state machine to a known state */
- if (priv->tagger_data.stampable_skb) {
- kfree_skb(priv->tagger_data.stampable_skb);
- priv->tagger_data.stampable_skb = NULL;
+ if (tagger_data->stampable_skb) {
+ kfree_skb(tagger_data->stampable_skb);
+ tagger_data->stampable_skb = NULL;
}
ptp_cancel_worker_sync(ptp_data->clock);
skb_queue_purge(&tagger_data->skb_txtstamp_queue);
@@ -88,6 +88,7 @@ static int sja1105_change_rxtstamping(struct sja1105_private *priv,
int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
{
+ struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
struct sja1105_private *priv = ds->priv;
struct hwtstamp_config config;
bool rx_on;
@@ -116,17 +117,17 @@ int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
break;
}
- if (rx_on != test_bit(SJA1105_HWTS_RX_EN, &priv->tagger_data.state)) {
- clear_bit(SJA1105_HWTS_RX_EN, &priv->tagger_data.state);
+ if (rx_on != test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state)) {
+ clear_bit(SJA1105_HWTS_RX_EN, &tagger_data->state);
- rc = sja1105_change_rxtstamping(priv, rx_on);
+ rc = sja1105_change_rxtstamping(priv, tagger_data, rx_on);
if (rc < 0) {
dev_err(ds->dev,
"Failed to change RX timestamping: %d\n", rc);
return rc;
}
if (rx_on)
- set_bit(SJA1105_HWTS_RX_EN, &priv->tagger_data.state);
+ set_bit(SJA1105_HWTS_RX_EN, &tagger_data->state);
}
if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
@@ -136,6 +137,7 @@ int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
{
+ struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
struct sja1105_private *priv = ds->priv;
struct hwtstamp_config config;
@@ -144,7 +146,7 @@ int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
config.tx_type = HWTSTAMP_TX_ON;
else
config.tx_type = HWTSTAMP_TX_OFF;
- if (test_bit(SJA1105_HWTS_RX_EN, &priv->tagger_data.state))
+ if (test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
else
config.rx_filter = HWTSTAMP_FILTER_NONE;
@@ -417,10 +419,11 @@ static long sja1105_rxtstamp_work(struct ptp_clock_info *ptp)
bool sja1105_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
{
+ struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
struct sja1105_private *priv = ds->priv;
struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
- if (!test_bit(SJA1105_HWTS_RX_EN, &priv->tagger_data.state))
+ if (!test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
return false;
/* We need to read the full PTP clock to reconstruct the Rx
@@ -459,13 +462,11 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
*/
void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
{
+ struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
struct sk_buff *clone = SJA1105_SKB_CB(skb)->clone;
struct sja1105_private *priv = ds->priv;
- struct sja1105_tagger_data *tagger_data;
u8 ts_id;
- tagger_data = &priv->tagger_data;
-
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
spin_lock(&priv->ts_id_lock);
@@ -897,7 +898,6 @@ static struct ptp_pin_desc sja1105_ptp_pin = {
int sja1105_ptp_clock_register(struct dsa_switch *ds)
{
struct sja1105_private *priv = ds->priv;
- struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
ptp_data->caps = (struct ptp_clock_info) {
@@ -919,9 +919,6 @@ int sja1105_ptp_clock_register(struct dsa_switch *ds)
/* Only used on SJA1105 */
skb_queue_head_init(&ptp_data->skb_rxtstamp_queue);
- /* Only used on SJA1110 */
- skb_queue_head_init(&tagger_data->skb_txtstamp_queue);
- spin_lock_init(&tagger_data->meta_lock);
ptp_data->clock = ptp_clock_register(&ptp_data->caps, ds->dev);
if (IS_ERR_OR_NULL(ptp_data->clock))
@@ -937,8 +934,8 @@ int sja1105_ptp_clock_register(struct dsa_switch *ds)
void sja1105_ptp_clock_unregister(struct dsa_switch *ds)
{
+ struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
struct sja1105_private *priv = ds->priv;
- struct sja1105_tagger_data *tagger_data = &priv->tagger_data;
struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
if (IS_ERR_OR_NULL(ptp_data->clock))
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index d8ee53085c09..9f7d42cbbc08 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -84,9 +84,12 @@ static inline s64 sja1105_ticks_to_ns(s64 ticks)
return ticks * SJA1105_TICK_NS;
}
-static inline bool dsa_port_is_sja1105(struct dsa_port *dp)
+static inline struct sja1105_tagger_data *
+sja1105_tagger_data(struct dsa_switch *ds)
{
- return true;
+ BUG_ON(ds->dst->tag_ops->proto != DSA_TAG_PROTO_SJA1105);
+
+ return ds->tagger_data;
}
#endif /* _NET_DSA_SJA1105_H */
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index fc2af71b965c..f3c1b31645f5 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -125,7 +125,7 @@ static inline bool sja1105_is_meta_frame(const struct sk_buff *skb)
static struct sk_buff *sja1105_defer_xmit(struct dsa_port *dp,
struct sk_buff *skb)
{
- struct sja1105_tagger_data *tagger_data = dp->priv;
+ struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(dp->ds);
void (*xmit_work_fn)(struct kthread_work *work);
struct sja1105_deferred_xmit_work *xmit_work;
struct kthread_worker *xmit_worker;
@@ -368,10 +368,10 @@ static struct sk_buff
*/
if (is_link_local) {
struct dsa_port *dp = dsa_slave_to_port(skb->dev);
- struct sja1105_tagger_data *tagger_data = dp->priv;
+ struct sja1105_tagger_data *tagger_data;
+ struct dsa_switch *ds = dp->ds;
- if (unlikely(!dsa_port_is_sja1105(dp)))
- return skb;
+ tagger_data = sja1105_tagger_data(ds);
if (!test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
/* Do normal processing. */
@@ -382,7 +382,7 @@ static struct sk_buff
* that we were expecting?
*/
if (tagger_data->stampable_skb) {
- dev_err_ratelimited(dp->ds->dev,
+ dev_err_ratelimited(ds->dev,
"Expected meta frame, is %12llx "
"in the DSA master multicast filter?\n",
SJA1105_META_DMAC);
@@ -406,11 +406,11 @@ static struct sk_buff
*/
} else if (is_meta) {
struct dsa_port *dp = dsa_slave_to_port(skb->dev);
- struct sja1105_tagger_data *tagger_data = dp->priv;
+ struct sja1105_tagger_data *tagger_data;
+ struct dsa_switch *ds = dp->ds;
struct sk_buff *stampable_skb;
- if (unlikely(!dsa_port_is_sja1105(dp)))
- return skb;
+ tagger_data = sja1105_tagger_data(ds);
/* Drop the meta frame if we're not in the right state
* to process it.
@@ -427,14 +427,14 @@ static struct sk_buff
* that we were expecting?
*/
if (!stampable_skb) {
- dev_err_ratelimited(dp->ds->dev,
+ dev_err_ratelimited(ds->dev,
"Unexpected meta frame\n");
spin_unlock(&tagger_data->meta_lock);
return NULL;
}
if (stampable_skb->dev != skb->dev) {
- dev_err_ratelimited(dp->ds->dev,
+ dev_err_ratelimited(ds->dev,
"Meta frame on wrong port\n");
spin_unlock(&tagger_data->meta_lock);
return NULL;
@@ -543,20 +543,14 @@ static void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port,
u8 ts_id, enum sja1110_meta_tstamp dir,
u64 tstamp)
{
+ struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
struct sk_buff *skb, *skb_tmp, *skb_match = NULL;
- struct dsa_port *dp = dsa_to_port(ds, port);
- struct sja1105_tagger_data *tagger_data;
struct skb_shared_hwtstamps shwt = {0};
- if (!dsa_port_is_sja1105(dp))
- return;
-
/* We don't care about RX timestamps on the CPU port */
if (dir == SJA1110_META_TSTAMP_RX)
return;
- tagger_data = dp->priv;
-
spin_lock(&tagger_data->skb_txtstamp_queue.lock);
skb_queue_walk_safe(&tagger_data->skb_txtstamp_queue, skb, skb_tmp) {
@@ -737,11 +731,71 @@ static void sja1110_flow_dissect(const struct sk_buff *skb, __be16 *proto,
*proto = ((__be16 *)skb->data)[(VLAN_HLEN / 2) - 1];
}
+static void sja1105_disconnect(struct dsa_switch_tree *dst)
+{
+ struct sja1105_tagger_data *tagger_data;
+ struct dsa_port *dp;
+
+ list_for_each_entry(dp, &dst->ports, list) {
+ tagger_data = dp->ds->tagger_data;
+
+ if (!tagger_data)
+ continue;
+
+ if (tagger_data->xmit_worker)
+ kthread_destroy_worker(tagger_data->xmit_worker);
+
+ kfree(tagger_data);
+ dp->ds->tagger_data = NULL;
+ }
+}
+
+static int sja1105_connect(struct dsa_switch_tree *dst)
+{
+ struct sja1105_tagger_data *tagger_data;
+ struct kthread_worker *xmit_worker;
+ struct dsa_port *dp;
+ int err;
+
+ list_for_each_entry(dp, &dst->ports, list) {
+ if (dp->ds->tagger_data)
+ continue;
+
+ tagger_data = kzalloc(sizeof(*tagger_data), GFP_KERNEL);
+ if (!tagger_data) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ /* Only used on SJA1110 */
+ skb_queue_head_init(&tagger_data->skb_txtstamp_queue);
+ spin_lock_init(&tagger_data->meta_lock);
+
+ xmit_worker = kthread_create_worker(0, "dsa%d:%d_xmit",
+ dst->index, dp->ds->index);
+ if (IS_ERR(xmit_worker)) {
+ err = PTR_ERR(xmit_worker);
+ goto out;
+ }
+
+ tagger_data->xmit_worker = xmit_worker;
+ dp->ds->tagger_data = tagger_data;
+ }
+
+ return 0;
+
+out:
+ sja1105_disconnect(dst);
+ return err;
+}
+
static const struct dsa_device_ops sja1105_netdev_ops = {
.name = "sja1105",
.proto = DSA_TAG_PROTO_SJA1105,
.xmit = sja1105_xmit,
.rcv = sja1105_rcv,
+ .connect = sja1105_connect,
+ .disconnect = sja1105_disconnect,
.needed_headroom = VLAN_HLEN,
.flow_dissect = sja1105_flow_dissect,
.promisc_on_master = true,
@@ -755,6 +809,8 @@ static const struct dsa_device_ops sja1110_netdev_ops = {
.proto = DSA_TAG_PROTO_SJA1110,
.xmit = sja1110_xmit,
.rcv = sja1110_rcv,
+ .connect = sja1105_connect,
+ .disconnect = sja1105_disconnect,
.flow_dissect = sja1110_flow_dissect,
.needed_headroom = SJA1110_HEADER_LEN + VLAN_HLEN,
.needed_tailroom = SJA1110_RX_TRAILER_LEN + SJA1110_MAX_PADDING_LEN,
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 09/11] Revert "net: dsa: move sja1110_process_meta_tstamp inside the tagging protocol driver"
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
` (7 preceding siblings ...)
2021-12-08 20:05 ` [PATCH net-next 08/11] net: dsa: tag_sja1105: convert to tagger-owned data Vladimir Oltean
@ 2021-12-08 20:05 ` Vladimir Oltean
2021-12-08 20:05 ` [PATCH net-next 10/11] net: dsa: tag_sja1105: split sja1105_tagger_data into private and public sections Vladimir Oltean
2021-12-08 20:05 ` [PATCH net-next 11/11] net: dsa: remove dp->priv Vladimir Oltean
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:05 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
This reverts commit 6d709cadfde68dbd12bef12fcced6222226dcb06.
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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/sja1105_main.c | 1 +
drivers/net/dsa/sja1105/sja1105_ptp.c | 44 ++++++++++++++++++++---
drivers/net/dsa/sja1105/sja1105_ptp.h | 24 +++++++++++++
include/linux/dsa/sja1105.h | 26 ++++----------
net/dsa/tag_sja1105.c | 50 ++++----------------------
5 files changed, 78 insertions(+), 67 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 4f5ea5d6a623..9171fbea588c 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -2714,6 +2714,7 @@ static int sja1105_connect_tag_protocol(struct dsa_switch *ds,
case DSA_TAG_PROTO_SJA1105:
tagger_data = sja1105_tagger_data(ds);
tagger_data->xmit_work_fn = sja1105_port_deferred_xmit;
+ tagger_data->meta_tstamp_handler = sja1110_process_meta_tstamp;
return 0;
default:
return -EPROTONOSUPPORT;
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index b34e4674e217..a9f7e4ae0bb2 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -80,7 +80,7 @@ static int sja1105_change_rxtstamping(struct sja1105_private *priv,
tagger_data->stampable_skb = NULL;
}
ptp_cancel_worker_sync(ptp_data->clock);
- skb_queue_purge(&tagger_data->skb_txtstamp_queue);
+ skb_queue_purge(&ptp_data->skb_txtstamp_queue);
skb_queue_purge(&ptp_data->skb_rxtstamp_queue);
return sja1105_static_config_reload(priv, SJA1105_RX_HWTSTAMPING);
@@ -456,15 +456,48 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port,
return priv->info->rxtstamp(ds, port, skb);
}
+void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port, u8 ts_id,
+ enum sja1110_meta_tstamp dir, u64 tstamp)
+{
+ struct sja1105_private *priv = ds->priv;
+ struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
+ struct sk_buff *skb, *skb_tmp, *skb_match = NULL;
+ struct skb_shared_hwtstamps shwt = {0};
+
+ /* We don't care about RX timestamps on the CPU port */
+ if (dir == SJA1110_META_TSTAMP_RX)
+ return;
+
+ spin_lock(&ptp_data->skb_txtstamp_queue.lock);
+
+ skb_queue_walk_safe(&ptp_data->skb_txtstamp_queue, skb, skb_tmp) {
+ if (SJA1105_SKB_CB(skb)->ts_id != ts_id)
+ continue;
+
+ __skb_unlink(skb, &ptp_data->skb_txtstamp_queue);
+ skb_match = skb;
+
+ break;
+ }
+
+ spin_unlock(&ptp_data->skb_txtstamp_queue.lock);
+
+ if (WARN_ON(!skb_match))
+ return;
+
+ shwt.hwtstamp = ns_to_ktime(sja1105_ticks_to_ns(tstamp));
+ skb_complete_tx_timestamp(skb_match, &shwt);
+}
+
/* In addition to cloning the skb which is done by the common
* sja1105_port_txtstamp, we need to generate a timestamp ID and save the
* packet to the TX timestamping queue.
*/
void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
{
- struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
struct sk_buff *clone = SJA1105_SKB_CB(skb)->clone;
struct sja1105_private *priv = ds->priv;
+ struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
u8 ts_id;
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
@@ -479,7 +512,7 @@ void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
spin_unlock(&priv->ts_id_lock);
- skb_queue_tail(&tagger_data->skb_txtstamp_queue, clone);
+ skb_queue_tail(&ptp_data->skb_txtstamp_queue, clone);
}
/* Called from dsa_skb_tx_timestamp. This callback is just to clone
@@ -919,6 +952,8 @@ int sja1105_ptp_clock_register(struct dsa_switch *ds)
/* Only used on SJA1105 */
skb_queue_head_init(&ptp_data->skb_rxtstamp_queue);
+ /* Only used on SJA1110 */
+ skb_queue_head_init(&ptp_data->skb_txtstamp_queue);
ptp_data->clock = ptp_clock_register(&ptp_data->caps, ds->dev);
if (IS_ERR_OR_NULL(ptp_data->clock))
@@ -934,7 +969,6 @@ int sja1105_ptp_clock_register(struct dsa_switch *ds)
void sja1105_ptp_clock_unregister(struct dsa_switch *ds)
{
- struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
struct sja1105_private *priv = ds->priv;
struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
@@ -943,7 +977,7 @@ void sja1105_ptp_clock_unregister(struct dsa_switch *ds)
del_timer_sync(&ptp_data->extts_timer);
ptp_cancel_worker_sync(ptp_data->clock);
- skb_queue_purge(&tagger_data->skb_txtstamp_queue);
+ skb_queue_purge(&ptp_data->skb_txtstamp_queue);
skb_queue_purge(&ptp_data->skb_rxtstamp_queue);
ptp_clock_unregister(ptp_data->clock);
ptp_data->clock = NULL;
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h
index 3ae6b9fdd492..416461ee95d2 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.h
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.h
@@ -8,6 +8,21 @@
#if IS_ENABLED(CONFIG_NET_DSA_SJA1105_PTP)
+/* Timestamps are in units of 8 ns clock ticks (equivalent to
+ * a fixed 125 MHz clock).
+ */
+#define SJA1105_TICK_NS 8
+
+static inline s64 ns_to_sja1105_ticks(s64 ns)
+{
+ return ns / SJA1105_TICK_NS;
+}
+
+static inline s64 sja1105_ticks_to_ns(s64 ticks)
+{
+ return ticks * SJA1105_TICK_NS;
+}
+
/* Calculate the first base_time in the future that satisfies this
* relationship:
*
@@ -62,6 +77,10 @@ struct sja1105_ptp_data {
struct timer_list extts_timer;
/* Used only on SJA1105 to reconstruct partial timestamps */
struct sk_buff_head skb_rxtstamp_queue;
+ /* Used on SJA1110 where meta frames are generated only for
+ * 2-step TX timestamps
+ */
+ struct sk_buff_head skb_txtstamp_queue;
struct ptp_clock_info caps;
struct ptp_clock *clock;
struct sja1105_ptp_cmd cmd;
@@ -112,6 +131,9 @@ bool sja1105_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
bool sja1110_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb);
+void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port, u8 ts_id,
+ enum sja1110_meta_tstamp dir, u64 tstamp);
+
#else
struct sja1105_ptp_cmd;
@@ -178,6 +200,8 @@ static inline int sja1105_ptp_commit(struct dsa_switch *ds,
#define sja1110_rxtstamp NULL
#define sja1110_txtstamp NULL
+#define sja1110_process_meta_tstamp NULL
+
#endif /* IS_ENABLED(CONFIG_NET_DSA_SJA1105_PTP) */
#endif /* _SJA1105_PTP_H */
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index 9f7d42cbbc08..d216211b64f8 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -37,6 +37,11 @@
#define SJA1105_HWTS_RX_EN 0
+enum sja1110_meta_tstamp {
+ SJA1110_META_TSTAMP_TX = 0,
+ SJA1110_META_TSTAMP_RX = 1,
+};
+
struct sja1105_deferred_xmit_work {
struct dsa_port *dp;
struct sk_buff *skb;
@@ -51,12 +56,10 @@ struct sja1105_tagger_data {
*/
spinlock_t meta_lock;
unsigned long state;
- /* Used on SJA1110 where meta frames are generated only for
- * 2-step TX timestamps
- */
- struct sk_buff_head skb_txtstamp_queue;
struct kthread_worker *xmit_worker;
void (*xmit_work_fn)(struct kthread_work *work);
+ void (*meta_tstamp_handler)(struct dsa_switch *ds, int port, u8 ts_id,
+ enum sja1110_meta_tstamp dir, u64 tstamp);
};
struct sja1105_skb_cb {
@@ -69,21 +72,6 @@ struct sja1105_skb_cb {
#define SJA1105_SKB_CB(skb) \
((struct sja1105_skb_cb *)((skb)->cb))
-/* Timestamps are in units of 8 ns clock ticks (equivalent to
- * a fixed 125 MHz clock).
- */
-#define SJA1105_TICK_NS 8
-
-static inline s64 ns_to_sja1105_ticks(s64 ns)
-{
- return ns / SJA1105_TICK_NS;
-}
-
-static inline s64 sja1105_ticks_to_ns(s64 ticks)
-{
- return ticks * SJA1105_TICK_NS;
-}
-
static inline struct sja1105_tagger_data *
sja1105_tagger_data(struct dsa_switch *ds)
{
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index f3c1b31645f5..fe6a6d95bb26 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -4,7 +4,6 @@
#include <linux/if_vlan.h>
#include <linux/dsa/sja1105.h>
#include <linux/dsa/8021q.h>
-#include <linux/skbuff.h>
#include <linux/packing.h>
#include "dsa_priv.h"
@@ -54,11 +53,6 @@
#define SJA1110_TX_TRAILER_LEN 4
#define SJA1110_MAX_PADDING_LEN 15
-enum sja1110_meta_tstamp {
- SJA1110_META_TSTAMP_TX = 0,
- SJA1110_META_TSTAMP_RX = 1,
-};
-
/* Similar to is_link_local_ether_addr(hdr->h_dest) but also covers PTP */
static inline bool sja1105_is_link_local(const struct sk_buff *skb)
{
@@ -539,44 +533,12 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb,
is_meta);
}
-static void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port,
- u8 ts_id, enum sja1110_meta_tstamp dir,
- u64 tstamp)
-{
- struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(ds);
- struct sk_buff *skb, *skb_tmp, *skb_match = NULL;
- struct skb_shared_hwtstamps shwt = {0};
-
- /* We don't care about RX timestamps on the CPU port */
- if (dir == SJA1110_META_TSTAMP_RX)
- return;
-
- spin_lock(&tagger_data->skb_txtstamp_queue.lock);
-
- skb_queue_walk_safe(&tagger_data->skb_txtstamp_queue, skb, skb_tmp) {
- if (SJA1105_SKB_CB(skb)->ts_id != ts_id)
- continue;
-
- __skb_unlink(skb, &tagger_data->skb_txtstamp_queue);
- skb_match = skb;
-
- break;
- }
-
- spin_unlock(&tagger_data->skb_txtstamp_queue.lock);
-
- if (WARN_ON(!skb_match))
- return;
-
- shwt.hwtstamp = ns_to_ktime(sja1105_ticks_to_ns(tstamp));
- skb_complete_tx_timestamp(skb_match, &shwt);
-}
-
static struct sk_buff *sja1110_rcv_meta(struct sk_buff *skb, u16 rx_header)
{
u8 *buf = dsa_etype_header_pos_rx(skb) + SJA1110_HEADER_LEN;
int switch_id = SJA1110_RX_HEADER_SWITCH_ID(rx_header);
int n_ts = SJA1110_RX_HEADER_N_TS(rx_header);
+ struct sja1105_tagger_data *tagger_data;
struct net_device *master = skb->dev;
struct dsa_port *cpu_dp;
struct dsa_switch *ds;
@@ -590,6 +552,10 @@ static struct sk_buff *sja1110_rcv_meta(struct sk_buff *skb, u16 rx_header)
return NULL;
}
+ tagger_data = sja1105_tagger_data(ds);
+ if (!tagger_data->meta_tstamp_handler)
+ return NULL;
+
for (i = 0; i <= n_ts; i++) {
u8 ts_id, source_port, dir;
u64 tstamp;
@@ -599,8 +565,8 @@ static struct sk_buff *sja1110_rcv_meta(struct sk_buff *skb, u16 rx_header)
dir = (buf[1] & BIT(3)) >> 3;
tstamp = be64_to_cpu(*(__be64 *)(buf + 2));
- sja1110_process_meta_tstamp(ds, source_port, ts_id, dir,
- tstamp);
+ tagger_data->meta_tstamp_handler(ds, source_port, ts_id, dir,
+ tstamp);
buf += SJA1110_META_TSTAMP_SIZE;
}
@@ -767,8 +733,6 @@ static int sja1105_connect(struct dsa_switch_tree *dst)
goto out;
}
- /* Only used on SJA1110 */
- skb_queue_head_init(&tagger_data->skb_txtstamp_queue);
spin_lock_init(&tagger_data->meta_lock);
xmit_worker = kthread_create_worker(0, "dsa%d:%d_xmit",
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 10/11] net: dsa: tag_sja1105: split sja1105_tagger_data into private and public sections
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
` (8 preceding siblings ...)
2021-12-08 20:05 ` [PATCH net-next 09/11] Revert "net: dsa: move sja1110_process_meta_tstamp inside the tagging protocol driver" Vladimir Oltean
@ 2021-12-08 20:05 ` Vladimir Oltean
2021-12-08 20:05 ` [PATCH net-next 11/11] net: dsa: remove dp->priv Vladimir Oltean
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:05 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/sja1105/sja1105_ptp.c | 22 ++----
include/linux/dsa/sja1105.h | 13 +--
net/dsa/tag_sja1105.c | 109 +++++++++++++++++++-------
3 files changed, 91 insertions(+), 53 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index a9f7e4ae0bb2..be3068a935af 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -58,11 +58,10 @@ enum sja1105_ptp_clk_mode {
#define ptp_data_to_sja1105(d) \
container_of((d), struct sja1105_private, ptp_data)
-/* Must be called only with the tagger_data->state bit
- * SJA1105_HWTS_RX_EN cleared
+/* Must be called only while the RX timestamping state of the tagger
+ * is turned off
*/
static int sja1105_change_rxtstamping(struct sja1105_private *priv,
- struct sja1105_tagger_data *tagger_data,
bool on)
{
struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
@@ -74,11 +73,6 @@ static int sja1105_change_rxtstamping(struct sja1105_private *priv,
general_params->send_meta1 = on;
general_params->send_meta0 = on;
- /* Initialize the meta state machine to a known state */
- if (tagger_data->stampable_skb) {
- kfree_skb(tagger_data->stampable_skb);
- tagger_data->stampable_skb = NULL;
- }
ptp_cancel_worker_sync(ptp_data->clock);
skb_queue_purge(&ptp_data->skb_txtstamp_queue);
skb_queue_purge(&ptp_data->skb_rxtstamp_queue);
@@ -117,17 +111,17 @@ int sja1105_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
break;
}
- if (rx_on != test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state)) {
- clear_bit(SJA1105_HWTS_RX_EN, &tagger_data->state);
+ if (rx_on != tagger_data->rxtstamp_get_state(ds)) {
+ tagger_data->rxtstamp_set_state(ds, false);
- rc = sja1105_change_rxtstamping(priv, tagger_data, rx_on);
+ rc = sja1105_change_rxtstamping(priv, rx_on);
if (rc < 0) {
dev_err(ds->dev,
"Failed to change RX timestamping: %d\n", rc);
return rc;
}
if (rx_on)
- set_bit(SJA1105_HWTS_RX_EN, &tagger_data->state);
+ tagger_data->rxtstamp_set_state(ds, true);
}
if (copy_to_user(ifr->ifr_data, &config, sizeof(config)))
@@ -146,7 +140,7 @@ int sja1105_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
config.tx_type = HWTSTAMP_TX_ON;
else
config.tx_type = HWTSTAMP_TX_OFF;
- if (test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
+ if (tagger_data->rxtstamp_get_state(ds))
config.rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
else
config.rx_filter = HWTSTAMP_FILTER_NONE;
@@ -423,7 +417,7 @@ bool sja1105_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb)
struct sja1105_private *priv = ds->priv;
struct sja1105_ptp_data *ptp_data = &priv->ptp_data;
- if (!test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
+ if (!tagger_data->rxtstamp_get_state(ds))
return false;
/* We need to read the full PTP clock to reconstruct the Rx
diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h
index d216211b64f8..e9cb1ae6d742 100644
--- a/include/linux/dsa/sja1105.h
+++ b/include/linux/dsa/sja1105.h
@@ -35,8 +35,6 @@
#define SJA1105_META_SMAC 0x222222222222ull
#define SJA1105_META_DMAC 0x0180C200000Eull
-#define SJA1105_HWTS_RX_EN 0
-
enum sja1110_meta_tstamp {
SJA1110_META_TSTAMP_TX = 0,
SJA1110_META_TSTAMP_RX = 1,
@@ -50,16 +48,13 @@ struct sja1105_deferred_xmit_work {
/* Global tagger data */
struct sja1105_tagger_data {
- struct sk_buff *stampable_skb;
- /* Protects concurrent access to the meta state machine
- * from taggers running on multiple ports on SMP systems
- */
- spinlock_t meta_lock;
- unsigned long state;
- struct kthread_worker *xmit_worker;
+ /* Tagger to switch */
void (*xmit_work_fn)(struct kthread_work *work);
void (*meta_tstamp_handler)(struct dsa_switch *ds, int port, u8 ts_id,
enum sja1110_meta_tstamp dir, u64 tstamp);
+ /* Switch to tagger */
+ bool (*rxtstamp_get_state)(struct dsa_switch *ds);
+ void (*rxtstamp_set_state)(struct dsa_switch *ds, bool on);
};
struct sja1105_skb_cb {
diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
index fe6a6d95bb26..93d2484b2480 100644
--- a/net/dsa/tag_sja1105.c
+++ b/net/dsa/tag_sja1105.c
@@ -53,6 +53,25 @@
#define SJA1110_TX_TRAILER_LEN 4
#define SJA1110_MAX_PADDING_LEN 15
+#define SJA1105_HWTS_RX_EN 0
+
+struct sja1105_tagger_private {
+ struct sja1105_tagger_data data; /* Must be first */
+ unsigned long state;
+ /* Protects concurrent access to the meta state machine
+ * from taggers running on multiple ports on SMP systems
+ */
+ spinlock_t meta_lock;
+ struct sk_buff *stampable_skb;
+ struct kthread_worker *xmit_worker;
+};
+
+static struct sja1105_tagger_private *
+sja1105_tagger_private(struct dsa_switch *ds)
+{
+ return ds->tagger_data;
+}
+
/* Similar to is_link_local_ether_addr(hdr->h_dest) but also covers PTP */
static inline bool sja1105_is_link_local(const struct sk_buff *skb)
{
@@ -120,12 +139,13 @@ static struct sk_buff *sja1105_defer_xmit(struct dsa_port *dp,
struct sk_buff *skb)
{
struct sja1105_tagger_data *tagger_data = sja1105_tagger_data(dp->ds);
+ struct sja1105_tagger_private *priv = sja1105_tagger_private(dp->ds);
void (*xmit_work_fn)(struct kthread_work *work);
struct sja1105_deferred_xmit_work *xmit_work;
struct kthread_worker *xmit_worker;
xmit_work_fn = tagger_data->xmit_work_fn;
- xmit_worker = tagger_data->xmit_worker;
+ xmit_worker = priv->xmit_worker;
if (!xmit_work_fn || !xmit_worker)
return NULL;
@@ -362,32 +382,32 @@ static struct sk_buff
*/
if (is_link_local) {
struct dsa_port *dp = dsa_slave_to_port(skb->dev);
- struct sja1105_tagger_data *tagger_data;
+ struct sja1105_tagger_private *priv;
struct dsa_switch *ds = dp->ds;
- tagger_data = sja1105_tagger_data(ds);
+ priv = sja1105_tagger_private(ds);
- if (!test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
+ if (!test_bit(SJA1105_HWTS_RX_EN, &priv->state))
/* Do normal processing. */
return skb;
- spin_lock(&tagger_data->meta_lock);
+ spin_lock(&priv->meta_lock);
/* Was this a link-local frame instead of the meta
* that we were expecting?
*/
- if (tagger_data->stampable_skb) {
+ if (priv->stampable_skb) {
dev_err_ratelimited(ds->dev,
"Expected meta frame, is %12llx "
"in the DSA master multicast filter?\n",
SJA1105_META_DMAC);
- kfree_skb(tagger_data->stampable_skb);
+ kfree_skb(priv->stampable_skb);
}
/* Hold a reference to avoid dsa_switch_rcv
* from freeing the skb.
*/
- tagger_data->stampable_skb = skb_get(skb);
- spin_unlock(&tagger_data->meta_lock);
+ priv->stampable_skb = skb_get(skb);
+ spin_unlock(&priv->meta_lock);
/* Tell DSA we got nothing */
return NULL;
@@ -400,22 +420,22 @@ static struct sk_buff
*/
} else if (is_meta) {
struct dsa_port *dp = dsa_slave_to_port(skb->dev);
- struct sja1105_tagger_data *tagger_data;
+ struct sja1105_tagger_private *priv;
struct dsa_switch *ds = dp->ds;
struct sk_buff *stampable_skb;
- tagger_data = sja1105_tagger_data(ds);
+ priv = sja1105_tagger_private(ds);
/* Drop the meta frame if we're not in the right state
* to process it.
*/
- if (!test_bit(SJA1105_HWTS_RX_EN, &tagger_data->state))
+ if (!test_bit(SJA1105_HWTS_RX_EN, &priv->state))
return NULL;
- spin_lock(&tagger_data->meta_lock);
+ spin_lock(&priv->meta_lock);
- stampable_skb = tagger_data->stampable_skb;
- tagger_data->stampable_skb = NULL;
+ stampable_skb = priv->stampable_skb;
+ priv->stampable_skb = NULL;
/* Was this a meta frame instead of the link-local
* that we were expecting?
@@ -423,14 +443,14 @@ static struct sk_buff
if (!stampable_skb) {
dev_err_ratelimited(ds->dev,
"Unexpected meta frame\n");
- spin_unlock(&tagger_data->meta_lock);
+ spin_unlock(&priv->meta_lock);
return NULL;
}
if (stampable_skb->dev != skb->dev) {
dev_err_ratelimited(ds->dev,
"Meta frame on wrong port\n");
- spin_unlock(&tagger_data->meta_lock);
+ spin_unlock(&priv->meta_lock);
return NULL;
}
@@ -441,12 +461,36 @@ static struct sk_buff
skb = stampable_skb;
sja1105_transfer_meta(skb, meta);
- spin_unlock(&tagger_data->meta_lock);
+ spin_unlock(&priv->meta_lock);
}
return skb;
}
+static bool sja1105_rxtstamp_get_state(struct dsa_switch *ds)
+{
+ struct sja1105_tagger_private *priv = sja1105_tagger_private(ds);
+
+ return test_bit(SJA1105_HWTS_RX_EN, &priv->state);
+}
+
+static void sja1105_rxtstamp_set_state(struct dsa_switch *ds, bool on)
+{
+ struct sja1105_tagger_private *priv = sja1105_tagger_private(ds);
+
+ if (on)
+ set_bit(SJA1105_HWTS_RX_EN, &priv->state);
+ else
+ clear_bit(SJA1105_HWTS_RX_EN, &priv->state);
+
+ /* Initialize the meta state machine to a known state */
+ if (!priv->stampable_skb)
+ return;
+
+ kfree_skb(priv->stampable_skb);
+ priv->stampable_skb = NULL;
+}
+
static bool sja1105_skb_has_tag_8021q(const struct sk_buff *skb)
{
u16 tpid = ntohs(eth_hdr(skb)->h_proto);
@@ -699,26 +743,27 @@ static void sja1110_flow_dissect(const struct sk_buff *skb, __be16 *proto,
static void sja1105_disconnect(struct dsa_switch_tree *dst)
{
- struct sja1105_tagger_data *tagger_data;
+ struct sja1105_tagger_private *priv;
struct dsa_port *dp;
list_for_each_entry(dp, &dst->ports, list) {
- tagger_data = dp->ds->tagger_data;
+ priv = dp->ds->tagger_data;
- if (!tagger_data)
+ if (!priv)
continue;
- if (tagger_data->xmit_worker)
- kthread_destroy_worker(tagger_data->xmit_worker);
+ if (priv->xmit_worker)
+ kthread_destroy_worker(priv->xmit_worker);
- kfree(tagger_data);
- dp->ds->tagger_data = NULL;
+ kfree(priv);
+ dp->ds->priv = NULL;
}
}
static int sja1105_connect(struct dsa_switch_tree *dst)
{
struct sja1105_tagger_data *tagger_data;
+ struct sja1105_tagger_private *priv;
struct kthread_worker *xmit_worker;
struct dsa_port *dp;
int err;
@@ -727,13 +772,13 @@ static int sja1105_connect(struct dsa_switch_tree *dst)
if (dp->ds->tagger_data)
continue;
- tagger_data = kzalloc(sizeof(*tagger_data), GFP_KERNEL);
- if (!tagger_data) {
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv) {
err = -ENOMEM;
goto out;
}
- spin_lock_init(&tagger_data->meta_lock);
+ spin_lock_init(&priv->meta_lock);
xmit_worker = kthread_create_worker(0, "dsa%d:%d_xmit",
dst->index, dp->ds->index);
@@ -742,8 +787,12 @@ static int sja1105_connect(struct dsa_switch_tree *dst)
goto out;
}
- tagger_data->xmit_worker = xmit_worker;
- dp->ds->tagger_data = tagger_data;
+ priv->xmit_worker = xmit_worker;
+ /* Export functions for switch driver use */
+ tagger_data = &priv->data;
+ tagger_data->rxtstamp_get_state = sja1105_rxtstamp_get_state;
+ tagger_data->rxtstamp_set_state = sja1105_rxtstamp_set_state;
+ dp->ds->tagger_data = priv;
}
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH net-next 11/11] net: dsa: remove dp->priv
2021-12-08 20:04 [PATCH net-next 00/11] Replace DSA dp->priv with tagger-owned storage Vladimir Oltean
` (9 preceding siblings ...)
2021-12-08 20:05 ` [PATCH net-next 10/11] net: dsa: tag_sja1105: split sja1105_tagger_data into private and public sections Vladimir Oltean
@ 2021-12-08 20:05 ` Vladimir Oltean
10 siblings, 0 replies; 14+ messages in thread
From: Vladimir Oltean @ 2021-12-08 20:05 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Vivien Didelot,
Florian Fainelli, Martin Kaistra, Kurt Kanzenbach, Ansuel Smith,
Tobias Waldekranz
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: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/net/dsa.h | 6 ------
1 file changed, 6 deletions(-)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8b496c7e62ef..64d71968aa91 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -276,12 +276,6 @@ struct dsa_port {
struct list_head list;
- /*
- * Give the switch driver somewhere to hang its per-port private data
- * structures (accessible from the tagger).
- */
- void *priv;
-
/*
* Original copy of the master netdev ethtool_ops
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread