Hi Alexander,
alex.aring@xxxxxxxxx wrote on Wed, 27 Apr 2022 14:01:25 -0400:
> Hi,
>
> On Wed, Apr 27, 2022 at 12:47 PM Miquel Raynal
> <miquel.raynal@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
[...]
> > 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
