* [PATCH net 7/8] net: microchip: vcap api: Add a storage state to a VCAP rule
From: Steen Hegelund @ 2022-12-21 13:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Steen Hegelund, UNGLinuxDriver, Randy Dunlap, Casper Andersson,
Russell King, Wan Jiabing, Nathan Huckleberry, linux-kernel,
netdev, linux-arm-kernel, Steen Hegelund, Daniel Machon,
Horatiu Vultur, Lars Povlsen, Dan Carpenter
In-Reply-To: <20221221132517.2699698-1-steen.hegelund@microchip.com>
This allows a VCAP rule to be in one of 3 states:
- permanently stored in the VCAP HW (for rules that must always be present)
- enabled (stored in HW) when the corresponding lookup has been enabled
- disabled (stored in SW) when the lookup is disabled
This way important VCAP rules can be added even before the user enables the
VCAP lookups using a TC matchall filter.
Fixes: 4426b78c626d ("net: lan966x: Add port keyset config and callback interface")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
.../net/ethernet/microchip/vcap/vcap_api.c | 126 ++++++++++++++++--
.../microchip/vcap/vcap_api_debugfs.c | 52 +++++---
.../microchip/vcap/vcap_api_debugfs_kunit.c | 1 +
.../microchip/vcap/vcap_api_private.h | 9 +-
4 files changed, 161 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index d438554355e4..94df0e7b58ea 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -950,9 +950,12 @@ int vcap_lookup_rule_by_cookie(struct vcap_control *vctrl, u64 cookie)
}
EXPORT_SYMBOL_GPL(vcap_lookup_rule_by_cookie);
-/* Make a shallow copy of the rule without the fields */
-struct vcap_rule_internal *vcap_dup_rule(struct vcap_rule_internal *ri)
+/* Make a copy of the rule, shallow or full */
+static struct vcap_rule_internal *vcap_dup_rule(struct vcap_rule_internal *ri,
+ bool full)
{
+ struct vcap_client_actionfield *caf, *newcaf;
+ struct vcap_client_keyfield *ckf, *newckf;
struct vcap_rule_internal *duprule;
/* Allocate the client part */
@@ -965,6 +968,27 @@ struct vcap_rule_internal *vcap_dup_rule(struct vcap_rule_internal *ri)
/* No elements in these lists */
INIT_LIST_HEAD(&duprule->data.keyfields);
INIT_LIST_HEAD(&duprule->data.actionfields);
+
+ /* A full rule copy includes keys and actions */
+ if (!full)
+ return duprule;
+
+ list_for_each_entry(ckf, &ri->data.keyfields, ctrl.list) {
+ newckf = kzalloc(sizeof(*newckf), GFP_KERNEL);
+ if (!newckf)
+ return ERR_PTR(-ENOMEM);
+ memcpy(newckf, ckf, sizeof(*newckf));
+ list_add_tail(&newckf->ctrl.list, &duprule->data.keyfields);
+ }
+
+ list_for_each_entry(caf, &ri->data.actionfields, ctrl.list) {
+ newcaf = kzalloc(sizeof(*newcaf), GFP_KERNEL);
+ if (!newcaf)
+ return ERR_PTR(-ENOMEM);
+ memcpy(newcaf, caf, sizeof(*newcaf));
+ list_add_tail(&newcaf->ctrl.list, &duprule->data.actionfields);
+ }
+
return duprule;
}
@@ -1877,8 +1901,8 @@ static int vcap_insert_rule(struct vcap_rule_internal *ri,
ri->addr = vcap_next_rule_addr(admin->last_used_addr, ri);
admin->last_used_addr = ri->addr;
- /* Add a shallow copy of the rule to the VCAP list */
- duprule = vcap_dup_rule(ri);
+ /* Add a copy of the rule to the VCAP list */
+ duprule = vcap_dup_rule(ri, ri->state == VCAP_RS_DISABLED);
if (IS_ERR(duprule))
return PTR_ERR(duprule);
@@ -1891,8 +1915,8 @@ static int vcap_insert_rule(struct vcap_rule_internal *ri,
ri->addr = vcap_next_rule_addr(addr, ri);
addr = ri->addr;
- /* Add a shallow copy of the rule to the VCAP list */
- duprule = vcap_dup_rule(ri);
+ /* Add a copy of the rule to the VCAP list */
+ duprule = vcap_dup_rule(ri, ri->state == VCAP_RS_DISABLED);
if (IS_ERR(duprule))
return PTR_ERR(duprule);
@@ -1939,6 +1963,72 @@ static bool vcap_is_chain_used(struct vcap_control *vctrl,
return false;
}
+/* Fetch the next chain in the enabled list for the port */
+static int vcap_get_next_chain(struct vcap_control *vctrl,
+ struct net_device *ndev,
+ int dst_cid)
+{
+ struct vcap_enabled_port *eport;
+ struct vcap_admin *admin;
+
+ list_for_each_entry(admin, &vctrl->list, list) {
+ list_for_each_entry(eport, &admin->enabled, list) {
+ if (eport->ndev != ndev)
+ continue;
+ if (eport->src_cid == dst_cid)
+ return eport->dst_cid;
+ }
+ }
+
+ return 0;
+}
+
+static bool vcap_path_exist(struct vcap_control *vctrl, struct net_device *ndev,
+ int dst_cid)
+{
+ struct vcap_enabled_port *eport, *elem;
+ struct vcap_admin *admin;
+ int tmp;
+
+ /* Find first entry that starts from chain 0*/
+ list_for_each_entry(admin, &vctrl->list, list) {
+ list_for_each_entry(elem, &admin->enabled, list) {
+ if (elem->src_cid == 0 && elem->ndev == ndev) {
+ eport = elem;
+ break;
+ }
+ }
+ if (eport)
+ break;
+ }
+
+ if (!eport)
+ return false;
+
+ tmp = eport->dst_cid;
+ while (tmp != dst_cid && tmp != 0)
+ tmp = vcap_get_next_chain(vctrl, ndev, tmp);
+
+ return !!tmp;
+}
+
+/* Internal clients can always store their rules in HW
+ * External clients can store their rules if the chain is enabled all
+ * the way from chain 0, otherwise the rule will be cached until
+ * the chain is enabled.
+ */
+static void vcap_rule_set_state(struct vcap_rule_internal *ri)
+{
+ if (ri->data.user <= VCAP_USER_QOS)
+ ri->state = VCAP_RS_PERMANENT;
+ else if (vcap_path_exist(ri->vctrl, ri->ndev, ri->data.vcap_chain_id))
+ ri->state = VCAP_RS_ENABLED;
+ else
+ ri->state = VCAP_RS_DISABLED;
+ /* For now always store directly in HW */
+ ri->state = VCAP_RS_PERMANENT;
+}
+
/* Encode and write a validated rule to the VCAP */
int vcap_add_rule(struct vcap_rule *rule)
{
@@ -1952,6 +2042,8 @@ int vcap_add_rule(struct vcap_rule *rule)
return ret;
/* Insert the new rule in the list of vcap rules */
mutex_lock(&ri->admin->lock);
+
+ vcap_rule_set_state(ri);
ret = vcap_insert_rule(ri, &move);
if (ret < 0) {
pr_err("%s:%d: could not insert rule in vcap list: %d\n",
@@ -1960,6 +2052,13 @@ int vcap_add_rule(struct vcap_rule *rule)
}
if (move.count > 0)
vcap_move_rules(ri, &move);
+
+ if (ri->state == VCAP_RS_DISABLED) {
+ /* Erase the rule area */
+ ri->vctrl->ops->init(ri->ndev, ri->admin, ri->addr, ri->size);
+ goto out;
+ }
+
vcap_erase_cache(ri);
ret = vcap_encode_rule(ri);
if (ret) {
@@ -2069,9 +2168,13 @@ struct vcap_rule *vcap_get_rule(struct vcap_control *vctrl, u32 id)
if (!elem)
return NULL;
mutex_lock(&elem->admin->lock);
- ri = vcap_dup_rule(elem);
+ ri = vcap_dup_rule(elem, elem->state == VCAP_RS_DISABLED);
if (IS_ERR(ri))
goto unlock;
+
+ if (ri->state == VCAP_RS_DISABLED)
+ goto unlock;
+
err = vcap_read_rule(ri);
if (err) {
ri = ERR_PTR(err);
@@ -2109,6 +2212,11 @@ int vcap_mod_rule(struct vcap_rule *rule)
return -ENOENT;
mutex_lock(&ri->admin->lock);
+
+ vcap_rule_set_state(ri);
+ if (ri->state == VCAP_RS_DISABLED)
+ goto out;
+
/* Encode the bitstreams to the VCAP cache */
vcap_erase_cache(ri);
err = vcap_encode_rule(ri);
@@ -2201,7 +2309,7 @@ int vcap_del_rule(struct vcap_control *vctrl, struct net_device *ndev, u32 id)
mutex_lock(&admin->lock);
list_del(&ri->list);
vctrl->ops->init(ndev, admin, admin->last_used_addr, ri->size + gap);
- kfree(ri);
+ vcap_free_rule(&ri->data);
mutex_unlock(&admin->lock);
/* Update the last used address, set to default when no rules */
@@ -2230,7 +2338,7 @@ int vcap_del_rules(struct vcap_control *vctrl, struct vcap_admin *admin)
list_for_each_entry_safe(ri, next_ri, &admin->rules, list) {
vctrl->ops->init(ri->ndev, admin, ri->addr, ri->size);
list_del(&ri->list);
- kfree(ri);
+ vcap_free_rule(&ri->data);
}
admin->last_used_addr = admin->last_valid_addr;
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
index e0b206247f2e..d6a09ce75e4f 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
@@ -152,37 +152,45 @@ vcap_debugfs_show_rule_actionfield(struct vcap_control *vctrl,
out->prf(out->dst, "\n");
}
-static int vcap_debugfs_show_rule_keyset(struct vcap_rule_internal *ri,
- struct vcap_output_print *out)
+static int vcap_debugfs_show_keysets(struct vcap_rule_internal *ri,
+ struct vcap_output_print *out)
{
- struct vcap_control *vctrl = ri->vctrl;
struct vcap_admin *admin = ri->admin;
enum vcap_keyfield_set keysets[10];
- const struct vcap_field *keyfield;
- enum vcap_type vt = admin->vtype;
- struct vcap_client_keyfield *ckf;
struct vcap_keyset_list matches;
- u32 *maskstream;
- u32 *keystream;
- int res;
+ int err;
- keystream = admin->cache.keystream;
- maskstream = admin->cache.maskstream;
matches.keysets = keysets;
matches.cnt = 0;
matches.max = ARRAY_SIZE(keysets);
- res = vcap_find_keystream_keysets(vctrl, vt, keystream, maskstream,
+
+ err = vcap_find_keystream_keysets(ri->vctrl, admin->vtype,
+ admin->cache.keystream,
+ admin->cache.maskstream,
false, 0, &matches);
- if (res < 0) {
+ if (err) {
pr_err("%s:%d: could not find valid keysets: %d\n",
- __func__, __LINE__, res);
- return -EINVAL;
+ __func__, __LINE__, err);
+ return err;
}
+
out->prf(out->dst, " keysets:");
for (int idx = 0; idx < matches.cnt; ++idx)
out->prf(out->dst, " %s",
- vcap_keyset_name(vctrl, matches.keysets[idx]));
+ vcap_keyset_name(ri->vctrl, matches.keysets[idx]));
out->prf(out->dst, "\n");
+ return 0;
+}
+
+static int vcap_debugfs_show_rule_keyset(struct vcap_rule_internal *ri,
+ struct vcap_output_print *out)
+{
+ struct vcap_control *vctrl = ri->vctrl;
+ struct vcap_admin *admin = ri->admin;
+ const struct vcap_field *keyfield;
+ struct vcap_client_keyfield *ckf;
+
+ vcap_debugfs_show_keysets(ri, out);
out->prf(out->dst, " keyset_sw: %d\n", ri->keyset_sw);
out->prf(out->dst, " keyset_sw_regs: %d\n", ri->keyset_sw_regs);
@@ -233,6 +241,18 @@ static void vcap_show_admin_rule(struct vcap_control *vctrl,
out->prf(out->dst, " chain_id: %d\n", ri->data.vcap_chain_id);
out->prf(out->dst, " user: %d\n", ri->data.user);
out->prf(out->dst, " priority: %d\n", ri->data.priority);
+ out->prf(out->dst, " state: ");
+ switch (ri->state) {
+ case VCAP_RS_PERMANENT:
+ out->prf(out->dst, "permanent\n");
+ break;
+ case VCAP_RS_DISABLED:
+ out->prf(out->dst, "disabled\n");
+ break;
+ case VCAP_RS_ENABLED:
+ out->prf(out->dst, "enabled\n");
+ break;
+ }
vcap_debugfs_show_rule_keyset(ri, out);
vcap_debugfs_show_rule_actionset(ri, out);
}
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
index bef0b28a4a50..9211cb453a3d 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
@@ -445,6 +445,7 @@ static const char * const test_admin_expect[] = {
" chain_id: 0\n",
" user: 0\n",
" priority: 0\n",
+ " state: permanent\n",
" keysets: VCAP_KFS_MAC_ETYPE\n",
" keyset_sw: 6\n",
" keyset_sw_regs: 2\n",
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_private.h b/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
index 4fd21da97679..ce35af9a853d 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
@@ -13,6 +13,12 @@
#define to_intrule(rule) container_of((rule), struct vcap_rule_internal, data)
+enum vcap_rule_state {
+ VCAP_RS_PERMANENT, /* the rule is always stored in HW */
+ VCAP_RS_ENABLED, /* enabled in HW but can be disabled */
+ VCAP_RS_DISABLED, /* disabled (stored in SW) and can be enabled */
+};
+
/* Private VCAP API rule data */
struct vcap_rule_internal {
struct vcap_rule data; /* provided by the client */
@@ -29,6 +35,7 @@ struct vcap_rule_internal {
u32 addr; /* address in the VCAP at insertion */
u32 counter_id; /* counter id (if a dedicated counter is available) */
struct vcap_counter counter; /* last read counter value */
+ enum vcap_rule_state state; /* rule storage state */
};
/* Bit iterator for the VCAP cache streams */
@@ -43,8 +50,6 @@ struct vcap_stream_iter {
/* Check that the control has a valid set of callbacks */
int vcap_api_check(struct vcap_control *ctrl);
-/* Make a shallow copy of the rule without the fields */
-struct vcap_rule_internal *vcap_dup_rule(struct vcap_rule_internal *ri);
/* Erase the VCAP cache area used or encoding and decoding */
void vcap_erase_cache(struct vcap_rule_internal *ri);
--
2.39.0
^ permalink raw reply related
* [PATCH net 3/8] net: microchip: vcap api: Always enable VCAP lookups
From: Steen Hegelund @ 2022-12-21 13:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Steen Hegelund, UNGLinuxDriver, Randy Dunlap, Casper Andersson,
Russell King, Wan Jiabing, Nathan Huckleberry, linux-kernel,
netdev, linux-arm-kernel, Steen Hegelund, Daniel Machon,
Horatiu Vultur, Lars Povlsen, Dan Carpenter
In-Reply-To: <20221221132517.2699698-1-steen.hegelund@microchip.com>
This changes the VCAP lookups state to always be enabled so that it is
possible to add "internal" VCAP rules that must be available even though
the user has not yet enabled the VCAP chains via a TC matchall filter.
The API callback to enable and disable VCAP lookups is therefore removed.
Fixes: 4426b78c626d ("net: lan966x: Add port keyset config and callback interface")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
.../microchip/lan966x/lan966x_vcap_impl.c | 24 ++++-----------
.../microchip/sparx5/sparx5_vcap_debugfs.c | 2 +-
.../microchip/sparx5/sparx5_vcap_impl.c | 29 ++++---------------
.../net/ethernet/microchip/vcap/vcap_api.c | 6 +---
.../net/ethernet/microchip/vcap/vcap_api.h | 5 ----
.../microchip/vcap/vcap_api_debugfs_kunit.c | 9 +-----
.../ethernet/microchip/vcap/vcap_api_kunit.c | 9 +-----
7 files changed, 16 insertions(+), 68 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
index d8dc9fbb81e1..76a9fb113f50 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
@@ -95,10 +95,7 @@ lan966x_vcap_is2_get_port_keysets(struct net_device *dev, int lookup,
bool found = false;
u32 val;
- /* Check if the port keyset selection is enabled */
val = lan_rd(lan966x, ANA_VCAP_S2_CFG(port->chip_port));
- if (!ANA_VCAP_S2_CFG_ENA_GET(val))
- return -ENOENT;
/* Collect all keysets for the port in a list */
if (l3_proto == ETH_P_ALL)
@@ -393,20 +390,6 @@ static int lan966x_vcap_port_info(struct net_device *dev,
return 0;
}
-static int lan966x_vcap_enable(struct net_device *dev,
- struct vcap_admin *admin,
- bool enable)
-{
- struct lan966x_port *port = netdev_priv(dev);
- struct lan966x *lan966x = port->lan966x;
-
- lan_rmw(ANA_VCAP_S2_CFG_ENA_SET(enable),
- ANA_VCAP_S2_CFG_ENA,
- lan966x, ANA_VCAP_S2_CFG(port->chip_port));
-
- return 0;
-}
-
static struct vcap_operations lan966x_vcap_ops = {
.validate_keyset = lan966x_vcap_validate_keyset,
.add_default_fields = lan966x_vcap_add_default_fields,
@@ -417,7 +400,6 @@ static struct vcap_operations lan966x_vcap_ops = {
.update = lan966x_vcap_update,
.move = lan966x_vcap_move,
.port_info = lan966x_vcap_port_info,
- .enable = lan966x_vcap_enable,
};
static void lan966x_vcap_admin_free(struct vcap_admin *admin)
@@ -524,6 +506,12 @@ int lan966x_vcap_init(struct lan966x *lan966x)
list_add_tail(&admin->list, &ctrl->list);
}
+ for (int p = 0; p < lan966x->num_phys_ports; ++p)
+ if (lan966x->ports[p])
+ lan_rmw(ANA_VCAP_S2_CFG_ENA_SET(true),
+ ANA_VCAP_S2_CFG_ENA, lan966x,
+ ANA_VCAP_S2_CFG(lan966x->ports[p]->chip_port));
+
lan966x->vcap_ctrl = ctrl;
return 0;
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_debugfs.c b/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_debugfs.c
index b91e05ffe2f4..c9423adc92ce 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_debugfs.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_debugfs.c
@@ -29,7 +29,7 @@ static void sparx5_vcap_port_keys(struct sparx5 *sparx5,
/* Get lookup state */
value = spx5_rd(sparx5, ANA_ACL_VCAP_S2_CFG(port->portno));
out->prf(out->dst, "\n state: ");
- if (ANA_ACL_VCAP_S2_CFG_SEC_ENA_GET(value))
+ if (ANA_ACL_VCAP_S2_CFG_SEC_ENA_GET(value) & BIT(lookup))
out->prf(out->dst, "on");
else
out->prf(out->dst, "off");
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c b/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
index a0c126ba9a87..0d4b40997bb4 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
@@ -510,28 +510,6 @@ static void sparx5_vcap_move(struct net_device *ndev, struct vcap_admin *admin,
sparx5_vcap_wait_super_update(sparx5);
}
-/* Enable all lookups in the VCAP instance */
-static int sparx5_vcap_enable(struct net_device *ndev,
- struct vcap_admin *admin,
- bool enable)
-{
- struct sparx5_port *port = netdev_priv(ndev);
- struct sparx5 *sparx5;
- int portno;
-
- sparx5 = port->sparx5;
- portno = port->portno;
-
- /* For now we only consider IS2 */
- if (enable)
- spx5_wr(ANA_ACL_VCAP_S2_CFG_SEC_ENA_SET(0xf), sparx5,
- ANA_ACL_VCAP_S2_CFG(portno));
- else
- spx5_wr(ANA_ACL_VCAP_S2_CFG_SEC_ENA_SET(0), sparx5,
- ANA_ACL_VCAP_S2_CFG(portno));
- return 0;
-}
-
/* API callback operations: only IS2 is supported for now */
static struct vcap_operations sparx5_vcap_ops = {
.validate_keyset = sparx5_vcap_validate_keyset,
@@ -543,7 +521,6 @@ static struct vcap_operations sparx5_vcap_ops = {
.update = sparx5_vcap_update,
.move = sparx5_vcap_move,
.port_info = sparx5_port_info,
- .enable = sparx5_vcap_enable,
};
/* Enable lookups per port and set the keyset generation: only IS2 for now */
@@ -568,6 +545,12 @@ static void sparx5_vcap_port_key_selection(struct sparx5 *sparx5,
ANA_ACL_VCAP_S2_KEY_SEL(portno, lookup));
}
}
+ /* IS2 lookups are in bit 0:3 */
+ for (portno = 0; portno < SPX5_PORTS; ++portno)
+ spx5_rmw(ANA_ACL_VCAP_S2_CFG_SEC_ENA_SET(0xf),
+ ANA_ACL_VCAP_S2_CFG_SEC_ENA,
+ sparx5,
+ ANA_ACL_VCAP_S2_CFG(portno));
}
/* Disable lookups per port and set the keyset generation: only IS2 for now */
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index 67e0a3d9103a..9de5367fde42 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -738,7 +738,7 @@ int vcap_api_check(struct vcap_control *ctrl)
!ctrl->ops->add_default_fields || !ctrl->ops->cache_erase ||
!ctrl->ops->cache_write || !ctrl->ops->cache_read ||
!ctrl->ops->init || !ctrl->ops->update || !ctrl->ops->move ||
- !ctrl->ops->port_info || !ctrl->ops->enable) {
+ !ctrl->ops->port_info) {
pr_err("%s:%d: client operations are missing\n",
__func__, __LINE__);
return -ENOENT;
@@ -2654,10 +2654,6 @@ int vcap_enable_lookups(struct vcap_control *vctrl, struct net_device *ndev,
if (admin->vinst || chain_id > admin->first_cid)
return -EFAULT;
- err = vctrl->ops->enable(ndev, admin, enable);
- if (err)
- return err;
-
if (chain_id) {
if (vcap_is_enabled(admin, ndev, cookie))
return -EADDRINUSE;
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.h b/drivers/net/ethernet/microchip/vcap/vcap_api.h
index 689c7270f2a8..c61f13a65030 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.h
@@ -259,11 +259,6 @@ struct vcap_operations {
(struct net_device *ndev,
struct vcap_admin *admin,
struct vcap_output_print *out);
- /* enable/disable the lookups in a vcap instance */
- int (*enable)
- (struct net_device *ndev,
- struct vcap_admin *admin,
- bool enable);
};
/* VCAP API Client control interface */
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
index cf594668d5d9..bef0b28a4a50 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
@@ -221,13 +221,6 @@ static int vcap_test_port_info(struct net_device *ndev,
return 0;
}
-static int vcap_test_enable(struct net_device *ndev,
- struct vcap_admin *admin,
- bool enable)
-{
- return 0;
-}
-
static struct vcap_operations test_callbacks = {
.validate_keyset = test_val_keyset,
.add_default_fields = test_add_def_fields,
@@ -238,7 +231,6 @@ static struct vcap_operations test_callbacks = {
.update = test_cache_update,
.move = test_cache_move,
.port_info = vcap_test_port_info,
- .enable = vcap_test_enable,
};
static struct vcap_control test_vctrl = {
@@ -253,6 +245,7 @@ static void vcap_test_api_init(struct vcap_admin *admin)
INIT_LIST_HEAD(&test_vctrl.list);
INIT_LIST_HEAD(&admin->list);
INIT_LIST_HEAD(&admin->rules);
+ INIT_LIST_HEAD(&admin->enabled);
list_add_tail(&admin->list, &test_vctrl.list);
memset(test_updateaddr, 0, sizeof(test_updateaddr));
test_updateaddridx = 0;
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
index 944de5cb9114..cc6a62338162 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
@@ -211,13 +211,6 @@ static int vcap_test_port_info(struct net_device *ndev,
return 0;
}
-static int vcap_test_enable(struct net_device *ndev,
- struct vcap_admin *admin,
- bool enable)
-{
- return 0;
-}
-
static struct vcap_operations test_callbacks = {
.validate_keyset = test_val_keyset,
.add_default_fields = test_add_def_fields,
@@ -228,7 +221,6 @@ static struct vcap_operations test_callbacks = {
.update = test_cache_update,
.move = test_cache_move,
.port_info = vcap_test_port_info,
- .enable = vcap_test_enable,
};
static struct vcap_control test_vctrl = {
@@ -243,6 +235,7 @@ static void vcap_test_api_init(struct vcap_admin *admin)
INIT_LIST_HEAD(&test_vctrl.list);
INIT_LIST_HEAD(&admin->list);
INIT_LIST_HEAD(&admin->rules);
+ INIT_LIST_HEAD(&admin->enabled);
list_add_tail(&admin->list, &test_vctrl.list);
memset(test_updateaddr, 0, sizeof(test_updateaddr));
test_updateaddridx = 0;
--
2.39.0
^ permalink raw reply related
* [PATCH net 6/8] net: microchip: vcap api: Check chains when adding a tc flower filter
From: Steen Hegelund @ 2022-12-21 13:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Steen Hegelund, UNGLinuxDriver, Randy Dunlap, Casper Andersson,
Russell King, Wan Jiabing, Nathan Huckleberry, linux-kernel,
netdev, linux-arm-kernel, Steen Hegelund, Daniel Machon,
Horatiu Vultur, Lars Povlsen, Dan Carpenter
In-Reply-To: <20221221132517.2699698-1-steen.hegelund@microchip.com>
This changes the way the chain information verified when adding a new tc
flower filter.
When adding a flower filter it is now checked that the filter contains a
goto action to one of the IS2 VCAP lookups, except for the last lookup
which may omit this goto action.
It is also checked if you attempt to add multiple matchall filters to
enable the same VCAP lookup. This will be rejected.
Fixes: 4426b78c626d ("net: lan966x: Add port keyset config and callback interface")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
.../microchip/lan966x/lan966x_tc_flower.c | 30 +++++-----
.../microchip/sparx5/sparx5_tc_flower.c | 28 +++++----
.../net/ethernet/microchip/vcap/vcap_api.c | 59 +++++++++++--------
.../ethernet/microchip/vcap/vcap_api_client.h | 2 +
.../ethernet/microchip/vcap/vcap_api_kunit.c | 8 +--
5 files changed, 72 insertions(+), 55 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c
index ba3fa917d6b7..b66a8725a071 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c
@@ -82,8 +82,8 @@ static int lan966x_tc_flower_use_dissectors(struct flow_cls_offload *f,
}
static int lan966x_tc_flower_action_check(struct vcap_control *vctrl,
- struct flow_cls_offload *fco,
- struct vcap_admin *admin)
+ struct net_device *dev,
+ struct flow_cls_offload *fco)
{
struct flow_rule *rule = flow_cls_offload_flow_rule(fco);
struct flow_action_entry *actent, *last_actent = NULL;
@@ -109,21 +109,23 @@ static int lan966x_tc_flower_action_check(struct vcap_control *vctrl,
last_actent = actent; /* Save last action for later check */
}
- /* Check that last action is a goto */
- if (last_actent->id != FLOW_ACTION_GOTO) {
+ /* Check that last action is a goto
+ * The last chain/lookup does not need to have goto action
+ */
+ if (last_actent->id == FLOW_ACTION_GOTO) {
+ /* Check if the destination chain is in one of the VCAPs */
+ if (!vcap_is_next_lookup(vctrl, fco->common.chain_index,
+ last_actent->chain_index)) {
+ NL_SET_ERR_MSG_MOD(fco->common.extack,
+ "Invalid goto chain");
+ return -EINVAL;
+ }
+ } else if (!vcap_is_last_chain(vctrl, fco->common.chain_index)) {
NL_SET_ERR_MSG_MOD(fco->common.extack,
"Last action must be 'goto'");
return -EINVAL;
}
- /* Check if the goto chain is in the next lookup */
- if (!vcap_is_next_lookup(vctrl, fco->common.chain_index,
- last_actent->chain_index)) {
- NL_SET_ERR_MSG_MOD(fco->common.extack,
- "Invalid goto chain");
- return -EINVAL;
- }
-
/* Catch unsupported combinations of actions */
if (action_mask & BIT(FLOW_ACTION_TRAP) &&
action_mask & BIT(FLOW_ACTION_ACCEPT)) {
@@ -145,8 +147,8 @@ static int lan966x_tc_flower_add(struct lan966x_port *port,
struct vcap_rule *vrule;
int err, idx;
- err = lan966x_tc_flower_action_check(port->lan966x->vcap_ctrl, f,
- admin);
+ err = lan966x_tc_flower_action_check(port->lan966x->vcap_ctrl,
+ port->dev, f);
if (err)
return err;
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
index 1ed304a816cc..986e41d3bb28 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
@@ -573,8 +573,8 @@ static int sparx5_tc_use_dissectors(struct flow_cls_offload *fco,
}
static int sparx5_tc_flower_action_check(struct vcap_control *vctrl,
- struct flow_cls_offload *fco,
- struct vcap_admin *admin)
+ struct net_device *ndev,
+ struct flow_cls_offload *fco)
{
struct flow_rule *rule = flow_cls_offload_flow_rule(fco);
struct flow_action_entry *actent, *last_actent = NULL;
@@ -600,21 +600,23 @@ static int sparx5_tc_flower_action_check(struct vcap_control *vctrl,
last_actent = actent; /* Save last action for later check */
}
- /* Check that last action is a goto */
- if (last_actent->id != FLOW_ACTION_GOTO) {
+ /* Check if last action is a goto
+ * The last chain/lookup does not need to have a goto action
+ */
+ if (last_actent->id == FLOW_ACTION_GOTO) {
+ /* Check if the destination chain is in one of the VCAPs */
+ if (!vcap_is_next_lookup(vctrl, fco->common.chain_index,
+ last_actent->chain_index)) {
+ NL_SET_ERR_MSG_MOD(fco->common.extack,
+ "Invalid goto chain");
+ return -EINVAL;
+ }
+ } else if (!vcap_is_last_chain(vctrl, fco->common.chain_index)) {
NL_SET_ERR_MSG_MOD(fco->common.extack,
"Last action must be 'goto'");
return -EINVAL;
}
- /* Check if the goto chain is in the next lookup */
- if (!vcap_is_next_lookup(vctrl, fco->common.chain_index,
- last_actent->chain_index)) {
- NL_SET_ERR_MSG_MOD(fco->common.extack,
- "Invalid goto chain");
- return -EINVAL;
- }
-
/* Catch unsupported combinations of actions */
if (action_mask & BIT(FLOW_ACTION_TRAP) &&
action_mask & BIT(FLOW_ACTION_ACCEPT)) {
@@ -833,7 +835,7 @@ static int sparx5_tc_flower_replace(struct net_device *ndev,
vctrl = port->sparx5->vcap_ctrl;
- err = sparx5_tc_flower_action_check(vctrl, fco, admin);
+ err = sparx5_tc_flower_action_check(vctrl, ndev, fco);
if (err)
return err;
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index 12807bc0d385..d438554355e4 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -1553,39 +1553,31 @@ struct vcap_admin *vcap_find_admin(struct vcap_control *vctrl, int cid)
}
EXPORT_SYMBOL_GPL(vcap_find_admin);
-/* Is the next chain id in the following lookup, possible in another VCAP */
-bool vcap_is_next_lookup(struct vcap_control *vctrl, int cur_cid, int next_cid)
+/* Is the next chain id in one of the following lookups
+ * For now this does not support filters linked to other filters using
+ * keys and actions. That will be added later.
+ */
+bool vcap_is_next_lookup(struct vcap_control *vctrl, int src_cid, int dst_cid)
{
- struct vcap_admin *admin, *next_admin;
- int lookup, next_lookup;
+ struct vcap_admin *admin;
+ int next_cid;
- /* The offset must be at least one lookup */
- if (next_cid < cur_cid + VCAP_CID_LOOKUP_SIZE)
+ if (vcap_api_check(vctrl))
return false;
- if (vcap_api_check(vctrl))
+ /* The offset must be at least one lookup, round up */
+ next_cid = src_cid + VCAP_CID_LOOKUP_SIZE;
+ next_cid /= VCAP_CID_LOOKUP_SIZE;
+ next_cid *= VCAP_CID_LOOKUP_SIZE;
+
+ if (dst_cid < next_cid)
return false;
- admin = vcap_find_admin(vctrl, cur_cid);
+ admin = vcap_find_admin(vctrl, dst_cid);
if (!admin)
return false;
- /* If no VCAP contains the next chain, the next chain must be beyond
- * the last chain in the current VCAP
- */
- next_admin = vcap_find_admin(vctrl, next_cid);
- if (!next_admin)
- return next_cid > admin->last_cid;
-
- lookup = vcap_chain_id_to_lookup(admin, cur_cid);
- next_lookup = vcap_chain_id_to_lookup(next_admin, next_cid);
-
- /* Next lookup must be the following lookup */
- if (admin == next_admin || admin->vtype == next_admin->vtype)
- return next_lookup == lookup + 1;
-
- /* Must be the first lookup in the next VCAP instance */
- return next_lookup == 0;
+ return true;
}
EXPORT_SYMBOL_GPL(vcap_is_next_lookup);
@@ -2716,6 +2708,25 @@ int vcap_enable_lookups(struct vcap_control *vctrl, struct net_device *ndev,
}
EXPORT_SYMBOL_GPL(vcap_enable_lookups);
+/* Is this chain id the last lookup of all VCAPs */
+bool vcap_is_last_chain(struct vcap_control *vctrl, int cid)
+{
+ struct vcap_admin *admin;
+ int lookup;
+
+ if (vcap_api_check(vctrl))
+ return false;
+
+ admin = vcap_find_admin(vctrl, cid);
+ if (!admin)
+ return false;
+
+ /* This must be the last lookup in this VCAP type */
+ lookup = vcap_chain_id_to_lookup(admin, cid);
+ return lookup == admin->lookups - 1;
+}
+EXPORT_SYMBOL_GPL(vcap_is_last_chain);
+
/* Set a rule counter id (for certain vcaps only) */
void vcap_rule_set_counter_id(struct vcap_rule *rule, u32 counter_id)
{
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
index e07dc8d3c639..f44228436051 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
@@ -217,6 +217,8 @@ const struct vcap_field *vcap_lookup_keyfield(struct vcap_rule *rule,
int vcap_lookup_rule_by_cookie(struct vcap_control *vctrl, u64 cookie);
/* Is the next chain id in the following lookup, possible in another VCAP */
bool vcap_is_next_lookup(struct vcap_control *vctrl, int cur_cid, int next_cid);
+/* Is this chain id the last lookup of all VCAPs */
+bool vcap_is_last_chain(struct vcap_control *vctrl, int cid);
/* Provide all rules via a callback interface */
int vcap_rule_iter(struct vcap_control *vctrl,
int (*callback)(void *, struct vcap_rule *), void *arg);
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
index cc6a62338162..fdef9102a9b3 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
@@ -1865,7 +1865,7 @@ static void vcap_api_next_lookup_basic_test(struct kunit *test)
ret = vcap_is_next_lookup(&test_vctrl, 8300000, 8301000);
KUNIT_EXPECT_EQ(test, false, ret);
ret = vcap_is_next_lookup(&test_vctrl, 8300000, 8401000);
- KUNIT_EXPECT_EQ(test, true, ret);
+ KUNIT_EXPECT_EQ(test, false, ret);
}
static void vcap_api_next_lookup_advanced_test(struct kunit *test)
@@ -1926,9 +1926,9 @@ static void vcap_api_next_lookup_advanced_test(struct kunit *test)
ret = vcap_is_next_lookup(&test_vctrl, 1100000, 1201000);
KUNIT_EXPECT_EQ(test, true, ret);
ret = vcap_is_next_lookup(&test_vctrl, 1100000, 1301000);
- KUNIT_EXPECT_EQ(test, false, ret);
+ KUNIT_EXPECT_EQ(test, true, ret);
ret = vcap_is_next_lookup(&test_vctrl, 1100000, 8101000);
- KUNIT_EXPECT_EQ(test, false, ret);
+ KUNIT_EXPECT_EQ(test, true, ret);
ret = vcap_is_next_lookup(&test_vctrl, 1300000, 1401000);
KUNIT_EXPECT_EQ(test, true, ret);
ret = vcap_is_next_lookup(&test_vctrl, 1400000, 1501000);
@@ -1944,7 +1944,7 @@ static void vcap_api_next_lookup_advanced_test(struct kunit *test)
ret = vcap_is_next_lookup(&test_vctrl, 8300000, 8301000);
KUNIT_EXPECT_EQ(test, false, ret);
ret = vcap_is_next_lookup(&test_vctrl, 8300000, 8401000);
- KUNIT_EXPECT_EQ(test, true, ret);
+ KUNIT_EXPECT_EQ(test, false, ret);
}
static void vcap_api_filter_unsupported_keys_test(struct kunit *test)
--
2.39.0
^ permalink raw reply related
* [PATCH net 5/8] net: microchip: vcap api: Use src and dst chain id to chain VCAP lookups
From: Steen Hegelund @ 2022-12-21 13:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Steen Hegelund, UNGLinuxDriver, Randy Dunlap, Casper Andersson,
Russell King, Wan Jiabing, Nathan Huckleberry, linux-kernel,
netdev, linux-arm-kernel, Steen Hegelund, Daniel Machon,
Horatiu Vultur, Lars Povlsen, Dan Carpenter
In-Reply-To: <20221221132517.2699698-1-steen.hegelund@microchip.com>
This adds both the source and destination chain id to the information kept
for enabled port lookups.
This allows enabling and disabling a chain of lookups by walking the chain
information for a port.
This changes the way that VCAP lookups are enabled from userspace: instead
of one matchall rule that enables all the 4 Sparx5 IS2 lookups, you need a
matchall rule per lookup.
In practice that is done by adding one matchall rule in chain 0 to goto IS2
Lookup 0, and then for each lookup you add a rule per lookup (low priority)
that does a goto to the next lookup chain.
Examples:
If you want IS2 Lookup 0 to be enabled you add the same matchall filter as
before:
tc filter add dev eth12 ingress chain 0 prio 1000 handle 1000 matchall \
skip_sw action goto chain 8000000
If you also want to enable lookup 1 to 3 in IS2 and chain them you need
to add the following matchall filters:
tc filter add dev eth12 ingress chain 8000000 prio 1000 handle 1000 \
matchall skip_sw action goto chain 8100000
tc filter add dev eth12 ingress chain 8100000 prio 1000 handle 1000 \
matchall skip_sw action goto chain 8200000
tc filter add dev eth12 ingress chain 8200000 prio 1000 handle 1000 \
matchall skip_sw action goto chain 8300000
Fixes: 4426b78c626d ("net: lan966x: Add port keyset config and callback interface")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
.../ethernet/microchip/lan966x/lan966x_goto.c | 10 +-
.../ethernet/microchip/lan966x/lan966x_main.h | 3 +-
.../microchip/lan966x/lan966x_tc_matchall.c | 16 +--
.../microchip/sparx5/sparx5_tc_matchall.c | 16 +--
.../net/ethernet/microchip/vcap/vcap_api.c | 126 ++++++++++--------
.../ethernet/microchip/vcap/vcap_api_client.h | 5 +-
6 files changed, 92 insertions(+), 84 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_goto.c b/drivers/net/ethernet/microchip/lan966x/lan966x_goto.c
index bf0cfe24a8fc..9b18156eea1a 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_goto.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_goto.c
@@ -4,7 +4,7 @@
#include "vcap_api_client.h"
int lan966x_goto_port_add(struct lan966x_port *port,
- struct flow_action_entry *act,
+ int from_cid, int to_cid,
unsigned long goto_id,
struct netlink_ext_ack *extack)
{
@@ -12,7 +12,7 @@ int lan966x_goto_port_add(struct lan966x_port *port,
int err;
err = vcap_enable_lookups(lan966x->vcap_ctrl, port->dev,
- act->chain_index, goto_id,
+ from_cid, to_cid, goto_id,
true);
if (err == -EFAULT) {
NL_SET_ERR_MSG_MOD(extack, "Unsupported goto chain");
@@ -29,8 +29,6 @@ int lan966x_goto_port_add(struct lan966x_port *port,
return err;
}
- port->tc.goto_id = goto_id;
-
return 0;
}
@@ -41,14 +39,12 @@ int lan966x_goto_port_del(struct lan966x_port *port,
struct lan966x *lan966x = port->lan966x;
int err;
- err = vcap_enable_lookups(lan966x->vcap_ctrl, port->dev, 0,
+ err = vcap_enable_lookups(lan966x->vcap_ctrl, port->dev, 0, 0,
goto_id, false);
if (err) {
NL_SET_ERR_MSG_MOD(extack, "Could not disable VCAP lookups");
return err;
}
- port->tc.goto_id = 0;
-
return 0;
}
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 3491f1961835..0106f9487cbe 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -332,7 +332,6 @@ struct lan966x_port_tc {
unsigned long police_id;
unsigned long ingress_mirror_id;
unsigned long egress_mirror_id;
- unsigned long goto_id;
struct flow_stats police_stat;
struct flow_stats mirror_stat;
};
@@ -607,7 +606,7 @@ int lan966x_tc_flower(struct lan966x_port *port,
struct flow_cls_offload *f);
int lan966x_goto_port_add(struct lan966x_port *port,
- struct flow_action_entry *act,
+ int from_cid, int to_cid,
unsigned long goto_id,
struct netlink_ext_ack *extack);
int lan966x_goto_port_del(struct lan966x_port *port,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_matchall.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_matchall.c
index a539abaad9b6..20627323d656 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_matchall.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_matchall.c
@@ -24,7 +24,8 @@ static int lan966x_tc_matchall_add(struct lan966x_port *port,
return lan966x_mirror_port_add(port, act, f->cookie,
ingress, f->common.extack);
case FLOW_ACTION_GOTO:
- return lan966x_goto_port_add(port, act, f->cookie,
+ return lan966x_goto_port_add(port, f->common.chain_index,
+ act->chain_index, f->cookie,
f->common.extack);
default:
NL_SET_ERR_MSG_MOD(f->common.extack,
@@ -46,13 +47,8 @@ static int lan966x_tc_matchall_del(struct lan966x_port *port,
f->cookie == port->tc.egress_mirror_id) {
return lan966x_mirror_port_del(port, ingress,
f->common.extack);
- } else if (f->cookie == port->tc.goto_id) {
- return lan966x_goto_port_del(port, f->cookie,
- f->common.extack);
} else {
- NL_SET_ERR_MSG_MOD(f->common.extack,
- "Unsupported action");
- return -EOPNOTSUPP;
+ return lan966x_goto_port_del(port, f->cookie, f->common.extack);
}
return 0;
@@ -80,12 +76,6 @@ int lan966x_tc_matchall(struct lan966x_port *port,
struct tc_cls_matchall_offload *f,
bool ingress)
{
- if (!tc_cls_can_offload_and_chain0(port->dev, &f->common)) {
- NL_SET_ERR_MSG_MOD(f->common.extack,
- "Only chain zero is supported");
- return -EOPNOTSUPP;
- }
-
switch (f->command) {
case TC_CLSMATCHALL_REPLACE:
return lan966x_tc_matchall_add(port, f, ingress);
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
index 30dd61e5d150..d88a93f22606 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
@@ -31,6 +31,7 @@ static int sparx5_tc_matchall_replace(struct net_device *ndev,
switch (action->id) {
case FLOW_ACTION_GOTO:
err = vcap_enable_lookups(sparx5->vcap_ctrl, ndev,
+ tmo->common.chain_index,
action->chain_index, tmo->cookie,
true);
if (err == -EFAULT) {
@@ -43,6 +44,11 @@ static int sparx5_tc_matchall_replace(struct net_device *ndev,
"VCAP already enabled");
return -EOPNOTSUPP;
}
+ if (err == -EADDRNOTAVAIL) {
+ NL_SET_ERR_MSG_MOD(tmo->common.extack,
+ "Already matching this chain");
+ return -EOPNOTSUPP;
+ }
if (err) {
NL_SET_ERR_MSG_MOD(tmo->common.extack,
"Could not enable VCAP lookups");
@@ -66,8 +72,8 @@ static int sparx5_tc_matchall_destroy(struct net_device *ndev,
sparx5 = port->sparx5;
if (!tmo->rule && tmo->cookie) {
- err = vcap_enable_lookups(sparx5->vcap_ctrl, ndev, 0,
- tmo->cookie, false);
+ err = vcap_enable_lookups(sparx5->vcap_ctrl, ndev,
+ 0, 0, tmo->cookie, false);
if (err)
return err;
return 0;
@@ -80,12 +86,6 @@ int sparx5_tc_matchall(struct net_device *ndev,
struct tc_cls_matchall_offload *tmo,
bool ingress)
{
- if (!tc_cls_can_offload_and_chain0(ndev, &tmo->common)) {
- NL_SET_ERR_MSG_MOD(tmo->common.extack,
- "Only chain zero is supported");
- return -EOPNOTSUPP;
- }
-
switch (tmo->command) {
case TC_CLSMATCHALL_REPLACE:
return sparx5_tc_matchall_replace(ndev, tmo, ingress);
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index 486ab2c2baaa..12807bc0d385 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -37,11 +37,13 @@ struct vcap_rule_move {
int count; /* blocksize of addresses to move */
};
-/* Stores the filter cookie that enabled the port */
+/* Stores the filter cookie and chain id that enabled the port */
struct vcap_enabled_port {
struct list_head list; /* for insertion in enabled ports list */
struct net_device *ndev; /* the enabled port */
unsigned long cookie; /* filter that enabled the port */
+ int src_cid; /* source chain id */
+ int dst_cid; /* destination chain id */
};
void vcap_iter_set(struct vcap_stream_iter *itr, int sw_width,
@@ -1930,6 +1932,21 @@ static void vcap_move_rules(struct vcap_rule_internal *ri,
move->offset, move->count);
}
+/* Check if the chain is already used to enable a VCAP lookup for this port */
+static bool vcap_is_chain_used(struct vcap_control *vctrl,
+ struct net_device *ndev, int src_cid)
+{
+ struct vcap_enabled_port *eport;
+ struct vcap_admin *admin;
+
+ list_for_each_entry(admin, &vctrl->list, list)
+ list_for_each_entry(eport, &admin->enabled, list)
+ if (eport->src_cid == src_cid && eport->ndev == ndev)
+ return true;
+
+ return false;
+}
+
/* Encode and write a validated rule to the VCAP */
int vcap_add_rule(struct vcap_rule *rule)
{
@@ -2593,23 +2610,33 @@ void vcap_set_tc_exterr(struct flow_cls_offload *fco, struct vcap_rule *vrule)
EXPORT_SYMBOL_GPL(vcap_set_tc_exterr);
/* Check if this port is already enabled for this VCAP instance */
-static bool vcap_is_enabled(struct vcap_admin *admin, struct net_device *ndev,
- unsigned long cookie)
+static bool vcap_is_enabled(struct vcap_control *vctrl, struct net_device *ndev,
+ int dst_cid)
{
struct vcap_enabled_port *eport;
+ struct vcap_admin *admin;
- list_for_each_entry(eport, &admin->enabled, list)
- if (eport->cookie == cookie || eport->ndev == ndev)
- return true;
+ list_for_each_entry(admin, &vctrl->list, list)
+ list_for_each_entry(eport, &admin->enabled, list)
+ if (eport->dst_cid == dst_cid && eport->ndev == ndev)
+ return true;
return false;
}
-/* Enable this port for this VCAP instance */
-static int vcap_enable(struct vcap_admin *admin, struct net_device *ndev,
- unsigned long cookie)
+/* Enable this port and chain id in a VCAP instance */
+static int vcap_enable(struct vcap_control *vctrl, struct net_device *ndev,
+ unsigned long cookie, int src_cid, int dst_cid)
{
struct vcap_enabled_port *eport;
+ struct vcap_admin *admin;
+
+ if (src_cid >= dst_cid)
+ return -EFAULT;
+
+ admin = vcap_find_admin(vctrl, dst_cid);
+ if (!admin)
+ return -ENOENT;
eport = kzalloc(sizeof(*eport), GFP_KERNEL);
if (!eport)
@@ -2617,48 +2644,49 @@ static int vcap_enable(struct vcap_admin *admin, struct net_device *ndev,
eport->ndev = ndev;
eport->cookie = cookie;
+ eport->src_cid = src_cid;
+ eport->dst_cid = dst_cid;
+ mutex_lock(&admin->lock);
list_add_tail(&eport->list, &admin->enabled);
+ mutex_unlock(&admin->lock);
return 0;
}
-/* Disable this port for this VCAP instance */
-static int vcap_disable(struct vcap_admin *admin, struct net_device *ndev,
+/* Disable this port and chain id for a VCAP instance */
+static int vcap_disable(struct vcap_control *vctrl, struct net_device *ndev,
unsigned long cookie)
{
- struct vcap_enabled_port *eport;
+ struct vcap_enabled_port *elem, *eport = NULL;
+ struct vcap_admin *found = NULL, *admin;
- list_for_each_entry(eport, &admin->enabled, list) {
- if (eport->cookie == cookie && eport->ndev == ndev) {
- list_del(&eport->list);
- kfree(eport);
- return 0;
+ list_for_each_entry(admin, &vctrl->list, list) {
+ list_for_each_entry(elem, &admin->enabled, list) {
+ if (elem->cookie == cookie && elem->ndev == ndev) {
+ eport = elem;
+ found = admin;
+ break;
+ }
}
+ if (eport)
+ break;
}
- return -ENOENT;
-}
-
-/* Find the VCAP instance that enabled the port using a specific filter */
-static struct vcap_admin *vcap_find_admin_by_cookie(struct vcap_control *vctrl,
- unsigned long cookie)
-{
- struct vcap_enabled_port *eport;
- struct vcap_admin *admin;
-
- list_for_each_entry(admin, &vctrl->list, list)
- list_for_each_entry(eport, &admin->enabled, list)
- if (eport->cookie == cookie)
- return admin;
+ if (!eport)
+ return -ENOENT;
- return NULL;
+ mutex_lock(&found->lock);
+ list_del(&eport->list);
+ mutex_unlock(&found->lock);
+ kfree(eport);
+ return 0;
}
-/* Enable/Disable the VCAP instance lookups. Chain id 0 means disable */
+/* Enable/Disable the VCAP instance lookups */
int vcap_enable_lookups(struct vcap_control *vctrl, struct net_device *ndev,
- int chain_id, unsigned long cookie, bool enable)
+ int src_cid, int dst_cid, unsigned long cookie,
+ bool enable)
{
- struct vcap_admin *admin;
int err;
err = vcap_api_check(vctrl);
@@ -2668,29 +2696,23 @@ int vcap_enable_lookups(struct vcap_control *vctrl, struct net_device *ndev,
if (!ndev)
return -ENODEV;
- if (chain_id)
- admin = vcap_find_admin(vctrl, chain_id);
- else
- admin = vcap_find_admin_by_cookie(vctrl, cookie);
- if (!admin)
- return -ENOENT;
-
- /* first instance and first chain */
- if (admin->vinst || chain_id > admin->first_cid)
+ /* Source and destination must be the first chain in a lookup */
+ if (src_cid % VCAP_CID_LOOKUP_SIZE)
+ return -EFAULT;
+ if (dst_cid % VCAP_CID_LOOKUP_SIZE)
return -EFAULT;
- if (chain_id) {
- if (vcap_is_enabled(admin, ndev, cookie))
+ if (enable) {
+ if (vcap_is_enabled(vctrl, ndev, dst_cid))
return -EADDRINUSE;
- mutex_lock(&admin->lock);
- vcap_enable(admin, ndev, cookie);
+ if (vcap_is_chain_used(vctrl, ndev, src_cid))
+ return -EADDRNOTAVAIL;
+ err = vcap_enable(vctrl, ndev, cookie, src_cid, dst_cid);
} else {
- mutex_lock(&admin->lock);
- vcap_disable(admin, ndev, cookie);
+ err = vcap_disable(vctrl, ndev, cookie);
}
- mutex_unlock(&admin->lock);
- return 0;
+ return err;
}
EXPORT_SYMBOL_GPL(vcap_enable_lookups);
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
index 0319866f9c94..e07dc8d3c639 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_client.h
@@ -148,9 +148,10 @@ struct vcap_counter {
bool sticky;
};
-/* Enable/Disable the VCAP instance lookups. Chain id 0 means disable */
+/* Enable/Disable the VCAP instance lookups */
int vcap_enable_lookups(struct vcap_control *vctrl, struct net_device *ndev,
- int chain_id, unsigned long cookie, bool enable);
+ int from_cid, int to_cid, unsigned long cookie,
+ bool enable);
/* VCAP rule operations */
/* Allocate a rule and fill in the basic information */
--
2.39.0
^ permalink raw reply related
* [PATCH net 4/8] net: microchip: vcap api: Convert multi-word keys/actions when encoding
From: Steen Hegelund @ 2022-12-21 13:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Steen Hegelund, UNGLinuxDriver, Randy Dunlap, Casper Andersson,
Russell King, Wan Jiabing, Nathan Huckleberry, linux-kernel,
netdev, linux-arm-kernel, Steen Hegelund, Daniel Machon,
Horatiu Vultur, Lars Povlsen, Dan Carpenter
In-Reply-To: <20221221132517.2699698-1-steen.hegelund@microchip.com>
The conversion to the platform specific multi-word format is moved from the
key/action add functions to the encoding key/action.
This allows rules that are disabled (not in VCAP HW) to use the same format
for keys/actions as rules that have just been read from VCAP HW.
Fixes: 4426b78c626d ("net: lan966x: Add port keyset config and callback interface")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
.../net/ethernet/microchip/vcap/vcap_api.c | 243 ++++++++++--------
1 file changed, 134 insertions(+), 109 deletions(-)
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index 9de5367fde42..486ab2c2baaa 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -508,10 +508,133 @@ static void vcap_encode_keyfield_typegroups(struct vcap_control *vctrl,
vcap_encode_typegroups(cache->maskstream, sw_width, tgt, true);
}
+/* Copy data from src to dst but reverse the data in chunks of 32bits.
+ * For example if src is 00:11:22:33:44:55 where 55 is LSB the dst will
+ * have the value 22:33:44:55:00:11.
+ */
+static void vcap_copy_to_w32be(u8 *dst, const u8 *src, int size)
+{
+ for (int idx = 0; idx < size; ++idx) {
+ int first_byte_index = 0;
+ int nidx;
+
+ first_byte_index = size - (((idx >> 2) + 1) << 2);
+ if (first_byte_index < 0)
+ first_byte_index = 0;
+ nidx = idx + first_byte_index - (idx & ~0x3);
+ dst[nidx] = src[idx];
+ }
+}
+
+static void
+vcap_copy_from_client_keyfield(struct vcap_rule *rule,
+ struct vcap_client_keyfield *dst,
+ const struct vcap_client_keyfield *src)
+{
+ struct vcap_rule_internal *ri = to_intrule(rule);
+ const struct vcap_client_keyfield_data *sdata;
+ struct vcap_client_keyfield_data *ddata;
+ int size;
+
+ dst->ctrl.type = src->ctrl.type;
+ dst->ctrl.key = src->ctrl.key;
+ INIT_LIST_HEAD(&dst->ctrl.list);
+ sdata = &src->data;
+ ddata = &dst->data;
+
+ if (!ri->admin->w32be) {
+ memcpy(ddata, sdata, sizeof(dst->data));
+ return;
+ }
+
+ size = keyfield_size_table[dst->ctrl.type] / 2;
+
+ switch (dst->ctrl.type) {
+ case VCAP_FIELD_BIT:
+ case VCAP_FIELD_U32:
+ memcpy(ddata, sdata, sizeof(dst->data));
+ break;
+ case VCAP_FIELD_U48:
+ vcap_copy_to_w32be(ddata->u48.value, src->data.u48.value, size);
+ vcap_copy_to_w32be(ddata->u48.mask, src->data.u48.mask, size);
+ break;
+ case VCAP_FIELD_U56:
+ vcap_copy_to_w32be(ddata->u56.value, sdata->u56.value, size);
+ vcap_copy_to_w32be(ddata->u56.mask, sdata->u56.mask, size);
+ break;
+ case VCAP_FIELD_U64:
+ vcap_copy_to_w32be(ddata->u64.value, sdata->u64.value, size);
+ vcap_copy_to_w32be(ddata->u64.mask, sdata->u64.mask, size);
+ break;
+ case VCAP_FIELD_U72:
+ vcap_copy_to_w32be(ddata->u72.value, sdata->u72.value, size);
+ vcap_copy_to_w32be(ddata->u72.mask, sdata->u72.mask, size);
+ break;
+ case VCAP_FIELD_U112:
+ vcap_copy_to_w32be(ddata->u112.value, sdata->u112.value, size);
+ vcap_copy_to_w32be(ddata->u112.mask, sdata->u112.mask, size);
+ break;
+ case VCAP_FIELD_U128:
+ vcap_copy_to_w32be(ddata->u128.value, sdata->u128.value, size);
+ vcap_copy_to_w32be(ddata->u128.mask, sdata->u128.mask, size);
+ break;
+ }
+}
+
+static void
+vcap_copy_from_client_actionfield(struct vcap_rule *rule,
+ struct vcap_client_actionfield *dst,
+ const struct vcap_client_actionfield *src)
+{
+ struct vcap_rule_internal *ri = to_intrule(rule);
+ const struct vcap_client_actionfield_data *sdata;
+ struct vcap_client_actionfield_data *ddata;
+ int size;
+
+ dst->ctrl.type = src->ctrl.type;
+ dst->ctrl.action = src->ctrl.action;
+ INIT_LIST_HEAD(&dst->ctrl.list);
+ sdata = &src->data;
+ ddata = &dst->data;
+
+ if (!ri->admin->w32be) {
+ memcpy(ddata, sdata, sizeof(dst->data));
+ return;
+ }
+
+ size = actionfield_size_table[dst->ctrl.type];
+
+ switch (dst->ctrl.type) {
+ case VCAP_FIELD_BIT:
+ case VCAP_FIELD_U32:
+ memcpy(ddata, sdata, sizeof(dst->data));
+ break;
+ case VCAP_FIELD_U48:
+ vcap_copy_to_w32be(ddata->u48.value, sdata->u48.value, size);
+ break;
+ case VCAP_FIELD_U56:
+ vcap_copy_to_w32be(ddata->u56.value, sdata->u56.value, size);
+ break;
+ case VCAP_FIELD_U64:
+ vcap_copy_to_w32be(ddata->u64.value, sdata->u64.value, size);
+ break;
+ case VCAP_FIELD_U72:
+ vcap_copy_to_w32be(ddata->u72.value, sdata->u72.value, size);
+ break;
+ case VCAP_FIELD_U112:
+ vcap_copy_to_w32be(ddata->u112.value, sdata->u112.value, size);
+ break;
+ case VCAP_FIELD_U128:
+ vcap_copy_to_w32be(ddata->u128.value, sdata->u128.value, size);
+ break;
+ }
+}
+
static int vcap_encode_rule_keyset(struct vcap_rule_internal *ri)
{
const struct vcap_client_keyfield *ckf;
const struct vcap_typegroup *tg_table;
+ struct vcap_client_keyfield tempkf;
const struct vcap_field *kf_table;
int keyset_size;
@@ -552,7 +675,9 @@ static int vcap_encode_rule_keyset(struct vcap_rule_internal *ri)
__func__, __LINE__, ckf->ctrl.key);
return -EINVAL;
}
- vcap_encode_keyfield(ri, ckf, &kf_table[ckf->ctrl.key], tg_table);
+ vcap_copy_from_client_keyfield(&ri->data, &tempkf, ckf);
+ vcap_encode_keyfield(ri, &tempkf, &kf_table[ckf->ctrl.key],
+ tg_table);
}
/* Add typegroup bits to the key/mask bitstreams */
vcap_encode_keyfield_typegroups(ri->vctrl, ri, tg_table);
@@ -667,6 +792,7 @@ static int vcap_encode_rule_actionset(struct vcap_rule_internal *ri)
{
const struct vcap_client_actionfield *caf;
const struct vcap_typegroup *tg_table;
+ struct vcap_client_actionfield tempaf;
const struct vcap_field *af_table;
int actionset_size;
@@ -707,8 +833,9 @@ static int vcap_encode_rule_actionset(struct vcap_rule_internal *ri)
__func__, __LINE__, caf->ctrl.action);
return -EINVAL;
}
- vcap_encode_actionfield(ri, caf, &af_table[caf->ctrl.action],
- tg_table);
+ vcap_copy_from_client_actionfield(&ri->data, &tempaf, caf);
+ vcap_encode_actionfield(ri, &tempaf,
+ &af_table[caf->ctrl.action], tg_table);
}
/* Add typegroup bits to the entry bitstreams */
vcap_encode_actionfield_typegroups(ri, tg_table);
@@ -2140,69 +2267,6 @@ const struct vcap_field *vcap_lookup_keyfield(struct vcap_rule *rule,
}
EXPORT_SYMBOL_GPL(vcap_lookup_keyfield);
-/* Copy data from src to dst but reverse the data in chunks of 32bits.
- * For example if src is 00:11:22:33:44:55 where 55 is LSB the dst will
- * have the value 22:33:44:55:00:11.
- */
-static void vcap_copy_to_w32be(u8 *dst, u8 *src, int size)
-{
- for (int idx = 0; idx < size; ++idx) {
- int first_byte_index = 0;
- int nidx;
-
- first_byte_index = size - (((idx >> 2) + 1) << 2);
- if (first_byte_index < 0)
- first_byte_index = 0;
- nidx = idx + first_byte_index - (idx & ~0x3);
- dst[nidx] = src[idx];
- }
-}
-
-static void vcap_copy_from_client_keyfield(struct vcap_rule *rule,
- struct vcap_client_keyfield *field,
- struct vcap_client_keyfield_data *data)
-{
- struct vcap_rule_internal *ri = to_intrule(rule);
- int size;
-
- if (!ri->admin->w32be) {
- memcpy(&field->data, data, sizeof(field->data));
- return;
- }
-
- size = keyfield_size_table[field->ctrl.type] / 2;
- switch (field->ctrl.type) {
- case VCAP_FIELD_BIT:
- case VCAP_FIELD_U32:
- memcpy(&field->data, data, sizeof(field->data));
- break;
- case VCAP_FIELD_U48:
- vcap_copy_to_w32be(field->data.u48.value, data->u48.value, size);
- vcap_copy_to_w32be(field->data.u48.mask, data->u48.mask, size);
- break;
- case VCAP_FIELD_U56:
- vcap_copy_to_w32be(field->data.u56.value, data->u56.value, size);
- vcap_copy_to_w32be(field->data.u56.mask, data->u56.mask, size);
- break;
- case VCAP_FIELD_U64:
- vcap_copy_to_w32be(field->data.u64.value, data->u64.value, size);
- vcap_copy_to_w32be(field->data.u64.mask, data->u64.mask, size);
- break;
- case VCAP_FIELD_U72:
- vcap_copy_to_w32be(field->data.u72.value, data->u72.value, size);
- vcap_copy_to_w32be(field->data.u72.mask, data->u72.mask, size);
- break;
- case VCAP_FIELD_U112:
- vcap_copy_to_w32be(field->data.u112.value, data->u112.value, size);
- vcap_copy_to_w32be(field->data.u112.mask, data->u112.mask, size);
- break;
- case VCAP_FIELD_U128:
- vcap_copy_to_w32be(field->data.u128.value, data->u128.value, size);
- vcap_copy_to_w32be(field->data.u128.mask, data->u128.mask, size);
- break;
- }
-}
-
/* Check if the keyfield is already in the rule */
static bool vcap_keyfield_unique(struct vcap_rule *rule,
enum vcap_key_field key)
@@ -2260,9 +2324,9 @@ static int vcap_rule_add_key(struct vcap_rule *rule,
field = kzalloc(sizeof(*field), GFP_KERNEL);
if (!field)
return -ENOMEM;
+ memcpy(&field->data, data, sizeof(field->data));
field->ctrl.key = key;
field->ctrl.type = ftype;
- vcap_copy_from_client_keyfield(rule, field, data);
list_add_tail(&field->ctrl.list, &rule->keyfields);
return 0;
}
@@ -2370,45 +2434,6 @@ vcap_find_actionfield(struct vcap_rule *rule, enum vcap_action_field act)
return NULL;
}
-static void vcap_copy_from_client_actionfield(struct vcap_rule *rule,
- struct vcap_client_actionfield *field,
- struct vcap_client_actionfield_data *data)
-{
- struct vcap_rule_internal *ri = to_intrule(rule);
- int size;
-
- if (!ri->admin->w32be) {
- memcpy(&field->data, data, sizeof(field->data));
- return;
- }
-
- size = actionfield_size_table[field->ctrl.type];
- switch (field->ctrl.type) {
- case VCAP_FIELD_BIT:
- case VCAP_FIELD_U32:
- memcpy(&field->data, data, sizeof(field->data));
- break;
- case VCAP_FIELD_U48:
- vcap_copy_to_w32be(field->data.u48.value, data->u48.value, size);
- break;
- case VCAP_FIELD_U56:
- vcap_copy_to_w32be(field->data.u56.value, data->u56.value, size);
- break;
- case VCAP_FIELD_U64:
- vcap_copy_to_w32be(field->data.u64.value, data->u64.value, size);
- break;
- case VCAP_FIELD_U72:
- vcap_copy_to_w32be(field->data.u72.value, data->u72.value, size);
- break;
- case VCAP_FIELD_U112:
- vcap_copy_to_w32be(field->data.u112.value, data->u112.value, size);
- break;
- case VCAP_FIELD_U128:
- vcap_copy_to_w32be(field->data.u128.value, data->u128.value, size);
- break;
- }
-}
-
/* Check if the actionfield is already in the rule */
static bool vcap_actionfield_unique(struct vcap_rule *rule,
enum vcap_action_field act)
@@ -2466,9 +2491,9 @@ static int vcap_rule_add_action(struct vcap_rule *rule,
field = kzalloc(sizeof(*field), GFP_KERNEL);
if (!field)
return -ENOMEM;
+ memcpy(&field->data, data, sizeof(field->data));
field->ctrl.action = action;
field->ctrl.type = ftype;
- vcap_copy_from_client_actionfield(rule, field, data);
list_add_tail(&field->ctrl.list, &rule->actionfields);
return 0;
}
@@ -2745,7 +2770,7 @@ static int vcap_rule_mod_key(struct vcap_rule *rule,
field = vcap_find_keyfield(rule, key);
if (!field)
return vcap_rule_add_key(rule, key, ftype, data);
- vcap_copy_from_client_keyfield(rule, field, data);
+ memcpy(&field->data, data, sizeof(field->data));
return 0;
}
@@ -2771,7 +2796,7 @@ static int vcap_rule_mod_action(struct vcap_rule *rule,
field = vcap_find_actionfield(rule, action);
if (!field)
return vcap_rule_add_action(rule, action, ftype, data);
- vcap_copy_from_client_actionfield(rule, field, data);
+ memcpy(&field->data, data, sizeof(field->data));
return 0;
}
--
2.39.0
^ permalink raw reply related
* [PATCH net 2/8] net: microchip: sparx5: Reset VCAP counter for new rules
From: Steen Hegelund @ 2022-12-21 13:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Steen Hegelund, UNGLinuxDriver, Randy Dunlap, Casper Andersson,
Russell King, Wan Jiabing, Nathan Huckleberry, linux-kernel,
netdev, linux-arm-kernel, Steen Hegelund, Daniel Machon,
Horatiu Vultur, Lars Povlsen, Dan Carpenter
In-Reply-To: <20221221132517.2699698-1-steen.hegelund@microchip.com>
When a rule counter is external to the VCAP such as the Sparx5 IS2 counters
are, then this counter must be reset when a new rule is created.
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
drivers/net/ethernet/microchip/vcap/vcap_api.c | 3 +++
drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c | 4 ++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index b9b6432f4094..67e0a3d9103a 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -1808,6 +1808,7 @@ int vcap_add_rule(struct vcap_rule *rule)
{
struct vcap_rule_internal *ri = to_intrule(rule);
struct vcap_rule_move move = {0};
+ struct vcap_counter ctr = {0};
int ret;
ret = vcap_api_check(ri->vctrl);
@@ -1833,6 +1834,8 @@ int vcap_add_rule(struct vcap_rule *rule)
ret = vcap_write_rule(ri);
if (ret)
pr_err("%s:%d: rule write error: %d\n", __func__, __LINE__, ret);
+ /* Set the counter to zero */
+ ret = vcap_write_counter(ri, &ctr);
out:
mutex_unlock(&ri->admin->lock);
return ret;
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
index 76a31215ebfb..944de5cb9114 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
@@ -1343,8 +1343,8 @@ static void vcap_api_encode_rule_test(struct kunit *test)
u32 port_mask_rng_mask = 0x0f;
u32 igr_port_mask_value = 0xffabcd01;
u32 igr_port_mask_mask = ~0;
- /* counter is not written yet, so it is not in expwriteaddr */
- u32 expwriteaddr[] = {792, 793, 794, 795, 796, 797, 0};
+ /* counter is written as the last operation */
+ u32 expwriteaddr[] = {792, 793, 794, 795, 796, 797, 792};
int idx;
vcap_test_api_init(&is2_admin);
--
2.39.0
^ permalink raw reply related
* [PATCH net 1/8] net: microchip: vcap api: Erase VCAP cache before encoding rule
From: Steen Hegelund @ 2022-12-21 13:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Steen Hegelund, UNGLinuxDriver, Randy Dunlap, Casper Andersson,
Russell King, Wan Jiabing, Nathan Huckleberry, linux-kernel,
netdev, linux-arm-kernel, Steen Hegelund, Daniel Machon,
Horatiu Vultur, Lars Povlsen, Dan Carpenter
In-Reply-To: <20221221132517.2699698-1-steen.hegelund@microchip.com>
For consistency the VCAP cache area is erased just before the new rule is
being encoded.
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
---
drivers/net/ethernet/microchip/vcap/vcap_api.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index 664aae3e2acd..b9b6432f4094 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -1823,6 +1823,7 @@ int vcap_add_rule(struct vcap_rule *rule)
}
if (move.count > 0)
vcap_move_rules(ri, &move);
+ vcap_erase_cache(ri);
ret = vcap_encode_rule(ri);
if (ret) {
pr_err("%s:%d: rule encoding error: %d\n", __func__, __LINE__, ret);
@@ -1885,7 +1886,6 @@ struct vcap_rule *vcap_alloc_rule(struct vcap_control *vctrl,
ri->vctrl = vctrl; /* refer to the client */
if (vcap_set_rule_id(ri) == 0)
goto out_free;
- vcap_erase_cache(ri);
return (struct vcap_rule *)ri;
out_free:
--
2.39.0
^ permalink raw reply related
* [PATCH net 0/8] Add support for two classes of VCAP rules
From: Steen Hegelund @ 2022-12-21 13:25 UTC (permalink / raw)
To: David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Steen Hegelund, UNGLinuxDriver, Randy Dunlap, Casper Andersson,
Russell King, Wan Jiabing, Nathan Huckleberry, linux-kernel,
netdev, linux-arm-kernel, Steen Hegelund, Daniel Machon,
Horatiu Vultur, Lars Povlsen, Dan Carpenter
This adds support for two classes of VCAP rules:
- Permanent rules (added e.g. for PTP support)
- TC user rules (added by the TC userspace tool)
For this to work the VCAP Loopups must be enabled from boot, so that the
"internal" clients like PTP can add rules that are always active.
When the TC tool add a flower filter the VCAP rule corresponding to this
filter will be disabled (kept in memory) until a TC matchall filter creates
a link from chain 0 to the chain (lookup) where the flower filter was
added.
When the flower filter is enabled it will be written to the appropriate
VCAP lookup and become active in HW.
Likewise the flower filter will be disabled if there is no link from chain
0 to the chain of the filter (lookup), and when that happens the
corresponding VCAP rule will be read from the VCAP instance and stored in
memory until it is deleted or enabled again.
Steen Hegelund (8):
net: microchip: vcap api: Erase VCAP cache before encoding rule
net: microchip: sparx5: Reset VCAP counter for new rules
net: microchip: vcap api: Always enable VCAP lookups
net: microchip: vcap api: Convert multi-word keys/actions when
encoding
net: microchip: vcap api: Use src and dst chain id to chain VCAP
lookups
net: microchip: vcap api: Check chains when adding a tc flower filter
net: microchip: vcap api: Add a storage state to a VCAP rule
net: microchip: vcap api: Enable/Disable rules via chains in VCAP HW
.../ethernet/microchip/lan966x/lan966x_goto.c | 10 +-
.../ethernet/microchip/lan966x/lan966x_main.h | 3 +-
.../microchip/lan966x/lan966x_tc_flower.c | 30 +-
.../microchip/lan966x/lan966x_tc_matchall.c | 16 +-
.../microchip/lan966x/lan966x_vcap_impl.c | 24 +-
.../microchip/sparx5/sparx5_tc_flower.c | 28 +-
.../microchip/sparx5/sparx5_tc_matchall.c | 16 +-
.../microchip/sparx5/sparx5_vcap_debugfs.c | 2 +-
.../microchip/sparx5/sparx5_vcap_impl.c | 29 +-
.../net/ethernet/microchip/vcap/vcap_api.c | 752 +++++++++++++-----
.../net/ethernet/microchip/vcap/vcap_api.h | 5 -
.../ethernet/microchip/vcap/vcap_api_client.h | 8 +-
.../microchip/vcap/vcap_api_debugfs.c | 57 +-
.../microchip/vcap/vcap_api_debugfs_kunit.c | 10 +-
.../ethernet/microchip/vcap/vcap_api_kunit.c | 32 +-
.../microchip/vcap/vcap_api_private.h | 12 +-
16 files changed, 685 insertions(+), 349 deletions(-)
--
2.39.0
^ permalink raw reply
* Re: [RFC PATCH net-next v2 0/5] net/smc:Introduce SMC-D based loopback acceleration
From: Wen Gu @ 2022-12-21 13:14 UTC (permalink / raw)
To: Niklas Schnelle, kgraul, wenjia, jaka, davem, edumazet, kuba,
pabeni
Cc: linux-s390, netdev, linux-kernel
In-Reply-To: <42f2972f1dfe45a2741482f36fbbda5b5a56d8f1.camel@linux.ibm.com>
On 2022/12/20 22:02, Niklas Schnelle wrote:
> On Tue, 2022-12-20 at 11:21 +0800, Wen Gu wrote:
>> Hi, all
>>
>> # Background
>>
>> As previously mentioned in [1], we (Alibaba Cloud) are trying to use SMC
>> to accelerate TCP applications in cloud environment, improving inter-host
>> or inter-VM communication.
>>
>> In addition of these, we also found the value of SMC-D in scenario of local
>> inter-process communication, such as accelerate communication between containers
>> within the same host. So this RFC tries to provide a SMC-D loopback solution
>> in such scenario, to bring a significant improvement in latency and throughput
>> compared to TCP loopback.
>>
>> # Design
>>
>> This patch set provides a kind of SMC-D loopback solution.
>>
>> Patch #1/5 and #2/5 provide an SMC-D based dummy device, preparing for the
>> inter-process communication acceleration. Except for loopback acceleration,
>> the dummy device can also meet the requirements mentioned in [2], which is
>> providing a way to test SMC-D logic for broad community without ISM device.
>>
>> +------------------------------------------+
>> | +-----------+ +-----------+ |
>> | | process A | | process B | |
>> | +-----------+ +-----------+ |
>> | ^ ^ |
>> | | +---------------+ | |
>> | | | SMC stack | | |
>> | +--->| +-----------+ |<--| |
>> | | | dummy | | |
>> | | | device | | |
>> | +-+-----------+-+ |
>> | VM |
>> +------------------------------------------+
>>
>> Patch #3/5, #4/5, #5/5 provides a way to avoid data copy from sndbuf to RMB
>> and improve SMC-D loopback performance. Through extending smcd_ops with two
>> new semantic: attach_dmb and detach_dmb, sender's sndbuf shares the same
>> physical memory region with receiver's RMB. The data copied from userspace
>> to sender's sndbuf directly reaches the receiver's RMB without unnecessary
>> memory copy in the same kernel.
>>
>> +----------+ +----------+
>> | socket A | | socket B |
>> +----------+ +----------+
>> | ^
>> | +---------+ |
>> regard as | | ----------|
>> local sndbuf | B's | regard as
>> | | RMB | local RMB
>> |-------> | |
>> +---------+
>
> Hi Wen Gu,
>
> I maintain the s390 specific PCI support in Linux and would like to
> provide a bit of background on this. You're surely wondering why we
> even have a copy in there for our ISM virtual PCI device. To understand
> why this copy operation exists and why we need to keep it working, one
> needs a bit of s390 aka mainframe background.
>
> On s390 all (currently supported) native machines have a mandatory
> machine level hypervisor. All OSs whether z/OS or Linux run either on
> this machine level hypervisor as so called Logical Partitions (LPARs)
> or as second/third/… level guests on e.g. a KVM or z/VM hypervisor that
> in turn runs in an LPAR. Now, in terms of memory this machine level
> hypervisor sometimes called PR/SM unlike KVM, z/VM, or VMWare is a
> partitioning hypervisor without paging. This is one of the main reasons
> for the very-near-native performance of the machine hypervisor as the
> memory of its guests acts just like native RAM on other systems. It is
> never paged out and always accessible to IOMMU translated DMA from
> devices without the need for pinning pages and besides a trivial
> offset/limit adjustment an LPAR's MMU does the same amount of work as
> an MMU on a bare metal x86_64/ARM64 box.
>
> It also means however that when SMC-D is used to communicate between
> LPARs via an ISM device there is no way of mapping the DMBs to the
> same physical memory as there exists no MMU-like layer spanning
> partitions that could do such a mapping. Meanwhile for machine level
> firmware including the ISM virtual PCI device it is still possible to
> _copy_ memory between different memory partitions. So yeah while I do
> see the appeal of skipping the memcpy() for loopback or even between
> guests of a paging hypervisor such as KVM, which can map the DMBs on
> the same physical memory, we must keep in mind this original use case
> requiring a copy operation.
>
> Thanks,
> Niklas
>
Hi Niklas,
Thank you so much for the complete and detailed explanation! This provides
me a brand new perspective of s390 device that we hadn't dabbled in before.
Now I understand why shared memory is unavailable between different LPARs.
Our original intention of proposing loopback device and the incoming device
(virtio-ism) for inter-VM is to use SMC-D to accelerate communication in the
case with no existing s390 ISM devices. In our conception, s390 ISM device,
loopback device and virtio-ism device are parallel and are abstracted by smcd_ops.
+------------------------+
| SMC-D |
+------------------------+
-------- smcd_ops ---------
+------+ +------+ +------+
| s390 | | loop | |virtio|
| ISM | | back | | -ism |
| dev | | dev | | dev |
+------+ +------+ +------+
We also believe that keeping the existing design and behavior of s390 ISM
device is unshaken. What we want to get support for is some smcd_ops extension
for devices with optional beneficial capability, such as nocopy here (Let's call
it this for now), which is really helpful for us in inter-process and inter-VM
scenario.
And coincided with IBM's intention to add APIs between SMC-D and devices to
support various devices for SMC-D, as mentioned in [2], we send out this RFC and
the incoming virio-ism RFC, to provide some examples.
>>
>> # Benchmark Test
>>
>> * Test environments:
>> - VM with Intel Xeon Platinum 8 core 2.50GHz, 16 GiB mem.
>> - SMC sndbuf/RMB size 1MB.
>>
>> * Test object:
>> - TCP: run on TCP loopback.
>> - domain: run on UNIX domain.
>> - SMC lo: run on SMC loopback device with patch #1/5 ~ #2/5.
>> - SMC lo-nocpy: run on SMC loopback device with patch #1/5 ~ #5/5.
>>
>> 1. ipc-benchmark (see [3])
>>
>> - ./<foo> -c 1000000 -s 100
>>
>> TCP domain SMC-lo SMC-lo-nocpy
>> Message
>> rate (msg/s) 75140 129548(+72.41) 152266(+102.64%) 151914(+102.17%)
>
> Interesting that it does beat UNIX domain sockets. Also, see my below
> comment for nginx/wrk as this seems very similar.
>
>>
>> 2. sockperf
>>
>> - serv: <smc_run> taskset -c <cpu> sockperf sr --tcp
>> - clnt: <smc_run> taskset -c <cpu> sockperf { tp | pp } --tcp --msg-size={ 64000 for tp | 14 for pp } -i 127.0.0.1 -t 30
>>
>> TCP SMC-lo SMC-lo-nocpy
>> Bandwidth(MBps) 4943.359 4936.096(-0.15%) 8239.624(+66.68%)
>> Latency(us) 6.372 3.359(-47.28%) 3.25(-49.00%)
>>
>> 3. iperf3
>>
>> - serv: <smc_run> taskset -c <cpu> iperf3 -s
>> - clnt: <smc_run> taskset -c <cpu> iperf3 -c 127.0.0.1 -t 15
>>
>> TCP SMC-lo SMC-lo-nocpy
>> Bitrate(Gb/s) 40.5 41.4(+2.22%) 76.4(+88.64%)
>>
>> 4. nginx/wrk
>>
>> - serv: <smc_run> nginx
>> - clnt: <smc_run> wrk -t 8 -c 500 -d 30 http://127.0.0.1:80
>>
>> TCP SMC-lo SMC-lo-nocpy
>> Requests/s 154643.22 220894.03(+42.84%) 226754.3(+46.63%)
>
>
> This result is very interesting indeed. So with the much more realistic
> nginx/wrk workload it seems to copy hurts much less than the
> iperf3/sockperf would suggest while SMC-D itself seems to help more.
> I'd hope that this translates to actual applications as well. Maybe
> this makes SMC-D based loopback interesting even while keeping the
> copy, at least until we can come up with a sane way to work a no-copy
> variant into SMC-D?
>
I agree, nginx/wrk workload is much more realistic for many applications.
But we also encounter many other cases similar to sockperf on the cloud, which
requires high throughput, such as AI training and big data.
So avoidance of copying between DMBs can help these cases a lot :)
>>
>>
>> # Discussion
>>
>> 1. API between SMC-D and ISM device
>>
>> As Jan mentioned in [2], IBM are working on placing an API between SMC-D
>> and the ISM device for easier use of different "devices" for SMC-D.
>>
>> So, considering that the introduction of attach_dmb or detach_dmb can
>> effectively avoid data copying from sndbuf to RMB and brings obvious
>> throughput advantages in inter-VM or inter-process scenarios, can the
>> attach/detach semantics be taken into consideration when designing the
>> API to make it a standard ISM device behavior?
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Due to the reasons explained above this behavior can't be emulated by
> ISM devices at least not when crossing partitions. Not sure if we can
> still incorporate it in the API and allow for both copying and
> remapping SMC-D like devices, it definitely needs careful consideration
> and I think also a better understanding of the benefit for real world
> workloads.
>
Here I am not rigorous.
Nocopy shouldn't be a standard ISM device behavior indeed. Actually we hope it be a
standard optional _SMC-D_ device behavior and defined by smcd_ops.
For devices don't support these options, like ISM device on s390 architecture,
.attach_dmb/.detach_dmb and other reasonable extensions (which will be proposed to
discuss in incoming virtio-ism RFC) can be set to NULL or return invalid. And for
devices do support, they may be used for improving performance in some cases.
In addition, can I know more latest news about the API design? :) , like its scale, will
it be a almost refactor of existing interface or incremental patching? and its object,
will it be tailored for exact ISM behavior or to reserve some options for other devices,
like nocopy here? From my understanding of [2], it might be the latter?
>>
>> Maybe our RFC of SMC-D based inter-process acceleration (this one) and
>> inter-VM acceleration (will coming soon, which is the update of [1])
>> can provide some examples for new API design. And we are very glad to
>> discuss this on the mail list.
>>
>> 2. Way to select different ISM-like devices
>>
>> With the proposal of SMC-D loopback 'device' (this RFC) and incoming
>> device used for inter-VM acceleration as update of [1], SMC-D has more
>> options to choose from. So we need to consider that how to indicate
>> supported devices, how to determine which one to use, and their priority...
>
> Agree on this part, though it is for the SMC maintainers to decide, I
> think we would definitely want to be able to use any upcoming inter-VM
> devices on s390 possibly also in conjunction with ISM devices for
> communication across partitions.
>
Yes, this part needs to be discussed with SMC maintainers. And thank you, we are very glad
if our devices can be applied on s390 through the efforts.
Best Regards,
Wen Gu
>>
>> IMHO, this may require an update of CLC message and negotiation mechanism.
>> Again, we are very glad to discuss this with you on the mailing list.
>>
>> [1] https://lore.kernel.org/netdev/20220720170048.20806-1-tonylu@linux.alibaba.com/
>> [2] https://lore.kernel.org/netdev/35d14144-28f7-6129-d6d3-ba16dae7a646@linux.ibm.com/
>> [3] https://github.com/goldsborough/ipc-bench
>>
>> v1->v2
>> 1. Fix some build WARNINGs complained by kernel test rebot
>> Reported-by: kernel test robot <lkp@intel.com>
>> 2. Add iperf3 test data.
>>
>> Wen Gu (5):
>> net/smc: introduce SMC-D loopback device
>> net/smc: choose loopback device in SMC-D communication
>> net/smc: add dmb attach and detach interface
>> net/smc: avoid data copy from sndbuf to peer RMB in SMC-D loopback
>> net/smc: logic of cursors update in SMC-D loopback connections
>>
>> include/net/smc.h | 3 +
>> net/smc/Makefile | 2 +-
>> net/smc/af_smc.c | 88 +++++++++++-
>> net/smc/smc_cdc.c | 59 ++++++--
>> net/smc/smc_cdc.h | 1 +
>> net/smc/smc_clc.c | 4 +-
>> net/smc/smc_core.c | 62 +++++++++
>> net/smc/smc_core.h | 2 +
>> net/smc/smc_ism.c | 39 +++++-
>> net/smc/smc_ism.h | 2 +
>> net/smc/smc_loopback.c | 358 +++++++++++++++++++++++++++++++++++++++++++++++++
>> net/smc/smc_loopback.h | 63 +++++++++
>> 12 files changed, 662 insertions(+), 21 deletions(-)
>> create mode 100644 net/smc/smc_loopback.c
>> create mode 100644 net/smc/smc_loopback.h
>>
^ permalink raw reply
* Re: [PATCH] virtio_net: send notification coalescing command only if value changed
From: Alvaro Karsz @ 2022-12-21 13:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtualization, netdev, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
In-Reply-To: <20221221075855-mutt-send-email-mst@kernel.org>
> We'll always just do 2 commands, right?
This is my point, we are sending 2 commands at the moment, even though
one of them may be unnecessary.
> E.g. if a card lost the config resending it might help fix things.
How can it lose the config?
We can say the same about every command sent through the control vq.
> I don't think we should bother at this point.
Ok, I'll drop it.
Alvaro
^ permalink raw reply
* Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support
From: Andrew Lunn @ 2022-12-21 13:10 UTC (permalink / raw)
To: Christian Marangi
Cc: Russell King (Oracle), Florian Fainelli, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Jonathan Corbet, Pavel Machek,
John Crispin, netdev, devicetree, linux-kernel, linux-doc,
linux-leds, Tim Harvey, Alexander Stein, Rasmus Villemoes
In-Reply-To: <63a3038b.050a0220.d41c3.6f48@mx.google.com>
> > > I agree we need to make compromises. We cannot support every LED
> > > feature of every PHY, they are simply too diverse. Hopefully we can
> > > support some features of every PHY. In the worst case, a PHY simply
> > > cannot be controlled via this method, which is the current state
> > > today. So it is not worse off.
> >
> > ... and that compromise is that it's not going to be possible to enable
> > activity mode on 88e151x with how the code stands and with the
> > independent nature of "rx" and "tx" activity control currently in the
> > netdev trigger... making this whole approach somewhat useless for
> > Marvell PHYs.
>
> Again we can consider adding an activity mode. It seems logical that
> some switch may only support global traffic instead of independend tx or
> rx... The feature are not mutually exclusive. One include the other 2.
Looking at the software trigger, adding NETDEV_LED_RXTX looks simple
to do. I also suspect it will be used by more than Marvell.
> > We really need to see a working implementation for this code for more
> > than just one PHY to prove that it is actually possible for it to
> > support other PHYs. If not, it isn't actually solving the problem,
> > and we're going to continue getting custom implementations to configure
> > the LED settings.
> >
>
> Agree that we need other user for this to catch some problem in the
> implementation of this generic API.
We need a PHY driver implementation. The phylib core needs to be
involved, the cled code needs to call generic phylib functions which
take the phydev->lock before calling into the PHY driver. Probably the
phylib core can do all the memory allocation, and registration of the
LED to the LED core. If it is not too ugly, i would also do the DT
binding parsing in the core, so we don't end up with subtle
differences.
Andrew
^ permalink raw reply
* Re: [PATCH] virtio_net: send notification coalescing command only if value changed
From: Michael S. Tsirkin @ 2022-12-21 13:01 UTC (permalink / raw)
To: Alvaro Karsz
Cc: virtualization, netdev, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
In-Reply-To: <CAJs=3_CVUydOpH=a-RJLWUQ0_1EbkwKtGD2F3Xvw=dR5QFXP5g@mail.gmail.com>
On Wed, Dec 21, 2022 at 02:44:21PM +0200, Alvaro Karsz wrote:
> > Why do we bother? Resending needs more code and helps
> > reliability ...
>
> It just seems unnecessary.
> If a user changes just one parameter:
> $ ethtool -C <iface> tx-usecs 30
> It will trigger 2 commands, including
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, even though no rx parameter changed.
>
> If we'll add more ethtool coalescing parameters, changing one of the
> new parameter will trigger meaningless
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> commands.
>
> Alvaro
We'll always just do 2 commands, right?
I don't think we should bother at this point.
It might not be completely redundant.
E.g. if a card lost the config resending it might help fix things.
--
MST
^ permalink raw reply
* Re: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support
From: Christian Marangi @ 2022-12-21 13:00 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Jonathan Corbet, Pavel Machek, John Crispin,
netdev, devicetree, linux-kernel, linux-doc, linux-leds,
Tim Harvey, Alexander Stein, Rasmus Villemoes
In-Reply-To: <Y6LX43poXJ4k/7mv@shell.armlinux.org.uk>
On Wed, Dec 21, 2022 at 09:54:43AM +0000, Russell King (Oracle) wrote:
> On Wed, Dec 21, 2022 at 12:59:55AM +0100, Andrew Lunn wrote:
> > > > One thought on this approach though - if one has a PHY that supports
> > > > "activity" but not independent "rx" and "tx" activity indications
> > > > and it doesn't support software control, how would one enable activity
> > > > mode? There isn't a way to simultaneously enable both at the same
> > > > time... However, I need to check whether there are any PHYs that fall
> > > > into this category.
> > > >
> > >
> > > Problem is that for such feature and to have at least something working
> > > we need to face compromise. We really can't support each switch feature
> > > and have a generic API for everything.
> >
> > I agree we need to make compromises. We cannot support every LED
> > feature of every PHY, they are simply too diverse. Hopefully we can
> > support some features of every PHY. In the worst case, a PHY simply
> > cannot be controlled via this method, which is the current state
> > today. So it is not worse off.
>
> ... and that compromise is that it's not going to be possible to enable
> activity mode on 88e151x with how the code stands and with the
> independent nature of "rx" and "tx" activity control currently in the
> netdev trigger... making this whole approach somewhat useless for
> Marvell PHYs.
Again we can consider adding an activity mode. It seems logical that
some switch may only support global traffic instead of independend tx or
rx... The feature are not mutually exclusive. One include the other 2.
We already a simple workaround for the link mode where on the current
driver, if the link mode is enabled just all rule for 10 100 and 1000
mbps are enabled simulating a global link event.
>
> We really need to see a working implementation for this code for more
> than just one PHY to prove that it is actually possible for it to
> support other PHYs. If not, it isn't actually solving the problem,
> and we're going to continue getting custom implementations to configure
> the LED settings.
>
Agree that we need other user for this to catch some problem in the
implementation of this generic API.
--
Ansuel
^ permalink raw reply
* Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example
From: Andrew Lunn @ 2022-12-21 12:59 UTC (permalink / raw)
To: Christian Marangi
Cc: Rob Herring, Florian Fainelli, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski,
Jonathan Corbet, Pavel Machek, Russell King (Oracle),
John Crispin, netdev, devicetree, linux-kernel, linux-doc,
linux-leds, Tim Harvey, Alexander Stein, Rasmus Villemoes
In-Reply-To: <63a30221.050a0220.16e5f.653a@mx.google.com>
> An alternative way would be set the reg to be the global led number in
> the switch and deatch the phy from the calculation.
>
> Something like
> port 0 led 0 = reg 0
> port 0 led 1 = reg 1
> port 1 led 0 = reg 2
> port 1 led 1 = reg 3
I would not do this. It will make LED controllers embedded in switches
different to LEDs controllers embedded in PHYs. Ideally we want them
identical. One binding to rule them all.
Andrew
^ permalink raw reply
* Re: [PATCH v7 09/11] leds: trigger: netdev: add additional hardware only triggers
From: Christian Marangi @ 2022-12-21 12:56 UTC (permalink / raw)
To: Andrew Lunn
Cc: Russell King (Oracle), Florian Fainelli, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Jonathan Corbet, Pavel Machek,
John Crispin, netdev, devicetree, linux-kernel, linux-doc,
linux-leds, Tim Harvey, Alexander Stein, Rasmus Villemoes
In-Reply-To: <Y6JPYBQhtpZLadry@lunn.ch>
On Wed, Dec 21, 2022 at 01:12:16AM +0100, Andrew Lunn wrote:
> On Thu, Dec 15, 2022 at 05:35:47PM +0000, Russell King (Oracle) wrote:
> > On Thu, Dec 15, 2022 at 12:54:36AM +0100, Christian Marangi wrote:
> > > Add additional hardware only triggers commonly supported by switch LEDs.
> > >
> > > Additional modes:
> > > link_10: LED on with link up AND speed 10mbps
> > > link_100: LED on with link up AND speed 100mbps
> > > link_1000: LED on with link up AND speed 1000mbps
> > > half_duplex: LED on with link up AND half_duplex mode
> > > full_duplex: LED on with link up AND full duplex mode
> >
> > Looking at Marvell 88e151x, I don't think this is usable there.
> > We have the option of supporting link_1000 on one of the LEDs,
> > link_100 on another, and link_10 on the other. It's rather rare
> > for all three leds to be wired though.
>
> The 88e151x will need to enumerate what it actually supports from the
> above list, per LED. I also think we can carefully expand the list
> above, adding a few more modes. We just need to ensure what is added
> is reasonably generic, modes we expect multiple PHY to support. What
> we need to avoid is adding every single mode a PHY supports, but no
> other PHY has.
>
> > This is also a PHY where "activity" mode is supported (illuminated
> > or blinking if any traffic is transmitted or received) but may not
> > support individual directional traffic in hardware. However, it
> > does support forcing the LED on or off, so software mode can handle
> > those until the user selects a combination of modes that are
> > supported in the hardware.
> >
> > > Additional blink interval modes:
> > > blink_2hz: LED blink on any even at 2Hz (250ms)
> > > blink_4hz: LED blink on any even at 4Hz (125ms)
> > > blink_8hz: LED blink on any even at 8Hz (62ms)
> >
> > This seems too restrictive. For example, Marvell 88e151x supports
> > none of these, but does support 42, 84, 170, 340, 670ms.
>
> I would actually drop this whole idea of being able to configure the
> blink period. It seems like it is going to cause problems. I expect
> most PHYs actual share the period across multiple LEDs, which you
> cannot easily model here.
>
> So i would have the driver hard coded to pick a frequency at thats' it.
>
Yes I think "for now" it's the only way and just drop blink
configuration support.
--
Ansuel
^ permalink raw reply
* Re: [PATCH v7 11/11] dt-bindings: net: dsa: qca8k: add LEDs definition example
From: Christian Marangi @ 2022-12-21 12:54 UTC (permalink / raw)
To: Andrew Lunn
Cc: Rob Herring, Florian Fainelli, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Krzysztof Kozlowski,
Jonathan Corbet, Pavel Machek, Russell King (Oracle),
John Crispin, netdev, devicetree, linux-kernel, linux-doc,
linux-leds, Tim Harvey, Alexander Stein, Rasmus Villemoes
In-Reply-To: <Y6JkXnp0/lF4p0N1@lunn.ch>
On Wed, Dec 21, 2022 at 02:41:50AM +0100, Andrew Lunn wrote:
> > > > + };
> > > > +
> > > > + led@1 {
> > > > + reg = <1>;
> > > > + color = <LED_COLOR_ID_AMBER>;
> > > > + function = LED_FUNCTION_LAN;
> > > > + function-enumerator = <1>;
> > >
> > > Typo? These are supposed to be unique. Can't you use 'reg' in your case?
> >
> > reg in this context is the address of the PHY on the MDIO bus. This is
> > an Ethernet switch, so has many PHYs, each with its own address.
>
> Actually, i'm wrong about that. reg in this context is the LED number
> of the PHY. Typically there are 2 or 3 LEDs per PHY.
>
> There is no reason the properties need to be unique. Often the LEDs
> have 8 or 16 functions, identical for each LED, but with different
> reset defaults so they show different things.
>
Are we taking about reg or function-enumerator?
For reg it's really specific to the driver... My idea was that since a
single phy can have multiple leds attached, reg will represent the led
number.
This is an example of the dt implemented on a real device.
mdio {
#address-cells = <1>;
#size-cells = <0>;
phy_port1: phy@0 {
reg = <0>;
leds {
#address-cells = <1>;
#size-cells = <0>;
lan1_led@0 {
reg = <0>;
color = <LED_COLOR_ID_WHITE>;
function = LED_FUNCTION_LAN;
function-enumerator = <1>;
linux,default-trigger = "netdev";
};
lan1_led@1 {
reg = <1>;
color = <LED_COLOR_ID_AMBER>;
function = LED_FUNCTION_LAN;
function-enumerator = <1>;
linux,default-trigger = "netdev";
};
};
};
phy_port2: phy@1 {
reg = <1>;
leds {
#address-cells = <1>;
#size-cells = <0>;
lan2_led@0 {
reg = <0>;
color = <LED_COLOR_ID_WHITE>;
function = LED_FUNCTION_LAN;
function-enumerator = <2>;
linux,default-trigger = "netdev";
};
lan2_led@1 {
reg = <1>;
color = <LED_COLOR_ID_AMBER>;
function = LED_FUNCTION_LAN;
function-enumerator = <2>;
linux,default-trigger = "netdev";
};
};
};
phy_port3: phy@2 {
reg = <2>;
leds {
#address-cells = <1>;
#size-cells = <0>;
lan3_led@0 {
reg = <0>;
color = <LED_COLOR_ID_WHITE>;
function = LED_FUNCTION_LAN;
function-enumerator = <3>;
linux,default-trigger = "netdev";
};
lan3_led@1 {
reg = <1>;
color = <LED_COLOR_ID_AMBER>;
function = LED_FUNCTION_LAN;
function-enumerator = <3>;
linux,default-trigger = "netdev";
};
};
};
In the following implementation. Each port have 2 leds attached (out of
3) one white and one amber. The driver parse the reg and calculate the
offset to set the correct option with the regs by also checking the phy
number.
An alternative way would be set the reg to be the global led number in
the switch and deatch the phy from the calculation.
Something like
port 0 led 0 = reg 0
port 0 led 1 = reg 1
port 1 led 0 = reg 2
port 1 led 1 = reg 3
...
Using the function-enumerator can be problematic since ideally someone
would declare a dedicated function for wan led.
I'm very open to discuss and improve/fix this!
--
Ansuel
^ permalink raw reply
* Re: [PATCH] virtio_net: send notification coalescing command only if value changed
From: Alvaro Karsz @ 2022-12-21 12:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtualization, netdev, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
In-Reply-To: <20221221073256-mutt-send-email-mst@kernel.org>
> Why do we bother? Resending needs more code and helps
> reliability ...
It just seems unnecessary.
If a user changes just one parameter:
$ ethtool -C <iface> tx-usecs 30
It will trigger 2 commands, including
VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, even though no rx parameter changed.
If we'll add more ethtool coalescing parameters, changing one of the
new parameter will trigger meaningless
VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
commands.
Alvaro
^ permalink raw reply
* Re: kernel BUG in __skb_gso_segment
From: Greg KH @ 2022-12-21 12:41 UTC (permalink / raw)
To: Tudor Ambarus
Cc: Willem de Bruijn, mst, jasowang, virtualization, edumazet, davem,
kuba, pabeni, netdev, willemb, syzkaller, liuhangbin,
linux-kernel, joneslee
In-Reply-To: <fc60e8da-1187-ca2b-1aa8-28e01ea2769a@linaro.org>
On Wed, Dec 21, 2022 at 09:42:59AM +0200, Tudor Ambarus wrote:
>
>
> On 21.12.2022 09:37, Greg KH wrote:
> > On Wed, Dec 21, 2022 at 09:28:16AM +0200, Tudor Ambarus wrote:
> > > Hi,
> > >
> > > I added Greg KH to the thread, maybe he can shed some light on whether
> > > new support can be marked as fixes and backported to stable. The rules
> > > on what kind of patches are accepted into the -stable tree don't mention
> > > new support:
> > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> >
> > As you say, we don't take new features into older kernels. Unless they
> > fix a reported problem, if so, submit the git ids to us and we will be
> > glad to review them.
> >
>
> They do fix a bug. I'm taking care of it. Shall I update
> Documentation/process/stable-kernel-rules.rst to mention this rule as
> well?
How exactly would you change it, and why?
^ permalink raw reply
* [PATCH] UAPI: fix spelling typos in comments
From: Rong Tao @ 2022-12-21 12:36 UTC (permalink / raw)
To: 3chas3; +Cc: Rong Tao, moderated list:ATM, open list:ATM, open list
From: Rong Tao <rongtao@cestc.cn>
Fix the typo of 'Unsuported' in atmbr2684.h
Signed-off-by: Rong Tao <rongtao@cestc.cn>
---
include/uapi/linux/atmbr2684.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/atmbr2684.h b/include/uapi/linux/atmbr2684.h
index a9e2250cd720..d47c47d06f11 100644
--- a/include/uapi/linux/atmbr2684.h
+++ b/include/uapi/linux/atmbr2684.h
@@ -38,7 +38,7 @@
*/
#define BR2684_ENCAPS_VC (0) /* VC-mux */
#define BR2684_ENCAPS_LLC (1)
-#define BR2684_ENCAPS_AUTODETECT (2) /* Unsuported */
+#define BR2684_ENCAPS_AUTODETECT (2) /* Unsupported */
/*
* Is this VC bridged or routed?
--
2.39.0
^ permalink raw reply related
* Re: [PATCH] virtio_net: send notification coalescing command only if value changed
From: Michael S. Tsirkin @ 2022-12-21 12:33 UTC (permalink / raw)
To: Alvaro Karsz
Cc: virtualization, netdev, Jason Wang, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
In-Reply-To: <20221221120618.652074-1-alvaro.karsz@solid-run.com>
On Wed, Dec 21, 2022 at 02:06:18PM +0200, Alvaro Karsz wrote:
> Don't send a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET or
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command if the coalescing parameters
> haven't changed.
>
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Why do we bother? Resending needs more code and helps
reliability ...
> ---
> drivers/net/virtio_net.c | 48 ++++++++++++++++++++++------------------
> 1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7723b2a49d8..1d7118de62a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2760,31 +2760,37 @@ static int virtnet_send_notf_coal_cmds(struct virtnet_info *vi,
> struct virtio_net_ctrl_coal_tx coal_tx;
> struct virtio_net_ctrl_coal_rx coal_rx;
>
> - coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
> - coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
> - sg_init_one(&sgs_tx, &coal_tx, sizeof(coal_tx));
> -
> - if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> - VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
> - &sgs_tx))
> - return -EINVAL;
> + if (ec->tx_coalesce_usecs != vi->tx_usecs ||
> + ec->tx_max_coalesced_frames != vi->tx_max_packets) {
> + coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
> + coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
> + sg_init_one(&sgs_tx, &coal_tx, sizeof(coal_tx));
> +
> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> + VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
> + &sgs_tx))
> + return -EINVAL;
>
> - /* Save parameters */
> - vi->tx_usecs = ec->tx_coalesce_usecs;
> - vi->tx_max_packets = ec->tx_max_coalesced_frames;
> + /* Save parameters */
> + vi->tx_usecs = ec->tx_coalesce_usecs;
> + vi->tx_max_packets = ec->tx_max_coalesced_frames;
> + }
>
> - coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
> - coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
> - sg_init_one(&sgs_rx, &coal_rx, sizeof(coal_rx));
> + if (ec->rx_coalesce_usecs != vi->rx_usecs ||
> + ec->rx_max_coalesced_frames != vi->rx_max_packets) {
> + coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
> + coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
> + sg_init_one(&sgs_rx, &coal_rx, sizeof(coal_rx));
>
> - if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> - VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
> - &sgs_rx))
> - return -EINVAL;
> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> + VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
> + &sgs_rx))
> + return -EINVAL;
>
> - /* Save parameters */
> - vi->rx_usecs = ec->rx_coalesce_usecs;
> - vi->rx_max_packets = ec->rx_max_coalesced_frames;
> + /* Save parameters */
> + vi->rx_usecs = ec->rx_coalesce_usecs;
> + vi->rx_max_packets = ec->rx_max_coalesced_frames;
> + }
>
> return 0;
> }
> --
> 2.32.0
^ permalink raw reply
* Re: [PATCH net v2] net: phy: mxl-gpy: fix delay time required by loopback disable function
From: Russell King (Oracle) @ 2022-12-21 12:21 UTC (permalink / raw)
To: Xu Liang
Cc: andrew, hkallweit1, netdev, davem, kuba, hmehrtens, tmohren,
mohammad.athari.ismail, edumazet, pabeni
In-Reply-To: <20221221094358.29639-1-lxu@maxlinear.com>
Hi,
On Wed, Dec 21, 2022 at 05:43:58PM +0800, Xu Liang wrote:
> GPY2xx devices need 3 seconds to fully switch out of loopback mode
> before it can safely re-enter loopback mode.
Would it be better to record the time that loopback mode is exited,
and then delay an attempt to re-enter loopback mode if it's less than
three seconds since we exited?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* Re: [PATCH] net: phy: enhance Maxlinear GPY loopback disable function
From: Russell King (Oracle) @ 2022-12-21 12:20 UTC (permalink / raw)
To: Liang Xu
Cc: Michael Walle, andrew@lunn.ch, davem@davemloft.net,
hkallweit1@gmail.com, Hauke Mehrtens, kuba@kernel.org,
Ismail, Mohammad Athari, netdev@vger.kernel.org, Thomas Mohren
In-Reply-To: <PH7PR19MB56130FA71635455AE9DB8524BDE09@PH7PR19MB5613.namprd19.prod.outlook.com>
Hi,
On Wed, Dec 14, 2022 at 09:16:46AM +0000, Liang Xu wrote:
> From: Michael Walle <michael@walle.cc>
> Sent: Wednesday, December 14, 2022 5:00 PM
> Subject: Re: [PATCH] net: phy: enhance Maxlinear GPY loopback disable function
>
> This email was sent from outside of MaxLinear.
>
>
> Subject is missing the correct target, "net" in this case.
>
> > GPY need 3 seconds to switch out of loopback mode.
>
> What does that mean, what goes wrong with the current 100ms?
> Could you elaborate a bit more and update the commit message
> and the comment? Is this true for any GPY PHY supported by
> this driver?
>
> The internal state machine need almost 3s to fully restore to original state when leaving the loopback mode.
> This is true for all models supported by this driver.
> I will update the commit message and fix the target.
> Thank you.
When you state that it takes almost 3 seconds to fully restore the
state, what are you basing that upon - what are you using to determine
that the state has been fully restored?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply
* Re: [PATCH] wifi: rtl8xxxu: fixing transmisison failure for rtl8192eu
From: Bitterblue Smith @ 2022-12-21 12:14 UTC (permalink / raw)
To: Ping-Ke Shih, Jes.Sorensen@gmail.com, JunASAKA@zzy040330.moe
Cc: davem@davemloft.net, kvalo@kernel.org,
linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, kuba@kernel.org, edumazet@google.com,
pabeni@redhat.com
In-Reply-To: <fb0a7d6c0897464550ed7ee75c6318c525a0f001.camel@realtek.com>
On 21/12/2022 03:42, Ping-Ke Shih wrote:
> On Tue, 2022-12-20 at 15:03 +0200, Bitterblue Smith wrote:
>> On 20/12/2022 07:44, Ping-Ke Shih wrote:
>>>
>>>> -----Original Message-----
>>>> From: Jun ASAKA <JunASAKA@zzy040330.moe>
>>>> Sent: Saturday, December 17, 2022 11:07 AM
>>>> To: Jes.Sorensen@gmail.com
>>>> Cc: kvalo@kernel.org; davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>>>> pabeni@redhat.com;
>>>> linux-wireless@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Jun
>>>> ASAKA
>>>> <JunASAKA@zzy040330.moe>
>>>> Subject: [PATCH] wifi: rtl8xxxu: fixing transmisison failure for rtl8192eu
>>>>
>>>> Fixing transmission failure which results in
>>>> "authentication with ... timed out". This can be
>>>> fixed by disable the REG_TXPAUSE.
>>>>
>>>> Signed-off-by: Jun ASAKA <JunASAKA@zzy040330.moe>
>>>> ---
>>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 5 +++++
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>>>> index a7d76693c02d..9d0ed6760cb6 100644
>>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>>>> @@ -1744,6 +1744,11 @@ static void rtl8192e_enable_rf(struct rtl8xxxu_priv *priv)
>>>> val8 = rtl8xxxu_read8(priv, REG_PAD_CTRL1);
>>>> val8 &= ~BIT(0);
>>>> rtl8xxxu_write8(priv, REG_PAD_CTRL1, val8);
>>>> +
>>>> + /*
>>>> + * Fix transmission failure of rtl8192e.
>>>> + */
>>>> + rtl8xxxu_write8(priv, REG_TXPAUSE, 0x00);
>>>
>>> I trace when rtl8xxxu set REG_TXPAUSE=0xff that will stop TX.
>>> The occasions include RF calibration, LPS mode (called by power off), and
>>> going to stop. So, I think RF calibration does TX pause but not restore
>>> settings after calibration, and causes TX stuck. As the flow I traced,
>>> this patch looks reasonable. But, I wonder why other people don't meet
>>> this problem.
>>>
>> Other people have this problem too:
>> https://bugzilla.kernel.org/show_bug.cgi?id=196769
>> https://bugzilla.kernel.org/show_bug.cgi?id=216746
>
> In the threads, you have answered my question with
> "kernel 4.8.0 works, but 4.9.? does not work."
>
>>
>> The RF calibration does restore REG_TXPAUSE at the end. What happens is
>> when you plug in the device, something (mac80211? wpa_supplicant?) calls
>> rtl8xxxu_start(), then rtl8xxxu_stop(), then rtl8xxxu_start() again.
>> rtl8xxxu_stop() sets REG_TXPAUSE to 0xff and nothing sets it back to 0.
>>
>
> You are correct. That is clear to me. I miss the point that RF calibration
> does backup/restore registers containing REG_TXPAUSE.
>
> Then, I think my reviewed-by can be still applied, right?
>
> Ping-Ke
>
Yes.
^ permalink raw reply
* [PATCH] virtio_net: send notification coalescing command only if value changed
From: Alvaro Karsz @ 2022-12-21 12:06 UTC (permalink / raw)
To: virtualization, netdev
Cc: Alvaro Karsz, Michael S. Tsirkin, Jason Wang, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Don't send a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET or
VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command if the coalescing parameters
haven't changed.
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
---
drivers/net/virtio_net.c | 48 ++++++++++++++++++++++------------------
1 file changed, 27 insertions(+), 21 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7723b2a49d8..1d7118de62a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2760,31 +2760,37 @@ static int virtnet_send_notf_coal_cmds(struct virtnet_info *vi,
struct virtio_net_ctrl_coal_tx coal_tx;
struct virtio_net_ctrl_coal_rx coal_rx;
- coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
- coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
- sg_init_one(&sgs_tx, &coal_tx, sizeof(coal_tx));
-
- if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
- VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
- &sgs_tx))
- return -EINVAL;
+ if (ec->tx_coalesce_usecs != vi->tx_usecs ||
+ ec->tx_max_coalesced_frames != vi->tx_max_packets) {
+ coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
+ coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
+ sg_init_one(&sgs_tx, &coal_tx, sizeof(coal_tx));
+
+ if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
+ VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
+ &sgs_tx))
+ return -EINVAL;
- /* Save parameters */
- vi->tx_usecs = ec->tx_coalesce_usecs;
- vi->tx_max_packets = ec->tx_max_coalesced_frames;
+ /* Save parameters */
+ vi->tx_usecs = ec->tx_coalesce_usecs;
+ vi->tx_max_packets = ec->tx_max_coalesced_frames;
+ }
- coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
- coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
- sg_init_one(&sgs_rx, &coal_rx, sizeof(coal_rx));
+ if (ec->rx_coalesce_usecs != vi->rx_usecs ||
+ ec->rx_max_coalesced_frames != vi->rx_max_packets) {
+ coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
+ coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
+ sg_init_one(&sgs_rx, &coal_rx, sizeof(coal_rx));
- if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
- VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
- &sgs_rx))
- return -EINVAL;
+ if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
+ VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
+ &sgs_rx))
+ return -EINVAL;
- /* Save parameters */
- vi->rx_usecs = ec->rx_coalesce_usecs;
- vi->rx_max_packets = ec->rx_max_coalesced_frames;
+ /* Save parameters */
+ vi->rx_usecs = ec->rx_coalesce_usecs;
+ vi->rx_max_packets = ec->rx_max_coalesced_frames;
+ }
return 0;
}
--
2.32.0
^ permalink raw reply related
* Re: [PULL] Networking for next-6.1 #forregzbot
From: Thorsten Leemhuis @ 2022-12-21 11:30 UTC (permalink / raw)
To: regressions@lists.linux.dev; +Cc: netdev, linux-kernel
In-Reply-To: <6b971a4e-c7d8-411e-1f92-fda29b5b2fb9@kernel.org>
[Note: this mail contains only information for Linux kernel regression
tracking. Mails like these contain '#forregzbot' in the subject to make
then easy to spot and filter out. The author also tried to remove most
or all individuals from the list of recipients to spare them the hassle.]
On 16.12.22 11:49, Jiri Slaby wrote:
>
> On 04. 10. 22, 7:20, Jakub Kicinski wrote:
>> Joanne Koong (7):
>
>> net: Add a bhash2 table hashed by port and address
>
> This makes regression tests of python-ephemeral-port-reserve to fail.
Thanks for the report. To be sure below issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression
tracking bot:
#regzbot ^introduced 28044fc1d495
#regzbot title new: regression tests of python-ephemeral-port-reserve fail
#regzbot ignore-activity
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox