| Message ID | 20220517163450.240299-12-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: Synchronous Tx support | expand |
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. > + } > + rcu_read_unlock(); > + > + return is_down; > +} > + > static int ieee802154_mlme_op_pre(struct ieee802154_local *local) > { > return ieee802154_sync_and_hold_queue(local); > @@ -150,6 +169,12 @@ static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *sk > if (!local->open_count) > return -EBUSY; > > + /* 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? Also there is a missing rtnl_unlock(). - Alex
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
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; + } + rcu_read_unlock(); + + return is_down; +} + static int ieee802154_mlme_op_pre(struct ieee802154_local *local) { return ieee802154_sync_and_hold_queue(local); @@ -150,6 +169,12 @@ static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *sk if (!local->open_count) return -EBUSY; + /* 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; + ieee802154_tx(local, skb); ret = ieee802154_sync_queue(local);
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(+)