Re: [PATCH wpan-next 08/11] net: mac802154: Add a warning in the hot path

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Alexander,

alex.aring@gmail.com wrote on Wed, 27 Apr 2022 14:01:25 -0400:

> Hi,
> 
> On Wed, Apr 27, 2022 at 12:47 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > We should never start a transmission after the queue has been stopped.
> >
> > But because it might work we don't kill the function here but rather
> > warn loudly the user that something is wrong.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---

[...]

> > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> > index a8a83f0167bf..021dddfea542 100644
> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -124,6 +124,8 @@ bool ieee802154_queue_is_held(struct ieee802154_local *local)
> >  static netdev_tx_t
> >  ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
> >  {
> > +       WARN_ON_ONCE(ieee802154_queue_is_stopped(local));
> > +
> >         return ieee802154_tx(local, skb);
> >  }
> >
> > diff --git a/net/mac802154/util.c b/net/mac802154/util.c
> > index 847e0864b575..cfd17a7db532 100644
> > --- a/net/mac802154/util.c
> > +++ b/net/mac802154/util.c
> > @@ -44,6 +44,24 @@ void ieee802154_stop_queue(struct ieee802154_local *local)
> >         rcu_read_unlock();
> >  }
> >
> > +bool ieee802154_queue_is_stopped(struct ieee802154_local *local)
> > +{
> > +       struct ieee802154_sub_if_data *sdata;
> > +       bool stopped = true;
> > +
> > +       rcu_read_lock();
> > +       list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > +               if (!sdata->dev)
> > +                       continue;
> > +
> > +               if (!netif_queue_stopped(sdata->dev))
> > +                       stopped = false;
> > +       }
> > +       rcu_read_unlock();
> > +
> > +       return stopped;
> > +}  
> 
> sorry this makes no sense, you using net core functionality to check
> if a queue is stopped in a net core netif callback. Whereas the sense
> here for checking if the queue is really stopped is when 802.15.4
> thinks the queue is stopped vs net core netif callback running. It
> means for MLME-ops there are points we want to make sure that net core
> is not handling any xmit and we should check this point and not
> introducing net core functionality checks.

I think I've mixed two things, your remark makes complete sense. I
should instead here just check a 802.15.4 internal variable.

> btw: if it's hit your if branch the first time you can break?

Yes, we could definitely improve a bit the logic to break earlier, but
in the end these checks won't remain I believe.

> I am not done with the review, this is just what I see now and we can
> discuss that. Please be patient.

Sure, thanks for the quick feedback anyway!

hanks,
Miquèl