Re: [PATCH fwctl 2/5] fwctl/bnxt_en: Refactor aux bus functions to be more generic

[Pavan Chebbi]

> > +     int                     auxdev_id;
> > +     atomic_t                auxdev_state[__BNXT_AUXDEV_MAX];
> > +#define      BNXT_ADEV_STATE_NONE    0
> > +#define      BNXT_ADEV_STATE_INIT    1
> > +#define      BNXT_ADEV_STATE_ADD     2
>
> This is indeed an incorrect use of atomic_t. This type only ensures that
> individual arithmetic operations are performed atomically. It does not
> eliminate the need for the locks that were removed.

Thanks for your review. Since the individual bp's auxdev state is
going to be set only during probe(), and is read-only elsewhere,
atomic operations are enough, no?

>
> >  };
> >
> >  #define BNXT_NUM_RX_RING_STATS                       8
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > index 8cad7b982664..064d7bc4ce8d 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> > @@ -5100,7 +5100,7 @@ static void bnxt_self_test(struct net_device *dev, struct ethtool_test *etest,
>
> <...>
>
> >
> >  void bnxt_ulp_start(struct bnxt *bp, int err)
> >  {
> > -     struct bnxt_aux_priv *aux_priv = bp->aux_priv;
> > -     struct bnxt_en_dev *edev = bp->edev;
> > +     int i;
> >
> > -     if (!edev || err)
>
> err is no longer used, and keeping it would alter the behavior of
> bnxt_resume() and related paths. In any case, callers should not invoke
> bnxt_ulp_start() when an error has already occurred.

Yes, this I can remove.

>
> Thanks
>
> > -             return;
> > +     for (i = 0; i < __BNXT_AUXDEV_MAX; i++) {
> > +             struct bnxt_aux_priv *aux_priv;
> > +             struct auxiliary_device *adev;
> > +             struct bnxt_en_dev *edev;

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature