| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z49HTA5MYooJcH0g@mev-dev.igk.intel.com>
Date: Tue, 21 Jan 2025 08:05:48 +0100
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To: Aryan Srivastava <aryan.srivastava@...iedtelesis.co.nz>
Cc: Maxime Chevallier <maxime.chevallier@...tlin.com>,
Marcin Wojtas <marcin.s.wojtas@...il.com>,
Russell King <linux@...linux.org.uk>,
Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] net: mvpp2: Add parser configuration for DSA tags
On Tue, Jan 21, 2025 at 03:18:04PM +1300, Aryan Srivastava wrote:
> Allow the header parser to consider DSA and EDSA tagging. Currently the
> parser is always configured to use the MH tag, but this results in poor
> traffic distribution across queues and sub-optimal performance (in the
> case where DSA or EDSA tags are in the header).
>
> Add mechanism to check for tag type in use and then configure the
> parser correctly for this tag. This results in proper traffic
> distribution and hash calculation.
>
> Use mvpp2_get_tag instead of reading the MH register to determine tag
> type. As the MH register is set during mvpp2_open it is subject to
> change and not a proper reflection of the tagging type in use.
>
> Signed-off-by: Aryan Srivastava <aryan.srivastava@...iedtelesis.co.nz>
You didn't specify the tree, but it look like a feature implementation,
not a fix; net-next is closed now [1].
[1] https://lore.kernel.org/netdev/20250120163446.3bd93e30@kernel.org/#t
> ---
> Changes in v1:
> - use mvpp2_get_tag to ascertain tagging type in use.
>
> drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 3 ++
> .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 37 ++++++++++++++++++-
> .../net/ethernet/marvell/mvpp2/mvpp2_prs.c | 16 +++++---
> 3 files changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> index 44fe9b68d1c2..456f9aeb4d82 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> @@ -59,6 +59,8 @@
>
> /* Top Registers */
> #define MVPP2_MH_REG(port) (0x5040 + 4 * (port))
> +#define MVPP2_MH_EN BIT(0)
Defined, but not used.
> +#define MVPP2_DSA_NON_EXTENDED BIT(4)
> #define MVPP2_DSA_EXTENDED BIT(5)
> #define MVPP2_VER_ID_REG 0x50b0
> #define MVPP2_VER_PP22 0x10
> @@ -1538,6 +1540,7 @@ void mvpp2_dbgfs_cleanup(struct mvpp2 *priv);
> void mvpp2_dbgfs_exit(void);
>
> void mvpp23_rx_fifo_fc_en(struct mvpp2 *priv, int port, bool en);
> +int mvpp2_get_tag(struct net_device *dev);
>
> #ifdef CONFIG_MVPP2_PTP
> int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv);
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index dd76c1b7ed3a..3a954da7660f 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -38,6 +38,7 @@
> #include <net/page_pool/helpers.h>
> #include <net/tso.h>
> #include <linux/bpf_trace.h>
> +#include <net/dsa.h>
Nit, can be in alphabetic order (rest is, so it will be cleaner to add
new following alphabetic order).
>
> #include "mvpp2.h"
> #include "mvpp2_prs.h"
> @@ -4769,6 +4770,36 @@ static bool mvpp22_rss_is_supported(struct mvpp2_port *port)
> !(port->flags & MVPP2_F_LOOPBACK);
> }
>
> +int mvpp2_get_tag(struct net_device *dev)
> +{
> + int tag;
> + int dsa_proto = DSA_TAG_PROTO_NONE;
RCT (int dsa_proto before int tag)
> +
> +#if IS_ENABLED(CONFIG_NET_DSA)
> + if (netdev_uses_dsa(dev))
> + dsa_proto = dev->dsa_ptr->tag_ops->proto;
> +#endif
> +
> + switch (dsa_proto) {
> + case DSA_TAG_PROTO_DSA:
> + tag = MVPP2_TAG_TYPE_DSA;
> + break;
> + case DSA_TAG_PROTO_EDSA:
> + /**
> + * DSA_TAG_PROTO_EDSA and MVPP2_TAG_TYPE_EDSA are
> + * referring to separate things. MVPP2_TAG_TYPE_EDSA
> + * refers to extended DSA, while DSA_TAG_PROTO_EDSA
> + * refers to Ethertype DSA. Ethertype DSA requires no
> + * setting in the parser.
> + */
> + default:
> + tag = MVPP2_TAG_TYPE_MH;
> + break;
if (dsa_proto == DSA_TAG_PROTO_DSA)
return MVPP2_TAG_TYPE_DSA
resturn MVPP2_TAG_TYPE_MH;
looks simpler, you can move comment above the if.
> + }
> +
> + return tag;
> +}
> +
> static int mvpp2_open(struct net_device *dev)
> {
> struct mvpp2_port *port = netdev_priv(dev);
> @@ -4788,7 +4819,11 @@ static int mvpp2_open(struct net_device *dev)
> netdev_err(dev, "mvpp2_prs_mac_da_accept own addr failed\n");
> return err;
> }
> - err = mvpp2_prs_tag_mode_set(port->priv, port->id, MVPP2_TAG_TYPE_MH);
> +
> + if (netdev_uses_dsa(dev))
> + err = mvpp2_prs_tag_mode_set(port->priv, port->id, mvpp2_get_tag(dev));
In mvpp2_get_tag() netdev_uses_dsa() is guarded by CONFIG_NET_DSA,
shouldn't it be here too? Or better, check for it is already in
mvpp2_get_tag() you can simple call
err = mvpp2_prs_tag_mode_set(port->priv, port->id, mvpp2_get_tag(dev));
as mvpp2_get_tag(dev) returns MVPP2_TAG_TYPE_MH in case
netdev_uses_dsa() returns 0.
> + else
> + err = mvpp2_prs_tag_mode_set(port->priv, port->id, MVPP2_TAG_TYPE_MH);
> if (err) {
> netdev_err(dev, "mvpp2_prs_tag_mode_set failed\n");
> return err;
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c
> index 9af22f497a40..f14b9e8c301e 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c
> @@ -1963,7 +1963,7 @@ int mvpp2_prs_vid_entry_add(struct mvpp2_port *port, u16 vid)
> {
> unsigned int vid_start = MVPP2_PE_VID_FILT_RANGE_START +
> port->id * MVPP2_PRS_VLAN_FILT_MAX;
> - unsigned int mask = 0xfff, reg_val, shift;
> + unsigned int mask = 0xfff, tag, shift;
> struct mvpp2 *priv = port->priv;
> struct mvpp2_prs_entry pe;
> int tid;
> @@ -1973,8 +1973,8 @@ int mvpp2_prs_vid_entry_add(struct mvpp2_port *port, u16 vid)
> /* Scan TCAM and see if entry with this <vid,port> already exist */
> tid = mvpp2_prs_vid_range_find(port, vid, mask);
>
> - reg_val = mvpp2_read(priv, MVPP2_MH_REG(port->id));
> - if (reg_val & MVPP2_DSA_EXTENDED)
> + tag = mvpp2_get_tag(port->dev);
> + if (tag & MVPP2_DSA_EXTENDED)
I will drop tag and use mvpp2_get_tag() directly (it isn't used anywhere
else in this function). This is only preference, ignore it if you prefer
to store it somewhere.
Am I right that code under if is unreachable? tag can be 1 or 2
(MVPP2_TAG_TYPE_MH or MVPP2_TAG_TYPE_DSA) and MVPP2_DSA_EXTENDED is
BIT(5). It looks like sth is missing between getting the tag and
checking it with this value.
> shift = MVPP2_VLAN_TAG_EDSA_LEN;
> else
> shift = MVPP2_VLAN_TAG_LEN;
> @@ -2071,7 +2071,7 @@ void mvpp2_prs_vid_enable_filtering(struct mvpp2_port *port)
> {
> unsigned int tid = MVPP2_PRS_VID_PORT_DFLT(port->id);
> struct mvpp2 *priv = port->priv;
> - unsigned int reg_val, shift;
> + unsigned int tag, shift;
> struct mvpp2_prs_entry pe;
>
> if (priv->prs_shadow[tid].valid)
> @@ -2081,8 +2081,8 @@ void mvpp2_prs_vid_enable_filtering(struct mvpp2_port *port)
>
> pe.index = tid;
>
> - reg_val = mvpp2_read(priv, MVPP2_MH_REG(port->id));
> - if (reg_val & MVPP2_DSA_EXTENDED)
> + tag = mvpp2_get_tag(port->dev);
> + if (tag & MVPP2_DSA_EXTENDED)
The same here, unreachable.
> shift = MVPP2_VLAN_TAG_EDSA_LEN;
> else
> shift = MVPP2_VLAN_TAG_LEN;
> @@ -2393,6 +2393,8 @@ int mvpp2_prs_tag_mode_set(struct mvpp2 *priv, int port, int type)
> MVPP2_PRS_TAGGED, MVPP2_PRS_DSA);
> mvpp2_prs_dsa_tag_set(priv, port, false,
> MVPP2_PRS_UNTAGGED, MVPP2_PRS_DSA);
> + /* Set Marvell Header register for Ext. DSA tag */
> + mvpp2_write(priv, MVPP2_MH_REG(port), MVPP2_DSA_EXTENDED);
> break;
>
> case MVPP2_TAG_TYPE_DSA:
> @@ -2406,6 +2408,8 @@ int mvpp2_prs_tag_mode_set(struct mvpp2 *priv, int port, int type)
> MVPP2_PRS_TAGGED, MVPP2_PRS_EDSA);
> mvpp2_prs_dsa_tag_set(priv, port, false,
> MVPP2_PRS_UNTAGGED, MVPP2_PRS_EDSA);
> + /* Set Marvell Header register for DSA tag */
> + mvpp2_write(priv, MVPP2_MH_REG(port), MVPP2_DSA_NON_EXTENDED);
> break;
>
> case MVPP2_TAG_TYPE_MH:
> --
> 2.47.1
Powered by blists - more mailing lists