Message ID | 1523461818-15774-3-git-send-email-michael.chan@broadcom.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Series | bnxt_en: Fixes for net. | expand |
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; > + }
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.
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.
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.
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.
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().
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() :)
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; }