[PATCH net-next 0/6] Batch of devlink related fixes

[PATCH net-next 0/6] Batch of devlink related fixes

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Hi,

I'm asking to apply this batch of devlink fixes to net-next and not to
net, because most if not all fixes are for old code or/and can be considered
as cleanup.

It will cancel the need to deal with merge conflicts for my next devlink series :).

Thanks

Leon Romanovsky (6):
  bnxt_en: Check devlink allocation and registration status
  bnxt_en: Properly remove port parameter support
  devlink: Delete not used port parameters APIs
  devlink: Remove single line function obfuscations
  ice: Delete always true check of PF pointer
  qed: Don't ignore devlink allocation failures

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   5 +-
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  26 +---
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.h |  13 --
 drivers/net/ethernet/intel/ice/ice_main.c     |   3 -
 drivers/net/ethernet/qlogic/qede/qede_main.c  |  12 +-
 drivers/scsi/qedf/qedf_main.c                 |   2 +
 include/net/devlink.h                         |   6 -
 net/core/devlink.c                            | 123 +++++-------------
 8 files changed, 47 insertions(+), 143 deletions(-)

-- 
2.31.1

[Intel-wired-lan] [PATCH net-next 0/6] Batch of devlink related fixes

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Hi,

I'm asking to apply this batch of devlink fixes to net-next and not to
net, because most if not all fixes are for old code or/and can be considered
as cleanup.

It will cancel the need to deal with merge conflicts for my next devlink series :).

Thanks

Leon Romanovsky (6):
  bnxt_en: Check devlink allocation and registration status
  bnxt_en: Properly remove port parameter support
  devlink: Delete not used port parameters APIs
  devlink: Remove single line function obfuscations
  ice: Delete always true check of PF pointer
  qed: Don't ignore devlink allocation failures

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   5 +-
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  26 +---
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.h |  13 --
 drivers/net/ethernet/intel/ice/ice_main.c     |   3 -
 drivers/net/ethernet/qlogic/qede/qede_main.c  |  12 +-
 drivers/scsi/qedf/qedf_main.c                 |   2 +
 include/net/devlink.h                         |   6 -
 net/core/devlink.c                            | 123 +++++-------------
 8 files changed, 47 insertions(+), 143 deletions(-)

-- 
2.31.1

[PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

devlink is a software interface that doesn't depend on any hardware
capabilities. The failure in SW means memory issues, wrong parameters,
programmer error e.t.c.

Like any other such interface in the kernel, the returned status of
devlink APIs should be checked and propagated further and not ignored.

Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  5 ++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
 3 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 037767b370d5..4c483fd91dbe 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -13370,7 +13370,9 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 
 	bnxt_inv_fw_health_reg(bp);
-	bnxt_dl_register(bp);
+	rc = bnxt_dl_register(bp);
+	if (rc)
+		goto init_err_dl;
 
 	rc = register_netdev(dev);
 	if (rc)
@@ -13390,6 +13392,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 init_err_cleanup:
 	bnxt_dl_unregister(bp);
+init_err_dl:
 	bnxt_shutdown_tc(bp);
 	bnxt_clear_int_mode(bp);
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index bf7d3c17049b..dc0851f709f5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -134,7 +134,7 @@ void bnxt_dl_fw_reporters_create(struct bnxt *bp)
 {
 	struct bnxt_fw_health *health = bp->fw_health;
 
-	if (!bp->dl || !health)
+	if (!health)
 		return;
 
 	if (!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET) || health->fw_reset_reporter)
@@ -188,7 +188,7 @@ void bnxt_dl_fw_reporters_destroy(struct bnxt *bp, bool all)
 {
 	struct bnxt_fw_health *health = bp->fw_health;
 
-	if (!bp->dl || !health)
+	if (!health)
 		return;
 
 	if ((all || !(bp->fw_cap & BNXT_FW_CAP_HOT_RESET)) &&
@@ -781,6 +781,7 @@ int bnxt_dl_register(struct bnxt *bp)
 {
 	const struct devlink_ops *devlink_ops;
 	struct devlink_port_attrs attrs = {};
+	struct bnxt_dl *bp_dl;
 	struct devlink *dl;
 	int rc;
 
@@ -795,7 +796,9 @@ int bnxt_dl_register(struct bnxt *bp)
 		return -ENOMEM;
 	}
 
-	bnxt_link_bp_to_dl(bp, dl);
+	bp->dl = dl;
+	bp_dl = devlink_priv(dl);
+	bp_dl->bp = bp;
 
 	/* Add switchdev eswitch mode setting, if SRIOV supported */
 	if (pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV) &&
@@ -826,7 +829,6 @@ int bnxt_dl_register(struct bnxt *bp)
 err_dl_port_unreg:
 	devlink_port_unregister(&bp->dl_port);
 err_dl_free:
-	bnxt_link_bp_to_dl(bp, NULL);
 	devlink_free(dl);
 	return rc;
 }
@@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
 {
 	struct devlink *dl = bp->dl;
 
-	if (!dl)
-		return;
-
 	if (BNXT_PF(bp)) {
 		bnxt_dl_params_unregister(bp);
 		devlink_port_unregister(&bp->dl_port);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index d889f240da2b..406dc655a5fc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -20,19 +20,6 @@ static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
 	return ((struct bnxt_dl *)devlink_priv(dl))->bp;
 }
 
-/* To clear devlink pointer from bp, pass NULL dl */
-static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
-{
-	bp->dl = dl;
-
-	/* add a back pointer in dl to bp */
-	if (dl) {
-		struct bnxt_dl *bp_dl = devlink_priv(dl);
-
-		bp_dl->bp = bp;
-	}
-}
-
 #define NVM_OFF_MSIX_VEC_PER_PF_MAX	108
 #define NVM_OFF_MSIX_VEC_PER_PF_MIN	114
 #define NVM_OFF_IGNORE_ARI		164
-- 
2.31.1

[Intel-wired-lan] [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

devlink is a software interface that doesn't depend on any hardware
capabilities. The failure in SW means memory issues, wrong parameters,
programmer error e.t.c.

Like any other such interface in the kernel, the returned status of
devlink APIs should be checked and propagated further and not ignored.

Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  5 ++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
 3 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 037767b370d5..4c483fd91dbe 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -13370,7 +13370,9 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 
 	bnxt_inv_fw_health_reg(bp);
-	bnxt_dl_register(bp);
+	rc = bnxt_dl_register(bp);
+	if (rc)
+		goto init_err_dl;
 
 	rc = register_netdev(dev);
 	if (rc)
@@ -13390,6 +13392,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 init_err_cleanup:
 	bnxt_dl_unregister(bp);
+init_err_dl:
 	bnxt_shutdown_tc(bp);
 	bnxt_clear_int_mode(bp);
 
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index bf7d3c17049b..dc0851f709f5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -134,7 +134,7 @@ void bnxt_dl_fw_reporters_create(struct bnxt *bp)
 {
 	struct bnxt_fw_health *health = bp->fw_health;
 
-	if (!bp->dl || !health)
+	if (!health)
 		return;
 
 	if (!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET) || health->fw_reset_reporter)
@@ -188,7 +188,7 @@ void bnxt_dl_fw_reporters_destroy(struct bnxt *bp, bool all)
 {
 	struct bnxt_fw_health *health = bp->fw_health;
 
-	if (!bp->dl || !health)
+	if (!health)
 		return;
 
 	if ((all || !(bp->fw_cap & BNXT_FW_CAP_HOT_RESET)) &&
@@ -781,6 +781,7 @@ int bnxt_dl_register(struct bnxt *bp)
 {
 	const struct devlink_ops *devlink_ops;
 	struct devlink_port_attrs attrs = {};
+	struct bnxt_dl *bp_dl;
 	struct devlink *dl;
 	int rc;
 
@@ -795,7 +796,9 @@ int bnxt_dl_register(struct bnxt *bp)
 		return -ENOMEM;
 	}
 
-	bnxt_link_bp_to_dl(bp, dl);
+	bp->dl = dl;
+	bp_dl = devlink_priv(dl);
+	bp_dl->bp = bp;
 
 	/* Add switchdev eswitch mode setting, if SRIOV supported */
 	if (pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV) &&
@@ -826,7 +829,6 @@ int bnxt_dl_register(struct bnxt *bp)
 err_dl_port_unreg:
 	devlink_port_unregister(&bp->dl_port);
 err_dl_free:
-	bnxt_link_bp_to_dl(bp, NULL);
 	devlink_free(dl);
 	return rc;
 }
@@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
 {
 	struct devlink *dl = bp->dl;
 
-	if (!dl)
-		return;
-
 	if (BNXT_PF(bp)) {
 		bnxt_dl_params_unregister(bp);
 		devlink_port_unregister(&bp->dl_port);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index d889f240da2b..406dc655a5fc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -20,19 +20,6 @@ static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
 	return ((struct bnxt_dl *)devlink_priv(dl))->bp;
 }
 
-/* To clear devlink pointer from bp, pass NULL dl */
-static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
-{
-	bp->dl = dl;
-
-	/* add a back pointer in dl to bp */
-	if (dl) {
-		struct bnxt_dl *bp_dl = devlink_priv(dl);
-
-		bp_dl->bp = bp;
-	}
-}
-
 #define NVM_OFF_MSIX_VEC_PER_PF_MAX	108
 #define NVM_OFF_MSIX_VEC_PER_PF_MIN	114
 #define NVM_OFF_IGNORE_ARI		164
-- 
2.31.1

[PATCH net-next 2/6] bnxt_en: Properly remove port parameter support

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

This driver doesn't have any port parameters and registers
devlink port parameters with empty table. Remove the useless
calls to devlink_port_params_register and _unregister.

Fixes: da203dfa89ce ("Revert "devlink: Add a generic wake_on_lan port parameter"")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index dc0851f709f5..ed95e28d60ef 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -736,9 +736,6 @@ static const struct devlink_param bnxt_dl_params[] = {
 			     NULL),
 };
 
-static const struct devlink_param bnxt_dl_port_params[] = {
-};
-
 static int bnxt_dl_params_register(struct bnxt *bp)
 {
 	int rc;
@@ -753,14 +750,6 @@ static int bnxt_dl_params_register(struct bnxt *bp)
 			    rc);
 		return rc;
 	}
-	rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
-					  ARRAY_SIZE(bnxt_dl_port_params));
-	if (rc) {
-		netdev_err(bp->dev, "devlink_port_params_register failed\n");
-		devlink_params_unregister(bp->dl, bnxt_dl_params,
-					  ARRAY_SIZE(bnxt_dl_params));
-		return rc;
-	}
 	devlink_params_publish(bp->dl);
 
 	return 0;
@@ -773,8 +762,6 @@ static void bnxt_dl_params_unregister(struct bnxt *bp)
 
 	devlink_params_unregister(bp->dl, bnxt_dl_params,
 				  ARRAY_SIZE(bnxt_dl_params));
-	devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
-				       ARRAY_SIZE(bnxt_dl_port_params));
 }
 
 int bnxt_dl_register(struct bnxt *bp)
-- 
2.31.1

[Intel-wired-lan] [PATCH net-next 2/6] bnxt_en: Properly remove port parameter support

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

This driver doesn't have any port parameters and registers
devlink port parameters with empty table. Remove the useless
calls to devlink_port_params_register and _unregister.

Fixes: da203dfa89ce ("Revert "devlink: Add a generic wake_on_lan port parameter"")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index dc0851f709f5..ed95e28d60ef 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -736,9 +736,6 @@ static const struct devlink_param bnxt_dl_params[] = {
 			     NULL),
 };
 
-static const struct devlink_param bnxt_dl_port_params[] = {
-};
-
 static int bnxt_dl_params_register(struct bnxt *bp)
 {
 	int rc;
@@ -753,14 +750,6 @@ static int bnxt_dl_params_register(struct bnxt *bp)
 			    rc);
 		return rc;
 	}
-	rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
-					  ARRAY_SIZE(bnxt_dl_port_params));
-	if (rc) {
-		netdev_err(bp->dev, "devlink_port_params_register failed\n");
-		devlink_params_unregister(bp->dl, bnxt_dl_params,
-					  ARRAY_SIZE(bnxt_dl_params));
-		return rc;
-	}
 	devlink_params_publish(bp->dl);
 
 	return 0;
@@ -773,8 +762,6 @@ static void bnxt_dl_params_unregister(struct bnxt *bp)
 
 	devlink_params_unregister(bp->dl, bnxt_dl_params,
 				  ARRAY_SIZE(bnxt_dl_params));
-	devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
-				       ARRAY_SIZE(bnxt_dl_port_params));
 }
 
 int bnxt_dl_register(struct bnxt *bp)
-- 
2.31.1

[PATCH net-next 3/6] devlink: Delete not used port parameters APIs

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

There is no in-kernel users for the devlink port parameters API,
so let's remove it.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/net/devlink.h |  6 ------
 net/core/devlink.c    | 42 ------------------------------------------
 2 files changed, 48 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index c902e8e5f012..a7852a257bf6 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1653,12 +1653,6 @@ void devlink_param_unregister(struct devlink *devlink,
 			      const struct devlink_param *param);
 void devlink_params_publish(struct devlink *devlink);
 void devlink_params_unpublish(struct devlink *devlink);
-int devlink_port_params_register(struct devlink_port *devlink_port,
-				 const struct devlink_param *params,
-				 size_t params_count);
-void devlink_port_params_unregister(struct devlink_port *devlink_port,
-				    const struct devlink_param *params,
-				    size_t params_count);
 int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 				       union devlink_param_value *init_val);
 int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7d975057c2a9..9c071f4e609f 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -10117,48 +10117,6 @@ void devlink_params_unpublish(struct devlink *devlink)
 }
 EXPORT_SYMBOL_GPL(devlink_params_unpublish);
 
-/**
- *	devlink_port_params_register - register port configuration parameters
- *
- *	@devlink_port: devlink port
- *	@params: configuration parameters array
- *	@params_count: number of parameters provided
- *
- *	Register the configuration parameters supported by the port.
- */
-int devlink_port_params_register(struct devlink_port *devlink_port,
-				 const struct devlink_param *params,
-				 size_t params_count)
-{
-	return __devlink_params_register(devlink_port->devlink,
-					 devlink_port->index,
-					 &devlink_port->param_list, params,
-					 params_count,
-					 DEVLINK_CMD_PORT_PARAM_NEW,
-					 DEVLINK_CMD_PORT_PARAM_DEL);
-}
-EXPORT_SYMBOL_GPL(devlink_port_params_register);
-
-/**
- *	devlink_port_params_unregister - unregister port configuration
- *	parameters
- *
- *	@devlink_port: devlink port
- *	@params: configuration parameters array
- *	@params_count: number of parameters provided
- */
-void devlink_port_params_unregister(struct devlink_port *devlink_port,
-				    const struct devlink_param *params,
-				    size_t params_count)
-{
-	return __devlink_params_unregister(devlink_port->devlink,
-					   devlink_port->index,
-					   &devlink_port->param_list,
-					   params, params_count,
-					   DEVLINK_CMD_PORT_PARAM_DEL);
-}
-EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
-
 static int
 __devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
 				     union devlink_param_value *init_val)
-- 
2.31.1

[Intel-wired-lan] [PATCH net-next 3/6] devlink: Delete not used port parameters APIs

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

There is no in-kernel users for the devlink port parameters API,
so let's remove it.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/net/devlink.h |  6 ------
 net/core/devlink.c    | 42 ------------------------------------------
 2 files changed, 48 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index c902e8e5f012..a7852a257bf6 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1653,12 +1653,6 @@ void devlink_param_unregister(struct devlink *devlink,
 			      const struct devlink_param *param);
 void devlink_params_publish(struct devlink *devlink);
 void devlink_params_unpublish(struct devlink *devlink);
-int devlink_port_params_register(struct devlink_port *devlink_port,
-				 const struct devlink_param *params,
-				 size_t params_count);
-void devlink_port_params_unregister(struct devlink_port *devlink_port,
-				    const struct devlink_param *params,
-				    size_t params_count);
 int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 				       union devlink_param_value *init_val);
 int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 7d975057c2a9..9c071f4e609f 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -10117,48 +10117,6 @@ void devlink_params_unpublish(struct devlink *devlink)
 }
 EXPORT_SYMBOL_GPL(devlink_params_unpublish);
 
-/**
- *	devlink_port_params_register - register port configuration parameters
- *
- *	@devlink_port: devlink port
- *	@params: configuration parameters array
- *	@params_count: number of parameters provided
- *
- *	Register the configuration parameters supported by the port.
- */
-int devlink_port_params_register(struct devlink_port *devlink_port,
-				 const struct devlink_param *params,
-				 size_t params_count)
-{
-	return __devlink_params_register(devlink_port->devlink,
-					 devlink_port->index,
-					 &devlink_port->param_list, params,
-					 params_count,
-					 DEVLINK_CMD_PORT_PARAM_NEW,
-					 DEVLINK_CMD_PORT_PARAM_DEL);
-}
-EXPORT_SYMBOL_GPL(devlink_port_params_register);
-
-/**
- *	devlink_port_params_unregister - unregister port configuration
- *	parameters
- *
- *	@devlink_port: devlink port
- *	@params: configuration parameters array
- *	@params_count: number of parameters provided
- */
-void devlink_port_params_unregister(struct devlink_port *devlink_port,
-				    const struct devlink_param *params,
-				    size_t params_count)
-{
-	return __devlink_params_unregister(devlink_port->devlink,
-					   devlink_port->index,
-					   &devlink_port->param_list,
-					   params, params_count,
-					   DEVLINK_CMD_PORT_PARAM_DEL);
-}
-EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
-
 static int
 __devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
 				     union devlink_param_value *init_val)
-- 
2.31.1

[PATCH net-next 4/6] devlink: Remove single line function obfuscations

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

There is no need in extra one line functions to call relevant
functions only once.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 89 ++++++++++++++++++----------------------------
 1 file changed, 34 insertions(+), 55 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 9c071f4e609f..3ea33c689790 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -10117,13 +10117,26 @@ void devlink_params_unpublish(struct devlink *devlink)
 }
 EXPORT_SYMBOL_GPL(devlink_params_unpublish);
 
-static int
-__devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
-				     union devlink_param_value *init_val)
+/**
+ *	devlink_param_driverinit_value_get - get configuration parameter
+ *					     value for driver initializing
+ *
+ *	@devlink: devlink
+ *	@param_id: parameter ID
+ *	@init_val: value of parameter in driverinit configuration mode
+ *
+ *	This function should be used by the driver to get driverinit
+ *	configuration for initialization after reload command.
+ */
+int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
+				       union devlink_param_value *init_val)
 {
 	struct devlink_param_item *param_item;
 
-	param_item = devlink_param_find_by_id(param_list, param_id);
+	if (!devlink_reload_supported(devlink->ops))
+		return -EOPNOTSUPP;
+
+	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
 	if (!param_item)
 		return -EINVAL;
 
@@ -10139,17 +10152,26 @@ __devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
 
-static int
-__devlink_param_driverinit_value_set(struct devlink *devlink,
-				     unsigned int port_index,
-				     struct list_head *param_list, u32 param_id,
-				     union devlink_param_value init_val,
-				     enum devlink_command cmd)
+/**
+ *	devlink_param_driverinit_value_set - set value of configuration
+ *					     parameter for driverinit
+ *					     configuration mode
+ *
+ *	@devlink: devlink
+ *	@param_id: parameter ID
+ *	@init_val: value of parameter to set for driverinit configuration mode
+ *
+ *	This function should be used by the driver to set driverinit
+ *	configuration mode default value.
+ */
+int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
+				       union devlink_param_value init_val)
 {
 	struct devlink_param_item *param_item;
 
-	param_item = devlink_param_find_by_id(param_list, param_id);
+	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
 	if (!param_item)
 		return -EINVAL;
 
@@ -10163,52 +10185,9 @@ __devlink_param_driverinit_value_set(struct devlink *devlink,
 		param_item->driverinit_value = init_val;
 	param_item->driverinit_value_valid = true;
 
-	devlink_param_notify(devlink, port_index, param_item, cmd);
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_NEW);
 	return 0;
 }
-
-/**
- *	devlink_param_driverinit_value_get - get configuration parameter
- *					     value for driver initializing
- *
- *	@devlink: devlink
- *	@param_id: parameter ID
- *	@init_val: value of parameter in driverinit configuration mode
- *
- *	This function should be used by the driver to get driverinit
- *	configuration for initialization after reload command.
- */
-int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
-				       union devlink_param_value *init_val)
-{
-	if (!devlink_reload_supported(devlink->ops))
-		return -EOPNOTSUPP;
-
-	return __devlink_param_driverinit_value_get(&devlink->param_list,
-						    param_id, init_val);
-}
-EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
-
-/**
- *	devlink_param_driverinit_value_set - set value of configuration
- *					     parameter for driverinit
- *					     configuration mode
- *
- *	@devlink: devlink
- *	@param_id: parameter ID
- *	@init_val: value of parameter to set for driverinit configuration mode
- *
- *	This function should be used by the driver to set driverinit
- *	configuration mode default value.
- */
-int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
-				       union devlink_param_value init_val)
-{
-	return __devlink_param_driverinit_value_set(devlink, 0,
-						    &devlink->param_list,
-						    param_id, init_val,
-						    DEVLINK_CMD_PARAM_NEW);
-}
 EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_set);
 
 /**
-- 
2.31.1

[Intel-wired-lan] [PATCH net-next 4/6] devlink: Remove single line function obfuscations

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

There is no need in extra one line functions to call relevant
functions only once.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 net/core/devlink.c | 89 ++++++++++++++++++----------------------------
 1 file changed, 34 insertions(+), 55 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 9c071f4e609f..3ea33c689790 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -10117,13 +10117,26 @@ void devlink_params_unpublish(struct devlink *devlink)
 }
 EXPORT_SYMBOL_GPL(devlink_params_unpublish);
 
-static int
-__devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
-				     union devlink_param_value *init_val)
+/**
+ *	devlink_param_driverinit_value_get - get configuration parameter
+ *					     value for driver initializing
+ *
+ *	@devlink: devlink
+ *	@param_id: parameter ID
+ *	@init_val: value of parameter in driverinit configuration mode
+ *
+ *	This function should be used by the driver to get driverinit
+ *	configuration for initialization after reload command.
+ */
+int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
+				       union devlink_param_value *init_val)
 {
 	struct devlink_param_item *param_item;
 
-	param_item = devlink_param_find_by_id(param_list, param_id);
+	if (!devlink_reload_supported(devlink->ops))
+		return -EOPNOTSUPP;
+
+	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
 	if (!param_item)
 		return -EINVAL;
 
@@ -10139,17 +10152,26 @@ __devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
 
-static int
-__devlink_param_driverinit_value_set(struct devlink *devlink,
-				     unsigned int port_index,
-				     struct list_head *param_list, u32 param_id,
-				     union devlink_param_value init_val,
-				     enum devlink_command cmd)
+/**
+ *	devlink_param_driverinit_value_set - set value of configuration
+ *					     parameter for driverinit
+ *					     configuration mode
+ *
+ *	@devlink: devlink
+ *	@param_id: parameter ID
+ *	@init_val: value of parameter to set for driverinit configuration mode
+ *
+ *	This function should be used by the driver to set driverinit
+ *	configuration mode default value.
+ */
+int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
+				       union devlink_param_value init_val)
 {
 	struct devlink_param_item *param_item;
 
-	param_item = devlink_param_find_by_id(param_list, param_id);
+	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
 	if (!param_item)
 		return -EINVAL;
 
@@ -10163,52 +10185,9 @@ __devlink_param_driverinit_value_set(struct devlink *devlink,
 		param_item->driverinit_value = init_val;
 	param_item->driverinit_value_valid = true;
 
-	devlink_param_notify(devlink, port_index, param_item, cmd);
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_NEW);
 	return 0;
 }
-
-/**
- *	devlink_param_driverinit_value_get - get configuration parameter
- *					     value for driver initializing
- *
- *	@devlink: devlink
- *	@param_id: parameter ID
- *	@init_val: value of parameter in driverinit configuration mode
- *
- *	This function should be used by the driver to get driverinit
- *	configuration for initialization after reload command.
- */
-int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
-				       union devlink_param_value *init_val)
-{
-	if (!devlink_reload_supported(devlink->ops))
-		return -EOPNOTSUPP;
-
-	return __devlink_param_driverinit_value_get(&devlink->param_list,
-						    param_id, init_val);
-}
-EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
-
-/**
- *	devlink_param_driverinit_value_set - set value of configuration
- *					     parameter for driverinit
- *					     configuration mode
- *
- *	@devlink: devlink
- *	@param_id: parameter ID
- *	@init_val: value of parameter to set for driverinit configuration mode
- *
- *	This function should be used by the driver to set driverinit
- *	configuration mode default value.
- */
-int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
-				       union devlink_param_value init_val)
-{
-	return __devlink_param_driverinit_value_set(devlink, 0,
-						    &devlink->param_list,
-						    param_id, init_val,
-						    DEVLINK_CMD_PARAM_NEW);
-}
 EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_set);
 
 /**
-- 
2.31.1

[PATCH net-next 5/6] ice: Delete always true check of PF pointer

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

PF pointer is always valid when PCI core calls its .shutdown() and
.remove() callbacks. There is no need to check it again.

Fixes: 837f08fdecbe ("ice: Add basic driver framework for Intel(R) E800 Series")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 34e64533026a..aacc0b345bbe 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4593,9 +4593,6 @@ static void ice_remove(struct pci_dev *pdev)
 	struct ice_pf *pf = pci_get_drvdata(pdev);
 	int i;
 
-	if (!pf)
-		return;
-
 	for (i = 0; i < ICE_MAX_RESET_WAIT; i++) {
 		if (!ice_is_reset_in_progress(pf->state))
 			break;
-- 
2.31.1

[Intel-wired-lan] [PATCH net-next 5/6] ice: Delete always true check of PF pointer

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

PF pointer is always valid when PCI core calls its .shutdown() and
.remove() callbacks. There is no need to check it again.

Fixes: 837f08fdecbe ("ice: Add basic driver framework for Intel(R) E800 Series")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 34e64533026a..aacc0b345bbe 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4593,9 +4593,6 @@ static void ice_remove(struct pci_dev *pdev)
 	struct ice_pf *pf = pci_get_drvdata(pdev);
 	int i;
 
-	if (!pf)
-		return;
-
 	for (i = 0; i < ICE_MAX_RESET_WAIT; i++) {
 		if (!ice_is_reset_in_progress(pf->state))
 			break;
-- 
2.31.1

[PATCH net-next 6/6] qed: Don't ignore devlink allocation failures

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

devlink is a software interface that doesn't depend on any hardware
capabilities. The failure in SW means memory issues, wrong parameters,
programmer error e.t.c.

Like any other such interface in the kernel, the returned status of
devlink APIs should be checked and propagated further and not ignored.

Fixes: 755f982bb1ff ("qed/qede: make devlink survive recovery")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/qlogic/qede/qede_main.c | 12 +++++-------
 drivers/scsi/qedf/qedf_main.c                |  2 ++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index 9837bdb89cd4..ee4c3bd28a93 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -1176,19 +1176,17 @@ static int __qede_probe(struct pci_dev *pdev, u32 dp_module, u8 dp_level,
 		edev->devlink = qed_ops->common->devlink_register(cdev);
 		if (IS_ERR(edev->devlink)) {
 			DP_NOTICE(edev, "Cannot register devlink\n");
+			rc = PTR_ERR(edev->devlink);
 			edev->devlink = NULL;
-			/* Go on, we can live without devlink */
+			goto err3;
 		}
 	} else {
 		struct net_device *ndev = pci_get_drvdata(pdev);
+		struct qed_devlink *qdl;
 
 		edev = netdev_priv(ndev);
-
-		if (edev->devlink) {
-			struct qed_devlink *qdl = devlink_priv(edev->devlink);
-
-			qdl->cdev = cdev;
-		}
+		qdl = devlink_priv(edev->devlink);
+		qdl->cdev = cdev;
 		edev->cdev = cdev;
 		memset(&edev->stats, 0, sizeof(edev->stats));
 		memcpy(&edev->dev_info, &dev_info, sizeof(dev_info));
diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index 42d0d941dba5..94ee08fab46a 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -3416,7 +3416,9 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
 		qedf->devlink = qed_ops->common->devlink_register(qedf->cdev);
 		if (IS_ERR(qedf->devlink)) {
 			QEDF_ERR(&qedf->dbg_ctx, "Cannot register devlink\n");
+			rc = PTR_ERR(qedf->devlink);
 			qedf->devlink = NULL;
+			goto err2;
 		}
 	}
 
-- 
2.31.1

[Intel-wired-lan] [PATCH net-next 6/6] qed: Don't ignore devlink allocation failures

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

devlink is a software interface that doesn't depend on any hardware
capabilities. The failure in SW means memory issues, wrong parameters,
programmer error e.t.c.

Like any other such interface in the kernel, the returned status of
devlink APIs should be checked and propagated further and not ignored.

Fixes: 755f982bb1ff ("qed/qede: make devlink survive recovery")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/qlogic/qede/qede_main.c | 12 +++++-------
 drivers/scsi/qedf/qedf_main.c                |  2 ++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index 9837bdb89cd4..ee4c3bd28a93 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -1176,19 +1176,17 @@ static int __qede_probe(struct pci_dev *pdev, u32 dp_module, u8 dp_level,
 		edev->devlink = qed_ops->common->devlink_register(cdev);
 		if (IS_ERR(edev->devlink)) {
 			DP_NOTICE(edev, "Cannot register devlink\n");
+			rc = PTR_ERR(edev->devlink);
 			edev->devlink = NULL;
-			/* Go on, we can live without devlink */
+			goto err3;
 		}
 	} else {
 		struct net_device *ndev = pci_get_drvdata(pdev);
+		struct qed_devlink *qdl;
 
 		edev = netdev_priv(ndev);
-
-		if (edev->devlink) {
-			struct qed_devlink *qdl = devlink_priv(edev->devlink);
-
-			qdl->cdev = cdev;
-		}
+		qdl = devlink_priv(edev->devlink);
+		qdl->cdev = cdev;
 		edev->cdev = cdev;
 		memset(&edev->stats, 0, sizeof(edev->stats));
 		memcpy(&edev->dev_info, &dev_info, sizeof(dev_info));
diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index 42d0d941dba5..94ee08fab46a 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -3416,7 +3416,9 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
 		qedf->devlink = qed_ops->common->devlink_register(qedf->cdev);
 		if (IS_ERR(qedf->devlink)) {
 			QEDF_ERR(&qedf->dbg_ctx, "Cannot register devlink\n");
+			rc = PTR_ERR(qedf->devlink);
 			qedf->devlink = NULL;
+			goto err2;
 		}
 	}
 
-- 
2.31.1

Re: [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Edwin Peer]

On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> devlink is a software interface that doesn't depend on any hardware
> capabilities. The failure in SW means memory issues, wrong parameters,
> programmer error e.t.c.
>
> Like any other such interface in the kernel, the returned status of
> devlink APIs should be checked and propagated further and not ignored.
>
> Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  5 ++++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
>  3 files changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 037767b370d5..4c483fd91dbe 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -13370,7 +13370,9 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>         }
>
>         bnxt_inv_fw_health_reg(bp);
> -       bnxt_dl_register(bp);
> +       rc = bnxt_dl_register(bp);
> +       if (rc)
> +               goto init_err_dl;
>
>         rc = register_netdev(dev);
>         if (rc)
> @@ -13390,6 +13392,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
>  init_err_cleanup:
>         bnxt_dl_unregister(bp);
> +init_err_dl:
>         bnxt_shutdown_tc(bp);
>         bnxt_clear_int_mode(bp);
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index bf7d3c17049b..dc0851f709f5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -134,7 +134,7 @@ void bnxt_dl_fw_reporters_create(struct bnxt *bp)
>  {
>         struct bnxt_fw_health *health = bp->fw_health;
>
> -       if (!bp->dl || !health)
> +       if (!health)
>                 return;
>
>         if (!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET) || health->fw_reset_reporter)
> @@ -188,7 +188,7 @@ void bnxt_dl_fw_reporters_destroy(struct bnxt *bp, bool all)
>  {
>         struct bnxt_fw_health *health = bp->fw_health;
>
> -       if (!bp->dl || !health)
> +       if (!health)
>                 return;
>
>         if ((all || !(bp->fw_cap & BNXT_FW_CAP_HOT_RESET)) &&
> @@ -781,6 +781,7 @@ int bnxt_dl_register(struct bnxt *bp)
>  {
>         const struct devlink_ops *devlink_ops;
>         struct devlink_port_attrs attrs = {};
> +       struct bnxt_dl *bp_dl;
>         struct devlink *dl;
>         int rc;
>
> @@ -795,7 +796,9 @@ int bnxt_dl_register(struct bnxt *bp)
>                 return -ENOMEM;
>         }
>
> -       bnxt_link_bp_to_dl(bp, dl);
> +       bp->dl = dl;
> +       bp_dl = devlink_priv(dl);
> +       bp_dl->bp = bp;
>
>         /* Add switchdev eswitch mode setting, if SRIOV supported */
>         if (pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV) &&
> @@ -826,7 +829,6 @@ int bnxt_dl_register(struct bnxt *bp)
>  err_dl_port_unreg:
>         devlink_port_unregister(&bp->dl_port);
>  err_dl_free:
> -       bnxt_link_bp_to_dl(bp, NULL);
>         devlink_free(dl);
>         return rc;
>  }
> @@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
>  {
>         struct devlink *dl = bp->dl;
>
> -       if (!dl)
> -               return;
> -

minor nit: There's obviously nothing incorrect about doing this (and
adding the additional error label in the cleanup code above), but bnxt
has generally adopted a style of having cleanup functions being
idempotent. It generally makes error handling simpler and less error
prone.

>         if (BNXT_PF(bp)) {
>                 bnxt_dl_params_unregister(bp);
>                 devlink_port_unregister(&bp->dl_port);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> index d889f240da2b..406dc655a5fc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> @@ -20,19 +20,6 @@ static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
>         return ((struct bnxt_dl *)devlink_priv(dl))->bp;
>  }
>
> -/* To clear devlink pointer from bp, pass NULL dl */
> -static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
> -{
> -       bp->dl = dl;
> -
> -       /* add a back pointer in dl to bp */
> -       if (dl) {
> -               struct bnxt_dl *bp_dl = devlink_priv(dl);
> -
> -               bp_dl->bp = bp;
> -       }
> -}
> -
>  #define NVM_OFF_MSIX_VEC_PER_PF_MAX    108
>  #define NVM_OFF_MSIX_VEC_PER_PF_MIN    114
>  #define NVM_OFF_IGNORE_ARI             164
> --
> 2.31.1
>

Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

Regards,
Edwin Peer

[Intel-wired-lan] [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Edwin Peer]

On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> devlink is a software interface that doesn't depend on any hardware
> capabilities. The failure in SW means memory issues, wrong parameters,
> programmer error e.t.c.
>
> Like any other such interface in the kernel, the returned status of
> devlink APIs should be checked and propagated further and not ignored.
>
> Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  5 ++++-
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
>  3 files changed, 10 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 037767b370d5..4c483fd91dbe 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -13370,7 +13370,9 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>         }
>
>         bnxt_inv_fw_health_reg(bp);
> -       bnxt_dl_register(bp);
> +       rc = bnxt_dl_register(bp);
> +       if (rc)
> +               goto init_err_dl;
>
>         rc = register_netdev(dev);
>         if (rc)
> @@ -13390,6 +13392,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
>  init_err_cleanup:
>         bnxt_dl_unregister(bp);
> +init_err_dl:
>         bnxt_shutdown_tc(bp);
>         bnxt_clear_int_mode(bp);
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index bf7d3c17049b..dc0851f709f5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -134,7 +134,7 @@ void bnxt_dl_fw_reporters_create(struct bnxt *bp)
>  {
>         struct bnxt_fw_health *health = bp->fw_health;
>
> -       if (!bp->dl || !health)
> +       if (!health)
>                 return;
>
>         if (!(bp->fw_cap & BNXT_FW_CAP_HOT_RESET) || health->fw_reset_reporter)
> @@ -188,7 +188,7 @@ void bnxt_dl_fw_reporters_destroy(struct bnxt *bp, bool all)
>  {
>         struct bnxt_fw_health *health = bp->fw_health;
>
> -       if (!bp->dl || !health)
> +       if (!health)
>                 return;
>
>         if ((all || !(bp->fw_cap & BNXT_FW_CAP_HOT_RESET)) &&
> @@ -781,6 +781,7 @@ int bnxt_dl_register(struct bnxt *bp)
>  {
>         const struct devlink_ops *devlink_ops;
>         struct devlink_port_attrs attrs = {};
> +       struct bnxt_dl *bp_dl;
>         struct devlink *dl;
>         int rc;
>
> @@ -795,7 +796,9 @@ int bnxt_dl_register(struct bnxt *bp)
>                 return -ENOMEM;
>         }
>
> -       bnxt_link_bp_to_dl(bp, dl);
> +       bp->dl = dl;
> +       bp_dl = devlink_priv(dl);
> +       bp_dl->bp = bp;
>
>         /* Add switchdev eswitch mode setting, if SRIOV supported */
>         if (pci_find_ext_capability(bp->pdev, PCI_EXT_CAP_ID_SRIOV) &&
> @@ -826,7 +829,6 @@ int bnxt_dl_register(struct bnxt *bp)
>  err_dl_port_unreg:
>         devlink_port_unregister(&bp->dl_port);
>  err_dl_free:
> -       bnxt_link_bp_to_dl(bp, NULL);
>         devlink_free(dl);
>         return rc;
>  }
> @@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
>  {
>         struct devlink *dl = bp->dl;
>
> -       if (!dl)
> -               return;
> -

minor nit: There's obviously nothing incorrect about doing this (and
adding the additional error label in the cleanup code above), but bnxt
has generally adopted a style of having cleanup functions being
idempotent. It generally makes error handling simpler and less error
prone.

>         if (BNXT_PF(bp)) {
>                 bnxt_dl_params_unregister(bp);
>                 devlink_port_unregister(&bp->dl_port);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> index d889f240da2b..406dc655a5fc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> @@ -20,19 +20,6 @@ static inline struct bnxt *bnxt_get_bp_from_dl(struct devlink *dl)
>         return ((struct bnxt_dl *)devlink_priv(dl))->bp;
>  }
>
> -/* To clear devlink pointer from bp, pass NULL dl */
> -static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
> -{
> -       bp->dl = dl;
> -
> -       /* add a back pointer in dl to bp */
> -       if (dl) {
> -               struct bnxt_dl *bp_dl = devlink_priv(dl);
> -
> -               bp_dl->bp = bp;
> -       }
> -}
> -
>  #define NVM_OFF_MSIX_VEC_PER_PF_MAX    108
>  #define NVM_OFF_MSIX_VEC_PER_PF_MIN    114
>  #define NVM_OFF_IGNORE_ARI             164
> --
> 2.31.1
>

Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

Regards,
Edwin Peer

Re: [PATCH net-next 2/6] bnxt_en: Properly remove port parameter support

[Edwin Peer]

On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> This driver doesn't have any port parameters and registers
> devlink port parameters with empty table. Remove the useless
> calls to devlink_port_params_register and _unregister.
>
> Fixes: da203dfa89ce ("Revert "devlink: Add a generic wake_on_lan port parameter"")
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 -------------
>  1 file changed, 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index dc0851f709f5..ed95e28d60ef 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -736,9 +736,6 @@ static const struct devlink_param bnxt_dl_params[] = {
>                              NULL),
>  };
>
> -static const struct devlink_param bnxt_dl_port_params[] = {
> -};
> -
>  static int bnxt_dl_params_register(struct bnxt *bp)
>  {
>         int rc;
> @@ -753,14 +750,6 @@ static int bnxt_dl_params_register(struct bnxt *bp)
>                             rc);
>                 return rc;
>         }
> -       rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
> -                                         ARRAY_SIZE(bnxt_dl_port_params));
> -       if (rc) {
> -               netdev_err(bp->dev, "devlink_port_params_register failed\n");
> -               devlink_params_unregister(bp->dl, bnxt_dl_params,
> -                                         ARRAY_SIZE(bnxt_dl_params));
> -               return rc;
> -       }
>         devlink_params_publish(bp->dl);
>
>         return 0;
> @@ -773,8 +762,6 @@ static void bnxt_dl_params_unregister(struct bnxt *bp)
>
>         devlink_params_unregister(bp->dl, bnxt_dl_params,
>                                   ARRAY_SIZE(bnxt_dl_params));
> -       devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
> -                                      ARRAY_SIZE(bnxt_dl_port_params));
>  }
>
>  int bnxt_dl_register(struct bnxt *bp)
> --
> 2.31.1
>

Ah, looks like the revert in da203dfa89ce wasn't complete. Thanks for
the cleanup.

Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

Regards,
Edwin Peer

[Intel-wired-lan] [PATCH net-next 2/6] bnxt_en: Properly remove port parameter support

[Edwin Peer]

On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> From: Leon Romanovsky <leonro@nvidia.com>
>
> This driver doesn't have any port parameters and registers
> devlink port parameters with empty table. Remove the useless
> calls to devlink_port_params_register and _unregister.
>
> Fixes: da203dfa89ce ("Revert "devlink: Add a generic wake_on_lan port parameter"")
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 -------------
>  1 file changed, 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index dc0851f709f5..ed95e28d60ef 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -736,9 +736,6 @@ static const struct devlink_param bnxt_dl_params[] = {
>                              NULL),
>  };
>
> -static const struct devlink_param bnxt_dl_port_params[] = {
> -};
> -
>  static int bnxt_dl_params_register(struct bnxt *bp)
>  {
>         int rc;
> @@ -753,14 +750,6 @@ static int bnxt_dl_params_register(struct bnxt *bp)
>                             rc);
>                 return rc;
>         }
> -       rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
> -                                         ARRAY_SIZE(bnxt_dl_port_params));
> -       if (rc) {
> -               netdev_err(bp->dev, "devlink_port_params_register failed\n");
> -               devlink_params_unregister(bp->dl, bnxt_dl_params,
> -                                         ARRAY_SIZE(bnxt_dl_params));
> -               return rc;
> -       }
>         devlink_params_publish(bp->dl);
>
>         return 0;
> @@ -773,8 +762,6 @@ static void bnxt_dl_params_unregister(struct bnxt *bp)
>
>         devlink_params_unregister(bp->dl, bnxt_dl_params,
>                                   ARRAY_SIZE(bnxt_dl_params));
> -       devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
> -                                      ARRAY_SIZE(bnxt_dl_port_params));
>  }
>
>  int bnxt_dl_register(struct bnxt *bp)
> --
> 2.31.1
>

Ah, looks like the revert in da203dfa89ce wasn't complete. Thanks for
the cleanup.

Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

Regards,
Edwin Peer

Re: [PATCH net-next 0/6] Batch of devlink related fixes

[Jakub Kicinski]

On Thu, 23 Sep 2021 21:12:47 +0300 Leon Romanovsky wrote:
> I'm asking to apply this batch of devlink fixes to net-next and not to
> net, because most if not all fixes are for old code or/and can be considered
> as cleanup.
> 
> It will cancel the need to deal with merge conflicts for my next devlink series :).

Not sure how Dave will feel about adding fixes to net-next,
we do merge the trees weekly after all.

Otherwise the patches look fine.

[Intel-wired-lan] [PATCH net-next 0/6] Batch of devlink related fixes

[Jakub Kicinski]

On Thu, 23 Sep 2021 21:12:47 +0300 Leon Romanovsky wrote:
> I'm asking to apply this batch of devlink fixes to net-next and not to
> net, because most if not all fixes are for old code or/and can be considered
> as cleanup.
> 
> It will cancel the need to deal with merge conflicts for my next devlink series :).

Not sure how Dave will feel about adding fixes to net-next,
we do merge the trees weekly after all.

Otherwise the patches look fine.

Re: [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Leon Romanovsky]

On Thu, Sep 23, 2021 at 02:11:40PM -0700, Edwin Peer wrote:
> On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > devlink is a software interface that doesn't depend on any hardware
> > capabilities. The failure in SW means memory issues, wrong parameters,
> > programmer error e.t.c.
> >
> > Like any other such interface in the kernel, the returned status of
> > devlink APIs should be checked and propagated further and not ignored.
> >
> > Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  5 ++++-
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
> >  3 files changed, 10 insertions(+), 21 deletions(-)

<...>

> > @@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
> >  {
> >         struct devlink *dl = bp->dl;
> >
> > -       if (!dl)
> > -               return;
> > -
> 
> minor nit: There's obviously nothing incorrect about doing this (and
> adding the additional error label in the cleanup code above), but bnxt
> has generally adopted a style of having cleanup functions being
> idempotent. It generally makes error handling simpler and less error
> prone.

I would argue that opposite is true. Such "impossible" checks hide unwind
flow errors, missing releases e.t.c.

<...>

> >
> 
> Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

Thanks for the review.


> 
> Regards,
> Edwin Peer

[Intel-wired-lan] [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Leon Romanovsky]

On Thu, Sep 23, 2021 at 02:11:40PM -0700, Edwin Peer wrote:
> On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > devlink is a software interface that doesn't depend on any hardware
> > capabilities. The failure in SW means memory issues, wrong parameters,
> > programmer error e.t.c.
> >
> > Like any other such interface in the kernel, the returned status of
> > devlink APIs should be checked and propagated further and not ignored.
> >
> > Fixes: 4ab0c6a8ffd7 ("bnxt_en: add support to enable VF-representors")
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  5 ++++-
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 ++++++-------
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 13 -------------
> >  3 files changed, 10 insertions(+), 21 deletions(-)

<...>

> > @@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
> >  {
> >         struct devlink *dl = bp->dl;
> >
> > -       if (!dl)
> > -               return;
> > -
> 
> minor nit: There's obviously nothing incorrect about doing this (and
> adding the additional error label in the cleanup code above), but bnxt
> has generally adopted a style of having cleanup functions being
> idempotent. It generally makes error handling simpler and less error
> prone.

I would argue that opposite is true. Such "impossible" checks hide unwind
flow errors, missing releases e.t.c.

<...>

> >
> 
> Reviewed-by: Edwin Peer <edwin.peer@broadcom.com>

Thanks for the review.


> 
> Regards,
> Edwin Peer

Re: [PATCH net-next 0/6] Batch of devlink related fixes

[Leon Romanovsky]

On Thu, Sep 23, 2021 at 03:55:47PM -0700, Jakub Kicinski wrote:
> On Thu, 23 Sep 2021 21:12:47 +0300 Leon Romanovsky wrote:
> > I'm asking to apply this batch of devlink fixes to net-next and not to
> > net, because most if not all fixes are for old code or/and can be considered
> > as cleanup.
> > 
> > It will cancel the need to deal with merge conflicts for my next devlink series :).
> 
> Not sure how Dave will feel about adding fixes to net-next,
> we do merge the trees weekly after all.

My almost ready submission queue is:
➜  kernel git:(m/devlink) git l
693c1a9ac5b3 (HEAD -> m/devlink) devlink: Delete reload enable/disable interface
6d39354f8b44 net/mlx5: Register separate reload devlink ops for multiport device
1ac4e8811fd5 devlink: Allow set specific ops callbacks dynamically
de1849d3b348 devlink: Allow modification of devlink ops
7439a45dce72 net: dsa: Move devlink registration to be last devlink command
7dd23a327395 staging: qlge: Move devlink registration to be last devlink command
77f074c98b0d ptp: ocp: Move devlink registration to be last devlink command
fb3f4d40ad49 net: wwan: iosm: Move devlink_register to be last devlink command
87e95ee9275b netdevsim: Move devlink registration to be last devlink command
4173205af399 net: ethernet: ti: Move devlink registration to be last devlink command
bc633a0759f6 qed: Move devlink registration to be last devlink command
ead4e2027164 ionic: Move devlink registration to be last devlink command
bc5272ccc378 nfp: Move delink_register to be last command
a6521bf133d9 net: mscc: ocelot: delay devlink registration to the end
e0ca9a29cc20 mlxsw: core: Register devlink instance last
681ac1457516 net/mlx5: Accept devlink user input after driver initialization complete
9b1a2f4abaef net/mlx4: Move devlink_register to be the last initialization command
a3b2d9a95a51 net/prestera: Split devlink and traps registrations to separate routines
bbdf4842432f octeontx2: Move devlink registration to be last devlink command
5297e23f19e9 ice: Open devlink when device is ready
18af77a99cea net: hinic: Open device for the user access when it is ready
91a03cdc92e2 dpaa2-eth: Register devlink instance at the end of probe
dd5af984e53c liquidio: Overcome missing device lock protection in init/remove flows
efea109ba32e bnxt_en: Register devlink instance at the end devlink configuration
6a2b139bcf01 devlink: Notify users when objects are accessible

+ a couple of patches that removes "published" field from devlink parameters
and fix of old devlink bug where parameters were netlink notifications
were sent twice.

So it will be very helpful to keep this series in net-next.

> 
> Otherwise the patches look fine.

Thanks

[Intel-wired-lan] [PATCH net-next 0/6] Batch of devlink related fixes

[Leon Romanovsky]

On Thu, Sep 23, 2021 at 03:55:47PM -0700, Jakub Kicinski wrote:
> On Thu, 23 Sep 2021 21:12:47 +0300 Leon Romanovsky wrote:
> > I'm asking to apply this batch of devlink fixes to net-next and not to
> > net, because most if not all fixes are for old code or/and can be considered
> > as cleanup.
> > 
> > It will cancel the need to deal with merge conflicts for my next devlink series :).
> 
> Not sure how Dave will feel about adding fixes to net-next,
> we do merge the trees weekly after all.

My almost ready submission queue is:
?  kernel git:(m/devlink) git l
693c1a9ac5b3 (HEAD -> m/devlink) devlink: Delete reload enable/disable interface
6d39354f8b44 net/mlx5: Register separate reload devlink ops for multiport device
1ac4e8811fd5 devlink: Allow set specific ops callbacks dynamically
de1849d3b348 devlink: Allow modification of devlink ops
7439a45dce72 net: dsa: Move devlink registration to be last devlink command
7dd23a327395 staging: qlge: Move devlink registration to be last devlink command
77f074c98b0d ptp: ocp: Move devlink registration to be last devlink command
fb3f4d40ad49 net: wwan: iosm: Move devlink_register to be last devlink command
87e95ee9275b netdevsim: Move devlink registration to be last devlink command
4173205af399 net: ethernet: ti: Move devlink registration to be last devlink command
bc633a0759f6 qed: Move devlink registration to be last devlink command
ead4e2027164 ionic: Move devlink registration to be last devlink command
bc5272ccc378 nfp: Move delink_register to be last command
a6521bf133d9 net: mscc: ocelot: delay devlink registration to the end
e0ca9a29cc20 mlxsw: core: Register devlink instance last
681ac1457516 net/mlx5: Accept devlink user input after driver initialization complete
9b1a2f4abaef net/mlx4: Move devlink_register to be the last initialization command
a3b2d9a95a51 net/prestera: Split devlink and traps registrations to separate routines
bbdf4842432f octeontx2: Move devlink registration to be last devlink command
5297e23f19e9 ice: Open devlink when device is ready
18af77a99cea net: hinic: Open device for the user access when it is ready
91a03cdc92e2 dpaa2-eth: Register devlink instance at the end of probe
dd5af984e53c liquidio: Overcome missing device lock protection in init/remove flows
efea109ba32e bnxt_en: Register devlink instance at the end devlink configuration
6a2b139bcf01 devlink: Notify users when objects are accessible

+ a couple of patches that removes "published" field from devlink parameters
and fix of old devlink bug where parameters were netlink notifications
were sent twice.

So it will be very helpful to keep this series in net-next.

> 
> Otherwise the patches look fine.

Thanks

Re: [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Jakub Kicinski]

On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:
> > > @@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
> > >  {
> > >         struct devlink *dl = bp->dl;
> > >
> > > -       if (!dl)
> > > -               return;
> > > -  
> > 
> > minor nit: There's obviously nothing incorrect about doing this (and
> > adding the additional error label in the cleanup code above), but bnxt
> > has generally adopted a style of having cleanup functions being
> > idempotent. It generally makes error handling simpler and less error
> > prone.  
> 
> I would argue that opposite is true. Such "impossible" checks hide unwind
> flow errors, missing releases e.t.c.

+1, fwiw

[Intel-wired-lan] [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Jakub Kicinski]

On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:
> > > @@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
> > >  {
> > >         struct devlink *dl = bp->dl;
> > >
> > > -       if (!dl)
> > > -               return;
> > > -  
> > 
> > minor nit: There's obviously nothing incorrect about doing this (and
> > adding the additional error label in the cleanup code above), but bnxt
> > has generally adopted a style of having cleanup functions being
> > idempotent. It generally makes error handling simpler and less error
> > prone.  
> 
> I would argue that opposite is true. Such "impossible" checks hide unwind
> flow errors, missing releases e.t.c.

+1, fwiw

Re: [PATCH net-next 0/6] Batch of devlink related fixes

[David Miller]

From: Leon Romanovsky <leon@kernel.org>
Date: Thu, 23 Sep 2021 21:12:47 +0300

> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Hi,
> 
> I'm asking to apply this batch of devlink fixes to net-next and not to
> net, because most if not all fixes are for old code or/and can be considered
> as cleanup.
> 
> It will cancel the need to deal with merge conflicts for my next devlink series :).

ok, but just this one time.

I much rather this kind of stuff goes to net and we deal with the merge
conflicts that arise.

Thsnks!

[Intel-wired-lan] [PATCH net-next 0/6] Batch of devlink related fixes

[David Miller]

From: Leon Romanovsky <leon@kernel.org>
Date: Thu, 23 Sep 2021 21:12:47 +0300

> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Hi,
> 
> I'm asking to apply this batch of devlink fixes to net-next and not to
> net, because most if not all fixes are for old code or/and can be considered
> as cleanup.
> 
> It will cancel the need to deal with merge conflicts for my next devlink series :).

ok, but just this one time.

I much rather this kind of stuff goes to net and we deal with the merge
conflicts that arise.

Thsnks!

Re: [PATCH net-next 0/6] Batch of devlink related fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Thu, 23 Sep 2021 21:12:47 +0300 you wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Hi,
> 
> I'm asking to apply this batch of devlink fixes to net-next and not to
> net, because most if not all fixes are for old code or/and can be considered
> as cleanup.
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] bnxt_en: Check devlink allocation and registration status
    https://git.kernel.org/netdev/net-next/c/e624c70e1131
  - [net-next,2/6] bnxt_en: Properly remove port parameter support
    https://git.kernel.org/netdev/net-next/c/61415c3db3d9
  - [net-next,3/6] devlink: Delete not used port parameters APIs
    https://git.kernel.org/netdev/net-next/c/42ded61aa75e
  - [net-next,4/6] devlink: Remove single line function obfuscations
    https://git.kernel.org/netdev/net-next/c/8ba024dfaf61
  - [net-next,5/6] ice: Delete always true check of PF pointer
    https://git.kernel.org/netdev/net-next/c/2ff04286a956
  - [net-next,6/6] qed: Don't ignore devlink allocation failures
    https://git.kernel.org/netdev/net-next/c/e6a54d6f2213

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[Intel-wired-lan] [PATCH net-next 0/6] Batch of devlink related fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Thu, 23 Sep 2021 21:12:47 +0300 you wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Hi,
> 
> I'm asking to apply this batch of devlink fixes to net-next and not to
> net, because most if not all fixes are for old code or/and can be considered
> as cleanup.
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] bnxt_en: Check devlink allocation and registration status
    https://git.kernel.org/netdev/net-next/c/e624c70e1131
  - [net-next,2/6] bnxt_en: Properly remove port parameter support
    https://git.kernel.org/netdev/net-next/c/61415c3db3d9
  - [net-next,3/6] devlink: Delete not used port parameters APIs
    https://git.kernel.org/netdev/net-next/c/42ded61aa75e
  - [net-next,4/6] devlink: Remove single line function obfuscations
    https://git.kernel.org/netdev/net-next/c/8ba024dfaf61
  - [net-next,5/6] ice: Delete always true check of PF pointer
    https://git.kernel.org/netdev/net-next/c/2ff04286a956
  - [net-next,6/6] qed: Don't ignore devlink allocation failures
    https://git.kernel.org/netdev/net-next/c/e6a54d6f2213

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Edwin Peer]

On Thu, Sep 23, 2021 at 6:39 PM Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:
> > > minor nit: There's obviously nothing incorrect about doing this (and
> > > adding the additional error label in the cleanup code above), but bnxt
> > > has generally adopted a style of having cleanup functions being
> > > idempotent. It generally makes error handling simpler and less error
> > > prone.
> >
> > I would argue that opposite is true. Such "impossible" checks hide unwind
> > flow errors, missing releases e.t.c.
>
> +1, fwiw

I appreciate that being more explicit can improve visibility, but it
does not make error handling inherently less error prone, nor is it
simpler (ie. the opposite isn't true). Idempotency is orthogonal to
unwind flow or the presence or not of a particular unwind handler (one
can still enforce either in review). But, if release handlers are
independent (most in bnxt are), then permitting other orderings can be
perfectly valid and places less burden on achieving the canonical form
for correctness (ie. usage is simpler and less error prone). That's
not to say we should throw caution to the wind and allow arbitrary
unwind flows, but it does mean certain mistakes don't result in actual
bugs. There are other flexibility benefits too. A single, unwind
everything, handler can be reused in more than one context.

That said, isn't the more important question what style and
assumptions the surrounding code has adopted? In this particular case,
I checked that this change wouldn't introduce the possibility of a
double unwind, but in other contexts in this driver code base,
changing error handling in this piecemeal way might actually introduce
a bug in contexts where the caller has assumed the overall function is
idempotent. Isn't local consistency of style a more important concern,
especially given that you are not predominantly responsible for
maintenance of this driver? Dealing with this exception to the norm in
our driver certainly places an additional burden on us to remember to
treat this particular case with special care. We should either rework
all of bnxt error handling to adopt the more accepted canonical form,
or we should adopt the surrounding conventions. What we shouldn't do
is mix approaches in one driver.

Regards,
Edwin Peer

[Intel-wired-lan] [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Edwin Peer]

On Thu, Sep 23, 2021 at 6:39 PM Jakub Kicinski <kuba@kernel.org> wrote:

> On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:
> > > minor nit: There's obviously nothing incorrect about doing this (and
> > > adding the additional error label in the cleanup code above), but bnxt
> > > has generally adopted a style of having cleanup functions being
> > > idempotent. It generally makes error handling simpler and less error
> > > prone.
> >
> > I would argue that opposite is true. Such "impossible" checks hide unwind
> > flow errors, missing releases e.t.c.
>
> +1, fwiw

I appreciate that being more explicit can improve visibility, but it
does not make error handling inherently less error prone, nor is it
simpler (ie. the opposite isn't true). Idempotency is orthogonal to
unwind flow or the presence or not of a particular unwind handler (one
can still enforce either in review). But, if release handlers are
independent (most in bnxt are), then permitting other orderings can be
perfectly valid and places less burden on achieving the canonical form
for correctness (ie. usage is simpler and less error prone). That's
not to say we should throw caution to the wind and allow arbitrary
unwind flows, but it does mean certain mistakes don't result in actual
bugs. There are other flexibility benefits too. A single, unwind
everything, handler can be reused in more than one context.

That said, isn't the more important question what style and
assumptions the surrounding code has adopted? In this particular case,
I checked that this change wouldn't introduce the possibility of a
double unwind, but in other contexts in this driver code base,
changing error handling in this piecemeal way might actually introduce
a bug in contexts where the caller has assumed the overall function is
idempotent. Isn't local consistency of style a more important concern,
especially given that you are not predominantly responsible for
maintenance of this driver? Dealing with this exception to the norm in
our driver certainly places an additional burden on us to remember to
treat this particular case with special care. We should either rework
all of bnxt error handling to adopt the more accepted canonical form,
or we should adopt the surrounding conventions. What we shouldn't do
is mix approaches in one driver.

Regards,
Edwin Peer

Re: [PATCH net-next 0/6] Batch of devlink related fixes

[Leon Romanovsky]

On Fri, Sep 24, 2021 at 02:14:26PM +0100, David Miller wrote:
> From: Leon Romanovsky <leon@kernel.org>
> Date: Thu, 23 Sep 2021 21:12:47 +0300
> 
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Hi,
> > 
> > I'm asking to apply this batch of devlink fixes to net-next and not to
> > net, because most if not all fixes are for old code or/and can be considered
> > as cleanup.
> > 
> > It will cancel the need to deal with merge conflicts for my next devlink series :).
> 
> ok, but just this one time.

Thanks

> 
> I much rather this kind of stuff goes to net and we deal with the merge
> conflicts that arise.

I'll do.

> 
> Thsnks!

[Intel-wired-lan] [PATCH net-next 0/6] Batch of devlink related fixes

[Leon Romanovsky]

On Fri, Sep 24, 2021 at 02:14:26PM +0100, David Miller wrote:
> From: Leon Romanovsky <leon@kernel.org>
> Date: Thu, 23 Sep 2021 21:12:47 +0300
> 
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > Hi,
> > 
> > I'm asking to apply this batch of devlink fixes to net-next and not to
> > net, because most if not all fixes are for old code or/and can be considered
> > as cleanup.
> > 
> > It will cancel the need to deal with merge conflicts for my next devlink series :).
> 
> ok, but just this one time.

Thanks

> 
> I much rather this kind of stuff goes to net and we deal with the merge
> conflicts that arise.

I'll do.

> 
> Thsnks!

Re: [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Leon Romanovsky]

On Fri, Sep 24, 2021 at 10:20:32AM -0700, Edwin Peer wrote:
> On Thu, Sep 23, 2021 at 6:39 PM Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:
> > > > minor nit: There's obviously nothing incorrect about doing this (and
> > > > adding the additional error label in the cleanup code above), but bnxt
> > > > has generally adopted a style of having cleanup functions being
> > > > idempotent. It generally makes error handling simpler and less error
> > > > prone.
> > >
> > > I would argue that opposite is true. Such "impossible" checks hide unwind
> > > flow errors, missing releases e.t.c.
> >
> > +1, fwiw
> 
> I appreciate that being more explicit can improve visibility, but it
> does not make error handling inherently less error prone, nor is it
> simpler (ie. the opposite isn't true). Idempotency is orthogonal to
> unwind flow or the presence or not of a particular unwind handler (one
> can still enforce either in review). But, if release handlers are
> independent (most in bnxt are), then permitting other orderings can be
> perfectly valid and places less burden on achieving the canonical form
> for correctness (ie. usage is simpler and less error prone). That's
> not to say we should throw caution to the wind and allow arbitrary
> unwind flows, but it does mean certain mistakes don't result in actual
> bugs. There are other flexibility benefits too. A single, unwind
> everything, handler can be reused in more than one context.

And this is where the fun begins. Different context means different
lifetime expectations, maybe need of locking and unpredictable flows
from reader perspective.

For example, in this devlink case, it took me time to check all driver
to see that pf can't be null. 

The idea that adding code that maybe will be used can be seen as
anti-pattern.

Thanks

[Intel-wired-lan] [PATCH net-next 1/6] bnxt_en: Check devlink allocation and registration status

[Leon Romanovsky]

On Fri, Sep 24, 2021 at 10:20:32AM -0700, Edwin Peer wrote:
> On Thu, Sep 23, 2021 at 6:39 PM Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:
> > > > minor nit: There's obviously nothing incorrect about doing this (and
> > > > adding the additional error label in the cleanup code above), but bnxt
> > > > has generally adopted a style of having cleanup functions being
> > > > idempotent. It generally makes error handling simpler and less error
> > > > prone.
> > >
> > > I would argue that opposite is true. Such "impossible" checks hide unwind
> > > flow errors, missing releases e.t.c.
> >
> > +1, fwiw
> 
> I appreciate that being more explicit can improve visibility, but it
> does not make error handling inherently less error prone, nor is it
> simpler (ie. the opposite isn't true). Idempotency is orthogonal to
> unwind flow or the presence or not of a particular unwind handler (one
> can still enforce either in review). But, if release handlers are
> independent (most in bnxt are), then permitting other orderings can be
> perfectly valid and places less burden on achieving the canonical form
> for correctness (ie. usage is simpler and less error prone). That's
> not to say we should throw caution to the wind and allow arbitrary
> unwind flows, but it does mean certain mistakes don't result in actual
> bugs. There are other flexibility benefits too. A single, unwind
> everything, handler can be reused in more than one context.

And this is where the fun begins. Different context means different
lifetime expectations, maybe need of locking and unpredictable flows
from reader perspective.

For example, in this devlink case, it took me time to check all driver
to see that pf can't be null. 

The idea that adding code that maybe will be used can be seen as
anti-pattern.

Thanks