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

On Thu, Sep 23, 2021 at 02:11:40PM -0700, Edwin Peer wrote:

On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky [off-list ref] wrote:

quoted

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

<...>

quoted

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

<...>

quoted

Reviewed-by: Edwin Peer <redacted>

Thanks for the review.

Regards,
Edwin Peer