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

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

Hi Alex,

aahringo@redhat.com wrote on Tue, 17 May 2022 20:52:35 -0400:

> Hi,
> 
> On Tue, May 17, 2022 at 12:35 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > In order to be able to detect possible conflicts between the net
> > interface core and the ieee802154 core, let's add a warning in the slow
> > path: we want to be sure that whenever we start an asynchronous MLME
> > transmission (which can be fully asynchronous) the net core somehow
> > agrees that this transmission is possible, ie. the device was not
> > stopped. Warning in this case would allow us to track down more easily
> > possible issues with the MLME logic if we ever get reports.
> >
> > Unlike in the hot path, such a situation cannot be handled.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  net/mac802154/tx.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> > index e36aca788ea2..53a8be822e33 100644
> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -132,6 +132,25 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
> >         return ret;
> >  }
> >
> > +static bool ieee802154_netif_is_down(struct ieee802154_local *local)
> > +{
> > +       struct ieee802154_sub_if_data *sdata;
> > +       bool is_down = true;
> > +
> > +       rcu_read_lock();
> > +       list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> > +               if (!sdata->dev)
> > +                       continue;
> > +
> > +               is_down = !(sdata->dev->flags & IFF_UP);
> > +               if (is_down)
> > +                       break;  
> 
> I thought that the helper would be "netif_running()". It seems there
> are multiple ways to check if an interface is up.

Looking at net/core/dev.c shows that netif_running() may indicate a
transitory state (the interface was requested to be up or down) while
the IFF_UP flag really tells when the operation is done and the
interface actually is up or down.

Anyway they are both updated atomically wrt the rtnl lock. The sooner
the better, so let's check the interface state with netif_running().


[...]

> >
> > +       /* Warn if the ieee802154 core thinks MLME frames can be sent while the
> > +        * net interface expects this cannot happen.
> > +        */
> > +       if (WARN_ON_ONCE(ieee802154_netif_is_down(local)))
> > +               return -EHOSTDOWN;  
> 
> maybe also ENETDOWN?

Sure

> Also there is a missing rtnl_unlock().

Duly noted.

Thanks,
Miquèl