Message ID | 1500914069-15724-11-git-send-email-michael.chan@broadcom.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Hi Sathya, [auto build test ERROR on net-next/master] url: https://github.com/0day-ci/linux/commits/Michael-Chan/bnxt_en-Update-firmware-interface-spec-to-1-8-0/20170725-094835 config: x86_64-randconfig-x016-201730 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_init_one': >> drivers/net/ethernet/broadcom/bnxt/bnxt.c:7897:5: error: 'struct net_device' has no member named 'switchdev_ops'; did you mean 'netdev_ops'? dev->switchdev_ops = &bnxt_switchdev_ops; ^~ -- drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c: In function 'bnxt_vf_rep_netdev_init': >> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:305:5: error: 'struct net_device' has no member named 'switchdev_ops'; did you mean 'netdev_ops'? dev->switchdev_ops = &bnxt_vf_rep_switchdev_ops; ^~ In file included from include/linux/kernfs.h:13:0, from include/linux/sysfs.h:15, from include/linux/kobject.h:21, from include/linux/pci.h:28, from drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:9: drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c: In function 'bnxt_dl_eswitch_mode_set': drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:410:16: error: 'struct bnxt' has no member named 'sriov_lock' mutex_lock(&bp->sriov_lock); ^ include/linux/mutex.h:164:44: note: in definition of macro 'mutex_lock' #define mutex_lock(lock) mutex_lock_nested(lock, 0) ^~~~ drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:439:18: error: 'struct bnxt' has no member named 'sriov_lock' mutex_unlock(&bp->sriov_lock); ^~ vim +7897 drivers/net/ethernet/broadcom/bnxt/bnxt.c 7863 7864 static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) 7865 { 7866 static int version_printed; 7867 struct net_device *dev; 7868 struct bnxt *bp; 7869 int rc, max_irqs; 7870 7871 if (pci_is_bridge(pdev)) 7872 return -ENODEV; 7873 7874 if (version_printed++ == 0) 7875 pr_info("%s", version); 7876 7877 max_irqs = bnxt_get_max_irq(pdev); 7878 dev = alloc_etherdev_mq(sizeof(*bp), max_irqs); 7879 if (!dev) 7880 return -ENOMEM; 7881 7882 bp = netdev_priv(dev); 7883 7884 if (bnxt_vf_pciid(ent->driver_data)) 7885 bp->flags |= BNXT_FLAG_VF; 7886 7887 if (pdev->msix_cap) 7888 bp->flags |= BNXT_FLAG_MSIX_CAP; 7889 7890 rc = bnxt_init_board(pdev, dev); 7891 if (rc < 0) 7892 goto init_err_free; 7893 7894 dev->netdev_ops = &bnxt_netdev_ops; 7895 dev->watchdog_timeo = BNXT_TX_TIMEOUT; 7896 dev->ethtool_ops = &bnxt_ethtool_ops; > 7897 dev->switchdev_ops = &bnxt_switchdev_ops; 7898 pci_set_drvdata(pdev, dev); 7899 7900 rc = bnxt_alloc_hwrm_resources(bp); 7901 if (rc) 7902 goto init_err_pci_clean; 7903 7904 mutex_init(&bp->hwrm_cmd_lock); 7905 rc = bnxt_hwrm_ver_get(bp); 7906 if (rc) 7907 goto init_err_pci_clean; 7908 7909 if (bp->flags & BNXT_FLAG_SHORT_CMD) { 7910 rc = bnxt_alloc_hwrm_short_cmd_req(bp); 7911 if (rc) 7912 goto init_err_pci_clean; 7913 } 7914 7915 rc = bnxt_hwrm_func_reset(bp); 7916 if (rc) 7917 goto init_err_pci_clean; 7918 7919 bnxt_hwrm_fw_set_time(bp); 7920 7921 dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_SG | 7922 NETIF_F_TSO | NETIF_F_TSO6 | 7923 NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_GRE | 7924 NETIF_F_GSO_IPXIP4 | 7925 NETIF_F_GSO_UDP_TUNNEL_CSUM | NETIF_F_GSO_GRE_CSUM | 7926 NETIF_F_GSO_PARTIAL | NETIF_F_RXHASH | 7927 NETIF_F_RXCSUM | NETIF_F_GRO; 7928 7929 if (!BNXT_CHIP_TYPE_NITRO_A0(bp)) 7930 dev->hw_features |= NETIF_F_LRO; 7931 7932 dev->hw_enc_features = 7933 NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_SG | 7934 NETIF_F_TSO | NETIF_F_TSO6 | 7935 NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_GRE | 7936 NETIF_F_GSO_UDP_TUNNEL_CSUM | NETIF_F_GSO_GRE_CSUM | 7937 NETIF_F_GSO_IPXIP4 | NETIF_F_GSO_PARTIAL; 7938 dev->gso_partial_features = NETIF_F_GSO_UDP_TUNNEL_CSUM | 7939 NETIF_F_GSO_GRE_CSUM; 7940 dev->vlan_features = dev->hw_features | NETIF_F_HIGHDMA; 7941 dev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_TX | 7942 NETIF_F_HW_VLAN_STAG_RX | NETIF_F_HW_VLAN_STAG_TX; 7943 dev->features |= dev->hw_features | NETIF_F_HIGHDMA; 7944 dev->priv_flags |= IFF_UNICAST_FLT; 7945 7946 /* MTU range: 60 - 9500 */ 7947 dev->min_mtu = ETH_ZLEN; 7948 dev->max_mtu = BNXT_MAX_MTU; 7949 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, 25 Jul 2017 10:44:49 +0800, kbuild test robot wrote: > drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_init_one': > >> drivers/net/ethernet/broadcom/bnxt/bnxt.c:7897:5: error: 'struct net_device' has no member named 'switchdev_ops'; did you mean 'netdev_ops'? > dev->switchdev_ops = &bnxt_switchdev_ops; > ^~ Check out SWITCHDEV_SET_OPS() from commit d643a75ac2bc ("net: switchdev: add SET_SWITCHDEV_OPS helper").
On Mon, 24 Jul 2017 12:34:29 -0400, Michael Chan wrote: > From: Sathya Perla <sathya.perla@broadcom.com> > > This patch adds support for the switchdev_port_attr_get() and > ndo_get_phys_port_name() methods for the PF and the VF-reps. > Using this support a user application can deduce that the PF > (when in the ESWITCH_SWDEV mode) and it's VF-reps form a switch. > > Signed-off-by: Sathya Perla <sathya.perla@broadcom.com> > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > --- > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 57 +++++++++++++++++++++++++++ > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 2 + > drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 31 ++++++++++++++- > 3 files changed, 89 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index f262fe6..82cbe18 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > @@ -7546,6 +7546,61 @@ static int bnxt_bridge_setlink(struct net_device *dev, struct nlmsghdr *nlh, > return rc; > } > > +static int bnxt_get_phys_port_name(struct net_device *dev, char *buf, > + size_t len) > +{ > + struct bnxt *bp = netdev_priv(dev); > + int rc; > + > + /* The PF and it's VF-reps only support the switchdev framework */ > + if (!BNXT_PF(bp)) > + return -EOPNOTSUPP; > + > + /* The switch-id that the pf belongs to is exported by > + * the switchdev ndo. This name is just to distinguish from the > + * vf-rep ports. > + */ > + rc = snprintf(buf, len, "pf%d", bp->pf.port_id); This is worrisome. What do you mean by PF? In my experience, since for years PFs had one-to-one association with physical ports people use the terms interchangeably. This causes a lot of headaches in proper eswitch modelling. I'm not sure whether this is a HW limitation or engineers trying to make things "easy for users" but most VF-representor patchsets we've seen on netdev don't include PF representors. NICs have 3 types of ports - PFs, VFs and external ports/MACs. For some reason there is a tendency to keep pretending PF and physical port is the same entity, which among other things makes it impossible to install VF->PF rules (as in from VF to the host, not to the network). Most high performance NIC vendors also have multi-PF NICs, even though those are not deployed by wider public. If we keep pretending PF is the external port, those will get very awkward to model. This is a bit of a pet peeve of mine :) If this bnxt_get_phys_port_name() relates to the external port, please change your implementation to comply with Documentation/networking/switchdev.txt, in particular: > Suggested naming convention is "swXpYsZ", where X is the switch name or ID, Y > is the port name or ID, and Z is the sub-port name or ID. For example, sw1p1s0 > would be sub-port 0 on port 1 on switch 1. So for physical ports the convention is p<port_id>, and in case of breakout p<port_id>s<subport_id>. > +static int bnxt_vf_rep_get_phys_port_name(struct net_device *dev, char *buf, > + size_t len) > +{ > + struct bnxt_vf_rep *vf_rep = netdev_priv(dev); > + int rc; > + > + rc = snprintf(buf, len, "vfr%d", vf_rep->vf_idx); This is even worse. We already have two naming conventions in the kernel, mlx5 uses "%d" for legacy reasons. nfp uses pf%dvf%d for vfs, which is better for two reasons: (a) it works with multi-PF devices; (b) naming something representor from switchdev perspective is pointless, you're not calling the external port netdev a physical port representor, even though it has the exact same relation to the port as with VFs (i.e. egressing traffic on the netdev will cause the switch to send to the given port).
On Tue, Jul 25, 2017 at 10:15 AM, Jakub Kicinski <kubakici@wp.pl> wrote: ... >> +static int bnxt_get_phys_port_name(struct net_device *dev, char *buf, >> + size_t len) >> +{ >> + struct bnxt *bp = netdev_priv(dev); >> + int rc; >> + >> + /* The PF and it's VF-reps only support the switchdev framework */ >> + if (!BNXT_PF(bp)) >> + return -EOPNOTSUPP; >> + >> + /* The switch-id that the pf belongs to is exported by >> + * the switchdev ndo. This name is just to distinguish from the >> + * vf-rep ports. >> + */ >> + rc = snprintf(buf, len, "pf%d", bp->pf.port_id); > > This is worrisome. What do you mean by PF? In my experience, since > for years PFs had one-to-one association with physical ports people use > the terms interchangeably. This causes a lot of headaches in proper > eswitch modelling. > > I'm not sure whether this is a HW limitation or engineers trying to > make things "easy for users" but most VF-representor patchsets we've > seen on netdev don't include PF representors. NICs have 3 types of > ports - PFs, VFs and external ports/MACs. For some reason there is a > tendency to keep pretending PF and physical port is the same entity, > which among other things makes it impossible to install VF->PF rules > (as in from VF to the host, not to the network). > > Most high performance NIC vendors also have multi-PF NICs, even though > those are not deployed by wider public. If we keep pretending PF is > the external port, those will get very awkward to model. This is a bit > of a pet peeve of mine :) Jakub, we'll consider implementing a PF-rep as an add-on feature... > > If this bnxt_get_phys_port_name() relates to the external port, please > change your implementation to comply with > Documentation/networking/switchdev.txt, in particular: > >> Suggested naming convention is "swXpYsZ", where X is the switch name or ID, Y >> is the port name or ID, and Z is the sub-port name or ID. For example, sw1p1s0 >> would be sub-port 0 on port 1 on switch 1. > > So for physical ports the convention is p<port_id>, and in case of > breakout p<port_id>s<subport_id>. I'm sending a follow-up patch that fixes the naming for both the external port and the VF-reps as proposed in your RFC ( switchdev: clarify ndo_get_phys_port_name() formats) thanks, -Sathya
On Tue, 25 Jul 2017 15:25:19 +0530, Sathya Perla wrote: > >> Suggested naming convention is "swXpYsZ", where X is the switch name or ID, Y > >> is the port name or ID, and Z is the sub-port name or ID. For example, sw1p1s0 > >> would be sub-port 0 on port 1 on switch 1. > > > > So for physical ports the convention is p<port_id>, and in case of > > breakout p<port_id>s<subport_id>. > > I'm sending a follow-up patch that fixes the naming for both the > external port and the VF-reps as proposed in your RFC ( switchdev: > clarify ndo_get_phys_port_name() formats) Thanks!
On Tue, Jul 25, 2017 at 7:45 AM, Jakub Kicinski <kubakici@wp.pl> wrote: > This is even worse. We already have two naming conventions in the > kernel, mlx5 uses "%d" for legacy reasons. nfp uses pf%dvf%d for vfs, To make it clear, in mlx5 this is used only for the offloads/switchdev mode and not for legacy mode, in the latter mode, a VF probed to VM doesn't have host side representation so there's no where to use that. Or. > which is better for two reasons: (a) it works with multi-PF devices; > (b) naming something representor from switchdev perspective is > pointless, you're not calling the external port netdev a physical port > representor, even though it has the exact same relation to the port as > with VFs (i.e. egressing traffic on the netdev will cause the switch > to send to the given port).
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index f262fe6..82cbe18 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -7546,6 +7546,61 @@ static int bnxt_bridge_setlink(struct net_device *dev, struct nlmsghdr *nlh, return rc; } +static int bnxt_get_phys_port_name(struct net_device *dev, char *buf, + size_t len) +{ + struct bnxt *bp = netdev_priv(dev); + int rc; + + /* The PF and it's VF-reps only support the switchdev framework */ + if (!BNXT_PF(bp)) + return -EOPNOTSUPP; + + /* The switch-id that the pf belongs to is exported by + * the switchdev ndo. This name is just to distinguish from the + * vf-rep ports. + */ + rc = snprintf(buf, len, "pf%d", bp->pf.port_id); + + if (rc >= len) + return -EOPNOTSUPP; + return 0; +} + +int bnxt_port_attr_get(struct bnxt *bp, struct switchdev_attr *attr) +{ + if (bp->eswitch_mode != DEVLINK_ESWITCH_MODE_SWITCHDEV) + return -EOPNOTSUPP; + + /* The PF and it's VF-reps only support the switchdev framework */ + if (!BNXT_PF(bp)) + return -EOPNOTSUPP; + + switch (attr->id) { + case SWITCHDEV_ATTR_ID_PORT_PARENT_ID: + /* In SRIOV each PF-pool (PF + child VFs) serves as a + * switching domain, the PF's perm mac-addr can be used + * as the unique parent-id + */ + attr->u.ppid.id_len = ETH_ALEN; + ether_addr_copy(attr->u.ppid.id, bp->pf.mac_addr); + break; + default: + return -EOPNOTSUPP; + } + return 0; +} + +static int bnxt_swdev_port_attr_get(struct net_device *dev, + struct switchdev_attr *attr) +{ + return bnxt_port_attr_get(netdev_priv(dev), attr); +} + +static const struct switchdev_ops bnxt_switchdev_ops = { + .switchdev_port_attr_get = bnxt_swdev_port_attr_get +}; + static const struct net_device_ops bnxt_netdev_ops = { .ndo_open = bnxt_open, .ndo_start_xmit = bnxt_start_xmit, @@ -7579,6 +7634,7 @@ static int bnxt_bridge_setlink(struct net_device *dev, struct nlmsghdr *nlh, .ndo_xdp = bnxt_xdp, .ndo_bridge_getlink = bnxt_bridge_getlink, .ndo_bridge_setlink = bnxt_bridge_setlink, + .ndo_get_phys_port_name = bnxt_get_phys_port_name }; static void bnxt_remove_one(struct pci_dev *pdev) @@ -7838,6 +7894,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) dev->netdev_ops = &bnxt_netdev_ops; dev->watchdog_timeo = BNXT_TX_TIMEOUT; dev->ethtool_ops = &bnxt_ethtool_ops; + dev->switchdev_ops = &bnxt_switchdev_ops; pci_set_drvdata(pdev, dev); rc = bnxt_alloc_hwrm_resources(bp); diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 63756f0..2d84d57 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -21,6 +21,7 @@ #include <linux/interrupt.h> #include <net/devlink.h> #include <net/dst_metadata.h> +#include <net/switchdev.h> struct tx_bd { __le32 tx_bd_len_flags_type; @@ -1350,4 +1351,5 @@ int bnxt_reserve_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs, int bnxt_setup_mq_tc(struct net_device *dev, u8 tc); int bnxt_get_max_rings(struct bnxt *, int *, int *, bool); void bnxt_restore_pf_fw_resources(struct bnxt *bp); +int bnxt_port_attr_get(struct bnxt *bp, struct switchdev_attr *attr); #endif diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c index 60bdb181..83478e9 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c @@ -135,6 +135,18 @@ void bnxt_vf_rep_rx(struct bnxt *bp, struct sk_buff *skb) netif_receive_skb(skb); } +static int bnxt_vf_rep_get_phys_port_name(struct net_device *dev, char *buf, + size_t len) +{ + struct bnxt_vf_rep *vf_rep = netdev_priv(dev); + int rc; + + rc = snprintf(buf, len, "vfr%d", vf_rep->vf_idx); + if (rc >= len) + return -EOPNOTSUPP; + return 0; +} + static void bnxt_vf_rep_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info) { @@ -142,6 +154,21 @@ static void bnxt_vf_rep_get_drvinfo(struct net_device *dev, strlcpy(info->version, DRV_MODULE_VERSION, sizeof(info->version)); } +static int bnxt_vf_rep_port_attr_get(struct net_device *dev, + struct switchdev_attr *attr) +{ + struct bnxt_vf_rep *vf_rep = netdev_priv(dev); + + /* as only PORT_PARENT_ID is supported currently use common code + * between PF and VF-rep for now. + */ + return bnxt_port_attr_get(vf_rep->bp, attr); +} + +static const struct switchdev_ops bnxt_vf_rep_switchdev_ops = { + .switchdev_port_attr_get = bnxt_vf_rep_port_attr_get +}; + static const struct ethtool_ops bnxt_vf_rep_ethtool_ops = { .get_drvinfo = bnxt_vf_rep_get_drvinfo }; @@ -150,7 +177,8 @@ static void bnxt_vf_rep_get_drvinfo(struct net_device *dev, .ndo_open = bnxt_vf_rep_open, .ndo_stop = bnxt_vf_rep_close, .ndo_start_xmit = bnxt_vf_rep_xmit, - .ndo_get_stats64 = bnxt_vf_rep_get_stats64 + .ndo_get_stats64 = bnxt_vf_rep_get_stats64, + .ndo_get_phys_port_name = bnxt_vf_rep_get_phys_port_name }; /* Called when the parent PF interface is closed: @@ -274,6 +302,7 @@ static void bnxt_vf_rep_netdev_init(struct bnxt *bp, struct bnxt_vf_rep *vf_rep, dev->netdev_ops = &bnxt_vf_rep_netdev_ops; dev->ethtool_ops = &bnxt_vf_rep_ethtool_ops; + dev->switchdev_ops = &bnxt_vf_rep_switchdev_ops; /* Just inherit all the featues of the parent PF as the VF-R * uses the RX/TX rings of the parent PF */