[PATCH net-next 0/8] bnxt_en: devlink param updates

[PATCH net-next 0/8] bnxt_en: devlink param updates

[Vasundhara Volam]

This patchset adds support for 4 generic and 1 driver-specific devlink
parameters.

Also, this patchset adds support to return proper error code if
HWRM_NVM_GET/SET_VARIABLE commands return error code
HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED.

Vasundhara Volam (8):
  devlink: Add generic parameter hw_tc_offload
  devlink: Add generic parameter ignore_ari
  devlink: Add generic parameter msix_vec_per_pf_max
  devlink: Add generic parameter msix_vec_per_pf_min
  bnxt_en: Use hw_tc_offload and ignore_ari devlink parameters
  bnxt_en: return proper error when FW returns
    HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED
  bnxt_en: Use msix_vec_per_pf_max and msix_vec_per_pf_min devlink
    params.
  bnxt_en: Add a driver specific devlink parameter.

 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 92 ++++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  8 ++
 include/net/devlink.h                             | 16 ++++
 net/core/devlink.c                                | 20 +++++
 4 files changed, 132 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/8] devlink: Add generic parameter hw_tc_offload

[Vasundhara Volam]

hw_tc_offload - Enable/Disable TC flower offload in the device.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/net/devlink.h | 4 ++++
 net/core/devlink.c    | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index b9b89d6..a0e9ce9 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -362,6 +362,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_MAX_MACS,
 	DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV,
 	DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT,
+	DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -380,6 +381,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_REGION_SNAPSHOT_NAME "region_snapshot_enable"
 #define DEVLINK_PARAM_GENERIC_REGION_SNAPSHOT_TYPE DEVLINK_PARAM_TYPE_BOOL
 
+#define DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_NAME "hw_tc_offload"
+#define DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_TYPE DEVLINK_PARAM_TYPE_BOOL
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 65fc366..020daa1 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2676,6 +2676,11 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 		.name = DEVLINK_PARAM_GENERIC_REGION_SNAPSHOT_NAME,
 		.type = DEVLINK_PARAM_GENERIC_REGION_SNAPSHOT_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,
+		.name = DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_NAME,
+		.type = DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
1.8.3.1

Re: [PATCH net-next 1/8] devlink: Add generic parameter hw_tc_offload

[Jiri Pirko]

Tue, Sep 11, 2018 at 10:44:58AM CEST, vasundhara-v.volam@broadcom.com wrote:
>hw_tc_offload - Enable/Disable TC flower offload in the device.
>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/net/devlink.h | 4 ++++
> net/core/devlink.c    | 5 +++++
> 2 files changed, 9 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index b9b89d6..a0e9ce9 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -362,6 +362,7 @@ enum devlink_param_generic_id {
> 	DEVLINK_PARAM_GENERIC_ID_MAX_MACS,
> 	DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV,
> 	DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT,
>+	DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,

Could you please describe why do you need this here and why the
tc_offload flag in ethtool is not enough. How do you imagine the user
should use them together?

Re: [PATCH net-next 1/8] devlink: Add generic parameter hw_tc_offload

[Vasundhara Volam]

On Tue, Sep 11, 2018 at 3:25 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Sep 11, 2018 at 10:44:58AM CEST, vasundhara-v.volam@broadcom.com wrote:
> >hw_tc_offload - Enable/Disable TC flower offload in the device.
> >
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >---
> > include/net/devlink.h | 4 ++++
> > net/core/devlink.c    | 5 +++++
> > 2 files changed, 9 insertions(+)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index b9b89d6..a0e9ce9 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -362,6 +362,7 @@ enum devlink_param_generic_id {
> >       DEVLINK_PARAM_GENERIC_ID_MAX_MACS,
> >       DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV,
> >       DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT,
> >+      DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,
>
> Could you please describe why do you need this here and why the
> tc_offload flag in ethtool is not enough. How do you imagine the user
> should use them together?
Jiri, tc_offload flag in ethtool will modify feature in driver at
runtime. But I am adding
tc_offload param here to toggle this feature in NVM Config of our
adapter, whose configuration
mode is permanent and will be effective only with a reboot of the server.

User has to turn on tc_offload feature in NVM config of the adapter and then
enabling the tc_offload flag in ethtool will completely enable the
feature in driver.

Re: [PATCH net-next 1/8] devlink: Add generic parameter hw_tc_offload

[Jakub Kicinski]

On Wed, 12 Sep 2018 11:47:59 +0530, Vasundhara Volam wrote:
> On Tue, Sep 11, 2018 at 3:25 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >
> > Tue, Sep 11, 2018 at 10:44:58AM CEST, vasundhara-v.volam@broadcom.com wrote:  
> > >hw_tc_offload - Enable/Disable TC flower offload in the device.
> > >
> > >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > >---
> > > include/net/devlink.h | 4 ++++
> > > net/core/devlink.c    | 5 +++++
> > > 2 files changed, 9 insertions(+)
> > >
> > >diff --git a/include/net/devlink.h b/include/net/devlink.h
> > >index b9b89d6..a0e9ce9 100644
> > >--- a/include/net/devlink.h
> > >+++ b/include/net/devlink.h
> > >@@ -362,6 +362,7 @@ enum devlink_param_generic_id {
> > >       DEVLINK_PARAM_GENERIC_ID_MAX_MACS,
> > >       DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV,
> > >       DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT,
> > >+      DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,  
> >
> > Could you please describe why do you need this here and why the
> > tc_offload flag in ethtool is not enough. How do you imagine the user
> > should use them together?  
> Jiri, tc_offload flag in ethtool will modify feature in driver at
> runtime. But I am adding
> tc_offload param here to toggle this feature in NVM Config of our
> adapter, whose configuration
> mode is permanent and will be effective only with a reboot of the server.
> 
> User has to turn on tc_offload feature in NVM config of the adapter and then
> enabling the tc_offload flag in ethtool will completely enable the
> feature in driver.

Thanks for explaining, however, I don't think we have trouble
understanding *what* you are doing, but rather *why* you're doing it.

Re: [PATCH net-next 1/8] devlink: Add generic parameter hw_tc_offload

[Jiri Pirko]

Wed, Sep 12, 2018 at 08:34:22AM CEST, jakub.kicinski@netronome.com wrote:
>On Wed, 12 Sep 2018 11:47:59 +0530, Vasundhara Volam wrote:
>> On Tue, Sep 11, 2018 at 3:25 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> > Tue, Sep 11, 2018 at 10:44:58AM CEST, vasundhara-v.volam@broadcom.com wrote:  
>> > >hw_tc_offload - Enable/Disable TC flower offload in the device.
>> > >
>> > >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> > >---
>> > > include/net/devlink.h | 4 ++++
>> > > net/core/devlink.c    | 5 +++++
>> > > 2 files changed, 9 insertions(+)
>> > >
>> > >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> > >index b9b89d6..a0e9ce9 100644
>> > >--- a/include/net/devlink.h
>> > >+++ b/include/net/devlink.h
>> > >@@ -362,6 +362,7 @@ enum devlink_param_generic_id {
>> > >       DEVLINK_PARAM_GENERIC_ID_MAX_MACS,
>> > >       DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV,
>> > >       DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT,
>> > >+      DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,  
>> >
>> > Could you please describe why do you need this here and why the
>> > tc_offload flag in ethtool is not enough. How do you imagine the user
>> > should use them together?  
>> Jiri, tc_offload flag in ethtool will modify feature in driver at
>> runtime. But I am adding
>> tc_offload param here to toggle this feature in NVM Config of our
>> adapter, whose configuration
>> mode is permanent and will be effective only with a reboot of the server.
>> 
>> User has to turn on tc_offload feature in NVM config of the adapter and then
>> enabling the tc_offload flag in ethtool will completely enable the
>> feature in driver.
>
>Thanks for explaining, however, I don't think we have trouble
>understanding *what* you are doing, but rather *why* you're doing it.

Exactly. Why ethtool tc_offload flag is not enough?

[PATCH net-next 2/8] devlink: Add generic parameter ignore_ari

[Vasundhara Volam]

ignore_ari - Device ignores ARI(Alternate Routing ID) capability,
even when platforms has the support and creates same number of
partitions when platform does not support ARI capability.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/net/devlink.h | 4 ++++
 net/core/devlink.c    | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index a0e9ce9..8a6063e 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -363,6 +363,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV,
 	DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT,
 	DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,
+	DEVLINK_PARAM_GENERIC_ID_IGNORE_ARI,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -384,6 +385,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_NAME "hw_tc_offload"
 #define DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_TYPE DEVLINK_PARAM_TYPE_BOOL
 
+#define DEVLINK_PARAM_GENERIC_IGNORE_ARI_NAME "ignore_ari"
+#define DEVLINK_PARAM_GENERIC_IGNORE_ARI_TYPE DEVLINK_PARAM_TYPE_BOOL
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 020daa1..30f32e5 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2681,6 +2681,11 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 		.name = DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_NAME,
 		.type = DEVLINK_PARAM_GENERIC_HW_TC_OFFLOAD_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_IGNORE_ARI,
+		.name = DEVLINK_PARAM_GENERIC_IGNORE_ARI_NAME,
+		.type = DEVLINK_PARAM_GENERIC_IGNORE_ARI_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
1.8.3.1

[PATCH net-next 3/8] devlink: Add generic parameter msix_vec_per_pf_max

[Vasundhara Volam]

msix_vec_per_pf_max - This param sets the number of MSIX vectors
that the device requests from the host on driver initialization.
This value is set in the device which is applicable per PF.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/net/devlink.h | 4 ++++
 net/core/devlink.c    | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 8a6063e..9619a68 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -364,6 +364,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_REGION_SNAPSHOT,
 	DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,
 	DEVLINK_PARAM_GENERIC_ID_IGNORE_ARI,
+	DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -388,6 +389,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_IGNORE_ARI_NAME "ignore_ari"
 #define DEVLINK_PARAM_GENERIC_IGNORE_ARI_TYPE DEVLINK_PARAM_TYPE_BOOL
 
+#define DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MAX_NAME "msix_vec_per_pf_max"
+#define DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MAX_TYPE DEVLINK_PARAM_TYPE_U32
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 30f32e5..fce5c1d 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2686,6 +2686,11 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 		.name = DEVLINK_PARAM_GENERIC_IGNORE_ARI_NAME,
 		.type = DEVLINK_PARAM_GENERIC_IGNORE_ARI_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
+		.name = DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MAX_NAME,
+		.type = DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MAX_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
1.8.3.1

[PATCH net-next 4/8] devlink: Add generic parameter msix_vec_per_pf_min

[Vasundhara Volam]

msix_vec_per_pf_min - This param sets the number of minimal MSIX
vectors required for the device initialization. This value is set
in the device which limits MSIX vectors per PF.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 include/net/devlink.h | 4 ++++
 net/core/devlink.c    | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 9619a68..208cd4a 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -365,6 +365,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD,
 	DEVLINK_PARAM_GENERIC_ID_IGNORE_ARI,
 	DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
+	DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -392,6 +393,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MAX_NAME "msix_vec_per_pf_max"
 #define DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MAX_TYPE DEVLINK_PARAM_TYPE_U32
 
+#define DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MIN_NAME "msix_vec_per_pf_min"
+#define DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MIN_TYPE DEVLINK_PARAM_TYPE_U32
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index fce5c1d..177c474 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2691,6 +2691,11 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 		.name = DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MAX_NAME,
 		.type = DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MAX_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
+		.name = DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MIN_NAME,
+		.type = DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MIN_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
1.8.3.1

[PATCH net-next 5/8] bnxt_en: Use hw_tc_offload and ignore_ari devlink parameters

[Vasundhara Volam]

This patch adds support for following generic permanent mode
devlink parameters. Both are disabled by default. They can be
enabled using devlink param commands.

hw_tc_offload - Enable/Disable TC flower offload in the device.

ignore_ari - Device ignores ARI(Alternate Routing ID) capability,
even when platforms has the support and creates same number of
partitions when platform does not support ARI capability.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 12 ++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index f3b9fbc..582e5b5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -24,6 +24,10 @@
 static const struct bnxt_dl_nvm_param nvm_params[] = {
 	{DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, NVM_OFF_ENABLE_SRIOV,
 	 BNXT_NVM_SHARED_CFG, 1},
+	{DEVLINK_PARAM_GENERIC_ID_HW_TC_OFFLOAD, NVM_OFF_HW_TC_OFFLOAD,
+	 BNXT_NVM_SHARED_CFG, 1},
+	{DEVLINK_PARAM_GENERIC_ID_IGNORE_ARI, NVM_OFF_IGNORE_ARI,
+	 BNXT_NVM_SHARED_CFG, 1},
 };
 
 static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
@@ -105,6 +109,14 @@ static int bnxt_dl_nvm_param_set(struct devlink *dl, u32 id,
 			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
 			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
 			      NULL),
+	DEVLINK_PARAM_GENERIC(HW_TC_OFFLOAD,
+			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
+			      NULL),
+	DEVLINK_PARAM_GENERIC(IGNORE_ARI,
+			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
+			      NULL),
 };
 
 int bnxt_dl_register(struct bnxt *bp)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index 2f68dc0..da146492 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -33,6 +33,8 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
 	}
 }
 
+#define NVM_OFF_IGNORE_ARI		164
+#define NVM_OFF_HW_TC_OFFLOAD		170
 #define NVM_OFF_ENABLE_SRIOV		401
 
 enum bnxt_nvm_dir_type {
-- 
1.8.3.1

Re: [PATCH net-next 5/8] bnxt_en: Use hw_tc_offload and ignore_ari devlink parameters

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7214 bytes --]

Hi Vasundhara,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Vasundhara-Volam/bnxt_en-devlink-param-updates/20180914-141937
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=powerpc 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/net//ethernet/broadcom/bnxt/bnxt_devlink.c: In function 'bnxt_hwrm_nvm_req.constprop':
   drivers/net//ethernet/broadcom/bnxt/bnxt_devlink.c:38:27: warning: 'nvm_param.num_bits' may be used uninitialized in this function [-Wmaybe-uninitialized]
     struct bnxt_dl_nvm_param nvm_param;
                              ^~~~~~~~~
   drivers/net//ethernet/broadcom/bnxt/bnxt_devlink.c:53:5: warning: 'nvm_param.dir_type' may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (nvm_param.dir_type == BNXT_NVM_PORT_CFG)
        ^
   In file included from include/linux/byteorder/big_endian.h:5:0,
                    from arch/powerpc/include/uapi/asm/byteorder.h:14,
                    from include/asm-generic/bitops/le.h:6,
                    from arch/powerpc/include/asm/bitops.h:247,
                    from include/linux/bitops.h:19,
                    from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/pci.h:26,
                    from drivers/net//ethernet/broadcom/bnxt/bnxt_devlink.c:10:
>> include/uapi/linux/byteorder/big_endian.h:35:27: warning: 'nvm_param.offset' may be used uninitialized in this function [-Wmaybe-uninitialized]
    #define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
                              ^
   drivers/net//ethernet/broadcom/bnxt/bnxt_devlink.c:38:27: note: 'nvm_param.offset' was declared here
     struct bnxt_dl_nvm_param nvm_param;
                              ^~~~~~~~~
--
   drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c: In function 'bnxt_hwrm_nvm_req.constprop':
   drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c:38:27: warning: 'nvm_param.num_bits' may be used uninitialized in this function [-Wmaybe-uninitialized]
     struct bnxt_dl_nvm_param nvm_param;
                              ^~~~~~~~~
   drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c:53:5: warning: 'nvm_param.dir_type' may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (nvm_param.dir_type == BNXT_NVM_PORT_CFG)
        ^
   In file included from include/linux/byteorder/big_endian.h:5:0,
                    from arch/powerpc/include/uapi/asm/byteorder.h:14,
                    from include/asm-generic/bitops/le.h:6,
                    from arch/powerpc/include/asm/bitops.h:247,
                    from include/linux/bitops.h:19,
                    from include/linux/kernel.h:11,
                    from include/linux/list.h:9,
                    from include/linux/pci.h:26,
                    from drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c:10:
>> include/uapi/linux/byteorder/big_endian.h:35:27: warning: 'nvm_param.offset' may be used uninitialized in this function [-Wmaybe-uninitialized]
    #define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
                              ^
   drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c:38:27: note: 'nvm_param.offset' was declared here
     struct bnxt_dl_nvm_param nvm_param;
                              ^~~~~~~~~

vim +35 include/uapi/linux/byteorder/big_endian.h

5921e6f8 David Howells 2012-10-13  14  
5921e6f8 David Howells 2012-10-13  15  #define __constant_htonl(x) ((__force __be32)(__u32)(x))
5921e6f8 David Howells 2012-10-13  16  #define __constant_ntohl(x) ((__force __u32)(__be32)(x))
5921e6f8 David Howells 2012-10-13  17  #define __constant_htons(x) ((__force __be16)(__u16)(x))
5921e6f8 David Howells 2012-10-13  18  #define __constant_ntohs(x) ((__force __u16)(__be16)(x))
5921e6f8 David Howells 2012-10-13  19  #define __constant_cpu_to_le64(x) ((__force __le64)___constant_swab64((x)))
5921e6f8 David Howells 2012-10-13  20  #define __constant_le64_to_cpu(x) ___constant_swab64((__force __u64)(__le64)(x))
5921e6f8 David Howells 2012-10-13  21  #define __constant_cpu_to_le32(x) ((__force __le32)___constant_swab32((x)))
5921e6f8 David Howells 2012-10-13  22  #define __constant_le32_to_cpu(x) ___constant_swab32((__force __u32)(__le32)(x))
5921e6f8 David Howells 2012-10-13  23  #define __constant_cpu_to_le16(x) ((__force __le16)___constant_swab16((x)))
5921e6f8 David Howells 2012-10-13  24  #define __constant_le16_to_cpu(x) ___constant_swab16((__force __u16)(__le16)(x))
5921e6f8 David Howells 2012-10-13  25  #define __constant_cpu_to_be64(x) ((__force __be64)(__u64)(x))
5921e6f8 David Howells 2012-10-13  26  #define __constant_be64_to_cpu(x) ((__force __u64)(__be64)(x))
5921e6f8 David Howells 2012-10-13  27  #define __constant_cpu_to_be32(x) ((__force __be32)(__u32)(x))
5921e6f8 David Howells 2012-10-13  28  #define __constant_be32_to_cpu(x) ((__force __u32)(__be32)(x))
5921e6f8 David Howells 2012-10-13  29  #define __constant_cpu_to_be16(x) ((__force __be16)(__u16)(x))
5921e6f8 David Howells 2012-10-13  30  #define __constant_be16_to_cpu(x) ((__force __u16)(__be16)(x))
5921e6f8 David Howells 2012-10-13  31  #define __cpu_to_le64(x) ((__force __le64)__swab64((x)))
5921e6f8 David Howells 2012-10-13  32  #define __le64_to_cpu(x) __swab64((__force __u64)(__le64)(x))
5921e6f8 David Howells 2012-10-13  33  #define __cpu_to_le32(x) ((__force __le32)__swab32((x)))
5921e6f8 David Howells 2012-10-13  34  #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
5921e6f8 David Howells 2012-10-13 @35  #define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
5921e6f8 David Howells 2012-10-13  36  #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
5921e6f8 David Howells 2012-10-13  37  #define __cpu_to_be64(x) ((__force __be64)(__u64)(x))
5921e6f8 David Howells 2012-10-13  38  #define __be64_to_cpu(x) ((__force __u64)(__be64)(x))
5921e6f8 David Howells 2012-10-13  39  #define __cpu_to_be32(x) ((__force __be32)(__u32)(x))
5921e6f8 David Howells 2012-10-13  40  #define __be32_to_cpu(x) ((__force __u32)(__be32)(x))
5921e6f8 David Howells 2012-10-13  41  #define __cpu_to_be16(x) ((__force __be16)(__u16)(x))
5921e6f8 David Howells 2012-10-13  42  #define __be16_to_cpu(x) ((__force __u16)(__be16)(x))
5921e6f8 David Howells 2012-10-13  43  

:::::: The code at line 35 was first introduced by commit
:::::: 5921e6f8809b1616932ca4afd40fe449faa8fd88 UAPI: (Scripted) Disintegrate include/linux/byteorder

:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: David Howells <dhowells@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58692 bytes --]

[PATCH net-next 6/8] bnxt_en: return proper error when FW returns HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED

[Vasundhara Volam]

Return proper error code when Firmware returns
HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED for HWRM_NVM_GET/SET_VARIABLE
commands.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 582e5b5..8d4ba33 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -79,8 +79,12 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
 		memcpy(buf, data_addr, bytesize);
 
 	dma_free_coherent(&bp->pdev->dev, bytesize, data_addr, data_dma_addr);
-	if (rc)
+	if (rc == HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED) {
+		netdev_err(bp->dev, "PF does not have admin privileges to modify NVM config\n");
+		return -EACCES;
+	} else if (rc) {
 		return -EIO;
+	}
 	return 0;
 }
 
-- 
1.8.3.1

[PATCH net-next 7/8] bnxt_en: Use msix_vec_per_pf_max and msix_vec_per_pf_min devlink params.

[Vasundhara Volam]

This patch adds support for following generic permanent mode
devlink parameters. They can be modified using devlink param
commands.

msix_vec_per_pf_max - This param sets the number of MSIX vectors
that the device requests from the host on driver initialization.
This value is set in the device which limits MSIX vectors per PF.

msix_vec_per_pf_min - This param sets the number of minimal MSIX
vectors required for the device initialization. Value 0 indicates
a default value is selected. This value is set in the device which
limits MSIX vectors per PF.

Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 50 ++++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  5 +++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 8d4ba33..c9e7700 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -28,6 +28,10 @@
 	 BNXT_NVM_SHARED_CFG, 1},
 	{DEVLINK_PARAM_GENERIC_ID_IGNORE_ARI, NVM_OFF_IGNORE_ARI,
 	 BNXT_NVM_SHARED_CFG, 1},
+	{DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
+	 NVM_OFF_MSIX_VEC_PER_PF_MAX, BNXT_NVM_SHARED_CFG, 10},
+	{DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
+	 NVM_OFF_MSIX_VEC_PER_PF_MIN, BNXT_NVM_SHARED_CFG, 7},
 };
 
 static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
@@ -56,8 +60,22 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
 		idx = bp->pf.fw_fid - BNXT_FIRST_PF_FID;
 
 	bytesize = roundup(nvm_param.num_bits, BITS_PER_BYTE) / BITS_PER_BYTE;
-	if (nvm_param.num_bits == 1)
-		buf = &val->vbool;
+	switch (bytesize) {
+	case 1:
+		if (nvm_param.num_bits == 1)
+			buf = &val->vbool;
+		else
+			buf = &val->vu8;
+		break;
+	case 2:
+		buf = &val->vu16;
+		break;
+	case 4:
+		buf = &val->vu32;
+		break;
+	default:
+		return -EFAULT;
+	}
 
 	data_addr = dma_zalloc_coherent(&bp->pdev->dev, bytesize,
 					&data_dma_addr, GFP_KERNEL);
@@ -108,6 +126,26 @@ static int bnxt_dl_nvm_param_set(struct devlink *dl, u32 id,
 	return bnxt_hwrm_nvm_req(bp, id, &req, sizeof(req), &ctx->val);
 }
 
+static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
+				 union devlink_param_value val,
+				 struct netlink_ext_ack *extack)
+{
+	int max_val;
+
+	if (id == DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX)
+		max_val = BNXT_MSIX_VEC_MAX;
+
+	if (id == DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN)
+		max_val = BNXT_MSIX_VEC_MIN_MAX;
+
+	if (val.vu32 < 0 || val.vu32 > max_val) {
+		NL_SET_ERR_MSG_MOD(extack, "MSIX value is exceeding the range");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct devlink_param bnxt_dl_params[] = {
 	DEVLINK_PARAM_GENERIC(ENABLE_SRIOV,
 			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
@@ -121,6 +159,14 @@ static int bnxt_dl_nvm_param_set(struct devlink *dl, u32 id,
 			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
 			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
 			      NULL),
+	DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX,
+			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
+			      bnxt_dl_msix_validate),
+	DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN,
+			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
+			      bnxt_dl_msix_validate),
 };
 
 int bnxt_dl_register(struct bnxt *bp)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index da146492..0e67c05 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -33,10 +33,15 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
 	}
 }
 
+#define NVM_OFF_MSIX_VEC_PER_PF_MAX	108
+#define NVM_OFF_MSIX_VEC_PER_PF_MIN	114
 #define NVM_OFF_IGNORE_ARI		164
 #define NVM_OFF_HW_TC_OFFLOAD		170
 #define NVM_OFF_ENABLE_SRIOV		401
 
+#define BNXT_MSIX_VEC_MAX	1280
+#define BNXT_MSIX_VEC_MIN_MAX	128
+
 enum bnxt_nvm_dir_type {
 	BNXT_NVM_SHARED_CFG = 40,
 	BNXT_NVM_PORT_CFG,
-- 
1.8.3.1

[PATCH net-next 8/8] bnxt_en: Add a driver specific devlink parameter.

[Vasundhara Volam]

This patch adds following driver-specific permanent mode boolean
parameter.

gre_ver_check - When this param is disabled, device skips GRE
version check.

Signed-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 24 ++++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h |  1 +
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index c9e7700..9778b88 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -21,6 +21,11 @@
 #endif /* CONFIG_BNXT_SRIOV */
 };
 
+enum bnxt_dl_param_id {
+	BNXT_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
+	BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK,
+};
+
 static const struct bnxt_dl_nvm_param nvm_params[] = {
 	{DEVLINK_PARAM_GENERIC_ID_ENABLE_SRIOV, NVM_OFF_ENABLE_SRIOV,
 	 BNXT_NVM_SHARED_CFG, 1},
@@ -32,6 +37,8 @@
 	 NVM_OFF_MSIX_VEC_PER_PF_MAX, BNXT_NVM_SHARED_CFG, 10},
 	{DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
 	 NVM_OFF_MSIX_VEC_PER_PF_MIN, BNXT_NVM_SHARED_CFG, 7},
+	{BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, NVM_OFF_DIS_GRE_VER_CHECK,
+	 BNXT_NVM_SHARED_CFG, 1},
 };
 
 static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
@@ -111,9 +118,15 @@ static int bnxt_dl_nvm_param_get(struct devlink *dl, u32 id,
 {
 	struct hwrm_nvm_get_variable_input req = {0};
 	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+	int rc;
 
 	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_GET_VARIABLE, -1, -1);
-	return bnxt_hwrm_nvm_req(bp, id, &req, sizeof(req), &ctx->val);
+	rc = bnxt_hwrm_nvm_req(bp, id, &req, sizeof(req), &ctx->val);
+	if (!rc)
+		if (id == BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK)
+			ctx->val.vbool = !ctx->val.vbool;
+
+	return rc;
 }
 
 static int bnxt_dl_nvm_param_set(struct devlink *dl, u32 id,
@@ -123,6 +136,10 @@ static int bnxt_dl_nvm_param_set(struct devlink *dl, u32 id,
 	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
 
 	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_NVM_SET_VARIABLE, -1, -1);
+
+	if (id == BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK)
+		ctx->val.vbool = !ctx->val.vbool;
+
 	return bnxt_hwrm_nvm_req(bp, id, &req, sizeof(req), &ctx->val);
 }
 
@@ -167,6 +184,11 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
 			      BIT(DEVLINK_PARAM_CMODE_PERMANENT),
 			      bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
 			      bnxt_dl_msix_validate),
+	DEVLINK_PARAM_DRIVER(BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK,
+			     "gre_ver_check", DEVLINK_PARAM_TYPE_BOOL,
+			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			     bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
+			     NULL),
 };
 
 int bnxt_dl_register(struct bnxt *bp)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index 0e67c05..e36e41a 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -37,6 +37,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
 #define NVM_OFF_MSIX_VEC_PER_PF_MIN	114
 #define NVM_OFF_IGNORE_ARI		164
 #define NVM_OFF_HW_TC_OFFLOAD		170
+#define NVM_OFF_DIS_GRE_VER_CHECK	171
 #define NVM_OFF_ENABLE_SRIOV		401
 
 #define BNXT_MSIX_VEC_MAX	1280
-- 
1.8.3.1

Re: [PATCH net-next 0/8] bnxt_en: devlink param updates

[Jakub Kicinski]

On Tue, 11 Sep 2018 14:14:57 +0530, Vasundhara Volam wrote:
> This patchset adds support for 4 generic and 1 driver-specific devlink
> parameters.
> 
> Also, this patchset adds support to return proper error code if
> HWRM_NVM_GET/SET_VARIABLE commands return error code
> HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED.
> 
> Vasundhara Volam (8):
>   devlink: Add generic parameter hw_tc_offload

Much like Jiri, I can't help but wonder why do you need this?

>   devlink: Add generic parameter ignore_ari
>   devlink: Add generic parameter msix_vec_per_pf_max
>   devlink: Add generic parameter msix_vec_per_pf_min

IMHO more structured API would be preferable if possible.  The string
keys won't scale if you want to set the parameters per PF, and
creating more structured API for PCIe which is a relatively slow
moving HW spec seems tractable.

Not to mention the question Alex posed before about where this knobs
should actually live.  I'm personally fine with devlink.

>   bnxt_en: Use hw_tc_offload and ignore_ari devlink parameters
>   bnxt_en: return proper error when FW returns
>     HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED
>   bnxt_en: Use msix_vec_per_pf_max and msix_vec_per_pf_min devlink
>     params.
>   bnxt_en: Add a driver specific devlink parameter.

The details about the device specific devlink parameter are very much
lacking.  Why does the patch subject not mention any specifics?  What's
GRE in the first place?

You really need to provide more details and docs for all these
parameters.

Re: [PATCH net-next 0/8] bnxt_en: devlink param updates

[Jiri Pirko]

Tue, Sep 11, 2018 at 01:33:51PM CEST, jakub.kicinski@netronome.com wrote:
>On Tue, 11 Sep 2018 14:14:57 +0530, Vasundhara Volam wrote:
>> This patchset adds support for 4 generic and 1 driver-specific devlink
>> parameters.
>> 
>> Also, this patchset adds support to return proper error code if
>> HWRM_NVM_GET/SET_VARIABLE commands return error code
>> HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED.
>> 
>> Vasundhara Volam (8):
>>   devlink: Add generic parameter hw_tc_offload
>
>Much like Jiri, I can't help but wonder why do you need this?
>
>>   devlink: Add generic parameter ignore_ari
>>   devlink: Add generic parameter msix_vec_per_pf_max
>>   devlink: Add generic parameter msix_vec_per_pf_min
>
>IMHO more structured API would be preferable if possible.  The string
>keys won't scale if you want to set the parameters per PF, and
>creating more structured API for PCIe which is a relatively slow
>moving HW spec seems tractable.
>
>Not to mention the question Alex posed before about where this knobs
>should actually live.  I'm personally fine with devlink.
>
>>   bnxt_en: Use hw_tc_offload and ignore_ari devlink parameters
>>   bnxt_en: return proper error when FW returns
>>     HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED
>>   bnxt_en: Use msix_vec_per_pf_max and msix_vec_per_pf_min devlink
>>     params.
>>   bnxt_en: Add a driver specific devlink parameter.
>
>The details about the device specific devlink parameter are very much
>lacking.  Why does the patch subject not mention any specifics?  What's
>GRE in the first place?
>
>You really need to provide more details and docs for all these
>parameters.

We really need Documentation/networking/devlink-params.txt to describe
these.

Re: [PATCH net-next 0/8] bnxt_en: devlink param updates

[Vasundhara Volam]

On Tue, Sep 11, 2018 at 5:25 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Sep 11, 2018 at 01:33:51PM CEST, jakub.kicinski@netronome.com wrote:
> >On Tue, 11 Sep 2018 14:14:57 +0530, Vasundhara Volam wrote:
> >> This patchset adds support for 4 generic and 1 driver-specific devlink
> >> parameters.
> >>
> >> Also, this patchset adds support to return proper error code if
> >> HWRM_NVM_GET/SET_VARIABLE commands return error code
> >> HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED.
> >>
> >> Vasundhara Volam (8):
> >>   devlink: Add generic parameter hw_tc_offload
> >
> >Much like Jiri, I can't help but wonder why do you need this?
> >
> >>   devlink: Add generic parameter ignore_ari
> >>   devlink: Add generic parameter msix_vec_per_pf_max
> >>   devlink: Add generic parameter msix_vec_per_pf_min
> >
> >IMHO more structured API would be preferable if possible.  The string
> >keys won't scale if you want to set the parameters per PF, and
> >creating more structured API for PCIe which is a relatively slow
> >moving HW spec seems tractable.
> >
> >Not to mention the question Alex posed before about where this knobs
> >should actually live.  I'm personally fine with devlink.
> >
> >>   bnxt_en: Use hw_tc_offload and ignore_ari devlink parameters
> >>   bnxt_en: return proper error when FW returns
> >>     HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED
> >>   bnxt_en: Use msix_vec_per_pf_max and msix_vec_per_pf_min devlink
> >>     params.
> >>   bnxt_en: Add a driver specific devlink parameter.
> >
> >The details about the device specific devlink parameter are very much
> >lacking.  Why does the patch subject not mention any specifics?  What's
> >GRE in the first place?
> >
> >You really need to provide more details and docs for all these
> >parameters.
>
> We really need Documentation/networking/devlink-params.txt to describe
> these.
>
Sure. I will add the documentation in my next version of the patchset
for all generic and driver specific parameters.

Re: [PATCH net-next 0/8] bnxt_en: devlink param updates

[Vasundhara Volam]

On Tue, Sep 11, 2018 at 5:04 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Tue, 11 Sep 2018 14:14:57 +0530, Vasundhara Volam wrote:
> > This patchset adds support for 4 generic and 1 driver-specific devlink
> > parameters.
> >
> > Also, this patchset adds support to return proper error code if
> > HWRM_NVM_GET/SET_VARIABLE commands return error code
> > HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED.
> >
> > Vasundhara Volam (8):
> >   devlink: Add generic parameter hw_tc_offload
>
> Much like Jiri, I can't help but wonder why do you need this?
There is a request from our customer for a way to toggle tc_offload
feature in our adapter.
>
> >   devlink: Add generic parameter ignore_ari
> >   devlink: Add generic parameter msix_vec_per_pf_max
> >   devlink: Add generic parameter msix_vec_per_pf_min
>
> IMHO more structured API would be preferable if possible.  The string
> keys won't scale if you want to set the parameters per PF, and
> creating more structured API for PCIe which is a relatively slow
> moving HW spec seems tractable.
Sorry, could you please suggest an example? We will try to adapt.
>
> Not to mention the question Alex posed before about where this knobs
> should actually live.  I'm personally fine with devlink.
Okay
>
> >   bnxt_en: Use hw_tc_offload and ignore_ari devlink parameters
> >   bnxt_en: return proper error when FW returns
> >     HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED
> >   bnxt_en: Use msix_vec_per_pf_max and msix_vec_per_pf_min devlink
> >     params.
> >   bnxt_en: Add a driver specific devlink parameter.
>
> The details about the device specific devlink parameter are very much
> lacking.  Why does the patch subject not mention any specifics?  What's
> GRE in the first place?
>
> You really need to provide more details and docs for all these
> parameters.
Sure, I will add all the documentation in my next version of the patch. Thanks.

Re: [PATCH net-next 0/8] bnxt_en: devlink param updates

[Jakub Kicinski]

On Wed, 12 Sep 2018 12:09:37 +0530, Vasundhara Volam wrote:
> On Tue, Sep 11, 2018 at 5:04 PM Jakub Kicinski wrote:
> > On Tue, 11 Sep 2018 14:14:57 +0530, Vasundhara Volam wrote:  
> > > This patchset adds support for 4 generic and 1 driver-specific devlink
> > > parameters.
> > >
> > > Also, this patchset adds support to return proper error code if
> > > HWRM_NVM_GET/SET_VARIABLE commands return error code
> > > HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED.
> > >
> > > Vasundhara Volam (8):
> > >   devlink: Add generic parameter hw_tc_offload  
> >
> > Much like Jiri, I can't help but wonder why do you need this?  
>
> There is a request from our customer for a way to toggle tc_offload
> feature in our adapter.

Vasundhara, again, we don't need to know who asked you to do this, but
_why_.  What problem are you solving?  What is the customer trying to
achieve?

> > >   devlink: Add generic parameter ignore_ari
> > >   devlink: Add generic parameter msix_vec_per_pf_max
> > >   devlink: Add generic parameter msix_vec_per_pf_min  
> >
> > IMHO more structured API would be preferable if possible.  The string
> > keys won't scale if you want to set the parameters per PF, and
> > creating more structured API for PCIe which is a relatively slow
> > moving HW spec seems tractable.  
>
> Sorry, could you please suggest an example? We will try to adapt.

My thinking was that the same way devlink device has ports, it should
have PCIe functions as objects which then have attributes.  Instead of
making everything a string-identified device attribute.  But I'm not
dead set on this if others don't think its a good idea.

Re: [PATCH net-next 0/8] bnxt_en: devlink param updates

[Vasundhara Volam]

On Wed, Sep 12, 2018 at 3:20 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 12 Sep 2018 12:09:37 +0530, Vasundhara Volam wrote:
> > On Tue, Sep 11, 2018 at 5:04 PM Jakub Kicinski wrote:
> > > On Tue, 11 Sep 2018 14:14:57 +0530, Vasundhara Volam wrote:
> > > > This patchset adds support for 4 generic and 1 driver-specific devlink
> > > > parameters.
> > > >
> > > > Also, this patchset adds support to return proper error code if
> > > > HWRM_NVM_GET/SET_VARIABLE commands return error code
> > > > HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED.
> > > >
> > > > Vasundhara Volam (8):
> > > >   devlink: Add generic parameter hw_tc_offload
> > >
> > > Much like Jiri, I can't help but wonder why do you need this?
> >
> > There is a request from our customer for a way to toggle tc_offload
> > feature in our adapter.
>
> Vasundhara, again, we don't need to know who asked you to do this, but
> _why_.  What problem are you solving?  What is the customer trying to
> achieve?
For Brand new big features like TC_offload, few customers are not willing
to enable it by default in the adapter(Firmware). This was a subjective decision
to disable TC_offload by default in the adapter.
>
> > > >   devlink: Add generic parameter ignore_ari
> > > >   devlink: Add generic parameter msix_vec_per_pf_max
> > > >   devlink: Add generic parameter msix_vec_per_pf_min
> > >
> > > IMHO more structured API would be preferable if possible.  The string
> > > keys won't scale if you want to set the parameters per PF, and
> > > creating more structured API for PCIe which is a relatively slow
> > > moving HW spec seems tractable.
> >
> > Sorry, could you please suggest an example? We will try to adapt.
>
> My thinking was that the same way devlink device has ports, it should
> have PCIe functions as objects which then have attributes.  Instead of
> making everything a string-identified device attribute.  But I'm not
> dead set on this if others don't think its a good idea.
Actually this parameters are for the port but the value given to this param
is applicable for individual PF. That's the reason I have added "per_pf" string.
If you think this is not a good idea, I can move this params to driver-specific.

Re: [PATCH net-next 0/8] bnxt_en: devlink param updates

[Jiri Pirko]

Fri, Sep 14, 2018 at 06:17:07AM CEST, vasundhara-v.volam@broadcom.com wrote:
>On Wed, Sep 12, 2018 at 3:20 PM Jakub Kicinski
><jakub.kicinski@netronome.com> wrote:
>>
>> On Wed, 12 Sep 2018 12:09:37 +0530, Vasundhara Volam wrote:
>> > On Tue, Sep 11, 2018 at 5:04 PM Jakub Kicinski wrote:
>> > > On Tue, 11 Sep 2018 14:14:57 +0530, Vasundhara Volam wrote:
>> > > > This patchset adds support for 4 generic and 1 driver-specific devlink
>> > > > parameters.
>> > > >
>> > > > Also, this patchset adds support to return proper error code if
>> > > > HWRM_NVM_GET/SET_VARIABLE commands return error code
>> > > > HWRM_ERR_CODE_RESOURCE_ACCESS_DENIED.
>> > > >
>> > > > Vasundhara Volam (8):
>> > > >   devlink: Add generic parameter hw_tc_offload
>> > >
>> > > Much like Jiri, I can't help but wonder why do you need this?
>> >
>> > There is a request from our customer for a way to toggle tc_offload
>> > feature in our adapter.
>>
>> Vasundhara, again, we don't need to know who asked you to do this, but
>> _why_.  What problem are you solving?  What is the customer trying to
>> achieve?
>For Brand new big features like TC_offload, few customers are not willing
>to enable it by default in the adapter(Firmware). This was a subjective decision
>to disable TC_offload by default in the adapter.

Again, why? Why it cannot be enabled in FW and just enabled/disabled by
ethtool flag? Don't say that "customers want it" please...


>>
>> > > >   devlink: Add generic parameter ignore_ari
>> > > >   devlink: Add generic parameter msix_vec_per_pf_max
>> > > >   devlink: Add generic parameter msix_vec_per_pf_min
>> > >
>> > > IMHO more structured API would be preferable if possible.  The string
>> > > keys won't scale if you want to set the parameters per PF, and
>> > > creating more structured API for PCIe which is a relatively slow
>> > > moving HW spec seems tractable.
>> >
>> > Sorry, could you please suggest an example? We will try to adapt.
>>
>> My thinking was that the same way devlink device has ports, it should
>> have PCIe functions as objects which then have attributes.  Instead of
>> making everything a string-identified device attribute.  But I'm not
>> dead set on this if others don't think its a good idea.
>Actually this parameters are for the port but the value given to this param
>is applicable for individual PF. That's the reason I have added "per_pf" string.
>If you think this is not a good idea, I can move this params to driver-specific.