[PATCH net v2 0/6] bnxt_en: Fixes for net.

[PATCH net v2 0/6] bnxt_en: Fixes for net.

[Michael Chan]

This bug fix series include NULL pointer fixes in ethtool -x code path
and in the error clean up path when freeing IRQs, a ring accounting bug
that missed rings used by the RDMA driver, and 3 bug fixes related to TC
Flower and VF-reps.

v2: Fixed commit message of patch 4.  Changed the pound sign to $ sign
in front of the ip command.
 
Andy Gospodarek (1):
  bnxt_en: do not allow wildcard matches for L2 flows

Michael Chan (3):
  bnxt_en: Fix ethtool -x crash when device is down.
  bnxt_en: Need to include RDMA rings in bnxt_check_rings().
  bnxt_en: Fix NULL pointer dereference at bnxt_free_irq().

Sriharsha Basavapatna (2):
  bnxt_en: Ignore src port field in decap filter nodes
  bnxt_en: Support max-mtu with VF-reps

 drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  4 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 11 ++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c      | 63 ++++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c     | 30 +++++++++++
 4 files changed, 103 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH net v2 1/6] bnxt_en: Fix ethtool -x crash when device is down.

[Michael Chan]

Fix ethtool .get_rxfh() crash by checking for valid indirection table
address before copying the data.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 8d8ccd6..1f622ca 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -870,17 +870,22 @@ static int bnxt_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
 			 u8 *hfunc)
 {
 	struct bnxt *bp = netdev_priv(dev);
-	struct bnxt_vnic_info *vnic = &bp->vnic_info[0];
+	struct bnxt_vnic_info *vnic;
 	int i = 0;
 
 	if (hfunc)
 		*hfunc = ETH_RSS_HASH_TOP;
 
-	if (indir)
+	if (!bp->vnic_info)
+		return 0;
+
+	vnic = &bp->vnic_info[0];
+	if (indir && vnic->rss_table) {
 		for (i = 0; i < HW_HASH_INDEX_SIZE; i++)
 			indir[i] = le16_to_cpu(vnic->rss_table[i]);
+	}
 
-	if (key)
+	if (key && vnic->rss_hash_key)
 		memcpy(key, vnic->rss_hash_key, HW_HASH_KEY_SIZE);
 
 	return 0;
-- 
1.8.3.1

[PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows

[Michael Chan]

From: Andy Gospodarek <gospo@broadcom.com>

Before this patch the following commands would succeed as far as the
user was concerned:

$ tc qdisc add dev p1p1 ingress
$ tc filter add dev p1p1 parent ffff: protocol all \
	flower skip_sw action drop
$ tc filter add dev p1p1 parent ffff: protocol ipv4 \
	flower skip_sw src_mac 00:02:00:00:00:01/44 action drop

The current flow offload infrastructure used does not support wildcard
matching for ethernet headers, so do not allow the second or third
commands to succeed.  If a user wants to drop traffic on that interface
the protocol and MAC addresses need to be specified explicitly:

$ tc qdisc add dev p1p1 ingress
$ tc filter add dev p1p1 parent ffff: protocol arp \
	flower skip_sw action drop
$ tc filter add dev p1p1 parent ffff: protocol ipv4 \
	flower skip_sw action drop
...
$ tc filter add dev p1p1 parent ffff: protocol ipv4 \
	flower skip_sw src_mac 00:02:00:00:00:01 action drop
$ tc filter add dev p1p1 parent ffff: protocol ipv4 \
	flower skip_sw src_mac 00:02:00:00:00:02 action drop
...

There are also checks for VLAN parameters in this patch as other callers
may wildcard those parameters even if tc does not.  Using different
flow infrastructure could allow this to work in the future for L2 flows,
but for now it does not.

Fixes: 2ae7408fedfe ("bnxt_en: bnxt: add TC flower filter offload support")
Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 59 ++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 65c2cee..ac193408 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -377,6 +377,30 @@ static bool is_wildcard(void *mask, int len)
 	return true;
 }
 
+static bool is_exactmatch(void *mask, int len)
+{
+	const u8 *p = mask;
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (p[i] != 0xff)
+			return false;
+
+	return true;
+}
+
+static bool bits_set(void *key, int len)
+{
+	const u8 *p = key;
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (p[i] != 0)
+			return true;
+
+	return false;
+}
+
 static int bnxt_hwrm_cfa_flow_alloc(struct bnxt *bp, struct bnxt_tc_flow *flow,
 				    __le16 ref_flow_handle,
 				    __le32 tunnel_handle, __le16 *flow_handle)
@@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
 		return false;
 	}
 
+	/* Currently source/dest MAC cannot be partial wildcard  */
+	if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) &&
+	    !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) {
+		netdev_info(bp->dev, "Wildcard match unsupported for Source MAC\n");
+		return false;
+	}
+	if (bits_set(&flow->l2_key.dmac, sizeof(flow->l2_key.dmac)) &&
+	    !is_exactmatch(&flow->l2_mask.dmac, sizeof(flow->l2_mask.dmac))) {
+		netdev_info(bp->dev, "Wildcard match unsupported for Dest MAC\n");
+		return false;
+	}
+
+	/* Currently VLAN fields cannot be partial wildcard */
+	if (bits_set(&flow->l2_key.inner_vlan_tci,
+		     sizeof(flow->l2_key.inner_vlan_tci)) &&
+	    !is_exactmatch(&flow->l2_mask.inner_vlan_tci,
+			   sizeof(flow->l2_mask.inner_vlan_tci))) {
+		netdev_info(bp->dev, "Wildcard match unsupported for VLAN TCI\n");
+		return false;
+	}
+	if (bits_set(&flow->l2_key.inner_vlan_tpid,
+		     sizeof(flow->l2_key.inner_vlan_tpid)) &&
+	    !is_exactmatch(&flow->l2_mask.inner_vlan_tpid,
+			   sizeof(flow->l2_mask.inner_vlan_tpid))) {
+		netdev_info(bp->dev, "Wildcard match unsupported for VLAN TPID\n");
+		return false;
+	}
+
+	/* Currently Ethertype must be set */
+	if (!is_exactmatch(&flow->l2_mask.ether_type,
+			   sizeof(flow->l2_mask.ether_type))) {
+		netdev_info(bp->dev, "Wildcard match unsupported for Ethertype\n");
+		return false;
+	}
+
 	return true;
 }
 
-- 
1.8.3.1

[PATCH net v2 3/6] bnxt_en: Ignore src port field in decap filter nodes

[Michael Chan]

From: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>

The driver currently uses src port field (along with other fields) in the
decap tunnel key, while looking up and adding tunnel nodes. This leads to
redundant cfa_decap_filter_alloc() requests to the FW and flow-miss in the
flow engine. Fix this by ignoring the src port field in decap tunnel nodes.

Fixes: f484f6782e01 ("bnxt_en: add hwrm FW cmds for cfa_encap_record and decap_filter")
Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index ac193408..795f450 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -1051,8 +1051,10 @@ static int bnxt_tc_get_decap_handle(struct bnxt *bp, struct bnxt_tc_flow *flow,
 
 	/* Check if there's another flow using the same tunnel decap.
 	 * If not, add this tunnel to the table and resolve the other
-	 * tunnel header fileds
+	 * tunnel header fileds. Ignore src_port in the tunnel_key,
+	 * since it is not required for decap filters.
 	 */
+	decap_key->tp_src = 0;
 	decap_node = bnxt_tc_get_tunnel_node(bp, &tc_info->decap_table,
 					     &tc_info->decap_ht_params,
 					     decap_key);
-- 
1.8.3.1

[PATCH net v2 4/6] bnxt_en: Support max-mtu with VF-reps

[Michael Chan]

From: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>

While a VF is configured with a bigger mtu (> 1500), any packets that
are punted to the VF-rep (slow-path) get dropped by OVS kernel-datapath
with the following message: "dropped over-mtu packet". Fix this by
returning the max-mtu value for a VF-rep derived from its corresponding VF.
VF-rep's mtu can be changed using 'ip' command as shown in this example:

	$ ip link set bnxt0_pf0vf0 mtu 9000

Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 30 +++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index 2629040..38f635c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -64,6 +64,31 @@ static int hwrm_cfa_vfr_free(struct bnxt *bp, u16 vf_idx)
 	return rc;
 }
 
+static int bnxt_hwrm_vfr_qcfg(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
+			      u16 *max_mtu)
+{
+	struct hwrm_func_qcfg_output *resp = bp->hwrm_cmd_resp_addr;
+	struct hwrm_func_qcfg_input req = {0};
+	u16 mtu;
+	int rc;
+
+	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_FUNC_QCFG, -1, -1);
+	req.fid = cpu_to_le16(bp->pf.vf[vf_rep->vf_idx].fw_fid);
+
+	mutex_lock(&bp->hwrm_cmd_lock);
+
+	rc = _hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
+	if (!rc) {
+		mtu = le16_to_cpu(resp->max_mtu_configured);
+		if (!mtu)
+			*max_mtu = BNXT_MAX_MTU;
+		else
+			*max_mtu = mtu;
+	}
+	mutex_unlock(&bp->hwrm_cmd_lock);
+	return rc;
+}
+
 static int bnxt_vf_rep_open(struct net_device *dev)
 {
 	struct bnxt_vf_rep *vf_rep = netdev_priv(dev);
@@ -365,6 +390,7 @@ static void bnxt_vf_rep_netdev_init(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
 				    struct net_device *dev)
 {
 	struct net_device *pf_dev = bp->dev;
+	u16 max_mtu;
 
 	dev->netdev_ops = &bnxt_vf_rep_netdev_ops;
 	dev->ethtool_ops = &bnxt_vf_rep_ethtool_ops;
@@ -380,6 +406,10 @@ static void bnxt_vf_rep_netdev_init(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
 	bnxt_vf_rep_eth_addr_gen(bp->pf.mac_addr, vf_rep->vf_idx,
 				 dev->perm_addr);
 	ether_addr_copy(dev->dev_addr, dev->perm_addr);
+	/* Set VF-Rep's max-mtu to the corresponding VF's max-mtu */
+	if (!bnxt_hwrm_vfr_qcfg(bp, vf_rep, &max_mtu))
+		dev->max_mtu = max_mtu;
+	dev->min_mtu = ETH_ZLEN;
 }
 
 static int bnxt_pcie_dsn_get(struct bnxt *bp, u8 dsn[])
-- 
1.8.3.1

[PATCH net v2 5/6] bnxt_en: Need to include RDMA rings in bnxt_check_rings().

[Michael Chan]

With recent changes to reserve both L2 and RDMA rings, we need to include
the RDMA rings in bnxt_check_rings().  Otherwise we will under-estimate
the rings we need during ethtool -L and may lead to failure.

Fixes: fbcfc8e46741 ("bnxt_en: Reserve completion rings and MSIX for bnxt_re RDMA driver.")
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 1991f0c..9cb8b4b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7686,6 +7686,8 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
 	if (bp->flags & BNXT_FLAG_AGG_RINGS)
 		rx_rings <<= 1;
 	cp = sh ? max_t(int, tx_rings_needed, rx) : tx_rings_needed + rx;
+	if (bp->flags & BNXT_FLAG_NEW_RM)
+		cp += bnxt_get_ulp_msix_num(bp);
 	return bnxt_hwrm_check_rings(bp, tx_rings_needed, rx_rings, rx, cp,
 				     vnics);
 }
-- 
1.8.3.1

[PATCH net v2 6/6] bnxt_en: Fix NULL pointer dereference at bnxt_free_irq().

[Michael Chan]

When open fails during ethtool -L ring change, for example, the driver
may crash at bnxt_free_irq() because bp->bnapi is NULL.

If we fail to allocate all the new rings, bnxt_open_nic() will free
all the memory including bp->bnapi.  Subsequent call to bnxt_close_nic()
will try to dereference bp->bnapi in bnxt_free_irq().

Fix it by checking for !bp->bnapi in bnxt_free_irq().

Fixes: e5811b8c09df ("bnxt_en: Add IRQ remapping logic.")
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 9cb8b4b..f83769d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -6090,7 +6090,7 @@ static void bnxt_free_irq(struct bnxt *bp)
 	free_irq_cpu_rmap(bp->dev->rx_cpu_rmap);
 	bp->dev->rx_cpu_rmap = NULL;
 #endif
-	if (!bp->irq_tbl)
+	if (!bp->irq_tbl || !bp->bnapi)
 		return;
 
 	for (i = 0; i < bp->cp_nr_rings; i++) {
-- 
1.8.3.1

Re: [PATCH net v2 0/6] bnxt_en: Fixes for net.

[David Miller]

From: Michael Chan <michael.chan@broadcom.com>
Date: Wed, 11 Apr 2018 11:50:12 -0400

> This bug fix series include NULL pointer fixes in ethtool -x code path
> and in the error clean up path when freeing IRQs, a ring accounting bug
> that missed rings used by the RDMA driver, and 3 bug fixes related to TC
> Flower and VF-reps.
> 
> v2: Fixed commit message of patch 4.  Changed the pound sign to $ sign
> in front of the ip command.

Yep, that looks better.

Series applied, thanks Michael.

Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows

[Jakub Kicinski]

On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote:
> @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
>  		return false;
>  	}
>  
> +	/* Currently source/dest MAC cannot be partial wildcard  */
> +	if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) &&
> +	    !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) {
> +		netdev_info(bp->dev, "Wildcard match unsupported for Source MAC\n");

This wouldn't be something to do in net, but how do you feel about
using extack for messages like this?

> +		return false;
> +	}

Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows

[Michael Chan]

On Wed, Apr 11, 2018 at 11:43 AM, Jakub Kicinski <kubakici@wp.pl> wrote:
> On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote:
>> @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
>>               return false;
>>       }
>>
>> +     /* Currently source/dest MAC cannot be partial wildcard  */
>> +     if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) &&
>> +         !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) {
>> +             netdev_info(bp->dev, "Wildcard match unsupported for Source MAC\n");
>
> This wouldn't be something to do in net, but how do you feel about
> using extack for messages like this?
>

Sounds reasonable to me.  Just need to pass in the extack pointer to
this function to set the netlink error message.

Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows

[Andy Gospodarek]

On Wed, Apr 11, 2018 at 11:43:14AM -0700, Jakub Kicinski wrote:
> On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote:
> > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
> >  		return false;
> >  	}
> >  
> > +	/* Currently source/dest MAC cannot be partial wildcard  */
> > +	if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) &&
> > +	    !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) {
> > +		netdev_info(bp->dev, "Wildcard match unsupported for Source MAC\n");
> 
> This wouldn't be something to do in net, but how do you feel about
> using extack for messages like this?
> 

I agree 'net' would not have been the place for a change like that, but
I do think that would be a good idea.  It looks like we could easily
change the ndo_setup_tc to something like this:

        int                     (*ndo_setup_tc)(struct net_device *dev,
                                                enum tc_setup_type type,
                                                void *type_data,
						struct netlink_ext_ack *extack);

It also looks like most of the callers of ndo_setup_tc have infra in
place to pass extack easily when the call is sourced from a netlink
message.   The others can just pass in NULL or define a local
netlink_ext_ack variable for short-term use.

Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows

[Michael Chan]

On Wed, Apr 11, 2018 at 1:31 PM, Andy Gospodarek
<andrew.gospodarek@broadcom.com> wrote:
> On Wed, Apr 11, 2018 at 11:43:14AM -0700, Jakub Kicinski wrote:
>> On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote:
>> > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
>> >             return false;
>> >     }
>> >
>> > +   /* Currently source/dest MAC cannot be partial wildcard  */
>> > +   if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) &&
>> > +       !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) {
>> > +           netdev_info(bp->dev, "Wildcard match unsupported for Source MAC\n");
>>
>> This wouldn't be something to do in net, but how do you feel about
>> using extack for messages like this?
>>
>
> I agree 'net' would not have been the place for a change like that, but
> I do think that would be a good idea.  It looks like we could easily
> change the ndo_setup_tc to something like this:
>
>         int                     (*ndo_setup_tc)(struct net_device *dev,
>                                                 enum tc_setup_type type,
>                                                 void *type_data,
>                                                 struct netlink_ext_ack *extack);

I think the extack pointer is already in the tc_cls_common_offload
struct inside tc_cls_flower_offload struct.

Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows

[Andy Gospodarek]

On Wed, Apr 11, 2018 at 01:41:31PM -0700, Michael Chan wrote:
> On Wed, Apr 11, 2018 at 1:31 PM, Andy Gospodarek
> <andrew.gospodarek@broadcom.com> wrote:
> > On Wed, Apr 11, 2018 at 11:43:14AM -0700, Jakub Kicinski wrote:
> >> On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote:
> >> > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
> >> >             return false;
> >> >     }
> >> >
> >> > +   /* Currently source/dest MAC cannot be partial wildcard  */
> >> > +   if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) &&
> >> > +       !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) {
> >> > +           netdev_info(bp->dev, "Wildcard match unsupported for Source MAC\n");
> >>
> >> This wouldn't be something to do in net, but how do you feel about
> >> using extack for messages like this?
> >>
> >
> > I agree 'net' would not have been the place for a change like that, but
> > I do think that would be a good idea.  It looks like we could easily
> > change the ndo_setup_tc to something like this:
> >
> >         int                     (*ndo_setup_tc)(struct net_device *dev,
> >                                                 enum tc_setup_type type,
> >                                                 void *type_data,
> >                                                 struct netlink_ext_ack *extack);
> 
> I think the extack pointer is already in the tc_cls_common_offload
> struct inside tc_cls_flower_offload struct.

True, but I'm not sure that tc_cls_common_offload is used in all cases.
Take red_offload() as one of those.

Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows

[Michael Chan]

On Wed, Apr 11, 2018 at 1:50 PM, Andy Gospodarek
<andrew.gospodarek@broadcom.com> wrote:
> On Wed, Apr 11, 2018 at 01:41:31PM -0700, Michael Chan wrote:
>> On Wed, Apr 11, 2018 at 1:31 PM, Andy Gospodarek
>> <andrew.gospodarek@broadcom.com> wrote:
>> > On Wed, Apr 11, 2018 at 11:43:14AM -0700, Jakub Kicinski wrote:
>> >> On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote:
>> >> > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct bnxt_tc_flow *flow)
>> >> >             return false;
>> >> >     }
>> >> >
>> >> > +   /* Currently source/dest MAC cannot be partial wildcard  */
>> >> > +   if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) &&
>> >> > +       !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) {
>> >> > +           netdev_info(bp->dev, "Wildcard match unsupported for Source MAC\n");
>> >>
>> >> This wouldn't be something to do in net, but how do you feel about
>> >> using extack for messages like this?
>> >>
>> >
>> > I agree 'net' would not have been the place for a change like that, but
>> > I do think that would be a good idea.  It looks like we could easily
>> > change the ndo_setup_tc to something like this:
>> >
>> >         int                     (*ndo_setup_tc)(struct net_device *dev,
>> >                                                 enum tc_setup_type type,
>> >                                                 void *type_data,
>> >                                                 struct netlink_ext_ack *extack);
>>
>> I think the extack pointer is already in the tc_cls_common_offload
>> struct inside tc_cls_flower_offload struct.
>
> True, but I'm not sure that tc_cls_common_offload is used in all cases.
> Take red_offload() as one of those.

For Flower, we know we have the extack pointer in
tc_cls_common_offload struct and we can use it to set the netlink
error message.  The point is that we don't have to modify
ndo_setup_tc().

Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows

[Jakub Kicinski]

On Wed, 11 Apr 2018 13:55:11 -0700, Michael Chan wrote:
> On Wed, Apr 11, 2018 at 1:50 PM, Andy Gospodarek wrote:
> > On Wed, Apr 11, 2018 at 01:41:31PM -0700, Michael Chan wrote:  
> > True, but I'm not sure that tc_cls_common_offload is used in all cases.
> > Take red_offload() as one of those.  
> 
> For Flower, we know we have the extack pointer in
> tc_cls_common_offload struct and we can use it to set the netlink
> error message.  The point is that we don't have to modify
> ndo_setup_tc().

Yes, the extack is actually only populated when skip_sw is specified to
avoid warning users who don't care about offloads.

Flower offloads don't go via .ndo_setup_tc but TC block callbacks.  But
one day we will hopefully find a reasonable way to pass extack to qdisc
offloads as well..

FWIW your driver is actually already using extack under the veil of
tc_cls_can_offload_and_chain0() :)