On Thu, Sep 23, 2021 at 11:13 AM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
>
> From: Leon Romanovsky <leonro@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
Regards,
Edwin Peer
