Batch of devlink related fixes

From: Leon Romanovsky <[email protected]>

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 <[email protected]>

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

drivers/net/ethernet/broadcom/bnxt/bnxt.c +4 -1

index 037767b370d5..4c483fd91dbe 100644
@@ -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);
 

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +6 -4

index bf7d3c17049b..dc0851f709f5 100644
@@ -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;
 }

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +0 -0

        if (BNXT_PF(bp)) {
                bnxt_dl_params_unregister(bp);
                devlink_port_unregister(&bp->dl_port);

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h +0 -14

index d889f240da2b..406dc655a5fc 100644
@@ -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

Quoted 17 lines · Leon Romanovsky

From: Leon Romanovsky <[email protected]>

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 <[email protected]>

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

Quoted 4 lines

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +0 -0

@@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
 {
        struct devlink *dl = bp->dl;
 

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c void bnxt_dl_unregister(struct bnxt *bp) · line 840

Quoted 3 lines

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +0 -3

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

Quoted 1 line

Reviewed-by Edwin Peer <[email protected]>

Regards,
Edwin Peer

Quoted 4 lines

Quoted 4 lines

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +0 -0

@@ -835,9 +837,6 @@ void bnxt_dl_unregister(struct bnxt *bp)
 {
        struct devlink *dl = bp->dl;
 

Quoted 9 lines

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c void bnxt_dl_unregister(struct bnxt *bp) · line 840

Quoted 3 lines

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +0 -3

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

Quoted 8 lines · On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:

Quoted 5 lines

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

Quoted 11 lines · Jakub Kicinski

Quoted 8 lines · On Fri, 24 Sep 2021 02:11:19 +0300 Leon Romanovsky wrote:

Quoted 5 lines

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.

From: Leon Romanovsky <[email protected]>

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 <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 13 -------------
 1 file changed, 13 deletions(-)

drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +0 -14

index dc0851f709f5..ed95e28d60ef 100644
@@ -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

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

Quoted 5 lines · 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.

From: Leon Romanovsky <[email protected]>

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

From: Leon Romanovsky <[email protected]>
Date: Thu, 23 Sep 2021 21:12:47 +0300

Quoted 4 lines

From: Leon Romanovsky <[email protected]>

Hi,

Quoted 5 lines

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!