| Message ID | 20170725153046.44726-2-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,1/2] bnxt_en: add CONFIG_NET_SWITCHDEV dependency | expand |
On Tue, Jul 25, 2017 at 8:29 AM, Arnd Bergmann <arnd@arndb.de> wrote: > The sriov_lock is used to serialize the sriov code with the vfr code. > However, when SRIOV is disabled, the lock is not there at all, leading > to a build error: > > 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' > > We can either provide the mutex in this configuration, too, or > disable both SRIOV and VFR together. This implements the first > approach, since it seems like a reasonable configuration for > guest kernels to have, and the extra lock will be harmless when > there is no contention. > > Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Sathya already sent 3 patches to fix some of these issues. But I need to rework one of his patch and resend. Thanks.
On Tue, Jul 25, 2017 at 6:36 PM, Michael Chan <michael.chan@broadcom.com> wrote: > On Tue, Jul 25, 2017 at 8:29 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> The sriov_lock is used to serialize the sriov code with the vfr code. >> However, when SRIOV is disabled, the lock is not there at all, leading >> to a build error: >> >> 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' >> >> We can either provide the mutex in this configuration, too, or >> disable both SRIOV and VFR together. This implements the first >> approach, since it seems like a reasonable configuration for >> guest kernels to have, and the extra lock will be harmless when >> there is no contention. >> >> Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Sathya already sent 3 patches to fix some of these issues. But I need > to rework one of his patch and resend. Ok, thanks. I just ran into one more issue, and don't know if that's included as well. If not, please also add the patch below (or fold it into the one that adds the switchdev dependency to the ethernet driver): 8<---------- Subject: [PATCH] RDMA/bnxt_re: add NET_SWITCHDEV dependency The rdma side of BNXT enables the ethernet driver and has a list of its dependencies. However, the ethernet driver now also depends on NET_SWITCHDEV, so we have to add that dependency for both: warning: (INFINIBAND_BNXT_RE) selects BNXT which has unmet direct dependencies (NETDEVICES && ETHERNET && NET_VENDOR_BROADCOM && PCI && MAY_USE_DEVLINK && NET_SWITCHDEV) Signed-off-by: Arnd Bergmann <arnd@arndb.de>diff --git a/drivers/infiniband/hw/bnxt_re/Kconfig b/drivers/infiniband/hw/bnxt_re/Kconfig index 19982a4a9bba..0c296f00e74f 100644 --- a/drivers/infiniband/hw/bnxt_re/Kconfig +++ b/drivers/infiniband/hw/bnxt_re/Kconfig @@ -1,6 +1,7 @@ config INFINIBAND_BNXT_RE tristate "Broadcom Netxtreme HCA support" depends on ETHERNET && NETDEVICES && PCI && INET && DCB + depends on NET_SWITCHDEV select NET_VENDOR_BROADCOM select BNXT ---help---
On Wed, Jul 26, 2017 at 2:35 PM, Arnd Bergmann <arnd@arndb.de> wrote: [...] >> Sathya already sent 3 patches to fix some of these issues. But I need >> to rework one of his patch and resend. > > Ok, thanks. I just ran into one more issue, and don't know if that's included > as well. If not, please also add the patch below (or fold it into the one > that adds the switchdev dependency to the ethernet driver): > > 8<---------- > Subject: [PATCH] RDMA/bnxt_re: add NET_SWITCHDEV dependency > > The rdma side of BNXT enables the ethernet driver and has a list > of its dependencies. However, the ethernet driver now also depends > on NET_SWITCHDEV, so we have to add that dependency for both: Arnd, after the patch "bnxt_en: use SWITCHDEV_SET_OPS() for setting vf_rep_switchdev_ops" the bnxt_en driver doesn't need an explicit NET_SWITCHDEV dependency. So, the bnxt_re driver shouldn't need one either. Are you still seeing the bnxt_re issue even after pulling the above patch??
On Wed, Jul 26, 2017 at 3:18 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Jul 26, 2017 at 12:54 PM, Sathya Perla > <sathya.perla@broadcom.com> wrote: >> On Wed, Jul 26, 2017 at 2:35 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> [...] >>>> Sathya already sent 3 patches to fix some of these issues. But I need >>>> to rework one of his patch and resend. >>> >>> Ok, thanks. I just ran into one more issue, and don't know if that's included >>> as well. If not, please also add the patch below (or fold it into the one >>> that adds the switchdev dependency to the ethernet driver): >>> >>> 8<---------- >>> Subject: [PATCH] RDMA/bnxt_re: add NET_SWITCHDEV dependency >>> >>> The rdma side of BNXT enables the ethernet driver and has a list >>> of its dependencies. However, the ethernet driver now also depends >>> on NET_SWITCHDEV, so we have to add that dependency for both: >> >> Arnd, after the patch "bnxt_en: use SWITCHDEV_SET_OPS() for setting >> vf_rep_switchdev_ops" the bnxt_en driver doesn't need an explicit >> NET_SWITCHDEV dependency. So, the bnxt_re driver shouldn't need one >> either. Are you still seeing the bnxt_re issue even after pulling the >> above patch?? > > I think that's fine then. I missed that patch when it went in, so I only > needed the add-on since I still had my own earlier patch. I'll drop both > from my test tree now, and will let you know in case something else > remains. On today's linux-next: drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.o: In function `bnxt_dl_register': bnxt_vfr.c:(.text+0x1440): undefined reference to `devlink_alloc' bnxt_vfr.c:(.text+0x14c0): undefined reference to `devlink_register' bnxt_vfr.c:(.text+0x14e0): undefined reference to `devlink_free' drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.o: In function `bnxt_dl_unregister': bnxt_vfr.c:(.text+0x1534): undefined reference to `devlink_unregister' bnxt_vfr.c:(.text+0x153c): undefined reference to `devlink_free' I think you are just missing a "depends on MAY_USE_DEVLINK" in INFINIBAND_BNXT_RE, which uses 'select BNXT'. This is a tricky corner case for Kconfig, where the MAY_USE_DEVLINK dependency is silently ignored for BNXT as long as MAY_USE_DEVLINK is not "=n". Arnd
On Thu, Jul 27, 2017 at 1:18 PM, Arnd Bergmann <arnd@arndb.de> wrote: [...] > > On today's linux-next: > > drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.o: In function `bnxt_dl_register': > bnxt_vfr.c:(.text+0x1440): undefined reference to `devlink_alloc' > bnxt_vfr.c:(.text+0x14c0): undefined reference to `devlink_register' > bnxt_vfr.c:(.text+0x14e0): undefined reference to `devlink_free' > drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.o: In function `bnxt_dl_unregister': > bnxt_vfr.c:(.text+0x1534): undefined reference to `devlink_unregister' > bnxt_vfr.c:(.text+0x153c): undefined reference to `devlink_free' > > I think you are just missing a "depends on MAY_USE_DEVLINK" > in INFINIBAND_BNXT_RE, which uses 'select BNXT'. > > This is a tricky corner case for Kconfig, where the MAY_USE_DEVLINK > dependency is silently ignored for BNXT as long as MAY_USE_DEVLINK > is not "=n". Can you pls share your .config so that I can reproduce this issue and ensure that my fix really works...
On Thu, Jul 27, 2017 at 11:00 AM, Sathya Perla <sathya.perla@broadcom.com> wrote: > On Thu, Jul 27, 2017 at 1:18 PM, Arnd Bergmann <arnd@arndb.de> wrote: > [...] >> >> On today's linux-next: >> >> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.o: In function `bnxt_dl_register': >> bnxt_vfr.c:(.text+0x1440): undefined reference to `devlink_alloc' >> bnxt_vfr.c:(.text+0x14c0): undefined reference to `devlink_register' >> bnxt_vfr.c:(.text+0x14e0): undefined reference to `devlink_free' >> drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.o: In function `bnxt_dl_unregister': >> bnxt_vfr.c:(.text+0x1534): undefined reference to `devlink_unregister' >> bnxt_vfr.c:(.text+0x153c): undefined reference to `devlink_free' >> >> I think you are just missing a "depends on MAY_USE_DEVLINK" >> in INFINIBAND_BNXT_RE, which uses 'select BNXT'. >> >> This is a tricky corner case for Kconfig, where the MAY_USE_DEVLINK >> dependency is silently ignored for BNXT as long as MAY_USE_DEVLINK >> is not "=n". > > Can you pls share your .config so that I can reproduce this issue and > ensure that my fix really works... The configuration I used happened to be for arm64, I've pasted it to https://pastebin.com/dl/rSJ6jCQM now. You should be able to reproduce it on x86 as well, with anything using CONFIG_NET_DEVLINK=m and INFINIBAND_BNXT_RE=y. Arnd
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 82cbe1804821..9a9f5f394341 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -7949,8 +7949,8 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) #ifdef CONFIG_BNXT_SRIOV init_waitqueue_head(&bp->sriov_cfg_wait); - mutex_init(&bp->sriov_lock); #endif + mutex_init(&bp->sriov_lock); bp->gro_func = bnxt_gro_func_5730x; if (BNXT_CHIP_P4_PLUS(bp)) bp->gro_func = bnxt_gro_func_5731x; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 2d84d5719b70..a31ef843977a 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1239,13 +1239,12 @@ struct bnxt { wait_queue_head_t sriov_cfg_wait; bool sriov_cfg; #define BNXT_SRIOV_CFG_WAIT_TMO msecs_to_jiffies(10000) - +#endif /* lock to protect VF-rep creation/cleanup via * multiple paths such as ->sriov_configure() and * devlink ->eswitch_mode_set() */ struct mutex sriov_lock; -#endif #define BNXT_NTP_FLTR_MAX_FLTR 4096 #define BNXT_NTP_FLTR_HASH_SIZE 512
The sriov_lock is used to serialize the sriov code with the vfr code. However, when SRIOV is disabled, the lock is not there at all, leading to a build error: 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' We can either provide the mutex in this configuration, too, or disable both SRIOV and VFR together. This implements the first approach, since it seems like a reasonable configuration for guest kernels to have, and the extra lock will be harmless when there is no contention. Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) -- 2.9.0