diff mbox series

[net-next,02/13] bnxt_en: Add ethtool -N support for ether filters.

Message ID 20240205223202.25341-3-michael.chan@broadcom.com (mailing list archive)
State Accepted
Commit e462998abc6280ac093ca18e312d2a8ccc0df64b
Delegated to: Netdev Maintainers
Headers show
Series bnxt_en: Ntuple and RSS updates | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1048 this patch: 1048
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 1065 this patch: 1065
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1069 this patch: 1069
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 139 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-09--00-00 (tests: 687)

Commit Message

Michael Chan Feb. 5, 2024, 10:31 p.m. UTC 
Add ETHTOOL_SRXCLSRLINS and ETHTOOL_SRXCLSRLDEL support for inserting
and deleting L2 ether filter rules.  Destination MAC address and
optional VLAN are supported for each filter entry.  This is currently
only supported on older BCM573XX and BCM574XX chips only.

Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 34 +++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  3 +
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 69 ++++++++++++++++++-
 3 files changed, 103 insertions(+), 3 deletions(-)

Comments

Michal Swiatkowski Feb. 6, 2024, 7:04 a.m. UTC | #1 
On Mon, Feb 05, 2024 at 02:31:51PM -0800, Michael Chan wrote:
> Add ETHTOOL_SRXCLSRLINS and ETHTOOL_SRXCLSRLDEL support for inserting
> and deleting L2 ether filter rules.  Destination MAC address and
> optional VLAN are supported for each filter entry.  This is currently
> only supported on older BCM573XX and BCM574XX chips only.
> 
> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 34 +++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  3 +
>  .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 69 ++++++++++++++++++-
>  3 files changed, 103 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 91ecf514b924..3cc3504181c7 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -5519,6 +5519,40 @@ static struct bnxt_l2_filter *bnxt_alloc_l2_filter(struct bnxt *bp,
>  	return fltr;
>  }
>  
> +struct bnxt_l2_filter *bnxt_alloc_new_l2_filter(struct bnxt *bp,
> +						struct bnxt_l2_key *key,
> +						u16 flags)
> +{
> +	struct bnxt_l2_filter *fltr;
> +	u32 idx;
> +	int rc;
> +
> +	idx = jhash2(&key->filter_key, BNXT_L2_KEY_SIZE, bp->hash_seed) &
> +	      BNXT_L2_FLTR_HASH_MASK;
> +	spin_lock_bh(&bp->ntp_fltr_lock);
> +	fltr = __bnxt_lookup_l2_filter(bp, key, idx);
> +	if (fltr) {
> +		fltr = ERR_PTR(-EEXIST);
> +		goto l2_filter_exit;
> +	}
> +	fltr = kzalloc(sizeof(*fltr), GFP_ATOMIC);
> +	if (!fltr) {
> +		fltr = ERR_PTR(-ENOMEM);
> +		goto l2_filter_exit;
> +	}
> +	fltr->base.flags = flags;
> +	rc = bnxt_init_l2_filter(bp, fltr, key, idx);
> +	if (rc) {
> +		spin_unlock_bh(&bp->ntp_fltr_lock);
Why filter needs to be deleted without lock? If you can change the order
it looks more natural:

+if (rc) {
+	fltr = ERR_PTR(rc);
+	goto l2_filter_del;
+}

> +		bnxt_del_l2_filter(bp, fltr);
> +		return ERR_PTR(rc);
> +	}
> +
> +l2_filter_exit:
> +	spin_unlock_bh(&bp->ntp_fltr_lock);
> +	return fltr;
> +}
> +
>  static u16 bnxt_vf_target_id(struct bnxt_pf_info *pf, u16 vf_idx)
>  {
>  #ifdef CONFIG_BNXT_SRIOV
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index 4bd1cf01d99e..21721b8748bc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -2646,6 +2646,9 @@ int bnxt_hwrm_func_drv_rgtr(struct bnxt *bp, unsigned long *bmap,
>  			    int bmap_size, bool async_only);
>  int bnxt_hwrm_func_drv_unrgtr(struct bnxt *bp);
>  void bnxt_del_l2_filter(struct bnxt *bp, struct bnxt_l2_filter *fltr);
> +struct bnxt_l2_filter *bnxt_alloc_new_l2_filter(struct bnxt *bp,
> +						struct bnxt_l2_key *key,
> +						u16 flags);
>  int bnxt_hwrm_l2_filter_free(struct bnxt *bp, struct bnxt_l2_filter *fltr);
>  int bnxt_hwrm_l2_filter_alloc(struct bnxt *bp, struct bnxt_l2_filter *fltr);
>  int bnxt_hwrm_cfa_ntuple_filter_free(struct bnxt *bp,
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 1c8610386404..2d8e847e8fdd 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -1152,6 +1152,58 @@ static int bnxt_grxclsrule(struct bnxt *bp, struct ethtool_rxnfc *cmd)
>  	return rc;
>  }
>  
> +static int bnxt_add_l2_cls_rule(struct bnxt *bp,
> +				struct ethtool_rx_flow_spec *fs)
> +{
> +	u32 ring = ethtool_get_flow_spec_ring(fs->ring_cookie);
> +	u8 vf = ethtool_get_flow_spec_ring_vf(fs->ring_cookie);
> +	struct ethhdr *h_ether = &fs->h_u.ether_spec;
> +	struct ethhdr *m_ether = &fs->m_u.ether_spec;
> +	struct bnxt_l2_filter *fltr;
> +	struct bnxt_l2_key key;
> +	u16 vnic_id;
> +	u8 flags;
> +	int rc;
> +
> +	if (BNXT_CHIP_P5_PLUS(bp))
> +		return -EOPNOTSUPP;
> +
> +	if (!is_broadcast_ether_addr(m_ether->h_dest))
> +		return -EINVAL;
> +	ether_addr_copy(key.dst_mac_addr, h_ether->h_dest);
> +	key.vlan = 0;
> +	if (fs->flow_type & FLOW_EXT) {
> +		struct ethtool_flow_ext *m_ext = &fs->m_ext;
> +		struct ethtool_flow_ext *h_ext = &fs->h_ext;
> +
> +		if (m_ext->vlan_tci != htons(0xfff) || !h_ext->vlan_tci)
> +			return -EINVAL;
> +		key.vlan = ntohs(h_ext->vlan_tci);
> +	}
> +
> +	if (vf) {
> +		flags = BNXT_ACT_FUNC_DST;
> +		vnic_id = 0xffff;
> +		vf--;
> +	} else {
> +		flags = BNXT_ACT_RING_DST;
> +		vnic_id = bp->vnic_info[ring + 1].fw_vnic_id;
> +	}
> +	fltr = bnxt_alloc_new_l2_filter(bp, &key, flags);
> +	if (IS_ERR(fltr))
> +		return PTR_ERR(fltr);
> +
> +	fltr->base.fw_vnic_id = vnic_id;
> +	fltr->base.rxq = ring;
> +	fltr->base.vf_idx = vf;
> +	rc = bnxt_hwrm_l2_filter_alloc(bp, fltr);
> +	if (rc)
> +		bnxt_del_l2_filter(bp, fltr);
> +	else
> +		fs->location = fltr->base.sw_id;
> +	return rc;
> +}
> +
>  #define IPV4_ALL_MASK		((__force __be32)~0)
>  #define L4_PORT_ALL_MASK	((__force __be16)~0)
>  
> @@ -1335,7 +1387,7 @@ static int bnxt_srxclsrlins(struct bnxt *bp, struct ethtool_rxnfc *cmd)
>  		return -EINVAL;
>  	flow_type &= ~FLOW_EXT;
>  	if (flow_type == ETHER_FLOW)
> -		rc = -EOPNOTSUPP;
> +		rc = bnxt_add_l2_cls_rule(bp, fs);
>  	else
>  		rc = bnxt_add_ntuple_cls_rule(bp, fs);
>  	return rc;
> @@ -1346,11 +1398,22 @@ static int bnxt_srxclsrldel(struct bnxt *bp, struct ethtool_rxnfc *cmd)
>  	struct ethtool_rx_flow_spec *fs = &cmd->fs;
>  	struct bnxt_filter_base *fltr_base;
>  	struct bnxt_ntuple_filter *fltr;
> +	u32 id = fs->location;
>  
>  	rcu_read_lock();
> +	fltr_base = bnxt_get_one_fltr_rcu(bp, bp->l2_fltr_hash_tbl,
> +					  BNXT_L2_FLTR_HASH_SIZE, id);
> +	if (fltr_base) {
> +		struct bnxt_l2_filter *l2_fltr;
> +
> +		l2_fltr = container_of(fltr_base, struct bnxt_l2_filter, base);
> +		rcu_read_unlock();
> +		bnxt_hwrm_l2_filter_free(bp, l2_fltr);
> +		bnxt_del_l2_filter(bp, l2_fltr);
> +		return 0;
> +	}
>  	fltr_base = bnxt_get_one_fltr_rcu(bp, bp->ntp_fltr_hash_tbl,
> -					  BNXT_NTP_FLTR_HASH_SIZE,
> -					  fs->location);
> +					  BNXT_NTP_FLTR_HASH_SIZE, id);
>  	if (!fltr_base) {
>  		rcu_read_unlock();
>  		return -ENOENT;
> -- 
> 2.30.1
>
Michael Chan Feb. 6, 2024, 8:10 a.m. UTC | #2 
On Mon, Feb 5, 2024 at 11:04 PM Michal Swiatkowski
<michal.swiatkowski@linux.intel.com> wrote:
>
> On Mon, Feb 05, 2024 at 02:31:51PM -0800, Michael Chan wrote:
> > +     spin_lock_bh(&bp->ntp_fltr_lock);
> > +     fltr = __bnxt_lookup_l2_filter(bp, key, idx);
> > +     if (fltr) {
> > +             fltr = ERR_PTR(-EEXIST);
> > +             goto l2_filter_exit;
> > +     }
> > +     fltr = kzalloc(sizeof(*fltr), GFP_ATOMIC);
> > +     if (!fltr) {
> > +             fltr = ERR_PTR(-ENOMEM);
> > +             goto l2_filter_exit;
> > +     }
> > +     fltr->base.flags = flags;
> > +     rc = bnxt_init_l2_filter(bp, fltr, key, idx);
> > +     if (rc) {
> > +             spin_unlock_bh(&bp->ntp_fltr_lock);
> Why filter needs to be deleted without lock? If you can change the order
> it looks more natural:
>
> +if (rc) {
> +       fltr = ERR_PTR(rc);
> +       goto l2_filter_del;
> +}

Thanks for the review.  bnxt_del_l2_filter() will take the same lock
inside the function if it goes ahead to delete the filter.  That's why
the lock needs to be released first.
Michal Swiatkowski Feb. 6, 2024, 1:45 p.m. UTC | #3 
On Tue, Feb 06, 2024 at 12:10:32AM -0800, Michael Chan wrote:
> On Mon, Feb 5, 2024 at 11:04 PM Michal Swiatkowski
> <michal.swiatkowski@linux.intel.com> wrote:
> >
> > On Mon, Feb 05, 2024 at 02:31:51PM -0800, Michael Chan wrote:
> > > +     spin_lock_bh(&bp->ntp_fltr_lock);
> > > +     fltr = __bnxt_lookup_l2_filter(bp, key, idx);
> > > +     if (fltr) {
> > > +             fltr = ERR_PTR(-EEXIST);
> > > +             goto l2_filter_exit;
> > > +     }
> > > +     fltr = kzalloc(sizeof(*fltr), GFP_ATOMIC);
> > > +     if (!fltr) {
> > > +             fltr = ERR_PTR(-ENOMEM);
> > > +             goto l2_filter_exit;
> > > +     }
> > > +     fltr->base.flags = flags;
> > > +     rc = bnxt_init_l2_filter(bp, fltr, key, idx);
> > > +     if (rc) {
> > > +             spin_unlock_bh(&bp->ntp_fltr_lock);
> > Why filter needs to be deleted without lock? If you can change the order
> > it looks more natural:
> >
> > +if (rc) {
> > +       fltr = ERR_PTR(rc);
> > +       goto l2_filter_del;
> > +}
> 
> Thanks for the review.  bnxt_del_l2_filter() will take the same lock
> inside the function if it goes ahead to delete the filter.  That's why
> the lock needs to be released first.

Got it, thanks.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 91ecf514b924..3cc3504181c7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -5519,6 +5519,40 @@  static struct bnxt_l2_filter *bnxt_alloc_l2_filter(struct bnxt *bp,
 	return fltr;
 }
 
+struct bnxt_l2_filter *bnxt_alloc_new_l2_filter(struct bnxt *bp,
+						struct bnxt_l2_key *key,
+						u16 flags)
+{
+	struct bnxt_l2_filter *fltr;
+	u32 idx;
+	int rc;
+
+	idx = jhash2(&key->filter_key, BNXT_L2_KEY_SIZE, bp->hash_seed) &
+	      BNXT_L2_FLTR_HASH_MASK;
+	spin_lock_bh(&bp->ntp_fltr_lock);
+	fltr = __bnxt_lookup_l2_filter(bp, key, idx);
+	if (fltr) {
+		fltr = ERR_PTR(-EEXIST);
+		goto l2_filter_exit;
+	}
+	fltr = kzalloc(sizeof(*fltr), GFP_ATOMIC);
+	if (!fltr) {
+		fltr = ERR_PTR(-ENOMEM);
+		goto l2_filter_exit;
+	}
+	fltr->base.flags = flags;
+	rc = bnxt_init_l2_filter(bp, fltr, key, idx);
+	if (rc) {
+		spin_unlock_bh(&bp->ntp_fltr_lock);
+		bnxt_del_l2_filter(bp, fltr);
+		return ERR_PTR(rc);
+	}
+
+l2_filter_exit:
+	spin_unlock_bh(&bp->ntp_fltr_lock);
+	return fltr;
+}
+
 static u16 bnxt_vf_target_id(struct bnxt_pf_info *pf, u16 vf_idx)
 {
 #ifdef CONFIG_BNXT_SRIOV
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 4bd1cf01d99e..21721b8748bc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2646,6 +2646,9 @@  int bnxt_hwrm_func_drv_rgtr(struct bnxt *bp, unsigned long *bmap,
 			    int bmap_size, bool async_only);
 int bnxt_hwrm_func_drv_unrgtr(struct bnxt *bp);
 void bnxt_del_l2_filter(struct bnxt *bp, struct bnxt_l2_filter *fltr);
+struct bnxt_l2_filter *bnxt_alloc_new_l2_filter(struct bnxt *bp,
+						struct bnxt_l2_key *key,
+						u16 flags);
 int bnxt_hwrm_l2_filter_free(struct bnxt *bp, struct bnxt_l2_filter *fltr);
 int bnxt_hwrm_l2_filter_alloc(struct bnxt *bp, struct bnxt_l2_filter *fltr);
 int bnxt_hwrm_cfa_ntuple_filter_free(struct bnxt *bp,
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 1c8610386404..2d8e847e8fdd 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1152,6 +1152,58 @@  static int bnxt_grxclsrule(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 	return rc;
 }
 
+static int bnxt_add_l2_cls_rule(struct bnxt *bp,
+				struct ethtool_rx_flow_spec *fs)
+{
+	u32 ring = ethtool_get_flow_spec_ring(fs->ring_cookie);
+	u8 vf = ethtool_get_flow_spec_ring_vf(fs->ring_cookie);
+	struct ethhdr *h_ether = &fs->h_u.ether_spec;
+	struct ethhdr *m_ether = &fs->m_u.ether_spec;
+	struct bnxt_l2_filter *fltr;
+	struct bnxt_l2_key key;
+	u16 vnic_id;
+	u8 flags;
+	int rc;
+
+	if (BNXT_CHIP_P5_PLUS(bp))
+		return -EOPNOTSUPP;
+
+	if (!is_broadcast_ether_addr(m_ether->h_dest))
+		return -EINVAL;
+	ether_addr_copy(key.dst_mac_addr, h_ether->h_dest);
+	key.vlan = 0;
+	if (fs->flow_type & FLOW_EXT) {
+		struct ethtool_flow_ext *m_ext = &fs->m_ext;
+		struct ethtool_flow_ext *h_ext = &fs->h_ext;
+
+		if (m_ext->vlan_tci != htons(0xfff) || !h_ext->vlan_tci)
+			return -EINVAL;
+		key.vlan = ntohs(h_ext->vlan_tci);
+	}
+
+	if (vf) {
+		flags = BNXT_ACT_FUNC_DST;
+		vnic_id = 0xffff;
+		vf--;
+	} else {
+		flags = BNXT_ACT_RING_DST;
+		vnic_id = bp->vnic_info[ring + 1].fw_vnic_id;
+	}
+	fltr = bnxt_alloc_new_l2_filter(bp, &key, flags);
+	if (IS_ERR(fltr))
+		return PTR_ERR(fltr);
+
+	fltr->base.fw_vnic_id = vnic_id;
+	fltr->base.rxq = ring;
+	fltr->base.vf_idx = vf;
+	rc = bnxt_hwrm_l2_filter_alloc(bp, fltr);
+	if (rc)
+		bnxt_del_l2_filter(bp, fltr);
+	else
+		fs->location = fltr->base.sw_id;
+	return rc;
+}
+
 #define IPV4_ALL_MASK		((__force __be32)~0)
 #define L4_PORT_ALL_MASK	((__force __be16)~0)
 
@@ -1335,7 +1387,7 @@  static int bnxt_srxclsrlins(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 		return -EINVAL;
 	flow_type &= ~FLOW_EXT;
 	if (flow_type == ETHER_FLOW)
-		rc = -EOPNOTSUPP;
+		rc = bnxt_add_l2_cls_rule(bp, fs);
 	else
 		rc = bnxt_add_ntuple_cls_rule(bp, fs);
 	return rc;
@@ -1346,11 +1398,22 @@  static int bnxt_srxclsrldel(struct bnxt *bp, struct ethtool_rxnfc *cmd)
 	struct ethtool_rx_flow_spec *fs = &cmd->fs;
 	struct bnxt_filter_base *fltr_base;
 	struct bnxt_ntuple_filter *fltr;
+	u32 id = fs->location;
 
 	rcu_read_lock();
+	fltr_base = bnxt_get_one_fltr_rcu(bp, bp->l2_fltr_hash_tbl,
+					  BNXT_L2_FLTR_HASH_SIZE, id);
+	if (fltr_base) {
+		struct bnxt_l2_filter *l2_fltr;
+
+		l2_fltr = container_of(fltr_base, struct bnxt_l2_filter, base);
+		rcu_read_unlock();
+		bnxt_hwrm_l2_filter_free(bp, l2_fltr);
+		bnxt_del_l2_filter(bp, l2_fltr);
+		return 0;
+	}
 	fltr_base = bnxt_get_one_fltr_rcu(bp, bp->ntp_fltr_hash_tbl,
-					  BNXT_NTP_FLTR_HASH_SIZE,
-					  fs->location);
+					  BNXT_NTP_FLTR_HASH_SIZE, id);
 	if (!fltr_base) {
 		rcu_read_unlock();
 		return -ENOENT;