[PATCH net-next 1/2] bnxt_en: add CONFIG_NET_SWITCHDEV dependency

[PATCH net-next 1/2] bnxt_en: add CONFIG_NET_SWITCHDEV dependency

[Arnd Bergmann]

Without CONFIG_SWITCHDEV, we run into a compile-time error:

drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c: In function 'bnxt_vf_rep_netdev_init':
drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c:305:7: error: 'struct net_device' has no member named 'switchdev_ops'; did you mean 'netdev_ops'?

This adds a Kconfig dependency to prevent running into this invalid
configuration.

Fixes: c124a62ff2dd ("bnxt_en: add support for port_attr_get and and get_phys_port_name")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/ethernet/broadcom/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index 285f8bc25682..095bb816ab48 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -194,6 +194,7 @@ config BNXT
 	tristate "Broadcom NetXtreme-C/E support"
 	depends on PCI
 	depends on MAY_USE_DEVLINK
+	depends on NET_SWITCHDEV
 	select FW_LOADER
 	select LIBCRC32C
 	---help---
-- 
2.9.0

[PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Arnd Bergmann]

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(-)

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
-- 
2.9.0

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Michael Chan]

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.

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Arnd Bergmann]

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---

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Sathya Perla]

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??

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Arnd Bergmann]

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.

         Arnd

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Arnd Bergmann]

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

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Sathya Perla]

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...

Re: [PATCH net-next 2/2] bnxt_en: define sriov_lock unconditionally

[Arnd Bergmann]

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