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@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(-) <...> > > @@ -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@xxxxxxxxxxxx> Thanks for the review. > > Regards, > Edwin Peer