Hi, this is ready but require some additional test on a wider userbase.
The main reason for this is that we notice some routing problem in the
switch and it seems assisted learning is needed. Considering mdio is
quite slow due to the indirect write using this Ethernet alternative way
seems to be quicker.
The qca8k switch supports a special way to pass mdio read/write request
using specially crafted Ethernet packet.
This works by putting some defined data in the Ethernet header where the
mac source and dst should be placed. The Ethernet type header is set to qca
header and is set to a mdio read/write type.
This is used to communicate to the switch that this is a special packet
and should be parsed differently.
Currently we use Ethernet packet for
- MIB counter
- mdio read/write configuration
- phy read/write for each port
Current implementation of this use completion API to wait for the packet
to be processed by the tagger and has a timeout that fallback to the
legacy mdio way and mutex to enforce one transaction at time.
We now have connect()/disconnect() ops for the tagger. They are used to
allocate priv data in the dsa priv. The header still has to be put in
global include to make it usable by a dsa driver.
They are called when the tag is connect to the dst and the data is freed
using discconect on tagger change.
(if someone wonder why the bind function is put at in the general setup
function it's because tag is set in the cpu port where the notifier is
still not available and we require the notifier to sen the
tag_proto_connect() event.
We now have a tag_proto_connect() for the dsa driver used to put
additional data in the tagger priv (that is actually the dsa priv).
This is called using a switch event DSA_NOTIFIER_TAG_PROTO_CONNECT.
Current use for this is adding handler for the Ethernet packet to keep
the tagger code as dumb as possible.
The tagger priv implement only the handler for the special packet. All the
other stuff is placed in the qca8k_priv and the tagger has to access
it under lock.
We use the new API from Vladimir to track if the master port is
operational or not. We had to track many thing to reach a usable state.
Checking if the port is UP is not enough and tracking a NETDEV_CHANGE is
also not enough since it use also for other task. The correct way was
both track for interface UP and if a qdisc was assigned to the
interface. That tells us the port (and the tagger indirectly) is ready
to accept and process packet.
I tested this with multicpu port and with port6 set as the unique port and
it's sad.
It seems they implemented this feature in a bad way and this is only
supported with cpu port0. When cpu port6 is the unique port, the switch
doesn't send ack packet. With multicpu port, packet ack are not duplicated
and only cpu port0 sends them. This is the same for the MIB counter.
For this reason this feature is enabled only when cpu port0 is enabled and
operational.
Sorry if I missed any comments. This series is 2 month old so I think it
would be good to recheck everything. In the mean time this was tested on
and no regression are observed related to random port drop.
v7:
- Rebase on net-next changes
- Add bulk patches to speedup this even more
v6:
- Fix some error in ethtool handler caused by rebase/cleanup
v5:
- Adapt to new API fixes
- Fix a wrong logic for noop
- Add additional lock for master_state change
- Limit mdio Ethernet to cpu port0 (switch limitation)
- Add priority to these special packet
- Move mdio cache to qca8k_priv
v4:
- Remove duplicate patch sent by mistake.
v3:
- Include MIB with Ethernet packet.
- Include phy read/write with Ethernet packet.
- Reorganize code with new API.
- Introuce master tracking by Vladimir
v2:
- Address all suggestion from Vladimir.
Try to generilize this with connect/disconnect function from the
tagger and tag_proto_connect for the driver.
Ansuel Smith (14):
net: dsa: tag_qca: convert to FIELD macro
net: dsa: tag_qca: move define to include linux/dsa
net: dsa: tag_qca: enable promisc_on_master flag
net: dsa: tag_qca: add define for handling mgmt Ethernet packet
net: dsa: tag_qca: add define for handling MIB packet
net: dsa: tag_qca: add support for handling mgmt and MIB Ethernet
packet
net: dsa: qca8k: add tracking state of master port
net: dsa: qca8k: add support for mgmt read/write in Ethernet packet
net: dsa: qca8k: add support for mib autocast in Ethernet packet
net: dsa: qca8k: add support for phy read/write with mgmt Ethernet
net: dsa: qca8k: move page cache to driver priv
net: dsa: qca8k: cache lo and hi for mdio write
net: da: qca8k: add support for larger read/write size with mgmt
Ethernet
net: dsa: qca8k: introduce qca8k_bulk_read/write function
Vladimir Oltean (2):
net: dsa: provide switch operations for tracking the master state
net: dsa: replay master state events in
dsa_tree_{setup,teardown}_master
drivers/net/dsa/qca8k.c | 709 +++++++++++++++++++++++++++++++++---
drivers/net/dsa/qca8k.h | 46 ++-
include/linux/dsa/tag_qca.h | 82 +++++
include/net/dsa.h | 17 +
net/dsa/dsa2.c | 74 +++-
net/dsa/dsa_priv.h | 13 +
net/dsa/slave.c | 32 ++
net/dsa/switch.c | 15 +
net/dsa/tag_qca.c | 83 +++--
9 files changed, 993 insertions(+), 78 deletions(-)
create mode 100644 include/linux/dsa/tag_qca.h
--
2.33.1
From: Vladimir Oltean <[email protected]>
Certain drivers may need to send management traffic to the switch for
things like register access, FDB dump, etc, to accelerate what their
slow bus (SPI, I2C, MDIO) can already do.
Ethernet is faster (especially in bulk transactions) but is also more
unreliable, since the user may decide to bring the DSA master down (or
not bring it up), therefore severing the link between the host and the
attached switch.
Drivers needing Ethernet-based register access already should have
fallback logic to the slow bus if the Ethernet method fails, but that
fallback may be based on a timeout, and the I/O to the switch may slow
down to a halt if the master is down, because every Ethernet packet will
have to time out. The driver also doesn't have the option to turn off
Ethernet-based I/O momentarily, because it wouldn't know when to turn it
back on.
Which is where this change comes in. By tracking NETDEV_CHANGE,
NETDEV_UP and NETDEV_GOING_DOWN events on the DSA master, we should know
the exact interval of time during which this interface is reliably
available for traffic. Provide this information to switches so they can
use it as they wish.
An helper is added dsa_port_master_is_operational() to check if a master
port is operational.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
include/net/dsa.h | 17 +++++++++++++++++
net/dsa/dsa2.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
net/dsa/dsa_priv.h | 13 +++++++++++++
net/dsa/slave.c | 32 ++++++++++++++++++++++++++++++++
net/dsa/switch.c | 15 +++++++++++++++
5 files changed, 123 insertions(+)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 57b3e4e7413b..43c4153ef53a 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -278,6 +278,10 @@ struct dsa_port {
u8 devlink_port_setup:1;
+ /* Master state bits, valid only on CPU ports */
+ u8 master_admin_up:1;
+ u8 master_oper_up:1;
+
u8 setup:1;
struct device_node *dn;
@@ -478,6 +482,12 @@ static inline bool dsa_port_is_unused(struct dsa_port *dp)
return dp->type == DSA_PORT_TYPE_UNUSED;
}
+static inline bool dsa_port_master_is_operational(struct dsa_port *dp)
+{
+ return dsa_port_is_cpu(dp) && dp->master_admin_up &&
+ dp->master_oper_up;
+}
+
static inline bool dsa_is_unused_port(struct dsa_switch *ds, int p)
{
return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_UNUSED;
@@ -1036,6 +1046,13 @@ struct dsa_switch_ops {
int (*tag_8021q_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
u16 flags);
int (*tag_8021q_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
+
+ /*
+ * DSA master tracking operations
+ */
+ void (*master_state_change)(struct dsa_switch *ds,
+ const struct net_device *master,
+ bool operational);
};
#define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes) \
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 3d21521453fe..ff998c0ede02 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1279,6 +1279,52 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
return err;
}
+static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
+ struct net_device *master)
+{
+ struct dsa_notifier_master_state_info info;
+ struct dsa_port *cpu_dp = master->dsa_ptr;
+
+ info.master = master;
+ info.operational = dsa_port_master_is_operational(cpu_dp);
+
+ dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
+}
+
+void dsa_tree_master_admin_state_change(struct dsa_switch_tree *dst,
+ struct net_device *master,
+ bool up)
+{
+ struct dsa_port *cpu_dp = master->dsa_ptr;
+ bool notify = false;
+
+ if ((dsa_port_master_is_operational(cpu_dp)) !=
+ (up && cpu_dp->master_oper_up))
+ notify = true;
+
+ cpu_dp->master_admin_up = up;
+
+ if (notify)
+ dsa_tree_master_state_change(dst, master);
+}
+
+void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
+ struct net_device *master,
+ bool up)
+{
+ struct dsa_port *cpu_dp = master->dsa_ptr;
+ bool notify = false;
+
+ if ((dsa_port_master_is_operational(cpu_dp)) !=
+ (cpu_dp->master_admin_up && up))
+ notify = true;
+
+ cpu_dp->master_oper_up = up;
+
+ if (notify)
+ dsa_tree_master_state_change(dst, master);
+}
+
static struct dsa_port *dsa_port_touch(struct dsa_switch *ds, int index)
{
struct dsa_switch_tree *dst = ds->dst;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 760306f0012f..2bbfa9efe9f8 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -40,6 +40,7 @@ enum {
DSA_NOTIFIER_TAG_PROTO_DISCONNECT,
DSA_NOTIFIER_TAG_8021Q_VLAN_ADD,
DSA_NOTIFIER_TAG_8021Q_VLAN_DEL,
+ DSA_NOTIFIER_MASTER_STATE_CHANGE,
};
/* DSA_NOTIFIER_AGEING_TIME */
@@ -109,6 +110,12 @@ struct dsa_notifier_tag_8021q_vlan_info {
u16 vid;
};
+/* DSA_NOTIFIER_MASTER_STATE_CHANGE */
+struct dsa_notifier_master_state_info {
+ const struct net_device *master;
+ bool operational;
+};
+
struct dsa_switchdev_event_work {
struct dsa_switch *ds;
int port;
@@ -482,6 +489,12 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
struct net_device *master,
const struct dsa_device_ops *tag_ops,
const struct dsa_device_ops *old_tag_ops);
+void dsa_tree_master_admin_state_change(struct dsa_switch_tree *dst,
+ struct net_device *master,
+ bool up);
+void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
+ struct net_device *master,
+ bool up);
unsigned int dsa_bridge_num_get(const struct net_device *bridge_dev, int max);
void dsa_bridge_num_put(const struct net_device *bridge_dev,
unsigned int bridge_num);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 22241afcac81..2b5b0f294233 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2346,6 +2346,36 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
err = dsa_port_lag_change(dp, info->lower_state_info);
return notifier_from_errno(err);
}
+ case NETDEV_CHANGE:
+ case NETDEV_UP: {
+ /* Track state of master port.
+ * DSA driver may require the master port (and indirectly
+ * the tagger) to be available for some special operation.
+ */
+ if (netdev_uses_dsa(dev)) {
+ struct dsa_port *cpu_dp = dev->dsa_ptr;
+ struct dsa_switch_tree *dst = cpu_dp->ds->dst;
+
+ /* Track when the master port is UP */
+ dsa_tree_master_oper_state_change(dst, dev,
+ netif_oper_up(dev));
+
+ /* Track when the master port is ready and can accept
+ * packet.
+ * NETDEV_UP event is not enough to flag a port as ready.
+ * We also have to wait for linkwatch_do_dev to dev_activate
+ * and emit a NETDEV_CHANGE event.
+ * We check if a master port is ready by checking if the dev
+ * have a qdisc assigned and is not noop.
+ */
+ dsa_tree_master_admin_state_change(dst, dev,
+ !qdisc_tx_is_noop(dev));
+
+ return NOTIFY_OK;
+ }
+
+ return NOTIFY_DONE;
+ }
case NETDEV_GOING_DOWN: {
struct dsa_port *dp, *cpu_dp;
struct dsa_switch_tree *dst;
@@ -2357,6 +2387,8 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
cpu_dp = dev->dsa_ptr;
dst = cpu_dp->ds->dst;
+ dsa_tree_master_admin_state_change(dst, dev, false);
+
list_for_each_entry(dp, &dst->ports, list) {
if (!dsa_port_is_user(dp))
continue;
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index e3c7d2627a61..4e9cbe3a3127 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -683,6 +683,18 @@ dsa_switch_disconnect_tag_proto(struct dsa_switch *ds,
return 0;
}
+static int
+dsa_switch_master_state_change(struct dsa_switch *ds,
+ struct dsa_notifier_master_state_info *info)
+{
+ if (!ds->ops->master_state_change)
+ return 0;
+
+ ds->ops->master_state_change(ds, info->master, info->operational);
+
+ return 0;
+}
+
static int dsa_switch_event(struct notifier_block *nb,
unsigned long event, void *info)
{
@@ -756,6 +768,9 @@ static int dsa_switch_event(struct notifier_block *nb,
case DSA_NOTIFIER_TAG_8021Q_VLAN_DEL:
err = dsa_switch_tag_8021q_vlan_del(ds, info);
break;
+ case DSA_NOTIFIER_MASTER_STATE_CHANGE:
+ err = dsa_switch_master_state_change(ds, info);
+ break;
default:
err = -EOPNOTSUPP;
break;
--
2.33.1
From: Vladimir Oltean <[email protected]>
In order for switch driver to be able to make simple and reliable use of
the master tracking operations, they must also be notified of the
initial state of the DSA master, not just of the changes. This is
because they might enable certain features only during the time when
they know that the DSA master is up and running.
Therefore, this change explicitly checks the state of the DSA master
under the same rtnl_mutex as we were holding during the
dsa_master_setup() and dsa_master_teardown() call. The idea being that
if the DSA master became operational in between the moment in which it
became a DSA master (dsa_master_setup set dev->dsa_ptr) and the moment
when we checked for the master being up, there is a chance that we
would emit a ->master_state_change() call with no actual state change.
We need to avoid that by serializing the concurrent netdevice event with
us. If the netdevice event started before, we force it to finish before
we begin, because we take rtnl_lock before making netdev_uses_dsa()
return true. So we also handle that early event and do nothing on it.
Similarly, if the dev_open() attempt is concurrent with us, it will
attempt to take the rtnl_mutex, but we're holding it. We'll see that
the master flag IFF_UP isn't set, then when we release the rtnl_mutex
we'll process the NETDEV_UP notifier.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Ansuel Smith <[email protected]>
---
net/dsa/dsa2.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index ff998c0ede02..909b045c9b11 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -15,6 +15,7 @@
#include <linux/of.h>
#include <linux/of_net.h>
#include <net/devlink.h>
+#include <net/sch_generic.h>
#include "dsa_priv.h"
@@ -1064,9 +1065,18 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
list_for_each_entry(dp, &dst->ports, list) {
if (dsa_port_is_cpu(dp)) {
- err = dsa_master_setup(dp->master, dp);
+ struct net_device *master = dp->master;
+ bool admin_up = (master->flags & IFF_UP) &&
+ !qdisc_tx_is_noop(master);
+
+ err = dsa_master_setup(master, dp);
if (err)
return err;
+
+ /* Replay master state event */
+ dsa_tree_master_admin_state_change(dst, master, admin_up);
+ dsa_tree_master_oper_state_change(dst, master,
+ netif_oper_up(master));
}
}
@@ -1081,9 +1091,19 @@ static void dsa_tree_teardown_master(struct dsa_switch_tree *dst)
rtnl_lock();
- list_for_each_entry(dp, &dst->ports, list)
- if (dsa_port_is_cpu(dp))
- dsa_master_teardown(dp->master);
+ list_for_each_entry(dp, &dst->ports, list) {
+ if (dsa_port_is_cpu(dp)) {
+ struct net_device *master = dp->master;
+
+ /* Synthesizing an "admin down" state is sufficient for
+ * the switches to get a notification if the master is
+ * currently up and running.
+ */
+ dsa_tree_master_admin_state_change(dst, master, false);
+
+ dsa_master_teardown(master);
+ }
+ }
rtnl_unlock();
}
--
2.33.1
Convert driver to FIELD macro to drop redundant define.
Signed-off-by: Ansuel Smith <[email protected]>
---
net/dsa/tag_qca.c | 34 +++++++++++++++-------------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 1ea9401b8ace..55fa6b96b4eb 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -4,29 +4,24 @@
*/
#include <linux/etherdevice.h>
+#include <linux/bitfield.h>
#include "dsa_priv.h"
#define QCA_HDR_LEN 2
#define QCA_HDR_VERSION 0x2
-#define QCA_HDR_RECV_VERSION_MASK GENMASK(15, 14)
-#define QCA_HDR_RECV_VERSION_S 14
-#define QCA_HDR_RECV_PRIORITY_MASK GENMASK(13, 11)
-#define QCA_HDR_RECV_PRIORITY_S 11
-#define QCA_HDR_RECV_TYPE_MASK GENMASK(10, 6)
-#define QCA_HDR_RECV_TYPE_S 6
+#define QCA_HDR_RECV_VERSION GENMASK(15, 14)
+#define QCA_HDR_RECV_PRIORITY GENMASK(13, 11)
+#define QCA_HDR_RECV_TYPE GENMASK(10, 6)
#define QCA_HDR_RECV_FRAME_IS_TAGGED BIT(3)
-#define QCA_HDR_RECV_SOURCE_PORT_MASK GENMASK(2, 0)
-
-#define QCA_HDR_XMIT_VERSION_MASK GENMASK(15, 14)
-#define QCA_HDR_XMIT_VERSION_S 14
-#define QCA_HDR_XMIT_PRIORITY_MASK GENMASK(13, 11)
-#define QCA_HDR_XMIT_PRIORITY_S 11
-#define QCA_HDR_XMIT_CONTROL_MASK GENMASK(10, 8)
-#define QCA_HDR_XMIT_CONTROL_S 8
+#define QCA_HDR_RECV_SOURCE_PORT GENMASK(2, 0)
+
+#define QCA_HDR_XMIT_VERSION GENMASK(15, 14)
+#define QCA_HDR_XMIT_PRIORITY GENMASK(13, 11)
+#define QCA_HDR_XMIT_CONTROL GENMASK(10, 8)
#define QCA_HDR_XMIT_FROM_CPU BIT(7)
-#define QCA_HDR_XMIT_DP_BIT_MASK GENMASK(6, 0)
+#define QCA_HDR_XMIT_DP_BIT GENMASK(6, 0)
static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
{
@@ -40,8 +35,9 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
phdr = dsa_etype_header_pos_tx(skb);
/* Set the version field, and set destination port information */
- hdr = QCA_HDR_VERSION << QCA_HDR_XMIT_VERSION_S |
- QCA_HDR_XMIT_FROM_CPU | BIT(dp->index);
+ hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
+ hdr |= QCA_HDR_XMIT_FROM_CPU;
+ hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(dp->index));
*phdr = htons(hdr);
@@ -62,7 +58,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
hdr = ntohs(*phdr);
/* Make sure the version is correct */
- ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S;
+ ver = FIELD_GET(QCA_HDR_RECV_VERSION, hdr);
if (unlikely(ver != QCA_HDR_VERSION))
return NULL;
@@ -71,7 +67,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
dsa_strip_etype_header(skb, QCA_HDR_LEN);
/* Get source port information */
- port = (hdr & QCA_HDR_RECV_SOURCE_PORT_MASK);
+ port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, hdr);
skb->dev = dsa_master_find_slave(dev, 0, port);
if (!skb->dev)
--
2.33.1
Add all the required define to prepare support for mgmt read/write in
Ethernet packet. Any packet of this type has to be dropped as the only
use of these special packet is receive ack for an mgmt write request or
receive data for an mgmt read request.
A struct is used that emulates the Ethernet header but is used for a
different purpose.
Signed-off-by: Ansuel Smith <[email protected]>
---
include/linux/dsa/tag_qca.h | 44 +++++++++++++++++++++++++++++++++++++
net/dsa/tag_qca.c | 13 ++++++++---
2 files changed, 54 insertions(+), 3 deletions(-)
diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index c02d2d39ff4a..1a02f695f3a3 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -12,10 +12,54 @@
#define QCA_HDR_RECV_FRAME_IS_TAGGED BIT(3)
#define QCA_HDR_RECV_SOURCE_PORT GENMASK(2, 0)
+/* Packet type for recv */
+#define QCA_HDR_RECV_TYPE_NORMAL 0x0
+#define QCA_HDR_RECV_TYPE_MIB 0x1
+#define QCA_HDR_RECV_TYPE_RW_REG_ACK 0x2
+
#define QCA_HDR_XMIT_VERSION GENMASK(15, 14)
#define QCA_HDR_XMIT_PRIORITY GENMASK(13, 11)
#define QCA_HDR_XMIT_CONTROL GENMASK(10, 8)
#define QCA_HDR_XMIT_FROM_CPU BIT(7)
#define QCA_HDR_XMIT_DP_BIT GENMASK(6, 0)
+/* Packet type for xmit */
+#define QCA_HDR_XMIT_TYPE_NORMAL 0x0
+#define QCA_HDR_XMIT_TYPE_RW_REG 0x1
+
+/* Check code for a valid mgmt packet. Switch will ignore the packet
+ * with this wrong.
+ */
+#define QCA_HDR_MGMT_CHECK_CODE_VAL 0x5
+
+/* Specific define for in-band MDIO read/write with Ethernet packet */
+#define QCA_HDR_MGMT_SEQ_LEN 4 /* 4 byte for the seq */
+#define QCA_HDR_MGMT_COMMAND_LEN 4 /* 4 byte for the command */
+#define QCA_HDR_MGMT_DATA1_LEN 4 /* First 4 byte for the mdio data */
+#define QCA_HDR_MGMT_HEADER_LEN (QCA_HDR_MGMT_SEQ_LEN + \
+ QCA_HDR_MGMT_COMMAND_LEN + \
+ QCA_HDR_MGMT_DATA1_LEN)
+
+#define QCA_HDR_MGMT_DATA2_LEN 12 /* Other 12 byte for the mdio data */
+#define QCA_HDR_MGMT_PADDING_LEN 34 /* Padding to reach the min Ethernet packet */
+
+#define QCA_HDR_MGMT_PKG_LEN (QCA_HDR_MGMT_HEADER_LEN + \
+ QCA_HDR_LEN + \
+ QCA_HDR_MGMT_DATA2_LEN + \
+ QCA_HDR_MGMT_PADDING_LEN)
+
+#define QCA_HDR_MGMT_SEQ_NUM GENMASK(31, 0) /* 63, 32 */
+#define QCA_HDR_MGMT_CHECK_CODE GENMASK(31, 29) /* 31, 29 */
+#define QCA_HDR_MGMT_CMD BIT(28) /* 28 */
+#define QCA_HDR_MGMT_LENGTH GENMASK(23, 20) /* 23, 20 */
+#define QCA_HDR_MGMT_ADDR GENMASK(18, 0) /* 18, 0 */
+
+/* Special struct emulating a Ethernet header */
+struct mgmt_ethhdr {
+ u32 command; /* command bit 31:0 */
+ u32 seq; /* seq 63:32 */
+ u32 mdio_data; /* first 4byte mdio */
+ __be16 hdr; /* qca hdr */
+} __packed;
+
#endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index f8df49d5956f..c57d6e1a0c0c 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -32,10 +32,10 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
{
- u8 ver;
- u16 hdr;
- int port;
+ u16 hdr, pk_type;
__be16 *phdr;
+ int port;
+ u8 ver;
if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
return NULL;
@@ -48,6 +48,13 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
if (unlikely(ver != QCA_HDR_VERSION))
return NULL;
+ /* Get pk type */
+ pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
+
+ /* Ethernet MDIO read/write packet */
+ if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
+ return NULL;
+
/* Remove QCA tag and recalculate checksum */
skb_pull_rcsum(skb, QCA_HDR_LEN);
dsa_strip_etype_header(skb, QCA_HDR_LEN);
--
2.33.1
Ethernet MDIO packets are non-standard and DSA master expects the first
6 octets to be the MAC DA. To address these kind of packet, enable
promisc_on_master flag for the tagger.
Signed-off-by: Ansuel Smith <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
net/dsa/tag_qca.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 34e565e00ece..f8df49d5956f 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -68,6 +68,7 @@ static const struct dsa_device_ops qca_netdev_ops = {
.xmit = qca_tag_xmit,
.rcv = qca_tag_rcv,
.needed_headroom = QCA_HDR_LEN,
+ .promisc_on_master = true,
};
MODULE_LICENSE("GPL");
--
2.33.1
Move tag_qca define to include dir linux/dsa as the qca8k require access
to the tagger define to support in-band mdio read/write using ethernet
packet.
Signed-off-by: Ansuel Smith <[email protected]>
---
include/linux/dsa/tag_qca.h | 21 +++++++++++++++++++++
net/dsa/tag_qca.c | 16 +---------------
2 files changed, 22 insertions(+), 15 deletions(-)
create mode 100644 include/linux/dsa/tag_qca.h
diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
new file mode 100644
index 000000000000..c02d2d39ff4a
--- /dev/null
+++ b/include/linux/dsa/tag_qca.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __TAG_QCA_H
+#define __TAG_QCA_H
+
+#define QCA_HDR_LEN 2
+#define QCA_HDR_VERSION 0x2
+
+#define QCA_HDR_RECV_VERSION GENMASK(15, 14)
+#define QCA_HDR_RECV_PRIORITY GENMASK(13, 11)
+#define QCA_HDR_RECV_TYPE GENMASK(10, 6)
+#define QCA_HDR_RECV_FRAME_IS_TAGGED BIT(3)
+#define QCA_HDR_RECV_SOURCE_PORT GENMASK(2, 0)
+
+#define QCA_HDR_XMIT_VERSION GENMASK(15, 14)
+#define QCA_HDR_XMIT_PRIORITY GENMASK(13, 11)
+#define QCA_HDR_XMIT_CONTROL GENMASK(10, 8)
+#define QCA_HDR_XMIT_FROM_CPU BIT(7)
+#define QCA_HDR_XMIT_DP_BIT GENMASK(6, 0)
+
+#endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 55fa6b96b4eb..34e565e00ece 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -5,24 +5,10 @@
#include <linux/etherdevice.h>
#include <linux/bitfield.h>
+#include <linux/dsa/tag_qca.h>
#include "dsa_priv.h"
-#define QCA_HDR_LEN 2
-#define QCA_HDR_VERSION 0x2
-
-#define QCA_HDR_RECV_VERSION GENMASK(15, 14)
-#define QCA_HDR_RECV_PRIORITY GENMASK(13, 11)
-#define QCA_HDR_RECV_TYPE GENMASK(10, 6)
-#define QCA_HDR_RECV_FRAME_IS_TAGGED BIT(3)
-#define QCA_HDR_RECV_SOURCE_PORT GENMASK(2, 0)
-
-#define QCA_HDR_XMIT_VERSION GENMASK(15, 14)
-#define QCA_HDR_XMIT_PRIORITY GENMASK(13, 11)
-#define QCA_HDR_XMIT_CONTROL GENMASK(10, 8)
-#define QCA_HDR_XMIT_FROM_CPU BIT(7)
-#define QCA_HDR_XMIT_DP_BIT GENMASK(6, 0)
-
static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_port *dp = dsa_slave_to_port(dev);
--
2.33.1
There can be multiple qca8k switch on the same system. Move the static
qca8k_current_page to qca8k_priv and make it specific for each switch.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 47 +++++++++++++++++++++++------------------
drivers/net/dsa/qca8k.h | 9 ++++++++
2 files changed, 36 insertions(+), 20 deletions(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index e7bc0770bae9..c2f5414033d8 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -75,12 +75,6 @@ static const struct qca8k_mib_desc ar8327_mib[] = {
MIB_DESC(1, 0xac, "TXUnicast"),
};
-/* The 32bit switch registers are accessed indirectly. To achieve this we need
- * to set the page of the register. Track the last page that was set to reduce
- * mdio writes
- */
-static u16 qca8k_current_page = 0xffff;
-
static void
qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
{
@@ -134,11 +128,11 @@ qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
}
static int
-qca8k_set_page(struct mii_bus *bus, u16 page)
+qca8k_set_page(struct mii_bus *bus, u16 page, u16 *cached_page)
{
int ret;
- if (page == qca8k_current_page)
+ if (page == *cached_page)
return 0;
ret = bus->write(bus, 0x18, 0, page);
@@ -148,7 +142,7 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
return ret;
}
- qca8k_current_page = page;
+ *cached_page = page;
usleep_range(1000, 2000);
return 0;
}
@@ -343,6 +337,7 @@ static int
qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+ struct qca8k_mdio_cache *mdio_cache;
struct mii_bus *bus = priv->bus;
u16 r1, r2, page;
int ret;
@@ -350,11 +345,13 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
return 0;
+ mdio_cache = &priv->mdio_cache;
+
qca8k_split_addr(reg, &r1, &r2, &page);
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
- ret = qca8k_set_page(bus, page);
+ ret = qca8k_set_page(bus, page, &mdio_cache->page);
if (ret < 0)
goto exit;
@@ -369,6 +366,7 @@ static int
qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+ struct qca8k_mdio_cache *mdio_cache;
struct mii_bus *bus = priv->bus;
u16 r1, r2, page;
int ret;
@@ -376,11 +374,13 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
if (priv->mgmt_master && !qca8k_write_eth(priv, reg, val))
return 0;
+ mdio_cache = &priv->mdio_cache;
+
qca8k_split_addr(reg, &r1, &r2, &page);
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
- ret = qca8k_set_page(bus, page);
+ ret = qca8k_set_page(bus, page, &mdio_cache->page);
if (ret < 0)
goto exit;
@@ -395,6 +395,7 @@ static int
qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_val)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+ struct qca8k_mdio_cache *mdio_cache;
struct mii_bus *bus = priv->bus;
u16 r1, r2, page;
u32 val;
@@ -404,11 +405,13 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
!qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
return 0;
+ mdio_cache = &priv->mdio_cache;
+
qca8k_split_addr(reg, &r1, &r2, &page);
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
- ret = qca8k_set_page(bus, page);
+ ret = qca8k_set_page(bus, page, &mdio_cache->page);
if (ret < 0)
goto exit;
@@ -1046,7 +1049,8 @@ qca8k_mdio_busy_wait(struct mii_bus *bus, u32 reg, u32 mask)
}
static int
-qca8k_mdio_write(struct mii_bus *bus, int phy, int regnum, u16 data)
+qca8k_mdio_write(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
+ int phy, int regnum, u16 data)
{
u16 r1, r2, page;
u32 val;
@@ -1064,7 +1068,7 @@ qca8k_mdio_write(struct mii_bus *bus, int phy, int regnum, u16 data)
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
- ret = qca8k_set_page(bus, page);
+ ret = qca8k_set_page(bus, page, &cache->page);
if (ret)
goto exit;
@@ -1083,7 +1087,8 @@ qca8k_mdio_write(struct mii_bus *bus, int phy, int regnum, u16 data)
}
static int
-qca8k_mdio_read(struct mii_bus *bus, int phy, int regnum)
+qca8k_mdio_read(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
+ int phy, int regnum)
{
u16 r1, r2, page;
u32 val;
@@ -1100,7 +1105,7 @@ qca8k_mdio_read(struct mii_bus *bus, int phy, int regnum)
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
- ret = qca8k_set_page(bus, page);
+ ret = qca8k_set_page(bus, page, &cache->page);
if (ret)
goto exit;
@@ -1139,7 +1144,7 @@ qca8k_internal_mdio_write(struct mii_bus *slave_bus, int phy, int regnum, u16 da
return 0;
}
- return qca8k_mdio_write(bus, phy, regnum, data);
+ return qca8k_mdio_write(bus, &priv->mdio_cache, phy, regnum, data);
}
static int
@@ -1156,7 +1161,7 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
return ret;
}
- return qca8k_mdio_read(bus, phy, regnum);
+ return qca8k_mdio_read(bus, &priv->mdio_cache, phy, regnum);
}
static int
@@ -1179,7 +1184,7 @@ qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
return ret;
}
- return qca8k_mdio_write(priv->bus, port, regnum, data);
+ return qca8k_mdio_write(priv->bus, &priv->mdio_cache, port, regnum, data);
}
static int
@@ -1202,7 +1207,7 @@ qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
return ret;
}
- ret = qca8k_mdio_read(priv->bus, port, regnum);
+ ret = qca8k_mdio_read(priv->bus, &priv->mdio_cache, port, regnum);
if (ret < 0)
return 0xffff;
@@ -3001,6 +3006,8 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
return PTR_ERR(priv->regmap);
}
+ priv->mdio_cache.page = 0xffff;
+
/* Check the detected switch id */
ret = qca8k_read_switch_id(priv);
if (ret)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 952217db2047..77ffdc7b5aaa 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -363,6 +363,14 @@ struct qca8k_ports_config {
u8 rgmii_tx_delay[QCA8K_NUM_CPU_PORTS]; /* 0: CPU port0, 1: CPU port6 */
};
+struct qca8k_mdio_cache {
+/* The 32bit switch registers are accessed indirectly. To achieve this we need
+ * to set the page of the register. Track the last page that was set to reduce
+ * mdio writes
+ */
+ u16 page;
+};
+
struct qca8k_priv {
u8 switch_id;
u8 switch_revision;
@@ -383,6 +391,7 @@ struct qca8k_priv {
const struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
struct qca8k_mgmt_hdr_data mgmt_hdr_data;
struct qca8k_mib_hdr_data mib_hdr_data;
+ struct qca8k_mdio_cache mdio_cache;
};
struct qca8k_mib_desc {
--
2.33.1
Add qca8k side support for mgmt read/write in Ethernet packet.
qca8k supports some specially crafted Ethernet packet that can be used
for mgmt read/write instead of the legacy method uart/internal mdio.
This add support for the qca8k side to craft the packet and enqueue it.
Each port and the qca8k_priv have a special struct to put data in it.
The completion API is used to wait for the packet to be received back
with the requested data.
The various steps are:
1. Craft the special packet with the qca hdr set to mgmt read/write
mode.
2. Set the lock in the dedicated mgmt struct.
3. Reinit the completion.
4. Enqueue the packet.
5. Wait the packet to be received.
6. Use the data set by the tagger to complete the mdio operation.
If the completion timeouts or the ack value is not true, the legacy
mdio way is used.
It has to be considered that in the initial setup mdio is still used and
mdio is still used until DSA is ready to accept and tag packet.
tag_proto_connect() is used to fill the required handler for the tagger
to correctly parse and elaborate the special Ethernet mdio packet.
Locking is added to qca8k_master_change() to make sure no mgmt Ethernet
are in progress.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 206 ++++++++++++++++++++++++++++++++++++++++
drivers/net/dsa/qca8k.h | 13 +++
2 files changed, 219 insertions(+)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 4bc5064414b5..35711d010eb4 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -20,6 +20,7 @@
#include <linux/phylink.h>
#include <linux/gpio/consumer.h>
#include <linux/etherdevice.h>
+#include <linux/dsa/tag_qca.h>
#include "qca8k.h"
@@ -170,6 +171,174 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
return regmap_update_bits(priv->regmap, reg, mask, write_val);
}
+static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
+{
+ struct qca8k_mgmt_hdr_data *mgmt_hdr_data;
+ struct qca8k_priv *priv = ds->priv;
+ struct mgmt_ethhdr *mgmt_ethhdr;
+ u8 len, cmd;
+
+ mgmt_ethhdr = (struct mgmt_ethhdr *)skb_mac_header(skb);
+ mgmt_hdr_data = &priv->mgmt_hdr_data;
+
+ cmd = FIELD_GET(QCA_HDR_MGMT_CMD, mgmt_ethhdr->command);
+ len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
+
+ /* Make sure the seq match the requested packet */
+ if (mgmt_ethhdr->seq == mgmt_hdr_data->seq)
+ mgmt_hdr_data->ack = true;
+
+ if (cmd == MDIO_READ) {
+ mgmt_hdr_data->data[0] = mgmt_ethhdr->mdio_data;
+
+ /* Get the rest of the 12 byte of data */
+ if (len > QCA_HDR_MGMT_DATA1_LEN)
+ memcpy(mgmt_hdr_data->data + 1, skb->data,
+ QCA_HDR_MGMT_DATA2_LEN);
+ }
+
+ complete(&mgmt_hdr_data->rw_done);
+}
+
+static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
+ int seq_num, int priority)
+{
+ struct mgmt_ethhdr *mgmt_ethhdr;
+ struct sk_buff *skb;
+ u16 hdr;
+
+ skb = dev_alloc_skb(QCA_HDR_MGMT_PKG_LEN);
+ if (!skb)
+ return NULL;
+
+ skb_reset_mac_header(skb);
+ skb_set_network_header(skb, skb->len);
+
+ mgmt_ethhdr = skb_push(skb, QCA_HDR_MGMT_HEADER_LEN + QCA_HDR_LEN);
+
+ hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
+ hdr |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
+ hdr |= QCA_HDR_XMIT_FROM_CPU;
+ hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
+ hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
+
+ mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
+
+ mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
+ mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, 4);
+ mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
+ mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
+ QCA_HDR_MGMT_CHECK_CODE_VAL);
+
+ if (cmd == MDIO_WRITE)
+ mgmt_ethhdr->mdio_data = *val;
+
+ mgmt_ethhdr->hdr = htons(hdr);
+
+ skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
+
+ return skb;
+}
+
+static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
+{
+ struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
+ struct sk_buff *skb;
+ bool ack;
+ int ret;
+
+ skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+ if (!skb)
+ return -ENOMEM;
+
+ mutex_lock(&mgmt_hdr_data->mutex);
+
+ /* Recheck mgmt_master under lock to make sure it's operational */
+ if (!priv->mgmt_master)
+ return -EINVAL;
+
+ skb->dev = (struct net_device *)priv->mgmt_master;
+
+ reinit_completion(&mgmt_hdr_data->rw_done);
+ mgmt_hdr_data->seq = 200;
+ mgmt_hdr_data->ack = false;
+
+ dev_queue_xmit(skb);
+
+ ret = wait_for_completion_timeout(&mgmt_hdr_data->rw_done,
+ msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+
+ *val = mgmt_hdr_data->data[0];
+ ack = mgmt_hdr_data->ack;
+
+ mutex_unlock(&mgmt_hdr_data->mutex);
+
+ if (ret <= 0)
+ return -ETIMEDOUT;
+
+ if (!ack)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
+{
+ struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
+ struct sk_buff *skb;
+ bool ack;
+ int ret;
+
+ skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+ if (!skb)
+ return -ENOMEM;
+
+ mutex_lock(&mgmt_hdr_data->mutex);
+
+ /* Recheck mgmt_master under lock to make sure it's operational */
+ if (!priv->mgmt_master)
+ return -EINVAL;
+
+ skb->dev = (struct net_device *)priv->mgmt_master;
+
+ reinit_completion(&mgmt_hdr_data->rw_done);
+ mgmt_hdr_data->ack = false;
+ mgmt_hdr_data->seq = 200;
+
+ dev_queue_xmit(skb);
+
+ ret = wait_for_completion_timeout(&mgmt_hdr_data->rw_done,
+ msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+
+ ack = mgmt_hdr_data->ack;
+
+ mutex_unlock(&mgmt_hdr_data->mutex);
+
+ if (ret <= 0)
+ return -ETIMEDOUT;
+
+ if (!ack)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int
+qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
+{
+ u32 val = 0;
+ int ret;
+
+ ret = qca8k_read_eth(priv, reg, &val);
+ if (ret)
+ return ret;
+
+ val &= ~mask;
+ val |= write_val;
+
+ return qca8k_write_eth(priv, reg, val);
+}
+
static int
qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
{
@@ -178,6 +347,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
u16 r1, r2, page;
int ret;
+ if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
+ return 0;
+
qca8k_split_addr(reg, &r1, &r2, &page);
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -201,6 +373,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
u16 r1, r2, page;
int ret;
+ if (priv->mgmt_master && !qca8k_write_eth(priv, reg, val))
+ return 0;
+
qca8k_split_addr(reg, &r1, &r2, &page);
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -225,6 +400,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
u32 val;
int ret;
+ if (priv->mgmt_master &&
+ !qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
+ return 0;
+
qca8k_split_addr(reg, &r1, &r2, &page);
mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -2394,10 +2573,33 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
if (dp->index != 0)
return;
+ mutex_lock(&priv->mgmt_hdr_data.mutex);
+
if (operational)
priv->mgmt_master = master;
else
priv->mgmt_master = NULL;
+
+ mutex_unlock(&priv->mgmt_hdr_data.mutex);
+}
+
+static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
+ enum dsa_tag_protocol proto)
+{
+ struct qca_tagger_data *tagger_data;
+
+ switch (proto) {
+ case DSA_TAG_PROTO_QCA:
+ tagger_data = ds->tagger_data;
+
+ tagger_data->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
+
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
}
static const struct dsa_switch_ops qca8k_switch_ops = {
@@ -2436,6 +2638,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
.port_lag_join = qca8k_port_lag_join,
.port_lag_leave = qca8k_port_lag_leave,
.master_state_change = qca8k_master_change,
+ .connect_tag_protocol = qca8k_connect_tag_protocol,
};
static int qca8k_read_switch_id(struct qca8k_priv *priv)
@@ -2515,6 +2718,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
if (!priv->ds)
return -ENOMEM;
+ mutex_init(&priv->mgmt_hdr_data.mutex);
+ init_completion(&priv->mgmt_hdr_data.rw_done);
+
priv->ds->dev = &mdiodev->dev;
priv->ds->num_ports = QCA8K_NUM_PORTS;
priv->ds->priv = priv;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 9437369c60ca..a358a67044d3 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -11,6 +11,10 @@
#include <linux/delay.h>
#include <linux/regmap.h>
#include <linux/gpio.h>
+#include <linux/dsa/tag_qca.h>
+
+#define QCA8K_ETHERNET_MDIO_PRIORITY 7
+#define QCA8K_ETHERNET_TIMEOUT 100
#define QCA8K_NUM_PORTS 7
#define QCA8K_NUM_CPU_PORTS 2
@@ -328,6 +332,14 @@ enum {
QCA8K_CPU_PORT6,
};
+struct qca8k_mgmt_hdr_data {
+ struct completion rw_done;
+ struct mutex mutex; /* Enforce one mdio read/write at time */
+ bool ack;
+ u32 seq;
+ u32 data[4];
+};
+
struct qca8k_ports_config {
bool sgmii_rx_clk_falling_edge;
bool sgmii_tx_clk_falling_edge;
@@ -354,6 +366,7 @@ struct qca8k_priv {
struct gpio_desc *reset_gpio;
unsigned int port_mtu[QCA8K_NUM_PORTS];
const struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
+ struct qca8k_mgmt_hdr_data mgmt_hdr_data;
};
struct qca8k_mib_desc {
--
2.33.1
mgmt Ethernet packet can read/write up to 16byte at times. The len reg
is limited to 15 (0xf). The switch actually sends and accepts data in 4
different steps of len values.
Len steps:
- 0: nothing
- 1-4: first 4 byte
- 5-6: first 12 byte
- 7-15: all 16 byte
In the allock skb function we check if the len is 16 and we fix it to a
len of 15. It the read/write function interest to extract the real asked
data. The tagger handler will always copy the fully 16byte with a READ
command. This is useful for some big regs like the fdb reg that are
more than 4byte of data. This permits to introduce a bulk function that
will send and request the entire entry in one go.
Write function is changed and it does now require to pass the pointer to
val to also handle array val.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 56 ++++++++++++++++++++++++++++++-----------
1 file changed, 41 insertions(+), 15 deletions(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 2a43fb9aeef2..0183ce2d5b74 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -219,7 +219,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
if (cmd == MDIO_READ) {
mgmt_hdr_data->data[0] = mgmt_ethhdr->mdio_data;
- /* Get the rest of the 12 byte of data */
+ /* Get the rest of the 12 byte of data.
+ * The read/write function will extract the requested data.
+ */
if (len > QCA_HDR_MGMT_DATA1_LEN)
memcpy(mgmt_hdr_data->data + 1, skb->data,
QCA_HDR_MGMT_DATA2_LEN);
@@ -229,16 +231,30 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
}
static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
- int seq_num, int priority)
+ int seq_num, int priority, int len)
{
struct mgmt_ethhdr *mgmt_ethhdr;
struct sk_buff *skb;
+ int real_len;
+ u32 *data2;
u16 hdr;
skb = dev_alloc_skb(QCA_HDR_MGMT_PKG_LEN);
if (!skb)
return NULL;
+ /* Max value for len reg is 15 (0xf) but the switch actually return 16 byte
+ * Actually for some reason the steps are:
+ * 0: nothing
+ * 1-4: first 4 byte
+ * 5-6: first 12 byte
+ * 7-15: all 16 byte
+ */
+ if (len == 16)
+ real_len = 15;
+ else
+ real_len = len;
+
skb_reset_mac_header(skb);
skb_set_network_header(skb, skb->len);
@@ -253,7 +269,7 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
- mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, 4);
+ mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, real_len);
mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
QCA_HDR_MGMT_CHECK_CODE_VAL);
@@ -263,19 +279,22 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
mgmt_ethhdr->hdr = htons(hdr);
- skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
+ data2 = skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
+ if (cmd == MDIO_WRITE && len > QCA_HDR_MGMT_DATA1_LEN)
+ memcpy(data2, val + 1, len - QCA_HDR_MGMT_DATA1_LEN);
return skb;
}
-static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
+static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
{
struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
struct sk_buff *skb;
bool ack;
int ret;
- skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+ skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200,
+ QCA8K_ETHERNET_MDIO_PRIORITY, len);
if (!skb)
return -ENOMEM;
@@ -297,6 +316,9 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
*val = mgmt_hdr_data->data[0];
+ if (len > QCA_HDR_MGMT_DATA1_LEN)
+ memcpy(val + 1, mgmt_hdr_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
+
ack = mgmt_hdr_data->ack;
mutex_unlock(&mgmt_hdr_data->mutex);
@@ -310,14 +332,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
return 0;
}
-static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
+static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
{
struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
struct sk_buff *skb;
bool ack;
int ret;
- skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+ skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val, 200,
+ QCA8K_ETHERNET_MDIO_PRIORITY, len);
if (!skb)
return -ENOMEM;
@@ -357,14 +380,14 @@ qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 wri
u32 val = 0;
int ret;
- ret = qca8k_read_eth(priv, reg, &val);
+ ret = qca8k_read_eth(priv, reg, &val, 4);
if (ret)
return ret;
val &= ~mask;
val |= write_val;
- return qca8k_write_eth(priv, reg, val);
+ return qca8k_write_eth(priv, reg, &val, 4);
}
static int
@@ -376,7 +399,7 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
u16 r1, r2, page;
int ret;
- if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
+ if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val, 4))
return 0;
mdio_cache = &priv->mdio_cache;
@@ -405,7 +428,7 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
u16 r1, r2, page;
int ret;
- if (priv->mgmt_master && !qca8k_write_eth(priv, reg, val))
+ if (priv->mgmt_master && !qca8k_write_eth(priv, reg, &val, 4))
return 0;
mdio_cache = &priv->mdio_cache;
@@ -945,17 +968,20 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
/* Prealloc all the needed skb before the lock */
write_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
- &write_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+ &write_val, seq_num,
+ QCA8K_ETHERNET_PHY_PRIORITY, 4);
if (!write_skb)
return -ENOMEM;
clear_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
- &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+ &clear_val, seq_num,
+ QCA8K_ETHERNET_PHY_PRIORITY, 4);
if (!write_skb)
return -ENOMEM;
read_skb = qca8k_alloc_mdio_header(MDIO_READ, QCA8K_MDIO_MASTER_CTRL,
- &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+ &clear_val, seq_num,
+ QCA8K_ETHERNET_PHY_PRIORITY, 4);
if (!write_skb)
return -ENOMEM;
--
2.33.1
The switch can autocast MIB counter using Ethernet packet.
Add support for this and provide a handler for the tagger.
The switch will send packet with MIB counter for each port, the switch
will use completion API to wait for the correct packet to be received
and will complete the task only when each packet is received.
Although the handler will drop all the other packet, we still have to
consume each MIB packet to complete the request. This is done to prevent
mixed data with concurrent ethtool request.
connect_tag_protocol() is used to add the handler to the tag_qca tagger,
master_state_change() use the MIB lock to make sure no MIB Ethernet is
in progress.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 108 +++++++++++++++++++++++++++++++++++++++-
drivers/net/dsa/qca8k.h | 17 ++++++-
2 files changed, 122 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 35711d010eb4..f51a6d8993ff 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -811,7 +811,10 @@ qca8k_mib_init(struct qca8k_priv *priv)
int ret;
mutex_lock(&priv->reg_mutex);
- ret = regmap_set_bits(priv->regmap, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
+ ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
+ QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
+ FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_FLUSH) |
+ QCA8K_MIB_BUSY);
if (ret)
goto exit;
@@ -1882,6 +1885,97 @@ qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data)
ETH_GSTRING_LEN);
}
+static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *skb)
+{
+ const struct qca8k_match_data *match_data;
+ struct qca8k_mib_hdr_data *mib_hdr_data;
+ struct qca8k_priv *priv = ds->priv;
+ const struct qca8k_mib_desc *mib;
+ struct mib_ethhdr *mib_ethhdr;
+ int i, mib_len, offset = 0;
+ u64 *data;
+ u8 port;
+
+ mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
+ mib_hdr_data = &priv->mib_hdr_data;
+
+ /* The switch autocast every port. Ignore other packet and
+ * parse only the requested one.
+ */
+ port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
+ if (port != mib_hdr_data->req_port)
+ goto exit;
+
+ match_data = device_get_match_data(priv->dev);
+ data = mib_hdr_data->data;
+
+ for (i = 0; i < match_data->mib_count; i++) {
+ mib = &ar8327_mib[i];
+
+ /* First 3 mib are present in the skb head */
+ if (i < 3) {
+ data[i] = mib_ethhdr->data[i];
+ continue;
+ }
+
+ mib_len = sizeof(uint32_t);
+
+ /* Some mib are 64 bit wide */
+ if (mib->size == 2)
+ mib_len = sizeof(uint64_t);
+
+ /* Copy the mib value from packet to the */
+ memcpy(data + i, skb->data + offset, mib_len);
+
+ /* Set the offset for the next mib */
+ offset += mib_len;
+ }
+
+exit:
+ /* Complete on receiving all the mib packet */
+ if (refcount_dec_and_test(&mib_hdr_data->port_parsed))
+ complete(&mib_hdr_data->rw_done);
+}
+
+static int
+qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
+{
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct qca8k_mib_hdr_data *mib_hdr_data;
+ struct qca8k_priv *priv = ds->priv;
+ int ret;
+
+ mib_hdr_data = &priv->mib_hdr_data;
+
+ mutex_lock(&mib_hdr_data->mutex);
+
+ reinit_completion(&mib_hdr_data->rw_done);
+
+ mib_hdr_data->req_port = dp->index;
+ mib_hdr_data->data = data;
+ refcount_set(&mib_hdr_data->port_parsed, QCA8K_NUM_PORTS);
+
+ mutex_lock(&priv->reg_mutex);
+
+ /* Send mib autocast request */
+ ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
+ QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
+ FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_CAST) |
+ QCA8K_MIB_BUSY);
+
+ mutex_unlock(&priv->reg_mutex);
+
+ if (ret)
+ goto exit;
+
+ ret = wait_for_completion_timeout(&mib_hdr_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
+
+exit:
+ mutex_unlock(&mib_hdr_data->mutex);
+
+ return ret;
+}
+
static void
qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
uint64_t *data)
@@ -1893,6 +1987,10 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
u32 hi = 0;
int ret;
+ if (priv->mgmt_master &&
+ qca8k_get_ethtool_stats_eth(ds, port, data) > 0)
+ return;
+
match_data = of_device_get_match_data(priv->dev);
for (i = 0; i < match_data->mib_count; i++) {
@@ -2573,7 +2671,8 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
if (dp->index != 0)
return;
- mutex_lock(&priv->mgmt_hdr_data.mutex);
+ mutex_unlock(&priv->mgmt_hdr_data.mutex);
+ mutex_lock(&priv->mib_hdr_data.mutex);
if (operational)
priv->mgmt_master = master;
@@ -2581,6 +2680,7 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
priv->mgmt_master = NULL;
mutex_unlock(&priv->mgmt_hdr_data.mutex);
+ mutex_unlock(&priv->mib_hdr_data.mutex);
}
static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
@@ -2593,6 +2693,7 @@ static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
tagger_data = ds->tagger_data;
tagger_data->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
+ tagger_data->mib_autocast_handler = qca8k_mib_autocast_handler;
break;
default:
@@ -2721,6 +2822,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
mutex_init(&priv->mgmt_hdr_data.mutex);
init_completion(&priv->mgmt_hdr_data.rw_done);
+ mutex_init(&priv->mib_hdr_data.mutex);
+ init_completion(&priv->mib_hdr_data.rw_done);
+
priv->ds->dev = &mdiodev->dev;
priv->ds->num_ports = QCA8K_NUM_PORTS;
priv->ds->priv = priv;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index a358a67044d3..dc1365542aa3 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -67,7 +67,7 @@
#define QCA8K_REG_MODULE_EN 0x030
#define QCA8K_MODULE_EN_MIB BIT(0)
#define QCA8K_REG_MIB 0x034
-#define QCA8K_MIB_FLUSH BIT(24)
+#define QCA8K_MIB_FUNC GENMASK(26, 24)
#define QCA8K_MIB_CPU_KEEP BIT(20)
#define QCA8K_MIB_BUSY BIT(17)
#define QCA8K_MDIO_MASTER_CTRL 0x3c
@@ -317,6 +317,12 @@ enum qca8k_vlan_cmd {
QCA8K_VLAN_READ = 6,
};
+enum qca8k_mid_cmd {
+ QCA8K_MIB_FLUSH = 1,
+ QCA8K_MIB_FLUSH_PORT = 2,
+ QCA8K_MIB_CAST = 3,
+};
+
struct ar8xxx_port_status {
int enabled;
};
@@ -340,6 +346,14 @@ struct qca8k_mgmt_hdr_data {
u32 data[4];
};
+struct qca8k_mib_hdr_data {
+ struct completion rw_done;
+ struct mutex mutex; /* Process one command at time */
+ refcount_t port_parsed; /* Counter to track parsed port */
+ u8 req_port;
+ u64 *data; /* pointer to ethtool data */
+};
+
struct qca8k_ports_config {
bool sgmii_rx_clk_falling_edge;
bool sgmii_tx_clk_falling_edge;
@@ -367,6 +381,7 @@ struct qca8k_priv {
unsigned int port_mtu[QCA8K_NUM_PORTS];
const struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
struct qca8k_mgmt_hdr_data mgmt_hdr_data;
+ struct qca8k_mib_hdr_data mib_hdr_data;
};
struct qca8k_mib_desc {
--
2.33.1
Use mgmt Ethernet also for phy read/write if availabale. Use a different
seq number to make sure we receive the correct packet.
On any error, we fallback to the legacy mdio read/write.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 191 ++++++++++++++++++++++++++++++++++++++++
drivers/net/dsa/qca8k.h | 1 +
2 files changed, 192 insertions(+)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index f51a6d8993ff..e7bc0770bae9 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -848,6 +848,166 @@ qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
}
+static int
+qca8k_phy_eth_busy_wait(struct qca8k_mgmt_hdr_data *phy_hdr_data,
+ struct sk_buff *read_skb, u32 *val)
+{
+ struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
+ bool ack;
+ int ret;
+
+ reinit_completion(&phy_hdr_data->rw_done);
+ phy_hdr_data->seq = 400;
+ phy_hdr_data->ack = false;
+
+ dev_queue_xmit(skb);
+
+ ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
+ QCA8K_ETHERNET_TIMEOUT);
+
+ ack = phy_hdr_data->ack;
+
+ if (ret <= 0)
+ return -ETIMEDOUT;
+
+ if (!ack)
+ return -EINVAL;
+
+ *val = phy_hdr_data->data[0];
+
+ return 0;
+}
+
+static int
+qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
+ int regnum, u16 data)
+{
+ struct sk_buff *write_skb, *clear_skb, *read_skb;
+ struct qca8k_mgmt_hdr_data *phy_hdr_data;
+ const struct net_device *mgmt_master;
+ u32 write_val, clear_val = 0, val;
+ int seq_num = 400;
+ int ret, ret1;
+ bool ack;
+
+ if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
+ return -EINVAL;
+
+ phy_hdr_data = &priv->mgmt_hdr_data;
+
+ write_val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
+ QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+ QCA8K_MDIO_MASTER_REG_ADDR(regnum);
+
+ if (read) {
+ write_val |= QCA8K_MDIO_MASTER_READ;
+ } else {
+ write_val |= QCA8K_MDIO_MASTER_WRITE;
+ write_val |= QCA8K_MDIO_MASTER_DATA(data);
+ }
+
+ /* Prealloc all the needed skb before the lock */
+ write_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
+ &write_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+ if (!write_skb)
+ return -ENOMEM;
+
+ clear_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
+ &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+ if (!write_skb)
+ return -ENOMEM;
+
+ read_skb = qca8k_alloc_mdio_header(MDIO_READ, QCA8K_MDIO_MASTER_CTRL,
+ &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+ if (!write_skb)
+ return -ENOMEM;
+
+ /* Actually start the request:
+ * 1. Send mdio master packet
+ * 2. Busy Wait for mdio master command
+ * 3. Get the data if we are reading
+ * 4. Reset the mdio master (even with error)
+ */
+ mutex_lock(&phy_hdr_data->mutex);
+
+ /* Recheck mgmt_master under lock to make sure it's operational */
+ mgmt_master = priv->mgmt_master;
+ if (!mgmt_master)
+ return -EINVAL;
+
+ read_skb->dev = (struct net_device *)mgmt_master;
+ clear_skb->dev = (struct net_device *)mgmt_master;
+ write_skb->dev = (struct net_device *)mgmt_master;
+
+ reinit_completion(&phy_hdr_data->rw_done);
+ phy_hdr_data->ack = false;
+ phy_hdr_data->seq = seq_num;
+
+ dev_queue_xmit(write_skb);
+
+ ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
+ QCA8K_ETHERNET_TIMEOUT);
+
+ ack = phy_hdr_data->ack;
+
+ if (ret <= 0) {
+ ret = -ETIMEDOUT;
+ goto exit;
+ }
+
+ if (!ack) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
+ !(val & QCA8K_MDIO_MASTER_BUSY), 0,
+ QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
+ phy_hdr_data, read_skb, &val);
+
+ if (ret < 0 && ret1 < 0) {
+ ret = ret1;
+ goto exit;
+ }
+
+ if (read) {
+ reinit_completion(&phy_hdr_data->rw_done);
+ phy_hdr_data->ack = false;
+
+ dev_queue_xmit(read_skb);
+
+ ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
+ QCA8K_ETHERNET_TIMEOUT);
+
+ ack = phy_hdr_data->ack;
+
+ if (ret <= 0) {
+ ret = -ETIMEDOUT;
+ goto exit;
+ }
+
+ if (!ack) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ ret = phy_hdr_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
+ }
+
+exit:
+ reinit_completion(&phy_hdr_data->rw_done);
+ phy_hdr_data->ack = false;
+
+ dev_queue_xmit(clear_skb);
+
+ wait_for_completion_timeout(&phy_hdr_data->rw_done,
+ QCA8K_ETHERNET_TIMEOUT);
+
+ mutex_unlock(&phy_hdr_data->mutex);
+
+ return ret;
+}
+
static u32
qca8k_port_to_phy(int port)
{
@@ -970,6 +1130,14 @@ qca8k_internal_mdio_write(struct mii_bus *slave_bus, int phy, int regnum, u16 da
{
struct qca8k_priv *priv = slave_bus->priv;
struct mii_bus *bus = priv->bus;
+ int ret;
+
+ /* Use mdio Ethernet when available, fallback to legacy one on error */
+ if (priv->mgmt_master) {
+ ret = qca8k_phy_eth_command(priv, false, phy, regnum, data);
+ if (!ret)
+ return 0;
+ }
return qca8k_mdio_write(bus, phy, regnum, data);
}
@@ -979,6 +1147,14 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
{
struct qca8k_priv *priv = slave_bus->priv;
struct mii_bus *bus = priv->bus;
+ int ret;
+
+ /* Use mdio Ethernet when available, fallback to legacy one on error */
+ if (priv->mgmt_master) {
+ ret = qca8k_phy_eth_command(priv, true, phy, regnum, 0);
+ if (ret >= 0)
+ return ret;
+ }
return qca8k_mdio_read(bus, phy, regnum);
}
@@ -987,6 +1163,7 @@ static int
qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
{
struct qca8k_priv *priv = ds->priv;
+ int ret;
/* Check if the legacy mapping should be used and the
* port is not correctly mapped to the right PHY in the
@@ -995,6 +1172,13 @@ qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
if (priv->legacy_phy_port_mapping)
port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
+ /* Use mdio Ethernet when available, fallback to legacy one on error */
+ if (priv->mgmt_master) {
+ ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
+ if (!ret)
+ return ret;
+ }
+
return qca8k_mdio_write(priv->bus, port, regnum, data);
}
@@ -1011,6 +1195,13 @@ qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
if (priv->legacy_phy_port_mapping)
port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
+ /* Use mdio Ethernet when available, fallback to legacy one on error */
+ if (priv->mgmt_master) {
+ ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
+ if (ret >= 0)
+ return ret;
+ }
+
ret = qca8k_mdio_read(priv->bus, port, regnum);
if (ret < 0)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index dc1365542aa3..952217db2047 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -14,6 +14,7 @@
#include <linux/dsa/tag_qca.h>
#define QCA8K_ETHERNET_MDIO_PRIORITY 7
+#define QCA8K_ETHERNET_PHY_PRIORITY 6
#define QCA8K_ETHERNET_TIMEOUT 100
#define QCA8K_NUM_PORTS 7
--
2.33.1
From Documentation, we can cache lo and hi the same way we do with the
page. This massively reduce the mdio write as 3/4 of the time as we only
require to write the lo or hi part for a mdio write.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 60 ++++++++++++++++++++++++++++++++---------
drivers/net/dsa/qca8k.h | 5 ++++
2 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index c2f5414033d8..2a43fb9aeef2 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -88,6 +88,42 @@ qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
*page = regaddr & 0x3ff;
}
+static int
+qca8k_set_lo(struct mii_bus *bus, int phy_id, u32 regnum,
+ u16 lo, u16 *cached_lo)
+{
+ int ret;
+
+ if (lo == *cached_lo)
+ return 0;
+
+ ret = bus->write(bus, phy_id, regnum, lo);
+ if (ret < 0)
+ dev_err_ratelimited(&bus->dev,
+ "failed to write qca8k 32bit lo register\n");
+
+ *cached_lo = lo;
+ return 0;
+}
+
+static int
+qca8k_set_hi(struct mii_bus *bus, int phy_id, u32 regnum,
+ u16 hi, u16 *cached_hi)
+{
+ int ret;
+
+ if (hi == *cached_hi)
+ return 0;
+
+ ret = bus->write(bus, phy_id, regnum, hi);
+ if (ret < 0)
+ dev_err_ratelimited(&bus->dev,
+ "failed to write qca8k 32bit hi register\n");
+
+ *cached_hi = hi;
+ return 0;
+}
+
static int
qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val)
{
@@ -111,7 +147,8 @@ qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val)
}
static void
-qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
+qca8k_mii_write32(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
+ int phy_id, u32 regnum, u32 val)
{
u16 lo, hi;
int ret;
@@ -119,12 +156,9 @@ qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
lo = val & 0xffff;
hi = (u16)(val >> 16);
- ret = bus->write(bus, phy_id, regnum, lo);
+ ret = qca8k_set_lo(bus, phy_id, regnum, lo, &cache->lo);
if (ret >= 0)
- ret = bus->write(bus, phy_id, regnum + 1, hi);
- if (ret < 0)
- dev_err_ratelimited(&bus->dev,
- "failed to write qca8k 32bit register\n");
+ ret = qca8k_set_hi(bus, phy_id, regnum + 1, hi, &cache->hi);
}
static int
@@ -384,7 +418,7 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
if (ret < 0)
goto exit;
- qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+ qca8k_mii_write32(bus, mdio_cache, 0x10 | r2, r1, val);
exit:
mutex_unlock(&bus->mdio_lock);
@@ -421,7 +455,7 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
val &= ~mask;
val |= write_val;
- qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+ qca8k_mii_write32(bus, mdio_cache, 0x10 | r2, r1, val);
exit:
mutex_unlock(&bus->mdio_lock);
@@ -1072,14 +1106,14 @@ qca8k_mdio_write(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
if (ret)
goto exit;
- qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+ qca8k_mii_write32(bus, cache, 0x10 | r2, r1, val);
ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
QCA8K_MDIO_MASTER_BUSY);
exit:
/* even if the busy_wait timeouts try to clear the MASTER_EN */
- qca8k_mii_write32(bus, 0x10 | r2, r1, 0);
+ qca8k_mii_write32(bus, cache, 0x10 | r2, r1, 0);
mutex_unlock(&bus->mdio_lock);
@@ -1109,7 +1143,7 @@ qca8k_mdio_read(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
if (ret)
goto exit;
- qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+ qca8k_mii_write32(bus, cache, 0x10 | r2, r1, val);
ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
QCA8K_MDIO_MASTER_BUSY);
@@ -1120,7 +1154,7 @@ qca8k_mdio_read(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
exit:
/* even if the busy_wait timeouts try to clear the MASTER_EN */
- qca8k_mii_write32(bus, 0x10 | r2, r1, 0);
+ qca8k_mii_write32(bus, cache, 0x10 | r2, r1, 0);
mutex_unlock(&bus->mdio_lock);
@@ -3007,6 +3041,8 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
}
priv->mdio_cache.page = 0xffff;
+ priv->mdio_cache.lo = 0xffff;
+ priv->mdio_cache.hi = 0xffff;
/* Check the detected switch id */
ret = qca8k_read_switch_id(priv);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 77ffdc7b5aaa..9ecd4d221906 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -369,6 +369,11 @@ struct qca8k_mdio_cache {
* mdio writes
*/
u16 page;
+/* lo and hi can also be cached and from Documentation we can skip one
+ * extra mdio write if lo or hi is didn't change.
+ */
+ u16 lo;
+ u16 hi;
};
struct qca8k_priv {
--
2.33.1
Add connect/disconnect helper to assign private struct to the dsa switch.
Add support for Ethernet mgm and MIB if the dsa driver provide an handler
to correctly parse and elaborate the data.
Signed-off-by: Ansuel Smith <[email protected]>
---
include/linux/dsa/tag_qca.h | 7 +++++++
net/dsa/tag_qca.c | 39 ++++++++++++++++++++++++++++++++++---
2 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index 87dd84e31304..de5a45f5b398 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -72,4 +72,11 @@ struct mib_ethhdr {
__be16 hdr; /* qca hdr */
} __packed;
+struct qca_tagger_data {
+ void (*rw_reg_ack_handler)(struct dsa_switch *ds,
+ struct sk_buff *skb);
+ void (*mib_autocast_handler)(struct dsa_switch *ds,
+ struct sk_buff *skb);
+};
+
#endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index fdaa1b322d25..dc81c72133eb 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -5,6 +5,7 @@
#include <linux/etherdevice.h>
#include <linux/bitfield.h>
+#include <net/dsa.h>
#include <linux/dsa/tag_qca.h>
#include "dsa_priv.h"
@@ -32,11 +33,16 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
{
+ struct qca_tagger_data *tagger_data;
+ struct dsa_port *dp = dev->dsa_ptr;
+ struct dsa_switch *ds = dp->ds;
u16 hdr, pk_type;
__be16 *phdr;
int port;
u8 ver;
+ tagger_data = ds->tagger_data;
+
if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
return NULL;
@@ -51,13 +57,19 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
/* Get pk type */
pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
- /* Ethernet MDIO read/write packet */
- if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
+ /* Ethernet mgmt read/write packet */
+ if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
+ if (tagger_data->rw_reg_ack_handler)
+ tagger_data->rw_reg_ack_handler(ds, skb);
return NULL;
+ }
/* Ethernet MIB counter packet */
- if (pk_type == QCA_HDR_RECV_TYPE_MIB)
+ if (pk_type == QCA_HDR_RECV_TYPE_MIB) {
+ if (tagger_data->mib_autocast_handler)
+ tagger_data->mib_autocast_handler(ds, skb);
return NULL;
+ }
/* Remove QCA tag and recalculate checksum */
skb_pull_rcsum(skb, QCA_HDR_LEN);
@@ -73,9 +85,30 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
return skb;
}
+static int qca_tag_connect(struct dsa_switch *ds)
+{
+ struct qca_tagger_data *tagger_data;
+
+ tagger_data = kzalloc(sizeof(*tagger_data), GFP_KERNEL);
+ if (!tagger_data)
+ return -ENOMEM;
+
+ ds->tagger_data = tagger_data;
+
+ return 0;
+}
+
+static void qca_tag_disconnect(struct dsa_switch *ds)
+{
+ kfree(ds->tagger_data);
+ ds->tagger_data = NULL;
+}
+
static const struct dsa_device_ops qca_netdev_ops = {
.name = "qca",
.proto = DSA_TAG_PROTO_QCA,
+ .connect = qca_tag_connect,
+ .disconnect = qca_tag_disconnect,
.xmit = qca_tag_xmit,
.rcv = qca_tag_rcv,
.needed_headroom = QCA_HDR_LEN,
--
2.33.1
Introduce qca8k_bulk_read/write() function to use mgmt Ethernet way to
read/write packet in bulk. Make use of this new function in the fdb
function.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 55 ++++++++++++++++++++++++++++++++---------
1 file changed, 43 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 0183ce2d5b74..b8d063c58675 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -390,6 +390,43 @@ qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 wri
return qca8k_write_eth(priv, reg, &val, 4);
}
+static int
+qca8k_bulk_read(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
+{
+ int i, count = len / sizeof(u32), ret;
+
+ if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val, len))
+ return 0;
+
+ for (i = 0; i < count; i++) {
+ ret = regmap_read(priv->regmap, reg + (i * 4), val + i);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int
+qca8k_bulk_write(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
+{
+ int i, count = len / sizeof(u32), ret;
+ u32 tmp;
+
+ if (priv->mgmt_master && !qca8k_write_eth(priv, reg, val, len))
+ return 0;
+
+ for (i = 0; i < count; i++) {
+ tmp = val[i];
+
+ ret = regmap_write(priv->regmap, reg + (i * 4), tmp);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
static int
qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
{
@@ -535,17 +572,13 @@ qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
static int
qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
{
- u32 reg[4], val;
- int i, ret;
+ u32 reg[4];
+ int ret;
/* load the ARL table into an array */
- for (i = 0; i < 4; i++) {
- ret = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4), &val);
- if (ret < 0)
- return ret;
-
- reg[i] = val;
- }
+ ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, 12);
+ if (ret)
+ return ret;
/* vid - 83:72 */
fdb->vid = FIELD_GET(QCA8K_ATU_VID_MASK, reg[2]);
@@ -569,7 +602,6 @@ qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
u8 aging)
{
u32 reg[3] = { 0 };
- int i;
/* vid - 83:72 */
reg[2] = FIELD_PREP(QCA8K_ATU_VID_MASK, vid);
@@ -586,8 +618,7 @@ qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR5_MASK, mac[5]);
/* load the array into the ARL table */
- for (i = 0; i < 3; i++)
- qca8k_write(priv, QCA8K_REG_ATU_DATA0 + (i * 4), reg[i]);
+ qca8k_bulk_write(priv, QCA8K_REG_ATU_DATA0, reg, 12);
}
static int
--
2.33.1
MDIO/MIB Ethernet require the master port and the tagger availabale to
correctly work. Use the new api master_state_change to track when master
is operational or not and set a bool in qca8k_priv.
We cache the first cached master available and we check if other cpu
port are operational when the cached one goes down.
This cached master will later be used by mdio read/write and mib request to
correctly use the working function.
qca8k implementation for MDIO/MIB Ethernet is bad. CPU port0 is the only
one that answers with the ack packet or sends MIB Ethernet packets. For
this reason the master_state_change ignore CPU port6 and checkes only
CPU port0 if it's operational and enables this mode.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/net/dsa/qca8k.c | 18 ++++++++++++++++++
drivers/net/dsa/qca8k.h | 1 +
2 files changed, 19 insertions(+)
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 039694518788..4bc5064414b5 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -2383,6 +2383,23 @@ qca8k_port_lag_leave(struct dsa_switch *ds, int port,
return qca8k_lag_refresh_portmap(ds, port, lag, true);
}
+static void
+qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
+ bool operational)
+{
+ struct dsa_port *dp = master->dsa_ptr;
+ struct qca8k_priv *priv = ds->priv;
+
+ /* Ethernet MIB/MDIO is only supported for CPU port 0 */
+ if (dp->index != 0)
+ return;
+
+ if (operational)
+ priv->mgmt_master = master;
+ else
+ priv->mgmt_master = NULL;
+}
+
static const struct dsa_switch_ops qca8k_switch_ops = {
.get_tag_protocol = qca8k_get_tag_protocol,
.setup = qca8k_setup,
@@ -2418,6 +2435,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
.get_phy_flags = qca8k_get_phy_flags,
.port_lag_join = qca8k_port_lag_join,
.port_lag_leave = qca8k_port_lag_leave,
+ .master_state_change = qca8k_master_change,
};
static int qca8k_read_switch_id(struct qca8k_priv *priv)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index ab4a417b25a9..9437369c60ca 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -353,6 +353,7 @@ struct qca8k_priv {
struct dsa_switch_ops ops;
struct gpio_desc *reset_gpio;
unsigned int port_mtu[QCA8K_NUM_PORTS];
+ const struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
};
struct qca8k_mib_desc {
--
2.33.1
Add struct to correctly parse a mib Ethernet packet.
Signed-off-by: Ansuel Smith <[email protected]>
---
include/linux/dsa/tag_qca.h | 10 ++++++++++
net/dsa/tag_qca.c | 4 ++++
2 files changed, 14 insertions(+)
diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index 1a02f695f3a3..87dd84e31304 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -62,4 +62,14 @@ struct mgmt_ethhdr {
__be16 hdr; /* qca hdr */
} __packed;
+enum mdio_cmd {
+ MDIO_WRITE = 0x0,
+ MDIO_READ
+};
+
+struct mib_ethhdr {
+ u32 data[3]; /* first 3 mib counter */
+ __be16 hdr; /* qca hdr */
+} __packed;
+
#endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index c57d6e1a0c0c..fdaa1b322d25 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -55,6 +55,10 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
return NULL;
+ /* Ethernet MIB counter packet */
+ if (pk_type == QCA_HDR_RECV_TYPE_MIB)
+ return NULL;
+
/* Remove QCA tag and recalculate checksum */
skb_pull_rcsum(skb, QCA_HDR_LEN);
dsa_strip_etype_header(skb, QCA_HDR_LEN);
--
2.33.1
On Sun, Jan 23, 2022 at 02:33:25AM +0100, Ansuel Smith wrote:
> Move tag_qca define to include dir linux/dsa as the qca8k require access
> to the tagger define to support in-band mdio read/write using ethernet
> packet.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Sun, Jan 23, 2022 at 02:33:24AM +0100, Ansuel Smith wrote:
> Convert driver to FIELD macro to drop redundant define.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Sun, Jan 23, 2022 at 02:33:27AM +0100, Ansuel Smith wrote:
> Add all the required define to prepare support for mgmt read/write in
> Ethernet packet. Any packet of this type has to be dropped as the only
> use of these special packet is receive ack for an mgmt write request or
> receive data for an mgmt read request.
> A struct is used that emulates the Ethernet header but is used for a
> different purpose.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> include/linux/dsa/tag_qca.h | 44 +++++++++++++++++++++++++++++++++++++
> net/dsa/tag_qca.c | 13 ++++++++---
> 2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> index c02d2d39ff4a..1a02f695f3a3 100644
> --- a/include/linux/dsa/tag_qca.h
> +++ b/include/linux/dsa/tag_qca.h
> @@ -12,10 +12,54 @@
> #define QCA_HDR_RECV_FRAME_IS_TAGGED BIT(3)
> #define QCA_HDR_RECV_SOURCE_PORT GENMASK(2, 0)
>
> +/* Packet type for recv */
> +#define QCA_HDR_RECV_TYPE_NORMAL 0x0
> +#define QCA_HDR_RECV_TYPE_MIB 0x1
> +#define QCA_HDR_RECV_TYPE_RW_REG_ACK 0x2
> +
> #define QCA_HDR_XMIT_VERSION GENMASK(15, 14)
> #define QCA_HDR_XMIT_PRIORITY GENMASK(13, 11)
> #define QCA_HDR_XMIT_CONTROL GENMASK(10, 8)
> #define QCA_HDR_XMIT_FROM_CPU BIT(7)
> #define QCA_HDR_XMIT_DP_BIT GENMASK(6, 0)
>
> +/* Packet type for xmit */
> +#define QCA_HDR_XMIT_TYPE_NORMAL 0x0
> +#define QCA_HDR_XMIT_TYPE_RW_REG 0x1
> +
> +/* Check code for a valid mgmt packet. Switch will ignore the packet
> + * with this wrong.
> + */
> +#define QCA_HDR_MGMT_CHECK_CODE_VAL 0x5
> +
> +/* Specific define for in-band MDIO read/write with Ethernet packet */
> +#define QCA_HDR_MGMT_SEQ_LEN 4 /* 4 byte for the seq */
> +#define QCA_HDR_MGMT_COMMAND_LEN 4 /* 4 byte for the command */
> +#define QCA_HDR_MGMT_DATA1_LEN 4 /* First 4 byte for the mdio data */
> +#define QCA_HDR_MGMT_HEADER_LEN (QCA_HDR_MGMT_SEQ_LEN + \
> + QCA_HDR_MGMT_COMMAND_LEN + \
> + QCA_HDR_MGMT_DATA1_LEN)
> +
> +#define QCA_HDR_MGMT_DATA2_LEN 12 /* Other 12 byte for the mdio data */
> +#define QCA_HDR_MGMT_PADDING_LEN 34 /* Padding to reach the min Ethernet packet */
> +
> +#define QCA_HDR_MGMT_PKG_LEN (QCA_HDR_MGMT_HEADER_LEN + \
> + QCA_HDR_LEN + \
> + QCA_HDR_MGMT_DATA2_LEN + \
> + QCA_HDR_MGMT_PADDING_LEN)
s/PKG/PKT/
> +
> +#define QCA_HDR_MGMT_SEQ_NUM GENMASK(31, 0) /* 63, 32 */
> +#define QCA_HDR_MGMT_CHECK_CODE GENMASK(31, 29) /* 31, 29 */
> +#define QCA_HDR_MGMT_CMD BIT(28) /* 28 */
> +#define QCA_HDR_MGMT_LENGTH GENMASK(23, 20) /* 23, 20 */
> +#define QCA_HDR_MGMT_ADDR GENMASK(18, 0) /* 18, 0 */
> +
> +/* Special struct emulating a Ethernet header */
> +struct mgmt_ethhdr {
> + u32 command; /* command bit 31:0 */
> + u32 seq; /* seq 63:32 */
> + u32 mdio_data; /* first 4byte mdio */
> + __be16 hdr; /* qca hdr */
> +} __packed;
Could you name this something a bit more specific, like qca_mgmt_ethhdr?
> +
> #endif /* __TAG_QCA_H */
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index f8df49d5956f..c57d6e1a0c0c 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -32,10 +32,10 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
>
> static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> {
> - u8 ver;
> - u16 hdr;
> - int port;
> + u16 hdr, pk_type;
> __be16 *phdr;
> + int port;
> + u8 ver;
>
> if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
> return NULL;
> @@ -48,6 +48,13 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> if (unlikely(ver != QCA_HDR_VERSION))
> return NULL;
>
> + /* Get pk type */
> + pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
> +
> + /* Ethernet MDIO read/write packet */
> + if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
> + return NULL;
> +
> /* Remove QCA tag and recalculate checksum */
> skb_pull_rcsum(skb, QCA_HDR_LEN);
> dsa_strip_etype_header(skb, QCA_HDR_LEN);
> --
> 2.33.1
>
On Sun, Jan 23, 2022 at 02:33:29AM +0100, Ansuel Smith wrote:
> Add connect/disconnect helper to assign private struct to the dsa switch.
> Add support for Ethernet mgm and MIB if the dsa driver provide an handler
> to correctly parse and elaborate the data.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
This seems ok to me, would like some feedback from Andrew, Vivien and
Florian too.
Reviewed-by: Vladimir Oltean <[email protected]>
On Mon, Jan 24, 2022 at 06:05:37PM +0200, Vladimir Oltean wrote:
> On Sun, Jan 23, 2022 at 02:33:27AM +0100, Ansuel Smith wrote:
> > Add all the required define to prepare support for mgmt read/write in
> > Ethernet packet. Any packet of this type has to be dropped as the only
> > use of these special packet is receive ack for an mgmt write request or
> > receive data for an mgmt read request.
> > A struct is used that emulates the Ethernet header but is used for a
> > different purpose.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > include/linux/dsa/tag_qca.h | 44 +++++++++++++++++++++++++++++++++++++
> > net/dsa/tag_qca.c | 13 ++++++++---
> > 2 files changed, 54 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> > index c02d2d39ff4a..1a02f695f3a3 100644
> > --- a/include/linux/dsa/tag_qca.h
> > +++ b/include/linux/dsa/tag_qca.h
> > @@ -12,10 +12,54 @@
> > #define QCA_HDR_RECV_FRAME_IS_TAGGED BIT(3)
> > #define QCA_HDR_RECV_SOURCE_PORT GENMASK(2, 0)
> >
> > +/* Packet type for recv */
> > +#define QCA_HDR_RECV_TYPE_NORMAL 0x0
> > +#define QCA_HDR_RECV_TYPE_MIB 0x1
> > +#define QCA_HDR_RECV_TYPE_RW_REG_ACK 0x2
> > +
> > #define QCA_HDR_XMIT_VERSION GENMASK(15, 14)
> > #define QCA_HDR_XMIT_PRIORITY GENMASK(13, 11)
> > #define QCA_HDR_XMIT_CONTROL GENMASK(10, 8)
> > #define QCA_HDR_XMIT_FROM_CPU BIT(7)
> > #define QCA_HDR_XMIT_DP_BIT GENMASK(6, 0)
> >
> > +/* Packet type for xmit */
> > +#define QCA_HDR_XMIT_TYPE_NORMAL 0x0
> > +#define QCA_HDR_XMIT_TYPE_RW_REG 0x1
> > +
> > +/* Check code for a valid mgmt packet. Switch will ignore the packet
> > + * with this wrong.
> > + */
> > +#define QCA_HDR_MGMT_CHECK_CODE_VAL 0x5
> > +
> > +/* Specific define for in-band MDIO read/write with Ethernet packet */
> > +#define QCA_HDR_MGMT_SEQ_LEN 4 /* 4 byte for the seq */
> > +#define QCA_HDR_MGMT_COMMAND_LEN 4 /* 4 byte for the command */
> > +#define QCA_HDR_MGMT_DATA1_LEN 4 /* First 4 byte for the mdio data */
> > +#define QCA_HDR_MGMT_HEADER_LEN (QCA_HDR_MGMT_SEQ_LEN + \
> > + QCA_HDR_MGMT_COMMAND_LEN + \
> > + QCA_HDR_MGMT_DATA1_LEN)
> > +
> > +#define QCA_HDR_MGMT_DATA2_LEN 12 /* Other 12 byte for the mdio data */
> > +#define QCA_HDR_MGMT_PADDING_LEN 34 /* Padding to reach the min Ethernet packet */
> > +
> > +#define QCA_HDR_MGMT_PKG_LEN (QCA_HDR_MGMT_HEADER_LEN + \
> > + QCA_HDR_LEN + \
> > + QCA_HDR_MGMT_DATA2_LEN + \
> > + QCA_HDR_MGMT_PADDING_LEN)
>
> s/PKG/PKT/
>
> > +
> > +#define QCA_HDR_MGMT_SEQ_NUM GENMASK(31, 0) /* 63, 32 */
> > +#define QCA_HDR_MGMT_CHECK_CODE GENMASK(31, 29) /* 31, 29 */
> > +#define QCA_HDR_MGMT_CMD BIT(28) /* 28 */
> > +#define QCA_HDR_MGMT_LENGTH GENMASK(23, 20) /* 23, 20 */
> > +#define QCA_HDR_MGMT_ADDR GENMASK(18, 0) /* 18, 0 */
> > +
> > +/* Special struct emulating a Ethernet header */
> > +struct mgmt_ethhdr {
> > + u32 command; /* command bit 31:0 */
> > + u32 seq; /* seq 63:32 */
> > + u32 mdio_data; /* first 4byte mdio */
> > + __be16 hdr; /* qca hdr */
> > +} __packed;
>
> Could you name this something a bit more specific, like qca_mgmt_ethhdr?
>
Sure will fix in v8.
> > +
> > #endif /* __TAG_QCA_H */
> > diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> > index f8df49d5956f..c57d6e1a0c0c 100644
> > --- a/net/dsa/tag_qca.c
> > +++ b/net/dsa/tag_qca.c
> > @@ -32,10 +32,10 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
> >
> > static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> > {
> > - u8 ver;
> > - u16 hdr;
> > - int port;
> > + u16 hdr, pk_type;
> > __be16 *phdr;
> > + int port;
> > + u8 ver;
> >
> > if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
> > return NULL;
> > @@ -48,6 +48,13 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> > if (unlikely(ver != QCA_HDR_VERSION))
> > return NULL;
> >
> > + /* Get pk type */
> > + pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
> > +
> > + /* Ethernet MDIO read/write packet */
> > + if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
> > + return NULL;
> > +
> > /* Remove QCA tag and recalculate checksum */
> > skb_pull_rcsum(skb, QCA_HDR_LEN);
> > dsa_strip_etype_header(skb, QCA_HDR_LEN);
> > --
> > 2.33.1
> >
>
--
Ansuel
On Sun, Jan 23, 2022 at 02:33:31AM +0100, Ansuel Smith wrote:
> Add qca8k side support for mgmt read/write in Ethernet packet.
> qca8k supports some specially crafted Ethernet packet that can be used
> for mgmt read/write instead of the legacy method uart/internal mdio.
> This add support for the qca8k side to craft the packet and enqueue it.
> Each port and the qca8k_priv have a special struct to put data in it.
> The completion API is used to wait for the packet to be received back
> with the requested data.
>
> The various steps are:
> 1. Craft the special packet with the qca hdr set to mgmt read/write
> mode.
> 2. Set the lock in the dedicated mgmt struct.
> 3. Reinit the completion.
> 4. Enqueue the packet.
> 5. Wait the packet to be received.
> 6. Use the data set by the tagger to complete the mdio operation.
>
> If the completion timeouts or the ack value is not true, the legacy
> mdio way is used.
>
> It has to be considered that in the initial setup mdio is still used and
> mdio is still used until DSA is ready to accept and tag packet.
>
> tag_proto_connect() is used to fill the required handler for the tagger
> to correctly parse and elaborate the special Ethernet mdio packet.
>
> Locking is added to qca8k_master_change() to make sure no mgmt Ethernet
> are in progress.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/net/dsa/qca8k.c | 206 ++++++++++++++++++++++++++++++++++++++++
> drivers/net/dsa/qca8k.h | 13 +++
> 2 files changed, 219 insertions(+)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 4bc5064414b5..35711d010eb4 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -20,6 +20,7 @@
> #include <linux/phylink.h>
> #include <linux/gpio/consumer.h>
> #include <linux/etherdevice.h>
> +#include <linux/dsa/tag_qca.h>
>
> #include "qca8k.h"
>
> @@ -170,6 +171,174 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> return regmap_update_bits(priv->regmap, reg, mask, write_val);
> }
>
> +static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> +{
> + struct qca8k_mgmt_hdr_data *mgmt_hdr_data;
> + struct qca8k_priv *priv = ds->priv;
> + struct mgmt_ethhdr *mgmt_ethhdr;
> + u8 len, cmd;
> +
> + mgmt_ethhdr = (struct mgmt_ethhdr *)skb_mac_header(skb);
> + mgmt_hdr_data = &priv->mgmt_hdr_data;
> +
> + cmd = FIELD_GET(QCA_HDR_MGMT_CMD, mgmt_ethhdr->command);
> + len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
> +
> + /* Make sure the seq match the requested packet */
> + if (mgmt_ethhdr->seq == mgmt_hdr_data->seq)
> + mgmt_hdr_data->ack = true;
> +
> + if (cmd == MDIO_READ) {
> + mgmt_hdr_data->data[0] = mgmt_ethhdr->mdio_data;
> +
> + /* Get the rest of the 12 byte of data */
> + if (len > QCA_HDR_MGMT_DATA1_LEN)
> + memcpy(mgmt_hdr_data->data + 1, skb->data,
> + QCA_HDR_MGMT_DATA2_LEN);
> + }
> +
> + complete(&mgmt_hdr_data->rw_done);
> +}
> +
> +static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> + int seq_num, int priority)
> +{
> + struct mgmt_ethhdr *mgmt_ethhdr;
> + struct sk_buff *skb;
> + u16 hdr;
> +
> + skb = dev_alloc_skb(QCA_HDR_MGMT_PKG_LEN);
> + if (!skb)
> + return NULL;
> +
> + skb_reset_mac_header(skb);
> + skb_set_network_header(skb, skb->len);
> +
> + mgmt_ethhdr = skb_push(skb, QCA_HDR_MGMT_HEADER_LEN + QCA_HDR_LEN);
> +
> + hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
> + hdr |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
> + hdr |= QCA_HDR_XMIT_FROM_CPU;
> + hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
> + hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
> +
> + mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
> +
> + mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
> + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, 4);
> + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
> + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
> + QCA_HDR_MGMT_CHECK_CODE_VAL);
> +
> + if (cmd == MDIO_WRITE)
> + mgmt_ethhdr->mdio_data = *val;
> +
> + mgmt_ethhdr->hdr = htons(hdr);
> +
> + skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
> +
> + return skb;
> +}
> +
> +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> +{
> + struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> + struct sk_buff *skb;
> + bool ack;
> + int ret;
> +
> + skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> + if (!skb)
> + return -ENOMEM;
> +
> + mutex_lock(&mgmt_hdr_data->mutex);
> +
> + /* Recheck mgmt_master under lock to make sure it's operational */
> + if (!priv->mgmt_master)
mutex_unlock and kfree_skb
Also, why "recheck under lock"? Why not check just under lock?
> + return -EINVAL;
> +
> + skb->dev = (struct net_device *)priv->mgmt_master;
> +
> + reinit_completion(&mgmt_hdr_data->rw_done);
> + mgmt_hdr_data->seq = 200;
> + mgmt_hdr_data->ack = false;
> +
> + dev_queue_xmit(skb);
> +
> + ret = wait_for_completion_timeout(&mgmt_hdr_data->rw_done,
> + msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> +
> + *val = mgmt_hdr_data->data[0];
> + ack = mgmt_hdr_data->ack;
> +
> + mutex_unlock(&mgmt_hdr_data->mutex);
> +
> + if (ret <= 0)
> + return -ETIMEDOUT;
> +
> + if (!ack)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> +{
> + struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> + struct sk_buff *skb;
> + bool ack;
> + int ret;
> +
> + skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> + if (!skb)
> + return -ENOMEM;
> +
> + mutex_lock(&mgmt_hdr_data->mutex);
> +
> + /* Recheck mgmt_master under lock to make sure it's operational */
> + if (!priv->mgmt_master)
mutex_unlock and kfree_skb
> + return -EINVAL;
> +
> + skb->dev = (struct net_device *)priv->mgmt_master;
> +
> + reinit_completion(&mgmt_hdr_data->rw_done);
> + mgmt_hdr_data->ack = false;
> + mgmt_hdr_data->seq = 200;
> +
> + dev_queue_xmit(skb);
> +
> + ret = wait_for_completion_timeout(&mgmt_hdr_data->rw_done,
> + msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> +
> + ack = mgmt_hdr_data->ack;
> +
> + mutex_unlock(&mgmt_hdr_data->mutex);
> +
> + if (ret <= 0)
> + return -ETIMEDOUT;
> +
> + if (!ack)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int
> +qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> +{
> + u32 val = 0;
> + int ret;
> +
> + ret = qca8k_read_eth(priv, reg, &val);
> + if (ret)
> + return ret;
> +
> + val &= ~mask;
> + val |= write_val;
> +
> + return qca8k_write_eth(priv, reg, val);
> +}
> +
> static int
> qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> {
> @@ -178,6 +347,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> u16 r1, r2, page;
> int ret;
>
> + if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
What happens if you remove this priv->mgmt_master check from outside the
lock, and reorder the skb allocation with the priv->mgmt_master check?
> + return 0;
> +
> qca8k_split_addr(reg, &r1, &r2, &page);
>
> mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -201,6 +373,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
> u16 r1, r2, page;
> int ret;
>
> + if (priv->mgmt_master && !qca8k_write_eth(priv, reg, val))
> + return 0;
> +
> qca8k_split_addr(reg, &r1, &r2, &page);
>
> mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -225,6 +400,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
> u32 val;
> int ret;
>
> + if (priv->mgmt_master &&
> + !qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
> + return 0;
> +
> qca8k_split_addr(reg, &r1, &r2, &page);
>
> mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -2394,10 +2573,33 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> if (dp->index != 0)
> return;
>
> + mutex_lock(&priv->mgmt_hdr_data.mutex);
> +
> if (operational)
> priv->mgmt_master = master;
> else
> priv->mgmt_master = NULL;
> +
> + mutex_unlock(&priv->mgmt_hdr_data.mutex);
> +}
> +
> +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> + enum dsa_tag_protocol proto)
> +{
> + struct qca_tagger_data *tagger_data;
> +
> + switch (proto) {
> + case DSA_TAG_PROTO_QCA:
> + tagger_data = ds->tagger_data;
> +
> + tagger_data->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> +
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> }
>
> static const struct dsa_switch_ops qca8k_switch_ops = {
> @@ -2436,6 +2638,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> .port_lag_join = qca8k_port_lag_join,
> .port_lag_leave = qca8k_port_lag_leave,
> .master_state_change = qca8k_master_change,
> + .connect_tag_protocol = qca8k_connect_tag_protocol,
> };
>
> static int qca8k_read_switch_id(struct qca8k_priv *priv)
> @@ -2515,6 +2718,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> if (!priv->ds)
> return -ENOMEM;
>
> + mutex_init(&priv->mgmt_hdr_data.mutex);
> + init_completion(&priv->mgmt_hdr_data.rw_done);
> +
> priv->ds->dev = &mdiodev->dev;
> priv->ds->num_ports = QCA8K_NUM_PORTS;
> priv->ds->priv = priv;
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 9437369c60ca..a358a67044d3 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -11,6 +11,10 @@
> #include <linux/delay.h>
> #include <linux/regmap.h>
> #include <linux/gpio.h>
> +#include <linux/dsa/tag_qca.h>
> +
> +#define QCA8K_ETHERNET_MDIO_PRIORITY 7
> +#define QCA8K_ETHERNET_TIMEOUT 100
>
> #define QCA8K_NUM_PORTS 7
> #define QCA8K_NUM_CPU_PORTS 2
> @@ -328,6 +332,14 @@ enum {
> QCA8K_CPU_PORT6,
> };
>
> +struct qca8k_mgmt_hdr_data {
> + struct completion rw_done;
> + struct mutex mutex; /* Enforce one mdio read/write at time */
> + bool ack;
> + u32 seq;
> + u32 data[4];
> +};
> +
> struct qca8k_ports_config {
> bool sgmii_rx_clk_falling_edge;
> bool sgmii_tx_clk_falling_edge;
> @@ -354,6 +366,7 @@ struct qca8k_priv {
> struct gpio_desc *reset_gpio;
> unsigned int port_mtu[QCA8K_NUM_PORTS];
> const struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
> + struct qca8k_mgmt_hdr_data mgmt_hdr_data;
> };
>
> struct qca8k_mib_desc {
> --
> 2.33.1
>
On Mon, Jan 24, 2022 at 06:32:36PM +0200, Vladimir Oltean wrote:
> On Sun, Jan 23, 2022 at 02:33:31AM +0100, Ansuel Smith wrote:
> > Add qca8k side support for mgmt read/write in Ethernet packet.
> > qca8k supports some specially crafted Ethernet packet that can be used
> > for mgmt read/write instead of the legacy method uart/internal mdio.
> > This add support for the qca8k side to craft the packet and enqueue it.
> > Each port and the qca8k_priv have a special struct to put data in it.
> > The completion API is used to wait for the packet to be received back
> > with the requested data.
> >
> > The various steps are:
> > 1. Craft the special packet with the qca hdr set to mgmt read/write
> > mode.
> > 2. Set the lock in the dedicated mgmt struct.
> > 3. Reinit the completion.
> > 4. Enqueue the packet.
> > 5. Wait the packet to be received.
> > 6. Use the data set by the tagger to complete the mdio operation.
> >
> > If the completion timeouts or the ack value is not true, the legacy
> > mdio way is used.
> >
> > It has to be considered that in the initial setup mdio is still used and
> > mdio is still used until DSA is ready to accept and tag packet.
> >
> > tag_proto_connect() is used to fill the required handler for the tagger
> > to correctly parse and elaborate the special Ethernet mdio packet.
> >
> > Locking is added to qca8k_master_change() to make sure no mgmt Ethernet
> > are in progress.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > drivers/net/dsa/qca8k.c | 206 ++++++++++++++++++++++++++++++++++++++++
> > drivers/net/dsa/qca8k.h | 13 +++
> > 2 files changed, 219 insertions(+)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 4bc5064414b5..35711d010eb4 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -20,6 +20,7 @@
> > #include <linux/phylink.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/etherdevice.h>
> > +#include <linux/dsa/tag_qca.h>
> >
> > #include "qca8k.h"
> >
> > @@ -170,6 +171,174 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> > return regmap_update_bits(priv->regmap, reg, mask, write_val);
> > }
> >
> > +static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> > +{
> > + struct qca8k_mgmt_hdr_data *mgmt_hdr_data;
> > + struct qca8k_priv *priv = ds->priv;
> > + struct mgmt_ethhdr *mgmt_ethhdr;
> > + u8 len, cmd;
> > +
> > + mgmt_ethhdr = (struct mgmt_ethhdr *)skb_mac_header(skb);
> > + mgmt_hdr_data = &priv->mgmt_hdr_data;
> > +
> > + cmd = FIELD_GET(QCA_HDR_MGMT_CMD, mgmt_ethhdr->command);
> > + len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
> > +
> > + /* Make sure the seq match the requested packet */
> > + if (mgmt_ethhdr->seq == mgmt_hdr_data->seq)
> > + mgmt_hdr_data->ack = true;
> > +
> > + if (cmd == MDIO_READ) {
> > + mgmt_hdr_data->data[0] = mgmt_ethhdr->mdio_data;
> > +
> > + /* Get the rest of the 12 byte of data */
> > + if (len > QCA_HDR_MGMT_DATA1_LEN)
> > + memcpy(mgmt_hdr_data->data + 1, skb->data,
> > + QCA_HDR_MGMT_DATA2_LEN);
> > + }
> > +
> > + complete(&mgmt_hdr_data->rw_done);
> > +}
> > +
> > +static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> > + int seq_num, int priority)
> > +{
> > + struct mgmt_ethhdr *mgmt_ethhdr;
> > + struct sk_buff *skb;
> > + u16 hdr;
> > +
> > + skb = dev_alloc_skb(QCA_HDR_MGMT_PKG_LEN);
> > + if (!skb)
> > + return NULL;
> > +
> > + skb_reset_mac_header(skb);
> > + skb_set_network_header(skb, skb->len);
> > +
> > + mgmt_ethhdr = skb_push(skb, QCA_HDR_MGMT_HEADER_LEN + QCA_HDR_LEN);
> > +
> > + hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
> > + hdr |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
> > + hdr |= QCA_HDR_XMIT_FROM_CPU;
> > + hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
> > + hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
> > +
> > + mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
> > +
> > + mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
> > + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, 4);
> > + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
> > + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
> > + QCA_HDR_MGMT_CHECK_CODE_VAL);
> > +
> > + if (cmd == MDIO_WRITE)
> > + mgmt_ethhdr->mdio_data = *val;
> > +
> > + mgmt_ethhdr->hdr = htons(hdr);
> > +
> > + skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
> > +
> > + return skb;
> > +}
> > +
> > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > +{
> > + struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> > + struct sk_buff *skb;
> > + bool ack;
> > + int ret;
> > +
> > + skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + mutex_lock(&mgmt_hdr_data->mutex);
> > +
> > + /* Recheck mgmt_master under lock to make sure it's operational */
> > + if (!priv->mgmt_master)
>
> mutex_unlock and kfree_skb
>
> Also, why "recheck under lock"? Why not check just under lock?
>
Tell me if the logic is wrong.
We use the mgmt_master (outside lock) to understand if the eth mgmt is
available. Then to make sure it's actually usable when the operation is
actually done (and to prevent any panic if the master is dropped or for
whatever reason mgmt_master is not available anymore) we do the check
another time under lock.
It's really just to save extra lock when mgmt_master is not available.
The check under lock is to handle case when the mgmt_master is removed
while a mgmt eth is pending (corner case but still worth checking).
If you have suggestions on how to handle this corner case without
introducing an extra lock in the read/write function, I would really
appreaciate it.
Now that I think about it, considering eth mgmt will be the main way and
mdio as a fallback... wonder if the extra lock is acceptable anyway.
In the near future ipq40xx will use qca8k, but will have his own regmap
functions so we they won't be affected by these extra locking.
Don't know what is worst. Extra locking when mgmt_master is not
avaialable or double check. (I assume for a cleaner code the extra lock
is preferred)
> > + return -EINVAL;
> > +
> > + skb->dev = (struct net_device *)priv->mgmt_master;
> > +
> > + reinit_completion(&mgmt_hdr_data->rw_done);
> > + mgmt_hdr_data->seq = 200;
> > + mgmt_hdr_data->ack = false;
> > +
> > + dev_queue_xmit(skb);
> > +
> > + ret = wait_for_completion_timeout(&mgmt_hdr_data->rw_done,
> > + msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> > +
> > + *val = mgmt_hdr_data->data[0];
> > + ack = mgmt_hdr_data->ack;
> > +
> > + mutex_unlock(&mgmt_hdr_data->mutex);
> > +
> > + if (ret <= 0)
> > + return -ETIMEDOUT;
> > +
> > + if (!ack)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> > +{
> > + struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> > + struct sk_buff *skb;
> > + bool ack;
> > + int ret;
> > +
> > + skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + mutex_lock(&mgmt_hdr_data->mutex);
> > +
> > + /* Recheck mgmt_master under lock to make sure it's operational */
> > + if (!priv->mgmt_master)
>
> mutex_unlock and kfree_skb
>
> > + return -EINVAL;
> > +
> > + skb->dev = (struct net_device *)priv->mgmt_master;
> > +
> > + reinit_completion(&mgmt_hdr_data->rw_done);
> > + mgmt_hdr_data->ack = false;
> > + mgmt_hdr_data->seq = 200;
> > +
> > + dev_queue_xmit(skb);
> > +
> > + ret = wait_for_completion_timeout(&mgmt_hdr_data->rw_done,
> > + msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> > +
> > + ack = mgmt_hdr_data->ack;
> > +
> > + mutex_unlock(&mgmt_hdr_data->mutex);
> > +
> > + if (ret <= 0)
> > + return -ETIMEDOUT;
> > +
> > + if (!ack)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> > +{
> > + u32 val = 0;
> > + int ret;
> > +
> > + ret = qca8k_read_eth(priv, reg, &val);
> > + if (ret)
> > + return ret;
> > +
> > + val &= ~mask;
> > + val |= write_val;
> > +
> > + return qca8k_write_eth(priv, reg, val);
> > +}
> > +
> > static int
> > qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > {
> > @@ -178,6 +347,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > u16 r1, r2, page;
> > int ret;
> >
> > + if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
>
> What happens if you remove this priv->mgmt_master check from outside the
> lock, and reorder the skb allocation with the priv->mgmt_master check?
>
> > + return 0;
> > +
> > qca8k_split_addr(reg, &r1, &r2, &page);
> >
> > mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -201,6 +373,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
> > u16 r1, r2, page;
> > int ret;
> >
> > + if (priv->mgmt_master && !qca8k_write_eth(priv, reg, val))
> > + return 0;
> > +
> > qca8k_split_addr(reg, &r1, &r2, &page);
> >
> > mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -225,6 +400,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
> > u32 val;
> > int ret;
> >
> > + if (priv->mgmt_master &&
> > + !qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
> > + return 0;
> > +
> > qca8k_split_addr(reg, &r1, &r2, &page);
> >
> > mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -2394,10 +2573,33 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> > if (dp->index != 0)
> > return;
> >
> > + mutex_lock(&priv->mgmt_hdr_data.mutex);
> > +
> > if (operational)
> > priv->mgmt_master = master;
> > else
> > priv->mgmt_master = NULL;
> > +
> > + mutex_unlock(&priv->mgmt_hdr_data.mutex);
> > +}
> > +
> > +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> > + enum dsa_tag_protocol proto)
> > +{
> > + struct qca_tagger_data *tagger_data;
> > +
> > + switch (proto) {
> > + case DSA_TAG_PROTO_QCA:
> > + tagger_data = ds->tagger_data;
> > +
> > + tagger_data->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> > +
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > }
> >
> > static const struct dsa_switch_ops qca8k_switch_ops = {
> > @@ -2436,6 +2638,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> > .port_lag_join = qca8k_port_lag_join,
> > .port_lag_leave = qca8k_port_lag_leave,
> > .master_state_change = qca8k_master_change,
> > + .connect_tag_protocol = qca8k_connect_tag_protocol,
> > };
> >
> > static int qca8k_read_switch_id(struct qca8k_priv *priv)
> > @@ -2515,6 +2718,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> > if (!priv->ds)
> > return -ENOMEM;
> >
> > + mutex_init(&priv->mgmt_hdr_data.mutex);
> > + init_completion(&priv->mgmt_hdr_data.rw_done);
> > +
> > priv->ds->dev = &mdiodev->dev;
> > priv->ds->num_ports = QCA8K_NUM_PORTS;
> > priv->ds->priv = priv;
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index 9437369c60ca..a358a67044d3 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -11,6 +11,10 @@
> > #include <linux/delay.h>
> > #include <linux/regmap.h>
> > #include <linux/gpio.h>
> > +#include <linux/dsa/tag_qca.h>
> > +
> > +#define QCA8K_ETHERNET_MDIO_PRIORITY 7
> > +#define QCA8K_ETHERNET_TIMEOUT 100
> >
> > #define QCA8K_NUM_PORTS 7
> > #define QCA8K_NUM_CPU_PORTS 2
> > @@ -328,6 +332,14 @@ enum {
> > QCA8K_CPU_PORT6,
> > };
> >
> > +struct qca8k_mgmt_hdr_data {
> > + struct completion rw_done;
> > + struct mutex mutex; /* Enforce one mdio read/write at time */
> > + bool ack;
> > + u32 seq;
> > + u32 data[4];
> > +};
> > +
> > struct qca8k_ports_config {
> > bool sgmii_rx_clk_falling_edge;
> > bool sgmii_tx_clk_falling_edge;
> > @@ -354,6 +366,7 @@ struct qca8k_priv {
> > struct gpio_desc *reset_gpio;
> > unsigned int port_mtu[QCA8K_NUM_PORTS];
> > const struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
> > + struct qca8k_mgmt_hdr_data mgmt_hdr_data;
> > };
> >
> > struct qca8k_mib_desc {
> > --
> > 2.33.1
> >
>
--
Ansuel
On Mon, Jan 24, 2022 at 05:48:32PM +0100, Ansuel Smith wrote:
> > > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > > +{
> > > + struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> > > + struct sk_buff *skb;
> > > + bool ack;
> > > + int ret;
> > > +
> > > + skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > > + if (!skb)
> > > + return -ENOMEM;
> > > +
> > > + mutex_lock(&mgmt_hdr_data->mutex);
> > > +
> > > + /* Recheck mgmt_master under lock to make sure it's operational */
> > > + if (!priv->mgmt_master)
> >
> > mutex_unlock and kfree_skb
> >
> > Also, why "recheck under lock"? Why not check just under lock?
> >
>
> Tell me if the logic is wrong.
> We use the mgmt_master (outside lock) to understand if the eth mgmt is
> available. Then to make sure it's actually usable when the operation is
> actually done (and to prevent any panic if the master is dropped or for
> whatever reason mgmt_master is not available anymore) we do the check
> another time under lock.
>
> It's really just to save extra lock when mgmt_master is not available.
> The check under lock is to handle case when the mgmt_master is removed
> while a mgmt eth is pending (corner case but still worth checking).
>
> If you have suggestions on how to handle this corner case without
> introducing an extra lock in the read/write function, I would really
> appreaciate it.
> Now that I think about it, considering eth mgmt will be the main way and
> mdio as a fallback... wonder if the extra lock is acceptable anyway.
> In the near future ipq40xx will use qca8k, but will have his own regmap
> functions so we they won't be affected by these extra locking.
>
> Don't know what is worst. Extra locking when mgmt_master is not
> avaialable or double check. (I assume for a cleaner code the extra lock
> is preferred)
I don't think there's a hidden bug in the code (other than the one I
mentioned, which is that you don't unlock or free resources at all on
the error path, which is quite severe), but also, I don't know if this
is such a performance-sensitive operation to justify a gratuitous
"optimization".
As you mention, if the Ethernet management will be the main I/O access
method, then the common case will take a single lock. You aren't really
saving much.
On Sun, Jan 23, 2022 at 02:33:33AM +0100, Ansuel Smith wrote:
> Use mgmt Ethernet also for phy read/write if availabale. Use a different
> seq number to make sure we receive the correct packet.
At some point, you'll have to do something about those sequence numbers.
Hardcoding 200 and 400 isn't going to get you very far, it's prone to
errors. How about dealing with it now? If treating them as actual
sequence numbers isn't useful because you can't have multiple packets in
flight due to reordering concerns, at least create a macro for each
sequence number used by the driver for packet identification.
> On any error, we fallback to the legacy mdio read/write.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/net/dsa/qca8k.c | 191 ++++++++++++++++++++++++++++++++++++++++
> drivers/net/dsa/qca8k.h | 1 +
> 2 files changed, 192 insertions(+)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index f51a6d8993ff..e7bc0770bae9 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -848,6 +848,166 @@ qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
> regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
> }
>
> +static int
> +qca8k_phy_eth_busy_wait(struct qca8k_mgmt_hdr_data *phy_hdr_data,
> + struct sk_buff *read_skb, u32 *val)
> +{
> + struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
> + bool ack;
> + int ret;
> +
> + reinit_completion(&phy_hdr_data->rw_done);
> + phy_hdr_data->seq = 400;
> + phy_hdr_data->ack = false;
> +
> + dev_queue_xmit(skb);
> +
> + ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> + QCA8K_ETHERNET_TIMEOUT);
> +
> + ack = phy_hdr_data->ack;
> +
> + if (ret <= 0)
> + return -ETIMEDOUT;
> +
> + if (!ack)
> + return -EINVAL;
> +
> + *val = phy_hdr_data->data[0];
> +
> + return 0;
> +}
> +
> +static int
> +qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
> + int regnum, u16 data)
> +{
> + struct sk_buff *write_skb, *clear_skb, *read_skb;
> + struct qca8k_mgmt_hdr_data *phy_hdr_data;
> + const struct net_device *mgmt_master;
> + u32 write_val, clear_val = 0, val;
> + int seq_num = 400;
> + int ret, ret1;
> + bool ack;
> +
> + if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
> + return -EINVAL;
> +
> + phy_hdr_data = &priv->mgmt_hdr_data;
> +
> + write_val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
> + QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
> + QCA8K_MDIO_MASTER_REG_ADDR(regnum);
> +
> + if (read) {
> + write_val |= QCA8K_MDIO_MASTER_READ;
> + } else {
> + write_val |= QCA8K_MDIO_MASTER_WRITE;
> + write_val |= QCA8K_MDIO_MASTER_DATA(data);
> + }
> +
> + /* Prealloc all the needed skb before the lock */
> + write_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
> + &write_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> + if (!write_skb)
> + return -ENOMEM;
> +
> + clear_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
> + &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> + if (!write_skb)
Clean up the resources!!! (not just here but everywhere)
> + return -ENOMEM;
> +
> + read_skb = qca8k_alloc_mdio_header(MDIO_READ, QCA8K_MDIO_MASTER_CTRL,
> + &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> + if (!write_skb)
> + return -ENOMEM;
> +
> + /* Actually start the request:
> + * 1. Send mdio master packet
> + * 2. Busy Wait for mdio master command
> + * 3. Get the data if we are reading
> + * 4. Reset the mdio master (even with error)
> + */
> + mutex_lock(&phy_hdr_data->mutex);
Shouldn't qca8k_master_change() also take phy_hdr_data->mutex?
> +
> + /* Recheck mgmt_master under lock to make sure it's operational */
> + mgmt_master = priv->mgmt_master;
> + if (!mgmt_master)
> + return -EINVAL;
> +
> + read_skb->dev = (struct net_device *)mgmt_master;
> + clear_skb->dev = (struct net_device *)mgmt_master;
> + write_skb->dev = (struct net_device *)mgmt_master;
If you need the master to be a non-const pointer, just make the DSA
method give you a non-const pointer.
> +
> + reinit_completion(&phy_hdr_data->rw_done);
> + phy_hdr_data->ack = false;
> + phy_hdr_data->seq = seq_num;
> +
> + dev_queue_xmit(write_skb);
> +
> + ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> + QCA8K_ETHERNET_TIMEOUT);
> +
> + ack = phy_hdr_data->ack;
> +
> + if (ret <= 0) {
> + ret = -ETIMEDOUT;
> + goto exit;
> + }
> +
> + if (!ack) {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
> + !(val & QCA8K_MDIO_MASTER_BUSY), 0,
> + QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
> + phy_hdr_data, read_skb, &val);
> +
> + if (ret < 0 && ret1 < 0) {
> + ret = ret1;
> + goto exit;
> + }
> +
> + if (read) {
> + reinit_completion(&phy_hdr_data->rw_done);
> + phy_hdr_data->ack = false;
> +
> + dev_queue_xmit(read_skb);
> +
> + ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> + QCA8K_ETHERNET_TIMEOUT);
> +
> + ack = phy_hdr_data->ack;
> +
> + if (ret <= 0) {
> + ret = -ETIMEDOUT;
> + goto exit;
> + }
> +
> + if (!ack) {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + ret = phy_hdr_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
> + }
> +
> +exit:
> + reinit_completion(&phy_hdr_data->rw_done);
> + phy_hdr_data->ack = false;
> +
> + dev_queue_xmit(clear_skb);
> +
> + wait_for_completion_timeout(&phy_hdr_data->rw_done,
> + QCA8K_ETHERNET_TIMEOUT);
> +
> + mutex_unlock(&phy_hdr_data->mutex);
> +
> + return ret;
> +}
> +
> static u32
> qca8k_port_to_phy(int port)
> {
> @@ -970,6 +1130,14 @@ qca8k_internal_mdio_write(struct mii_bus *slave_bus, int phy, int regnum, u16 da
> {
> struct qca8k_priv *priv = slave_bus->priv;
> struct mii_bus *bus = priv->bus;
> + int ret;
> +
> + /* Use mdio Ethernet when available, fallback to legacy one on error */
> + if (priv->mgmt_master) {
> + ret = qca8k_phy_eth_command(priv, false, phy, regnum, data);
> + if (!ret)
> + return 0;
> + }
>
> return qca8k_mdio_write(bus, phy, regnum, data);
> }
> @@ -979,6 +1147,14 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
> {
> struct qca8k_priv *priv = slave_bus->priv;
> struct mii_bus *bus = priv->bus;
> + int ret;
> +
> + /* Use mdio Ethernet when available, fallback to legacy one on error */
> + if (priv->mgmt_master) {
> + ret = qca8k_phy_eth_command(priv, true, phy, regnum, 0);
> + if (ret >= 0)
> + return ret;
> + }
>
> return qca8k_mdio_read(bus, phy, regnum);
> }
> @@ -987,6 +1163,7 @@ static int
> qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
> {
> struct qca8k_priv *priv = ds->priv;
> + int ret;
>
> /* Check if the legacy mapping should be used and the
> * port is not correctly mapped to the right PHY in the
> @@ -995,6 +1172,13 @@ qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
> if (priv->legacy_phy_port_mapping)
> port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
>
> + /* Use mdio Ethernet when available, fallback to legacy one on error */
> + if (priv->mgmt_master) {
> + ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
> + if (!ret)
> + return ret;
> + }
> +
> return qca8k_mdio_write(priv->bus, port, regnum, data);
> }
>
> @@ -1011,6 +1195,13 @@ qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
> if (priv->legacy_phy_port_mapping)
> port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
>
> + /* Use mdio Ethernet when available, fallback to legacy one on error */
> + if (priv->mgmt_master) {
> + ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
> + if (ret >= 0)
> + return ret;
> + }
> +
> ret = qca8k_mdio_read(priv->bus, port, regnum);
>
> if (ret < 0)
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index dc1365542aa3..952217db2047 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -14,6 +14,7 @@
> #include <linux/dsa/tag_qca.h>
>
> #define QCA8K_ETHERNET_MDIO_PRIORITY 7
> +#define QCA8K_ETHERNET_PHY_PRIORITY 6
> #define QCA8K_ETHERNET_TIMEOUT 100
>
> #define QCA8K_NUM_PORTS 7
> --
> 2.33.1
>
On Sun, Jan 23, 2022 at 02:33:32AM +0100, Ansuel Smith wrote:
> The switch can autocast MIB counter using Ethernet packet.
> Add support for this and provide a handler for the tagger.
> The switch will send packet with MIB counter for each port, the switch
> will use completion API to wait for the correct packet to be received
> and will complete the task only when each packet is received.
> Although the handler will drop all the other packet, we still have to
> consume each MIB packet to complete the request. This is done to prevent
> mixed data with concurrent ethtool request.
>
> connect_tag_protocol() is used to add the handler to the tag_qca tagger,
> master_state_change() use the MIB lock to make sure no MIB Ethernet is
> in progress.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/net/dsa/qca8k.c | 108 +++++++++++++++++++++++++++++++++++++++-
> drivers/net/dsa/qca8k.h | 17 ++++++-
> 2 files changed, 122 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 35711d010eb4..f51a6d8993ff 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -811,7 +811,10 @@ qca8k_mib_init(struct qca8k_priv *priv)
> int ret;
>
> mutex_lock(&priv->reg_mutex);
> - ret = regmap_set_bits(priv->regmap, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
> + ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
> + QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
> + FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_FLUSH) |
> + QCA8K_MIB_BUSY);
> if (ret)
> goto exit;
>
> @@ -1882,6 +1885,97 @@ qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data)
> ETH_GSTRING_LEN);
> }
>
> +static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *skb)
> +{
> + const struct qca8k_match_data *match_data;
> + struct qca8k_mib_hdr_data *mib_hdr_data;
> + struct qca8k_priv *priv = ds->priv;
> + const struct qca8k_mib_desc *mib;
> + struct mib_ethhdr *mib_ethhdr;
> + int i, mib_len, offset = 0;
> + u64 *data;
> + u8 port;
> +
> + mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
> + mib_hdr_data = &priv->mib_hdr_data;
> +
> + /* The switch autocast every port. Ignore other packet and
> + * parse only the requested one.
> + */
> + port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
> + if (port != mib_hdr_data->req_port)
> + goto exit;
> +
> + match_data = device_get_match_data(priv->dev);
> + data = mib_hdr_data->data;
> +
> + for (i = 0; i < match_data->mib_count; i++) {
> + mib = &ar8327_mib[i];
> +
> + /* First 3 mib are present in the skb head */
> + if (i < 3) {
> + data[i] = mib_ethhdr->data[i];
> + continue;
> + }
> +
> + mib_len = sizeof(uint32_t);
> +
> + /* Some mib are 64 bit wide */
> + if (mib->size == 2)
> + mib_len = sizeof(uint64_t);
> +
> + /* Copy the mib value from packet to the */
> + memcpy(data + i, skb->data + offset, mib_len);
> +
> + /* Set the offset for the next mib */
> + offset += mib_len;
> + }
> +
> +exit:
> + /* Complete on receiving all the mib packet */
> + if (refcount_dec_and_test(&mib_hdr_data->port_parsed))
> + complete(&mib_hdr_data->rw_done);
> +}
> +
> +static int
> +qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
> +{
> + struct dsa_port *dp = dsa_to_port(ds, port);
> + struct qca8k_mib_hdr_data *mib_hdr_data;
> + struct qca8k_priv *priv = ds->priv;
> + int ret;
> +
> + mib_hdr_data = &priv->mib_hdr_data;
> +
> + mutex_lock(&mib_hdr_data->mutex);
> +
> + reinit_completion(&mib_hdr_data->rw_done);
> +
> + mib_hdr_data->req_port = dp->index;
> + mib_hdr_data->data = data;
> + refcount_set(&mib_hdr_data->port_parsed, QCA8K_NUM_PORTS);
> +
> + mutex_lock(&priv->reg_mutex);
> +
> + /* Send mib autocast request */
> + ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
> + QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
> + FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_CAST) |
> + QCA8K_MIB_BUSY);
> +
> + mutex_unlock(&priv->reg_mutex);
> +
> + if (ret)
> + goto exit;
> +
> + ret = wait_for_completion_timeout(&mib_hdr_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
> +
> +exit:
> + mutex_unlock(&mib_hdr_data->mutex);
> +
> + return ret;
> +}
> +
> static void
> qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
> uint64_t *data)
> @@ -1893,6 +1987,10 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
> u32 hi = 0;
> int ret;
>
> + if (priv->mgmt_master &&
> + qca8k_get_ethtool_stats_eth(ds, port, data) > 0)
> + return;
> +
> match_data = of_device_get_match_data(priv->dev);
>
> for (i = 0; i < match_data->mib_count; i++) {
> @@ -2573,7 +2671,8 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> if (dp->index != 0)
> return;
>
> - mutex_lock(&priv->mgmt_hdr_data.mutex);
> + mutex_unlock(&priv->mgmt_hdr_data.mutex);
Why do you unlock mgmt_hdr_data here?
> + mutex_lock(&priv->mib_hdr_data.mutex);
>
> if (operational)
> priv->mgmt_master = master;
> @@ -2581,6 +2680,7 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> priv->mgmt_master = NULL;
>
> mutex_unlock(&priv->mgmt_hdr_data.mutex);
> + mutex_unlock(&priv->mib_hdr_data.mutex);
Please unlock in the reverse order of locking.
> }
>
> static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> @@ -2593,6 +2693,7 @@ static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> tagger_data = ds->tagger_data;
>
> tagger_data->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> + tagger_data->mib_autocast_handler = qca8k_mib_autocast_handler;
>
> break;
> default:
> @@ -2721,6 +2822,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> mutex_init(&priv->mgmt_hdr_data.mutex);
> init_completion(&priv->mgmt_hdr_data.rw_done);
>
> + mutex_init(&priv->mib_hdr_data.mutex);
> + init_completion(&priv->mib_hdr_data.rw_done);
> +
> priv->ds->dev = &mdiodev->dev;
> priv->ds->num_ports = QCA8K_NUM_PORTS;
> priv->ds->priv = priv;
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index a358a67044d3..dc1365542aa3 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -67,7 +67,7 @@
> #define QCA8K_REG_MODULE_EN 0x030
> #define QCA8K_MODULE_EN_MIB BIT(0)
> #define QCA8K_REG_MIB 0x034
> -#define QCA8K_MIB_FLUSH BIT(24)
> +#define QCA8K_MIB_FUNC GENMASK(26, 24)
> #define QCA8K_MIB_CPU_KEEP BIT(20)
> #define QCA8K_MIB_BUSY BIT(17)
> #define QCA8K_MDIO_MASTER_CTRL 0x3c
> @@ -317,6 +317,12 @@ enum qca8k_vlan_cmd {
> QCA8K_VLAN_READ = 6,
> };
>
> +enum qca8k_mid_cmd {
> + QCA8K_MIB_FLUSH = 1,
> + QCA8K_MIB_FLUSH_PORT = 2,
> + QCA8K_MIB_CAST = 3,
> +};
> +
> struct ar8xxx_port_status {
> int enabled;
> };
> @@ -340,6 +346,14 @@ struct qca8k_mgmt_hdr_data {
> u32 data[4];
> };
>
> +struct qca8k_mib_hdr_data {
> + struct completion rw_done;
> + struct mutex mutex; /* Process one command at time */
> + refcount_t port_parsed; /* Counter to track parsed port */
> + u8 req_port;
> + u64 *data; /* pointer to ethtool data */
> +};
> +
> struct qca8k_ports_config {
> bool sgmii_rx_clk_falling_edge;
> bool sgmii_tx_clk_falling_edge;
> @@ -367,6 +381,7 @@ struct qca8k_priv {
> unsigned int port_mtu[QCA8K_NUM_PORTS];
> const struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */
> struct qca8k_mgmt_hdr_data mgmt_hdr_data;
> + struct qca8k_mib_hdr_data mib_hdr_data;
> };
>
> struct qca8k_mib_desc {
> --
> 2.33.1
>
On Tue, Jan 25, 2022 at 05:03:55PM +0200, Vladimir Oltean wrote:
> On Sun, Jan 23, 2022 at 02:33:33AM +0100, Ansuel Smith wrote:
> > Use mgmt Ethernet also for phy read/write if availabale. Use a different
> > seq number to make sure we receive the correct packet.
>
> At some point, you'll have to do something about those sequence numbers.
> Hardcoding 200 and 400 isn't going to get you very far, it's prone to
> errors. How about dealing with it now? If treating them as actual
> sequence numbers isn't useful because you can't have multiple packets in
> flight due to reordering concerns, at least create a macro for each
> sequence number used by the driver for packet identification.
>
Is documenting define and adding some inline function acceptable? That
should make the separation more clear and also prepare for a future
implementation. The way I see an use for the seq number is something
like a global workqueue that would handle all this stuff and be the one
that handle the seq number.
I mean another way would be just use a counter that will overflow and
remove all this garbage with hardcoded seq number.
(think will follow this path and just implement a correct seq number...)
> > On any error, we fallback to the legacy mdio read/write.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > drivers/net/dsa/qca8k.c | 191 ++++++++++++++++++++++++++++++++++++++++
> > drivers/net/dsa/qca8k.h | 1 +
> > 2 files changed, 192 insertions(+)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index f51a6d8993ff..e7bc0770bae9 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -848,6 +848,166 @@ qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
> > regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
> > }
> >
> > +static int
> > +qca8k_phy_eth_busy_wait(struct qca8k_mgmt_hdr_data *phy_hdr_data,
> > + struct sk_buff *read_skb, u32 *val)
> > +{
> > + struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
> > + bool ack;
> > + int ret;
> > +
> > + reinit_completion(&phy_hdr_data->rw_done);
> > + phy_hdr_data->seq = 400;
> > + phy_hdr_data->ack = false;
> > +
> > + dev_queue_xmit(skb);
> > +
> > + ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> > + QCA8K_ETHERNET_TIMEOUT);
> > +
> > + ack = phy_hdr_data->ack;
> > +
> > + if (ret <= 0)
> > + return -ETIMEDOUT;
> > +
> > + if (!ack)
> > + return -EINVAL;
> > +
> > + *val = phy_hdr_data->data[0];
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
> > + int regnum, u16 data)
> > +{
> > + struct sk_buff *write_skb, *clear_skb, *read_skb;
> > + struct qca8k_mgmt_hdr_data *phy_hdr_data;
> > + const struct net_device *mgmt_master;
> > + u32 write_val, clear_val = 0, val;
> > + int seq_num = 400;
> > + int ret, ret1;
> > + bool ack;
> > +
> > + if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
> > + return -EINVAL;
> > +
> > + phy_hdr_data = &priv->mgmt_hdr_data;
> > +
> > + write_val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
> > + QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
> > + QCA8K_MDIO_MASTER_REG_ADDR(regnum);
> > +
> > + if (read) {
> > + write_val |= QCA8K_MDIO_MASTER_READ;
> > + } else {
> > + write_val |= QCA8K_MDIO_MASTER_WRITE;
> > + write_val |= QCA8K_MDIO_MASTER_DATA(data);
> > + }
> > +
> > + /* Prealloc all the needed skb before the lock */
> > + write_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
> > + &write_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> > + if (!write_skb)
> > + return -ENOMEM;
> > +
> > + clear_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
> > + &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> > + if (!write_skb)
>
> Clean up the resources!!! (not just here but everywhere)
>
Sorry!!! I'm checking all the skb to free and wow this is a mess...
Hope when I will send v8 you won't have a stoke checking this mess ahahahah
> > + return -ENOMEM;
> > +
> > + read_skb = qca8k_alloc_mdio_header(MDIO_READ, QCA8K_MDIO_MASTER_CTRL,
> > + &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> > + if (!write_skb)
> > + return -ENOMEM;
> > +
> > + /* Actually start the request:
> > + * 1. Send mdio master packet
> > + * 2. Busy Wait for mdio master command
> > + * 3. Get the data if we are reading
> > + * 4. Reset the mdio master (even with error)
> > + */
> > + mutex_lock(&phy_hdr_data->mutex);
>
> Shouldn't qca8k_master_change() also take phy_hdr_data->mutex?
>
Is actually the normal mgmg_hdr_data.
phy_hdr_data = &priv->mgmt_hdr_data;
Should I remove this and use mgmt_hdr_data directly to remove any
confusion?
> > +
> > + /* Recheck mgmt_master under lock to make sure it's operational */
> > + mgmt_master = priv->mgmt_master;
> > + if (!mgmt_master)
> > + return -EINVAL;
> > +
> > + read_skb->dev = (struct net_device *)mgmt_master;
> > + clear_skb->dev = (struct net_device *)mgmt_master;
> > + write_skb->dev = (struct net_device *)mgmt_master;
>
> If you need the master to be a non-const pointer, just make the DSA
> method give you a non-const pointer.
>
> > +
> > + reinit_completion(&phy_hdr_data->rw_done);
> > + phy_hdr_data->ack = false;
> > + phy_hdr_data->seq = seq_num;
> > +
> > + dev_queue_xmit(write_skb);
> > +
> > + ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> > + QCA8K_ETHERNET_TIMEOUT);
> > +
> > + ack = phy_hdr_data->ack;
> > +
> > + if (ret <= 0) {
> > + ret = -ETIMEDOUT;
> > + goto exit;
> > + }
> > +
> > + if (!ack) {
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
> > + !(val & QCA8K_MDIO_MASTER_BUSY), 0,
> > + QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
> > + phy_hdr_data, read_skb, &val);
> > +
> > + if (ret < 0 && ret1 < 0) {
> > + ret = ret1;
> > + goto exit;
> > + }
> > +
> > + if (read) {
> > + reinit_completion(&phy_hdr_data->rw_done);
> > + phy_hdr_data->ack = false;
> > +
> > + dev_queue_xmit(read_skb);
> > +
> > + ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> > + QCA8K_ETHERNET_TIMEOUT);
> > +
> > + ack = phy_hdr_data->ack;
> > +
> > + if (ret <= 0) {
> > + ret = -ETIMEDOUT;
> > + goto exit;
> > + }
> > +
> > + if (!ack) {
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + ret = phy_hdr_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
> > + }
> > +
> > +exit:
> > + reinit_completion(&phy_hdr_data->rw_done);
> > + phy_hdr_data->ack = false;
> > +
> > + dev_queue_xmit(clear_skb);
> > +
> > + wait_for_completion_timeout(&phy_hdr_data->rw_done,
> > + QCA8K_ETHERNET_TIMEOUT);
> > +
> > + mutex_unlock(&phy_hdr_data->mutex);
> > +
> > + return ret;
> > +}
> > +
> > static u32
> > qca8k_port_to_phy(int port)
> > {
> > @@ -970,6 +1130,14 @@ qca8k_internal_mdio_write(struct mii_bus *slave_bus, int phy, int regnum, u16 da
> > {
> > struct qca8k_priv *priv = slave_bus->priv;
> > struct mii_bus *bus = priv->bus;
> > + int ret;
> > +
> > + /* Use mdio Ethernet when available, fallback to legacy one on error */
> > + if (priv->mgmt_master) {
> > + ret = qca8k_phy_eth_command(priv, false, phy, regnum, data);
> > + if (!ret)
> > + return 0;
> > + }
> >
> > return qca8k_mdio_write(bus, phy, regnum, data);
> > }
> > @@ -979,6 +1147,14 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
> > {
> > struct qca8k_priv *priv = slave_bus->priv;
> > struct mii_bus *bus = priv->bus;
> > + int ret;
> > +
> > + /* Use mdio Ethernet when available, fallback to legacy one on error */
> > + if (priv->mgmt_master) {
> > + ret = qca8k_phy_eth_command(priv, true, phy, regnum, 0);
> > + if (ret >= 0)
> > + return ret;
> > + }
> >
> > return qca8k_mdio_read(bus, phy, regnum);
> > }
> > @@ -987,6 +1163,7 @@ static int
> > qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
> > {
> > struct qca8k_priv *priv = ds->priv;
> > + int ret;
> >
> > /* Check if the legacy mapping should be used and the
> > * port is not correctly mapped to the right PHY in the
> > @@ -995,6 +1172,13 @@ qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
> > if (priv->legacy_phy_port_mapping)
> > port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> >
> > + /* Use mdio Ethernet when available, fallback to legacy one on error */
> > + if (priv->mgmt_master) {
> > + ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
> > + if (!ret)
> > + return ret;
> > + }
> > +
> > return qca8k_mdio_write(priv->bus, port, regnum, data);
> > }
> >
> > @@ -1011,6 +1195,13 @@ qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
> > if (priv->legacy_phy_port_mapping)
> > port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> >
> > + /* Use mdio Ethernet when available, fallback to legacy one on error */
> > + if (priv->mgmt_master) {
> > + ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
> > + if (ret >= 0)
> > + return ret;
> > + }
> > +
> > ret = qca8k_mdio_read(priv->bus, port, regnum);
> >
> > if (ret < 0)
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index dc1365542aa3..952217db2047 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -14,6 +14,7 @@
> > #include <linux/dsa/tag_qca.h>
> >
> > #define QCA8K_ETHERNET_MDIO_PRIORITY 7
> > +#define QCA8K_ETHERNET_PHY_PRIORITY 6
> > #define QCA8K_ETHERNET_TIMEOUT 100
> >
> > #define QCA8K_NUM_PORTS 7
> > --
> > 2.33.1
> >
--
Ansuel
On Wed, Jan 26, 2022 at 12:14:55AM +0100, Ansuel Smith wrote:
> > At some point, you'll have to do something about those sequence numbers.
> > Hardcoding 200 and 400 isn't going to get you very far, it's prone to
> > errors. How about dealing with it now? If treating them as actual
> > sequence numbers isn't useful because you can't have multiple packets in
> > flight due to reordering concerns, at least create a macro for each
> > sequence number used by the driver for packet identification.
>
> Is documenting define and adding some inline function acceptable? That
> should make the separation more clear and also prepare for a future
> implementation. The way I see an use for the seq number is something
> like a global workqueue that would handle all this stuff and be the one
> that handle the seq number.
> I mean another way would be just use a counter that will overflow and
> remove all this garbage with hardcoded seq number.
> (think will follow this path and just implement a correct seq number...)
Cleanest would be, I think, to just treat the sequence number as a
rolling counter and use it to match the request to the response.
But I didn't object to your use of fixed numbers per packet type, just
to the lack of a #define behind those numbers.
> > > + mutex_lock(&phy_hdr_data->mutex);
> >
> > Shouldn't qca8k_master_change() also take phy_hdr_data->mutex?
> >
>
> Is actually the normal mgmg_hdr_data.
>
> phy_hdr_data = &priv->mgmt_hdr_data;
>
> Should I remove this and use mgmt_hdr_data directly to remove any
> confusion?
I am not thrilled by the naming of this data structure anyway
(why "hdr"?), but yes, I also got tricked by inconsistent naming.
Please choose a consistent name and stick with it.
On Wed, Jan 26, 2022 at 03:48:54AM +0200, Vladimir Oltean wrote:
> On Wed, Jan 26, 2022 at 12:14:55AM +0100, Ansuel Smith wrote:
> > > At some point, you'll have to do something about those sequence numbers.
> > > Hardcoding 200 and 400 isn't going to get you very far, it's prone to
> > > errors. How about dealing with it now? If treating them as actual
> > > sequence numbers isn't useful because you can't have multiple packets in
> > > flight due to reordering concerns, at least create a macro for each
> > > sequence number used by the driver for packet identification.
> >
> > Is documenting define and adding some inline function acceptable? That
> > should make the separation more clear and also prepare for a future
> > implementation. The way I see an use for the seq number is something
> > like a global workqueue that would handle all this stuff and be the one
> > that handle the seq number.
> > I mean another way would be just use a counter that will overflow and
> > remove all this garbage with hardcoded seq number.
> > (think will follow this path and just implement a correct seq number...)
>
> Cleanest would be, I think, to just treat the sequence number as a
> rolling counter and use it to match the request to the response.
> But I didn't object to your use of fixed numbers per packet type, just
> to the lack of a #define behind those numbers.
>
I'm just implementing this. (rolling counter)
> > > > + mutex_lock(&phy_hdr_data->mutex);
> > >
> > > Shouldn't qca8k_master_change() also take phy_hdr_data->mutex?
> > >
> >
> > Is actually the normal mgmg_hdr_data.
> >
> > phy_hdr_data = &priv->mgmt_hdr_data;
> >
> > Should I remove this and use mgmt_hdr_data directly to remove any
> > confusion?
>
> I am not thrilled by the naming of this data structure anyway
> (why "hdr"?), but yes, I also got tricked by inconsistent naming.
> Please choose a consistent name and stick with it.
Hdr as header stuff since all this stuff is put in the hdr. Should I
just drop hdr and use mgmt_data directly? Or mgmt_eth?
--
Ansuel
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> From: Vladimir Oltean <[email protected]>
>
> Certain drivers may need to send management traffic to the switch for
> things like register access, FDB dump, etc, to accelerate what their
> slow bus (SPI, I2C, MDIO) can already do.
>
> Ethernet is faster (especially in bulk transactions) but is also more
> unreliable, since the user may decide to bring the DSA master down (or
> not bring it up), therefore severing the link between the host and the
> attached switch.
>
> Drivers needing Ethernet-based register access already should have
> fallback logic to the slow bus if the Ethernet method fails, but that
> fallback may be based on a timeout, and the I/O to the switch may slow
> down to a halt if the master is down, because every Ethernet packet will
> have to time out. The driver also doesn't have the option to turn off
> Ethernet-based I/O momentarily, because it wouldn't know when to turn it
> back on.
>
> Which is where this change comes in. By tracking NETDEV_CHANGE,
> NETDEV_UP and NETDEV_GOING_DOWN events on the DSA master, we should know
> the exact interval of time during which this interface is reliably
> available for traffic. Provide this information to switches so they can
> use it as they wish.
>
> An helper is added dsa_port_master_is_operational() to check if a master
> port is operational.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Ansuel Smith <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> From: Vladimir Oltean <[email protected]>
>
> In order for switch driver to be able to make simple and reliable use of
> the master tracking operations, they must also be notified of the
> initial state of the DSA master, not just of the changes. This is
> because they might enable certain features only during the time when
> they know that the DSA master is up and running.
>
> Therefore, this change explicitly checks the state of the DSA master
> under the same rtnl_mutex as we were holding during the
> dsa_master_setup() and dsa_master_teardown() call. The idea being that
> if the DSA master became operational in between the moment in which it
> became a DSA master (dsa_master_setup set dev->dsa_ptr) and the moment
> when we checked for the master being up, there is a chance that we
> would emit a ->master_state_change() call with no actual state change.
> We need to avoid that by serializing the concurrent netdevice event with
> us. If the netdevice event started before, we force it to finish before
> we begin, because we take rtnl_lock before making netdev_uses_dsa()
> return true. So we also handle that early event and do nothing on it.
> Similarly, if the dev_open() attempt is concurrent with us, it will
> attempt to take the rtnl_mutex, but we're holding it. We'll see that
> the master flag IFF_UP isn't set, then when we release the rtnl_mutex
> we'll process the NETDEV_UP notifier.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Ansuel Smith <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> Convert driver to FIELD macro to drop redundant define.
>
> Signed-off-by: Ansuel Smith <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> Move tag_qca define to include dir linux/dsa as the qca8k require access
> to the tagger define to support in-band mdio read/write using ethernet
> packet.
>
> Signed-off-by: Ansuel Smith <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> Ethernet MDIO packets are non-standard and DSA master expects the first
> 6 octets to be the MAC DA. To address these kind of packet, enable
> promisc_on_master flag for the tagger.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> Add connect/disconnect helper to assign private struct to the dsa switch.
> Add support for Ethernet mgm and MIB if the dsa driver provide an handler
> to correctly parse and elaborate the data.
s/mgm/mgmt/
s/dsa/DSA/
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> include/linux/dsa/tag_qca.h | 7 +++++++
> net/dsa/tag_qca.c | 39 ++++++++++++++++++++++++++++++++++---
> 2 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> index 87dd84e31304..de5a45f5b398 100644
> --- a/include/linux/dsa/tag_qca.h
> +++ b/include/linux/dsa/tag_qca.h
> @@ -72,4 +72,11 @@ struct mib_ethhdr {
> __be16 hdr; /* qca hdr */
> } __packed;
>
> +struct qca_tagger_data {
> + void (*rw_reg_ack_handler)(struct dsa_switch *ds,
> + struct sk_buff *skb);
> + void (*mib_autocast_handler)(struct dsa_switch *ds,
> + struct sk_buff *skb);
> +};
> +
> #endif /* __TAG_QCA_H */
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index fdaa1b322d25..dc81c72133eb 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -5,6 +5,7 @@
>
> #include <linux/etherdevice.h>
> #include <linux/bitfield.h>
> +#include <net/dsa.h>
> #include <linux/dsa/tag_qca.h>
>
> #include "dsa_priv.h"
> @@ -32,11 +33,16 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
>
> static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> {
> + struct qca_tagger_data *tagger_data;
> + struct dsa_port *dp = dev->dsa_ptr;
> + struct dsa_switch *ds = dp->ds;
> u16 hdr, pk_type;
> __be16 *phdr;
> int port;
> u8 ver;
>
> + tagger_data = ds->tagger_data;
> +
> if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
> return NULL;
>
> @@ -51,13 +57,19 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> /* Get pk type */
> pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
>
> - /* Ethernet MDIO read/write packet */
> - if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
> + /* Ethernet mgmt read/write packet */
> + if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
> + if (tagger_data->rw_reg_ack_handler)
if (likely()) in case that happens to make a difference?
> + tagger_data->rw_reg_ack_handler(ds, skb);
> return NULL;
> + }
>
> /* Ethernet MIB counter packet */
> - if (pk_type == QCA_HDR_RECV_TYPE_MIB)
> + if (pk_type == QCA_HDR_RECV_TYPE_MIB) {
> + if (tagger_data->mib_autocast_handler)
Likewise
In any case, this looks good to me:
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> From Documentation, we can cache lo and hi the same way we do with the
> page. This massively reduce the mdio write as 3/4 of the time as we only
> require to write the lo or hi part for a mdio write.
>
> Signed-off-by: Ansuel Smith <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> Introduce qca8k_bulk_read/write() function to use mgmt Ethernet way to
> read/write packet in bulk. Make use of this new function in the fdb
> function.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
[snip]
> static int
> qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> {
> @@ -535,17 +572,13 @@ qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
> static int
> qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
> {
> - u32 reg[4], val;
> - int i, ret;
> + u32 reg[4];
> + int ret;
>
> /* load the ARL table into an array */
> - for (i = 0; i < 4; i++) {
> - ret = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4), &val);
> - if (ret < 0)
> - return ret;
> -
> - reg[i] = val;
> - }
> + ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, 12);
sizeof(reg)? How did you come up with 12 if we were executing the loop 4
times before or were we reading too much?
> + if (ret)
> + return ret;
>
> /* vid - 83:72 */
> fdb->vid = FIELD_GET(QCA8K_ATU_VID_MASK, reg[2]);
> @@ -569,7 +602,6 @@ qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
> u8 aging)
> {
> u32 reg[3] = { 0 };
> - int i;
>
> /* vid - 83:72 */
> reg[2] = FIELD_PREP(QCA8K_ATU_VID_MASK, vid);
> @@ -586,8 +618,7 @@ qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
> reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR5_MASK, mac[5]);
>
> /* load the array into the ARL table */
> - for (i = 0; i < 3; i++)
> - qca8k_write(priv, QCA8K_REG_ATU_DATA0 + (i * 4), reg[i]);
> + qca8k_bulk_write(priv, QCA8K_REG_ATU_DATA0, reg, 12);
sizeof(reg) would be more adequate here.
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> MDIO/MIB Ethernet require the master port and the tagger availabale to
> correctly work. Use the new api master_state_change to track when master
> is operational or not and set a bool in qca8k_priv.
> We cache the first cached master available and we check if other cpu
> port are operational when the cached one goes down.
> This cached master will later be used by mdio read/write and mib request to
> correctly use the working function.
>
> qca8k implementation for MDIO/MIB Ethernet is bad. CPU port0 is the only
> one that answers with the ack packet or sends MIB Ethernet packets. For
> this reason the master_state_change ignore CPU port6 and checkes only
> CPU port0 if it's operational and enables this mode.
s/checkes only/only checks/
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/net/dsa/qca8k.c | 18 ++++++++++++++++++
> drivers/net/dsa/qca8k.h | 1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 039694518788..4bc5064414b5 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -2383,6 +2383,23 @@ qca8k_port_lag_leave(struct dsa_switch *ds, int port,
> return qca8k_lag_refresh_portmap(ds, port, lag, true);
> }
>
> +static void
> +qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> + bool operational)
> +{
> + struct dsa_port *dp = master->dsa_ptr;
> + struct qca8k_priv *priv = ds->priv;
> +
> + /* Ethernet MIB/MDIO is only supported for CPU port 0 */
> + if (dp->index != 0)
We sort of have a define for this: QCA8K_CPU_PORT0 even though that enum
definition might be more accidental than on purpose.
> + return;
> +
> + if (operational)
> + priv->mgmt_master = master;
> + else
> + priv->mgmt_master = NULL;
priv->mgmt_master = operational ? master : NULL;
but this is really because the bike shed is blue. So in any case:
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> mgmt Ethernet packet can read/write up to 16byte at times. The len reg
> is limited to 15 (0xf). The switch actually sends and accepts data in 4
> different steps of len values.
> Len steps:
> - 0: nothing
> - 1-4: first 4 byte
> - 5-6: first 12 byte
> - 7-15: all 16 byte
This is really odd, it almost felt like the length was a byte enable
bitmask, but it is not?
>
> In the allock skb function we check if the len is 16 and we fix it to a
> len of 15.
s/allock/alloc/
> It the read/write function interest to extract the real asked
> data. The tagger handler will always copy the fully 16byte with a READ
> command. This is useful for some big regs like the fdb reg that are
> more than 4byte of data. This permits to introduce a bulk function that
> will send and request the entire entry in one go.
> Write function is changed and it does now require to pass the pointer to
> val to also handle array val.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/net/dsa/qca8k.c | 56 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 2a43fb9aeef2..0183ce2d5b74 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -219,7 +219,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> if (cmd == MDIO_READ) {
> mgmt_hdr_data->data[0] = mgmt_ethhdr->mdio_data;
>
> - /* Get the rest of the 12 byte of data */
> + /* Get the rest of the 12 byte of data.
> + * The read/write function will extract the requested data.
> + */
> if (len > QCA_HDR_MGMT_DATA1_LEN)
> memcpy(mgmt_hdr_data->data + 1, skb->data,
> QCA_HDR_MGMT_DATA2_LEN);
> @@ -229,16 +231,30 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> }
>
> static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> - int seq_num, int priority)
> + int seq_num, int priority, int len)
unsigned int len
> {
> struct mgmt_ethhdr *mgmt_ethhdr;
> struct sk_buff *skb;
> + int real_len;
Likewise.
> + u32 *data2;
> u16 hdr;
>
> skb = dev_alloc_skb(QCA_HDR_MGMT_PKG_LEN);
> if (!skb)
> return NULL;
>
> + /* Max value for len reg is 15 (0xf) but the switch actually return 16 byte
> + * Actually for some reason the steps are:
> + * 0: nothing
> + * 1-4: first 4 byte
> + * 5-6: first 12 byte
> + * 7-15: all 16 byte
> + */
> + if (len == 16)
> + real_len = 15;
> + else
> + real_len = len;
> +
> skb_reset_mac_header(skb);
> skb_set_network_header(skb, skb->len);
>
> @@ -253,7 +269,7 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
> mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
>
> mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
> - mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, 4);
> + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, real_len);
> mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
> mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
> QCA_HDR_MGMT_CHECK_CODE_VAL);
> @@ -263,19 +279,22 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
>
> mgmt_ethhdr->hdr = htons(hdr);
>
> - skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
> + data2 = skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
> + if (cmd == MDIO_WRITE && len > QCA_HDR_MGMT_DATA1_LEN)
> + memcpy(data2, val + 1, len - QCA_HDR_MGMT_DATA1_LEN);
>
> return skb;
> }
>
> -static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> {
> struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> struct sk_buff *skb;
> bool ack;
> int ret;
>
> - skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> + skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200,
> + QCA8K_ETHERNET_MDIO_PRIORITY, len);
> if (!skb)
> return -ENOMEM;
>
> @@ -297,6 +316,9 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
>
> *val = mgmt_hdr_data->data[0];
> + if (len > QCA_HDR_MGMT_DATA1_LEN)
> + memcpy(val + 1, mgmt_hdr_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> +
> ack = mgmt_hdr_data->ack;
>
> mutex_unlock(&mgmt_hdr_data->mutex);
> @@ -310,14 +332,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> return 0;
> }
>
> -static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> {
> struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> struct sk_buff *skb;
> bool ack;
> int ret;
>
> - skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> + skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val, 200,
> + QCA8K_ETHERNET_MDIO_PRIORITY, len);
> if (!skb)
> return -ENOMEM;
>
> @@ -357,14 +380,14 @@ qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 wri
> u32 val = 0;
> int ret;
>
> - ret = qca8k_read_eth(priv, reg, &val);
> + ret = qca8k_read_eth(priv, reg, &val, 4);
sizeof(val) instead of 4.
> if (ret)
> return ret;
>
> val &= ~mask;
> val |= write_val;
>
> - return qca8k_write_eth(priv, reg, val);
> + return qca8k_write_eth(priv, reg, &val, 4);
Likewise
> }
>
> static int
> @@ -376,7 +399,7 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> u16 r1, r2, page;
> int ret;
>
> - if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
> + if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val, 4))
Likewise and everywhere below as well.
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> There can be multiple qca8k switch on the same system. Move the static
> qca8k_current_page to qca8k_priv and make it specific for each switch.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/net/dsa/qca8k.c | 47 +++++++++++++++++++++++------------------
> drivers/net/dsa/qca8k.h | 9 ++++++++
> 2 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index e7bc0770bae9..c2f5414033d8 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -75,12 +75,6 @@ static const struct qca8k_mib_desc ar8327_mib[] = {
> MIB_DESC(1, 0xac, "TXUnicast"),
> };
>
> -/* The 32bit switch registers are accessed indirectly. To achieve this we need
> - * to set the page of the register. Track the last page that was set to reduce
> - * mdio writes
> - */
> -static u16 qca8k_current_page = 0xffff;
> -
> static void
> qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
> {
> @@ -134,11 +128,11 @@ qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
> }
>
> static int
> -qca8k_set_page(struct mii_bus *bus, u16 page)
> +qca8k_set_page(struct mii_bus *bus, u16 page, u16 *cached_page)
> {
bus->priv is assigned a qca8k_priv pointer, so we can just de-reference
it here from bus->priv and avoid changing a whole bunch of function
signatures that are now getting both a qca8k_priv *and* a
qca8k_mdio_cache set of pointers when you can just use back pointers to
those.
--
Florian
On Tue, Jan 25, 2022 at 07:45:29PM -0800, Florian Fainelli wrote:
>
>
> On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> > Introduce qca8k_bulk_read/write() function to use mgmt Ethernet way to
> > read/write packet in bulk. Make use of this new function in the fdb
> > function.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
>
> [snip]
>
> > static int
> > qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > {
> > @@ -535,17 +572,13 @@ qca8k_busy_wait(struct qca8k_priv *priv, u32 reg, u32 mask)
> > static int
> > qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
> > {
> > - u32 reg[4], val;
> > - int i, ret;
> > + u32 reg[4];
> > + int ret;
> > /* load the ARL table into an array */
> > - for (i = 0; i < 4; i++) {
> > - ret = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4), &val);
> > - if (ret < 0)
> > - return ret;
> > -
> > - reg[i] = val;
> > - }
> > + ret = qca8k_bulk_read(priv, QCA8K_REG_ATU_DATA0, reg, 12);
>
> sizeof(reg)? How did you come up with 12 if we were executing the loop 4
> times before or were we reading too much?
>
Exactly that. The acl table is actualy of 83 bits. Currently we read 128
bits but we only handle 96 as we never read/write in reg[3].
> > + if (ret)
> > + return ret;
> > /* vid - 83:72 */
> > fdb->vid = FIELD_GET(QCA8K_ATU_VID_MASK, reg[2]);
> > @@ -569,7 +602,6 @@ qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
> > u8 aging)
> > {
> > u32 reg[3] = { 0 };
> > - int i;
> > /* vid - 83:72 */
> > reg[2] = FIELD_PREP(QCA8K_ATU_VID_MASK, vid);
> > @@ -586,8 +618,7 @@ qca8k_fdb_write(struct qca8k_priv *priv, u16 vid, u8 port_mask, const u8 *mac,
> > reg[0] |= FIELD_PREP(QCA8K_ATU_ADDR5_MASK, mac[5]);
> > /* load the array into the ARL table */
> > - for (i = 0; i < 3; i++)
> > - qca8k_write(priv, QCA8K_REG_ATU_DATA0 + (i * 4), reg[i]);
> > + qca8k_bulk_write(priv, QCA8K_REG_ATU_DATA0, reg, 12);
>
> sizeof(reg) would be more adequate here.
> --
> Florian
--
Ansuel
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> Add all the required define to prepare support for mgmt read/write in
> Ethernet packet. Any packet of this type has to be dropped as the only
> use of these special packet is receive ack for an mgmt write request or
> receive data for an mgmt read request.
> A struct is used that emulates the Ethernet header but is used for a
> different purpose.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
[snip]
> +/* Special struct emulating a Ethernet header */
> +struct mgmt_ethhdr {
> + u32 command; /* command bit 31:0 */
> + u32 seq; /* seq 63:32 */
> + u32 mdio_data; /* first 4byte mdio */
> + __be16 hdr; /* qca hdr */
> +} __packed;
Might be worth adding a BUILD_BUG_ON(sizeof(struct mgmt_ethhdr) !=
QCA_HDR_MGMT_PKG_LEN) when you start making use of that structure?
--
Florian
On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> Add all the required define to prepare support for mgmt read/write in
> Ethernet packet. Any packet of this type has to be dropped as the only
> use of these special packet is receive ack for an mgmt write request or
> receive data for an mgmt read request.
> A struct is used that emulates the Ethernet header but is used for a
> different purpose.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> include/linux/dsa/tag_qca.h | 44 +++++++++++++++++++++++++++++++++++++
> net/dsa/tag_qca.c | 13 ++++++++---
> 2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> index c02d2d39ff4a..1a02f695f3a3 100644
> --- a/include/linux/dsa/tag_qca.h
> +++ b/include/linux/dsa/tag_qca.h
> @@ -12,10 +12,54 @@
> #define QCA_HDR_RECV_FRAME_IS_TAGGED BIT(3)
> #define QCA_HDR_RECV_SOURCE_PORT GENMASK(2, 0)
>
> +/* Packet type for recv */
> +#define QCA_HDR_RECV_TYPE_NORMAL 0x0
> +#define QCA_HDR_RECV_TYPE_MIB 0x1
> +#define QCA_HDR_RECV_TYPE_RW_REG_ACK 0x2
> +
> #define QCA_HDR_XMIT_VERSION GENMASK(15, 14)
> #define QCA_HDR_XMIT_PRIORITY GENMASK(13, 11)
> #define QCA_HDR_XMIT_CONTROL GENMASK(10, 8)
> #define QCA_HDR_XMIT_FROM_CPU BIT(7)
> #define QCA_HDR_XMIT_DP_BIT GENMASK(6, 0)
>
> +/* Packet type for xmit */
> +#define QCA_HDR_XMIT_TYPE_NORMAL 0x0
> +#define QCA_HDR_XMIT_TYPE_RW_REG 0x1
> +
> +/* Check code for a valid mgmt packet. Switch will ignore the packet
> + * with this wrong.
> + */
> +#define QCA_HDR_MGMT_CHECK_CODE_VAL 0x5
> +
> +/* Specific define for in-band MDIO read/write with Ethernet packet */
> +#define QCA_HDR_MGMT_SEQ_LEN 4 /* 4 byte for the seq */
> +#define QCA_HDR_MGMT_COMMAND_LEN 4 /* 4 byte for the command */
> +#define QCA_HDR_MGMT_DATA1_LEN 4 /* First 4 byte for the mdio data */
> +#define QCA_HDR_MGMT_HEADER_LEN (QCA_HDR_MGMT_SEQ_LEN + \
> + QCA_HDR_MGMT_COMMAND_LEN + \
> + QCA_HDR_MGMT_DATA1_LEN)
> +
> +#define QCA_HDR_MGMT_DATA2_LEN 12 /* Other 12 byte for the mdio data */
> +#define QCA_HDR_MGMT_PADDING_LEN 34 /* Padding to reach the min Ethernet packet */
> +
> +#define QCA_HDR_MGMT_PKG_LEN (QCA_HDR_MGMT_HEADER_LEN + \
> + QCA_HDR_LEN + \
> + QCA_HDR_MGMT_DATA2_LEN + \
> + QCA_HDR_MGMT_PADDING_LEN)
> +
> +#define QCA_HDR_MGMT_SEQ_NUM GENMASK(31, 0) /* 63, 32 */
> +#define QCA_HDR_MGMT_CHECK_CODE GENMASK(31, 29) /* 31, 29 */
> +#define QCA_HDR_MGMT_CMD BIT(28) /* 28 */
> +#define QCA_HDR_MGMT_LENGTH GENMASK(23, 20) /* 23, 20 */
> +#define QCA_HDR_MGMT_ADDR GENMASK(18, 0) /* 18, 0 */
> +
> +/* Special struct emulating a Ethernet header */
> +struct mgmt_ethhdr {
> + u32 command; /* command bit 31:0 */
> + u32 seq; /* seq 63:32 */
> + u32 mdio_data; /* first 4byte mdio */
> + __be16 hdr; /* qca hdr */
> +} __packed;
> +
> #endif /* __TAG_QCA_H */
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index f8df49d5956f..c57d6e1a0c0c 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -32,10 +32,10 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
>
> static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
> {
> - u8 ver;
> - u16 hdr;
> - int port;
> + u16 hdr, pk_type;
Should pk_type be u8 since there are only 5 bits for QCA_HDR_RECV_TYPE?
With Vladimir's comments addressed:
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Tue, Jan 25, 2022 at 07:48:27PM -0800, Florian Fainelli wrote:
>
>
> On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> > mgmt Ethernet packet can read/write up to 16byte at times. The len reg
> > is limited to 15 (0xf). The switch actually sends and accepts data in 4
> > different steps of len values.
> > Len steps:
> > - 0: nothing
> > - 1-4: first 4 byte
> > - 5-6: first 12 byte
> > - 7-15: all 16 byte
>
> This is really odd, it almost felt like the length was a byte enable
> bitmask, but it is not?
>
To me it seems like they match the size to the 3 different operation
that is
4: normal mdio / reg access
12: acl table to directly write and read it
16: offload table that is 16 byte long to directl write and read.
With this in mind, it does make sense why it bheave like that.
> >
> > In the allock skb function we check if the len is 16 and we fix it to a
> > len of 15.
>
> s/allock/alloc/
>
> > It the read/write function interest to extract the real asked
> > data. The tagger handler will always copy the fully 16byte with a READ
> > command. This is useful for some big regs like the fdb reg that are
> > more than 4byte of data. This permits to introduce a bulk function that
> > will send and request the entire entry in one go.
> > Write function is changed and it does now require to pass the pointer to
> > val to also handle array val.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > drivers/net/dsa/qca8k.c | 56 ++++++++++++++++++++++++++++++-----------
> > 1 file changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 2a43fb9aeef2..0183ce2d5b74 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -219,7 +219,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> > if (cmd == MDIO_READ) {
> > mgmt_hdr_data->data[0] = mgmt_ethhdr->mdio_data;
> > - /* Get the rest of the 12 byte of data */
> > + /* Get the rest of the 12 byte of data.
> > + * The read/write function will extract the requested data.
> > + */
> > if (len > QCA_HDR_MGMT_DATA1_LEN)
> > memcpy(mgmt_hdr_data->data + 1, skb->data,
> > QCA_HDR_MGMT_DATA2_LEN);
> > @@ -229,16 +231,30 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> > }
> > static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> > - int seq_num, int priority)
> > + int seq_num, int priority, int len)
>
> unsigned int len
>
> > {
> > struct mgmt_ethhdr *mgmt_ethhdr;
> > struct sk_buff *skb;
> > + int real_len;
>
> Likewise.
>
> > + u32 *data2;
> > u16 hdr;
> > skb = dev_alloc_skb(QCA_HDR_MGMT_PKG_LEN);
> > if (!skb)
> > return NULL;
> > + /* Max value for len reg is 15 (0xf) but the switch actually return 16 byte
> > + * Actually for some reason the steps are:
> > + * 0: nothing
> > + * 1-4: first 4 byte
> > + * 5-6: first 12 byte
> > + * 7-15: all 16 byte
> > + */
> > + if (len == 16)
> > + real_len = 15;
> > + else
> > + real_len = len;
> > +
> > skb_reset_mac_header(skb);
> > skb_set_network_header(skb, skb->len);
> > @@ -253,7 +269,7 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
> > mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
> > mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
> > - mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, 4);
> > + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, real_len);
> > mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
> > mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
> > QCA_HDR_MGMT_CHECK_CODE_VAL);
> > @@ -263,19 +279,22 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
> > mgmt_ethhdr->hdr = htons(hdr);
> > - skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
> > + data2 = skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
> > + if (cmd == MDIO_WRITE && len > QCA_HDR_MGMT_DATA1_LEN)
> > + memcpy(data2, val + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> > return skb;
> > }
> > -static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> > {
> > struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> > struct sk_buff *skb;
> > bool ack;
> > int ret;
> > - skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > + skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200,
> > + QCA8K_ETHERNET_MDIO_PRIORITY, len);
> > if (!skb)
> > return -ENOMEM;
> > @@ -297,6 +316,9 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> > *val = mgmt_hdr_data->data[0];
> > + if (len > QCA_HDR_MGMT_DATA1_LEN)
> > + memcpy(val + 1, mgmt_hdr_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> > +
> > ack = mgmt_hdr_data->ack;
> > mutex_unlock(&mgmt_hdr_data->mutex);
> > @@ -310,14 +332,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > return 0;
> > }
> > -static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> > +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> > {
> > struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> > struct sk_buff *skb;
> > bool ack;
> > int ret;
> > - skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > + skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val, 200,
> > + QCA8K_ETHERNET_MDIO_PRIORITY, len);
> > if (!skb)
> > return -ENOMEM;
> > @@ -357,14 +380,14 @@ qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 wri
> > u32 val = 0;
> > int ret;
> > - ret = qca8k_read_eth(priv, reg, &val);
> > + ret = qca8k_read_eth(priv, reg, &val, 4);
>
> sizeof(val) instead of 4.
>
> > if (ret)
> > return ret;
> > val &= ~mask;
> > val |= write_val;
> > - return qca8k_write_eth(priv, reg, val);
> > + return qca8k_write_eth(priv, reg, &val, 4);
>
> Likewise
>
> > }
> > static int
> > @@ -376,7 +399,7 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > u16 r1, r2, page;
> > int ret;
> > - if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
> > + if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val, 4))
>
> Likewise and everywhere below as well.
> --
> Florian
--
Ansuel
On 1/25/2022 8:01 PM, Ansuel Smith wrote:
> On Tue, Jan 25, 2022 at 07:54:15PM -0800, Florian Fainelli wrote:
>>
>>
>> On 1/22/2022 5:33 PM, Ansuel Smith wrote:
>>> Add all the required define to prepare support for mgmt read/write in
>>> Ethernet packet. Any packet of this type has to be dropped as the only
>>> use of these special packet is receive ack for an mgmt write request or
>>> receive data for an mgmt read request.
>>> A struct is used that emulates the Ethernet header but is used for a
>>> different purpose.
>>>
>>> Signed-off-by: Ansuel Smith <[email protected]>
>>> ---
>>
>> [snip]
>>
>>> +/* Special struct emulating a Ethernet header */
>>> +struct mgmt_ethhdr {
>>> + u32 command; /* command bit 31:0 */
>>> + u32 seq; /* seq 63:32 */
>>> + u32 mdio_data; /* first 4byte mdio */
>>> + __be16 hdr; /* qca hdr */
>>> +} __packed;
>>
>> Might be worth adding a BUILD_BUG_ON(sizeof(struct mgmt_ethhdr) !=
>> QCA_HDR_MGMT_PKG_LEN) when you start making use of that structure?
>
> Where should I put this check? Right after the struct definition,
> correct? (I just checked definition of the macro)
It would have to be in a call site where you use the structure, I have
not checked whether putting it in a static inline function in .h file
actually works if the inline function is not used at all.
--
Florian
On Tue, Jan 25, 2022 at 07:54:15PM -0800, Florian Fainelli wrote:
>
>
> On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> > Add all the required define to prepare support for mgmt read/write in
> > Ethernet packet. Any packet of this type has to be dropped as the only
> > use of these special packet is receive ack for an mgmt write request or
> > receive data for an mgmt read request.
> > A struct is used that emulates the Ethernet header but is used for a
> > different purpose.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
>
> [snip]
>
> > +/* Special struct emulating a Ethernet header */
> > +struct mgmt_ethhdr {
> > + u32 command; /* command bit 31:0 */
> > + u32 seq; /* seq 63:32 */
> > + u32 mdio_data; /* first 4byte mdio */
> > + __be16 hdr; /* qca hdr */
> > +} __packed;
>
> Might be worth adding a BUILD_BUG_ON(sizeof(struct mgmt_ethhdr) !=
> QCA_HDR_MGMT_PKG_LEN) when you start making use of that structure?
Where should I put this check? Right after the struct definition,
correct? (I just checked definition of the macro)
> --
> Florian
--
Ansuel
On Tue, Jan 25, 2022 at 08:02:53PM -0800, Florian Fainelli wrote:
>
>
> On 1/25/2022 8:01 PM, Ansuel Smith wrote:
> > On Tue, Jan 25, 2022 at 07:54:15PM -0800, Florian Fainelli wrote:
> > >
> > >
> > > On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> > > > Add all the required define to prepare support for mgmt read/write in
> > > > Ethernet packet. Any packet of this type has to be dropped as the only
> > > > use of these special packet is receive ack for an mgmt write request or
> > > > receive data for an mgmt read request.
> > > > A struct is used that emulates the Ethernet header but is used for a
> > > > different purpose.
> > > >
> > > > Signed-off-by: Ansuel Smith <[email protected]>
> > > > ---
> > >
> > > [snip]
> > >
> > > > +/* Special struct emulating a Ethernet header */
> > > > +struct mgmt_ethhdr {
> > > > + u32 command; /* command bit 31:0 */
> > > > + u32 seq; /* seq 63:32 */
> > > > + u32 mdio_data; /* first 4byte mdio */
> > > > + __be16 hdr; /* qca hdr */
> > > > +} __packed;
> > >
> > > Might be worth adding a BUILD_BUG_ON(sizeof(struct mgmt_ethhdr) !=
> > > QCA_HDR_MGMT_PKG_LEN) when you start making use of that structure?
> >
> > Where should I put this check? Right after the struct definition,
> > correct? (I just checked definition of the macro)
>
> It would have to be in a call site where you use the structure, I have not
> checked whether putting it in a static inline function in .h file actually
> works if the inline function is not used at all.
Think I can test that by just putting a wrong value in
QCA_HDR_MGMT_PKG_LEN and check if the error is triggered.
Will check if the macro will actually work.
> --
> Florian
--
Ansuel
On Tue, Jan 25, 2022 at 07:22:51PM -0800, Florian Fainelli wrote:
>
>
> On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> > From: Vladimir Oltean <[email protected]>
> >
> > Certain drivers may need to send management traffic to the switch for
> > things like register access, FDB dump, etc, to accelerate what their
> > slow bus (SPI, I2C, MDIO) can already do.
> >
> > Ethernet is faster (especially in bulk transactions) but is also more
> > unreliable, since the user may decide to bring the DSA master down (or
> > not bring it up), therefore severing the link between the host and the
> > attached switch.
> >
> > Drivers needing Ethernet-based register access already should have
> > fallback logic to the slow bus if the Ethernet method fails, but that
> > fallback may be based on a timeout, and the I/O to the switch may slow
> > down to a halt if the master is down, because every Ethernet packet will
> > have to time out. The driver also doesn't have the option to turn off
> > Ethernet-based I/O momentarily, because it wouldn't know when to turn it
> > back on.
> >
> > Which is where this change comes in. By tracking NETDEV_CHANGE,
> > NETDEV_UP and NETDEV_GOING_DOWN events on the DSA master, we should know
> > the exact interval of time during which this interface is reliably
> > available for traffic. Provide this information to switches so they can
> > use it as they wish.
> >
> > An helper is added dsa_port_master_is_operational() to check if a master
> > port is operational.
"The DSA master is able to pass traffic when it was brought
administratively up and is also operationally up. We introduce a helper
function named dsa_port_master_is_operational() which checks for the
proper conditions on a CPU port's DSA master."
> >
> > Signed-off-by: Vladimir Oltean <[email protected]>
> > Signed-off-by: Ansuel Smith <[email protected]>
>
> Reviewed-by: Florian Fainelli <[email protected]>
> --
> Florian
On Wed, Jan 26, 2022 at 02:57:03AM +0100, Ansuel Smith wrote:
> > > > Shouldn't qca8k_master_change() also take phy_hdr_data->mutex?
> > > >
> > >
> > > Is actually the normal mgmg_hdr_data.
> > >
> > > phy_hdr_data = &priv->mgmt_hdr_data;
> > >
> > > Should I remove this and use mgmt_hdr_data directly to remove any
> > > confusion?
> >
> > I am not thrilled by the naming of this data structure anyway
> > (why "hdr"?), but yes, I also got tricked by inconsistent naming.
> > Please choose a consistent name and stick with it.
>
> Hdr as header stuff since all this stuff is put in the hdr. Should I
> just drop hdr and use mgmt_data directly? Or mgmt_eth?
I don't have a strong preference because I can't find a good name.
Consistency in naming this feature is the most important part.
Maybe it is just me who is reading it this way, but I associate a
structure whose name contains "hdr" with something that pertains to data
from an skb (such as "mgmt_ethhdr" which is exactly that), hence the
earlier comment. I opened the manual and the phrasing that the vendor
uses is that "[ the switch ] supports the read/write register (sic)
through the Atheros header". So it makes more sense now and it's in line
with that, at least to some degree. I understand if you prefer not to
change it, but "mgmt_data" sounds less confusing to me.
On Sun, Jan 23, 2022 at 02:33:21AM +0100, Ansuel Smith wrote:
> Hi, this is ready but require some additional test on a wider userbase.
>
> The main reason for this is that we notice some routing problem in the
> switch and it seems assisted learning is needed. Considering mdio is
> quite slow due to the indirect write using this Ethernet alternative way
> seems to be quicker.
>
> The qca8k switch supports a special way to pass mdio read/write request
> using specially crafted Ethernet packet.
> This works by putting some defined data in the Ethernet header where the
> mac source and dst should be placed. The Ethernet type header is set to qca
> header and is set to a mdio read/write type.
> This is used to communicate to the switch that this is a special packet
> and should be parsed differently.
>
> Currently we use Ethernet packet for
> - MIB counter
> - mdio read/write configuration
> - phy read/write for each port
>
> Current implementation of this use completion API to wait for the packet
> to be processed by the tagger and has a timeout that fallback to the
> legacy mdio way and mutex to enforce one transaction at time.
>
> We now have connect()/disconnect() ops for the tagger. They are used to
> allocate priv data in the dsa priv. The header still has to be put in
> global include to make it usable by a dsa driver.
> They are called when the tag is connect to the dst and the data is freed
> using discconect on tagger change.
>
> (if someone wonder why the bind function is put at in the general setup
> function it's because tag is set in the cpu port where the notifier is
> still not available and we require the notifier to sen the
> tag_proto_connect() event.
>
> We now have a tag_proto_connect() for the dsa driver used to put
> additional data in the tagger priv (that is actually the dsa priv).
> This is called using a switch event DSA_NOTIFIER_TAG_PROTO_CONNECT.
> Current use for this is adding handler for the Ethernet packet to keep
> the tagger code as dumb as possible.
>
> The tagger priv implement only the handler for the special packet. All the
> other stuff is placed in the qca8k_priv and the tagger has to access
> it under lock.
>
> We use the new API from Vladimir to track if the master port is
> operational or not. We had to track many thing to reach a usable state.
> Checking if the port is UP is not enough and tracking a NETDEV_CHANGE is
> also not enough since it use also for other task. The correct way was
> both track for interface UP and if a qdisc was assigned to the
> interface. That tells us the port (and the tagger indirectly) is ready
> to accept and process packet.
>
> I tested this with multicpu port and with port6 set as the unique port and
> it's sad.
> It seems they implemented this feature in a bad way and this is only
> supported with cpu port0. When cpu port6 is the unique port, the switch
> doesn't send ack packet. With multicpu port, packet ack are not duplicated
> and only cpu port0 sends them. This is the same for the MIB counter.
> For this reason this feature is enabled only when cpu port0 is enabled and
> operational.
>
>
>
>
> Sorry if I missed any comments. This series is 2 month old so I think it
> would be good to recheck everything. In the mean time this was tested on
> and no regression are observed related to random port drop.
>
> v7:
> - Rebase on net-next changes
> - Add bulk patches to speedup this even more
> v6:
> - Fix some error in ethtool handler caused by rebase/cleanup
> v5:
> - Adapt to new API fixes
> - Fix a wrong logic for noop
> - Add additional lock for master_state change
> - Limit mdio Ethernet to cpu port0 (switch limitation)
> - Add priority to these special packet
> - Move mdio cache to qca8k_priv
> v4:
> - Remove duplicate patch sent by mistake.
> v3:
> - Include MIB with Ethernet packet.
> - Include phy read/write with Ethernet packet.
> - Reorganize code with new API.
> - Introuce master tracking by Vladimir
> v2:
> - Address all suggestion from Vladimir.
> Try to generilize this with connect/disconnect function from the
> tagger and tag_proto_connect for the driver.
>
> Ansuel Smith (14):
> net: dsa: tag_qca: convert to FIELD macro
> net: dsa: tag_qca: move define to include linux/dsa
> net: dsa: tag_qca: enable promisc_on_master flag
> net: dsa: tag_qca: add define for handling mgmt Ethernet packet
> net: dsa: tag_qca: add define for handling MIB packet
> net: dsa: tag_qca: add support for handling mgmt and MIB Ethernet
> packet
> net: dsa: qca8k: add tracking state of master port
> net: dsa: qca8k: add support for mgmt read/write in Ethernet packet
> net: dsa: qca8k: add support for mib autocast in Ethernet packet
> net: dsa: qca8k: add support for phy read/write with mgmt Ethernet
> net: dsa: qca8k: move page cache to driver priv
> net: dsa: qca8k: cache lo and hi for mdio write
> net: da: qca8k: add support for larger read/write size with mgmt
> Ethernet
> net: dsa: qca8k: introduce qca8k_bulk_read/write function
>
> Vladimir Oltean (2):
> net: dsa: provide switch operations for tracking the master state
> net: dsa: replay master state events in
> dsa_tree_{setup,teardown}_master
>
> drivers/net/dsa/qca8k.c | 709 +++++++++++++++++++++++++++++++++---
> drivers/net/dsa/qca8k.h | 46 ++-
> include/linux/dsa/tag_qca.h | 82 +++++
> include/net/dsa.h | 17 +
> net/dsa/dsa2.c | 74 +++-
> net/dsa/dsa_priv.h | 13 +
> net/dsa/slave.c | 32 ++
> net/dsa/switch.c | 15 +
> net/dsa/tag_qca.c | 83 +++--
> 9 files changed, 993 insertions(+), 78 deletions(-)
> create mode 100644 include/linux/dsa/tag_qca.h
>
> --
> 2.33.1
>
Hi,
sorry for the delay in sending v8, it's ready but I'm far from home and
I still need to check some mdio improvement with pointer handling.
Anyway I have some concern aboutall the skb alloc.
I wonder if that part can be improved at the cost of some additional
space used.
The idea Is to use the cache stuff also for the eth skb (or duplicate
it?) And use something like build_skb and recycle the skb space
everytime...
This comes from the fact that packet size is ALWAYS the same and it
seems stupid to allocate and free it everytime. Considering we also
enforce a one way transaction (we send packet and we wait for response)
this makes the allocation process even more stupid.
So I wonder if we would have some perf improvement/less load by
declaring the mgmt eth space and build an skb that always use that
preallocate space and just modify data.
I would really love some feedback considering qca8k is also used in very
low spec ath79 device where we need to reduce the load in every way
possible. Also if anyone have more ideas on how to improve this to make
it less heavy cpu side, feel free to point it out even if it would
mean that my implemenation is complete sh*t.
(The use of caching the address would permit us to reduce the write to
this preallocated space even more or ideally to send the same skb)
--
Ansuel
On 1/30/2022 5:59 AM, Ansuel Smith wrote:
>>
>
> Hi,
> sorry for the delay in sending v8, it's ready but I'm far from home and
> I still need to check some mdio improvement with pointer handling.
>
> Anyway I have some concern aboutall the skb alloc.
> I wonder if that part can be improved at the cost of some additional
> space used.
>
> The idea Is to use the cache stuff also for the eth skb (or duplicate
> it?) And use something like build_skb and recycle the skb space
> everytime...
> This comes from the fact that packet size is ALWAYS the same and it
> seems stupid to allocate and free it everytime. Considering we also
> enforce a one way transaction (we send packet and we wait for response)
> this makes the allocation process even more stupid.
>
> So I wonder if we would have some perf improvement/less load by
> declaring the mgmt eth space and build an skb that always use that
> preallocate space and just modify data.
>
> I would really love some feedback considering qca8k is also used in very
> low spec ath79 device where we need to reduce the load in every way
> possible. Also if anyone have more ideas on how to improve this to make
> it less heavy cpu side, feel free to point it out even if it would
> mean that my implemenation is complete sh*t.
>
> (The use of caching the address would permit us to reduce the write to
> this preallocated space even more or ideally to send the same skb)
I would say first things first: get this patch series included since it
is very close from being suitable for inclusion in net-next. Then you
can profile the I/O accesses over the management Ethernet frames and
devise a strategy to optimize them to make as little CPU cycles
intensive as possible.
build_skb() is not exactly a magic bullet that will solve all
performance problems, you still need the non-data portion of the skb to
be allocated, and also keep in mind that you need tail room at the end
of the data buffer in order for struct skb_shared_info to be written.
This means that the hardware is not allowed to write at the end of the
data buffer, or you must reduce the maximum RX length accordingly to
prevent that. Your frames are small enough here this is unlikely to be
an issue.
Since the MDIO layer does not really allow more than one outstanding
transaction per MDIO device at a time, you might be just fine with just
have a front and back skb set of buffers and alternating between these two.
--
Florian
On Tue, Jan 25, 2022 at 07:50:47PM -0800, Florian Fainelli wrote:
>
>
> On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> > There can be multiple qca8k switch on the same system. Move the static
> > qca8k_current_page to qca8k_priv and make it specific for each switch.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > drivers/net/dsa/qca8k.c | 47 +++++++++++++++++++++++------------------
> > drivers/net/dsa/qca8k.h | 9 ++++++++
> > 2 files changed, 36 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index e7bc0770bae9..c2f5414033d8 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -75,12 +75,6 @@ static const struct qca8k_mib_desc ar8327_mib[] = {
> > MIB_DESC(1, 0xac, "TXUnicast"),
> > };
> > -/* The 32bit switch registers are accessed indirectly. To achieve this we need
> > - * to set the page of the register. Track the last page that was set to reduce
> > - * mdio writes
> > - */
> > -static u16 qca8k_current_page = 0xffff;
> > -
> > static void
> > qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
> > {
> > @@ -134,11 +128,11 @@ qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
> > }
> > static int
> > -qca8k_set_page(struct mii_bus *bus, u16 page)
> > +qca8k_set_page(struct mii_bus *bus, u16 page, u16 *cached_page)
> > {
>
> bus->priv is assigned a qca8k_priv pointer, so we can just de-reference it
I just checked this and no. The priv bus we have in qca8k_set_page,
points to the mdio ipq8064 priv struct. (or the gpio-bitbang driver I
assume)
> here from bus->priv and avoid changing a whole bunch of function signatures
> that are now getting both a qca8k_priv *and* a qca8k_mdio_cache set of
> pointers when you can just use back pointers to those.
Should we change the function to provide qca8k_priv directly? Or I just
didn't understand your suggestion on how we can reduce the changes in
this patch.
> --
> Florian
--
Ansuel
On Sun, Jan 30, 2022 at 09:07:16AM -0800, Florian Fainelli wrote:
>
>
> On 1/30/2022 5:59 AM, Ansuel Smith wrote:
> > >
> >
> > Hi,
> > sorry for the delay in sending v8, it's ready but I'm far from home and
> > I still need to check some mdio improvement with pointer handling.
> >
> > Anyway I have some concern aboutall the skb alloc.
> > I wonder if that part can be improved at the cost of some additional
> > space used.
> >
> > The idea Is to use the cache stuff also for the eth skb (or duplicate
> > it?) And use something like build_skb and recycle the skb space
> > everytime...
> > This comes from the fact that packet size is ALWAYS the same and it
> > seems stupid to allocate and free it everytime. Considering we also
> > enforce a one way transaction (we send packet and we wait for response)
> > this makes the allocation process even more stupid.
> >
> > So I wonder if we would have some perf improvement/less load by
> > declaring the mgmt eth space and build an skb that always use that
> > preallocate space and just modify data.
> >
> > I would really love some feedback considering qca8k is also used in very
> > low spec ath79 device where we need to reduce the load in every way
> > possible. Also if anyone have more ideas on how to improve this to make
> > it less heavy cpu side, feel free to point it out even if it would
> > mean that my implemenation is complete sh*t.
> >
> > (The use of caching the address would permit us to reduce the write to
> > this preallocated space even more or ideally to send the same skb)
>
> I would say first things first: get this patch series included since it is
> very close from being suitable for inclusion in net-next. Then you can
> profile the I/O accesses over the management Ethernet frames and devise a
> strategy to optimize them to make as little CPU cycles intensive as
> possible.
>
Don't know if it's correct to continue this disccusion here.
> build_skb() is not exactly a magic bullet that will solve all performance
> problems, you still need the non-data portion of the skb to be allocated,
> and also keep in mind that you need tail room at the end of the data buffer
> in order for struct skb_shared_info to be written. This means that the
> hardware is not allowed to write at the end of the data buffer, or you must
> reduce the maximum RX length accordingly to prevent that. Your frames are
> small enough here this is unlikely to be an issue.
>
I did some test with a build_skb() implemenation and I just discovered
that It wouldn't work... Problem of build_skb() is that the driver will
release the data and that's exactly what I want to skip (one allocated
memory space that is reused for every skb)
Wonder if it would be acceptable to allocate a skb when master became
operational and use always that.
When this preallocated skb has to be used, the required data is changed
and the users of the skb is increased so that it's not free. In theory
all the skb shared data and head should be the same as what changes of
the packet is just the data and nothing else.
It looks like an hack but that is the only way I found to skip the
skb_free when the packet is processed. (increasing the skb users)
> Since the MDIO layer does not really allow more than one outstanding
> transaction per MDIO device at a time, you might be just fine with just have
> a front and back skb set of buffers and alternating between these two.
Another way as you suggested would be have 2 buffer and use build_skb to
use build the sbk around the allocated buffer. But still my main concern
is if the use of manually increasing the skb user is accepted to skip
any skb free from happening.
Hope I'm not too annoying with these kind of question.
> --
> Florian
--
Ansuel
On Thu, 3 Feb 2022 20:21:28 +0200 Vladimir Oltean wrote:
> To my knowledge, when you call dev_queue_xmit(), the skb is no longer
> yours, end of story, it doesn't matter whether you increase the refcount
> on it or not. The DSA master may choose to do whatever it wishes with
> that buffer after its TX completion interrupt fires: it may not call
> napi_consume_skb() but directly recycle that buffer in its pool of RX
> buffers, as part of some weird buffer recycling scheme. So you'll think
> that the buffer is yours, but it isn't, because the driver hasn't
> returned it to the allocator, and your writes for the next packet may be
> concurrent with some RX DMA transactions. I don't have a mainline
> example to give you, but I've seen the pattern, and I don't think it's
> illegal (although of course, I stand to be corrected if necessary).
Are we talking about holding onto the Tx skb here or also recycling
the Rx one? Sorry for another out of context comment in advance..
AFAIK in theory shared skbs are supposed to be untouched or unshared
explicitly by the driver on Tx. pktgen takes advantage of it.
We have IFF_TX_SKB_SHARING.
In practice everyone gets opted into SKB_SHARING because ether_setup()
sets the flag. A lot of drivers are not aware of the requirement and
will assume full ownership (and for example use skb->cb[]) :/
I don't think there is any Tx completion -> Rx pool recycling scheme
inside the drivers (if that's what you described).
On Thu, Feb 03, 2022 at 06:59:13PM +0100, Ansuel Smith wrote:
> On Sun, Jan 30, 2022 at 09:07:16AM -0800, Florian Fainelli wrote:
> > On 1/30/2022 5:59 AM, Ansuel Smith wrote:
> > > Hi,
> > > sorry for the delay in sending v8, it's ready but I'm far from home and
> > > I still need to check some mdio improvement with pointer handling.
> > >
> > > Anyway I have some concern aboutall the skb alloc.
> > > I wonder if that part can be improved at the cost of some additional
> > > space used.
> > >
> > > The idea Is to use the cache stuff also for the eth skb (or duplicate
> > > it?) And use something like build_skb and recycle the skb space
> > > everytime...
> > > This comes from the fact that packet size is ALWAYS the same and it
> > > seems stupid to allocate and free it everytime. Considering we also
> > > enforce a one way transaction (we send packet and we wait for response)
> > > this makes the allocation process even more stupid.
> > >
> > > So I wonder if we would have some perf improvement/less load by
> > > declaring the mgmt eth space and build an skb that always use that
> > > preallocate space and just modify data.
> > >
> > > I would really love some feedback considering qca8k is also used in very
> > > low spec ath79 device where we need to reduce the load in every way
> > > possible. Also if anyone have more ideas on how to improve this to make
> > > it less heavy cpu side, feel free to point it out even if it would
> > > mean that my implemenation is complete sh*t.
> > >
> > > (The use of caching the address would permit us to reduce the write to
> > > this preallocated space even more or ideally to send the same skb)
> >
> > I would say first things first: get this patch series included since it is
> > very close from being suitable for inclusion in net-next. Then you can
> > profile the I/O accesses over the management Ethernet frames and devise a
> > strategy to optimize them to make as little CPU cycles intensive as
> > possible.
> >
>
> Don't know if it's correct to continue this disccusion here.
>
> > build_skb() is not exactly a magic bullet that will solve all performance
> > problems, you still need the non-data portion of the skb to be allocated,
> > and also keep in mind that you need tail room at the end of the data buffer
> > in order for struct skb_shared_info to be written. This means that the
> > hardware is not allowed to write at the end of the data buffer, or you must
> > reduce the maximum RX length accordingly to prevent that. Your frames are
> > small enough here this is unlikely to be an issue.
> >
>
> I did some test with a build_skb() implemenation and I just discovered
> that It wouldn't work... Problem of build_skb() is that the driver will
> release the data and that's exactly what I want to skip (one allocated
> memory space that is reused for every skb)
>
> Wonder if it would be acceptable to allocate a skb when master became
> operational and use always that.
> When this preallocated skb has to be used, the required data is changed
> and the users of the skb is increased so that it's not free. In theory
> all the skb shared data and head should be the same as what changes of
> the packet is just the data and nothing else.
> It looks like an hack but that is the only way I found to skip the
> skb_free when the packet is processed. (increasing the skb users)
>
> > Since the MDIO layer does not really allow more than one outstanding
> > transaction per MDIO device at a time, you might be just fine with just have
> > a front and back skb set of buffers and alternating between these two.
>
> Another way as you suggested would be have 2 buffer and use build_skb to
> use build the sbk around the allocated buffer. But still my main concern
> is if the use of manually increasing the skb user is accepted to skip
> any skb free from happening.
>
> Hope I'm not too annoying with these kind of question.
To my knowledge, when you call dev_queue_xmit(), the skb is no longer
yours, end of story, it doesn't matter whether you increase the refcount
on it or not. The DSA master may choose to do whatever it wishes with
that buffer after its TX completion interrupt fires: it may not call
napi_consume_skb() but directly recycle that buffer in its pool of RX
buffers, as part of some weird buffer recycling scheme. So you'll think
that the buffer is yours, but it isn't, because the driver hasn't
returned it to the allocator, and your writes for the next packet may be
concurrent with some RX DMA transactions. I don't have a mainline
example to give you, but I've seen the pattern, and I don't think it's
illegal (although of course, I stand to be corrected if necessary).
On Thu, Feb 03, 2022 at 12:10:27PM -0800, Jakub Kicinski wrote:
> On Thu, 3 Feb 2022 20:21:28 +0200 Vladimir Oltean wrote:
> > To my knowledge, when you call dev_queue_xmit(), the skb is no longer
> > yours, end of story, it doesn't matter whether you increase the refcount
> > on it or not. The DSA master may choose to do whatever it wishes with
> > that buffer after its TX completion interrupt fires: it may not call
> > napi_consume_skb() but directly recycle that buffer in its pool of RX
> > buffers, as part of some weird buffer recycling scheme. So you'll think
> > that the buffer is yours, but it isn't, because the driver hasn't
> > returned it to the allocator, and your writes for the next packet may be
> > concurrent with some RX DMA transactions. I don't have a mainline
> > example to give you, but I've seen the pattern, and I don't think it's
> > illegal (although of course, I stand to be corrected if necessary).
>
> Are we talking about holding onto the Tx skb here or also recycling
> the Rx one? Sorry for another out of context comment in advance..
>
Here we are talking about tx skb. (We can't really operate on the
received skb handled by the tagger)
What I want to improve is the fact that we alloc 3 very small skb that
will have only some part of them modified and the rest equal on all 3
skb. So my idea is find a way to preallocate a space and build a skb
around it. In short as we force a 1:1 comunication where we send an skb
and we wait for the response to be processed, we can reuse the same skb
everytime and just modify the data in it. I'm asking if anyone have some
hint/direction without proposing a patch that is a big massive hack that
will be NACK in 0.1 ms LOL
> AFAIK in theory shared skbs are supposed to be untouched or unshared
> explicitly by the driver on Tx. pktgen takes advantage of it.
> We have IFF_TX_SKB_SHARING.
>
Will check how this flags is used. If you have any hint about this feel
free to suggest it.
> In practice everyone gets opted into SKB_SHARING because ether_setup()
> sets the flag. A lot of drivers are not aware of the requirement and
> will assume full ownership (and for example use skb->cb[]) :/
>
Consider that currently this will be used by stmmac driver, brcm driver
and someother atheros driver.
> I don't think there is any Tx completion -> Rx pool recycling scheme
> inside the drivers (if that's what you described).
--
Ansuel
On Thu, Feb 03, 2022 at 12:10:27PM -0800, Jakub Kicinski wrote:
> On Thu, 3 Feb 2022 20:21:28 +0200 Vladimir Oltean wrote:
> > To my knowledge, when you call dev_queue_xmit(), the skb is no longer
> > yours, end of story, it doesn't matter whether you increase the refcount
> > on it or not. The DSA master may choose to do whatever it wishes with
> > that buffer after its TX completion interrupt fires: it may not call
> > napi_consume_skb() but directly recycle that buffer in its pool of RX
> > buffers, as part of some weird buffer recycling scheme. So you'll think
> > that the buffer is yours, but it isn't, because the driver hasn't
> > returned it to the allocator, and your writes for the next packet may be
> > concurrent with some RX DMA transactions. I don't have a mainline
> > example to give you, but I've seen the pattern, and I don't think it's
> > illegal (although of course, I stand to be corrected if necessary).
>
> Are we talking about holding onto the Tx skb here or also recycling
> the Rx one? Sorry for another out of context comment in advance..
We're talking about the possibility that the DSA master holds onto the
TX skb, for the purpose of saving a netdev_alloc_skb() call later in the
RX path.
> AFAIK in theory shared skbs are supposed to be untouched or unshared
> explicitly by the driver on Tx. pktgen takes advantage of it.
> We have IFF_TX_SKB_SHARING.
>
> In practice everyone gets opted into SKB_SHARING because ether_setup()
> sets the flag. A lot of drivers are not aware of the requirement and
> will assume full ownership (and for example use skb->cb[]) :/
>
> I don't think there is any Tx completion -> Rx pool recycling scheme
> inside the drivers (if that's what you described).
You made me go look again at commit acb600def211 ("net: remove skb
recycling"), a revert of which is still carried in some vendor kernels.
So there was a skb_is_recycleable() function with this check:
if (skb_shared(skb))
return false;
which means that yes, my argument is basically invalid, skb_get() in DSA
will protect against skb recycling.
I know Ansuel is using OpenWRT, so not a stranger to vendor kernels &
network drivers. My comment wasn't as much a hard NACK as it was a word
of caution. DSA allows pairing any switch driver to any host controller
driver. If somebody hits the jackpot (probably won't be me, won't be
Ansuel), it won't be exactly fun for them until they figure out what's
going on, with the symptoms being random data corruption during switch
register access. But after I revisited the exact situation, I don't have
an example that proves to be problematic.