| Message ID | 20220512143314.235604-11-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: Synchronous Tx support | expand |
Hi, On Thu, May 12, 2022 at 10:34 AM 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. > > Set an atomic when the queue will remain stopped. Reset this atomic when > the queue actually gets restarded. Just check this atomic to know if the > transmission is legitimate, warn if it is not. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > include/net/cfg802154.h | 1 + > net/mac802154/tx.c | 16 +++++++++++++++- > net/mac802154/util.c | 1 + > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > index 8b6326aa2d42..a1370e87233e 100644 > --- a/include/net/cfg802154.h > +++ b/include/net/cfg802154.h > @@ -218,6 +218,7 @@ struct wpan_phy { > struct mutex queue_lock; > atomic_t ongoing_txs; > atomic_t hold_txs; > + atomic_t queue_stopped; Maybe some test_bit()/set_bit() is better there? - Alex
aahringo@redhat.com wrote on Sun, 15 May 2022 18:30:15 -0400: > Hi, > > On Thu, May 12, 2022 at 10:34 AM 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. > > > > Set an atomic when the queue will remain stopped. Reset this atomic when > > the queue actually gets restarded. Just check this atomic to know if the > > transmission is legitimate, warn if it is not. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > include/net/cfg802154.h | 1 + > > net/mac802154/tx.c | 16 +++++++++++++++- > > net/mac802154/util.c | 1 + > > 3 files changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > > index 8b6326aa2d42..a1370e87233e 100644 > > --- a/include/net/cfg802154.h > > +++ b/include/net/cfg802154.h > > @@ -218,6 +218,7 @@ struct wpan_phy { > > struct mutex queue_lock; > > atomic_t ongoing_txs; > > atomic_t hold_txs; > > + atomic_t queue_stopped; > > Maybe some test_bit()/set_bit() is better there? What do you mean? Shall I change the atomic_t type of queue_stopped? Isn't the atomic_t preferred in this situation?
miquel.raynal@bootlin.com wrote on Tue, 17 May 2022 15:36:55 +0200: > aahringo@redhat.com wrote on Sun, 15 May 2022 18:30:15 -0400: > > > Hi, > > > > On Thu, May 12, 2022 at 10:34 AM 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. > > > > > > Set an atomic when the queue will remain stopped. Reset this atomic when > > > the queue actually gets restarded. Just check this atomic to know if the > > > transmission is legitimate, warn if it is not. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > include/net/cfg802154.h | 1 + > > > net/mac802154/tx.c | 16 +++++++++++++++- > > > net/mac802154/util.c | 1 + > > > 3 files changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > > > index 8b6326aa2d42..a1370e87233e 100644 > > > --- a/include/net/cfg802154.h > > > +++ b/include/net/cfg802154.h > > > @@ -218,6 +218,7 @@ struct wpan_phy { > > > struct mutex queue_lock; > > > atomic_t ongoing_txs; > > > atomic_t hold_txs; > > > + atomic_t queue_stopped; > > > > Maybe some test_bit()/set_bit() is better there? > > What do you mean? Shall I change the atomic_t type of queue_stopped? > Isn't the atomic_t preferred in this situation? Actually I re-read the doc and that's right, a regular unsigned long used with test/set_bit might be preferred, I'll make the change. Thanks, Miquèl
Hi, On Tue, May 17, 2022 at 10:53 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > miquel.raynal@bootlin.com wrote on Tue, 17 May 2022 15:36:55 +0200: > > > aahringo@redhat.com wrote on Sun, 15 May 2022 18:30:15 -0400: > > > > > Hi, > > > > > > On Thu, May 12, 2022 at 10:34 AM 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. > > > > > > > > Set an atomic when the queue will remain stopped. Reset this atomic when > > > > the queue actually gets restarded. Just check this atomic to know if the > > > > transmission is legitimate, warn if it is not. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > include/net/cfg802154.h | 1 + > > > > net/mac802154/tx.c | 16 +++++++++++++++- > > > > net/mac802154/util.c | 1 + > > > > 3 files changed, 17 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > > > > index 8b6326aa2d42..a1370e87233e 100644 > > > > --- a/include/net/cfg802154.h > > > > +++ b/include/net/cfg802154.h > > > > @@ -218,6 +218,7 @@ struct wpan_phy { > > > > struct mutex queue_lock; > > > > atomic_t ongoing_txs; > > > > atomic_t hold_txs; > > > > + atomic_t queue_stopped; > > > > > > Maybe some test_bit()/set_bit() is better there? > > > > What do you mean? Shall I change the atomic_t type of queue_stopped? > > Isn't the atomic_t preferred in this situation? > > Actually I re-read the doc and that's right, a regular unsigned long Which doc is that? - Alex
Hi Alex, aahringo@redhat.com wrote on Tue, 17 May 2022 20:59:39 -0400: > Hi, > > On Tue, May 17, 2022 at 10:53 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > > > miquel.raynal@bootlin.com wrote on Tue, 17 May 2022 15:36:55 +0200: > > > > > aahringo@redhat.com wrote on Sun, 15 May 2022 18:30:15 -0400: > > > > > > > Hi, > > > > > > > > On Thu, May 12, 2022 at 10:34 AM 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. > > > > > > > > > > Set an atomic when the queue will remain stopped. Reset this atomic when > > > > > the queue actually gets restarded. Just check this atomic to know if the > > > > > transmission is legitimate, warn if it is not. > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > --- > > > > > include/net/cfg802154.h | 1 + > > > > > net/mac802154/tx.c | 16 +++++++++++++++- > > > > > net/mac802154/util.c | 1 + > > > > > 3 files changed, 17 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h > > > > > index 8b6326aa2d42..a1370e87233e 100644 > > > > > --- a/include/net/cfg802154.h > > > > > +++ b/include/net/cfg802154.h > > > > > @@ -218,6 +218,7 @@ struct wpan_phy { > > > > > struct mutex queue_lock; > > > > > atomic_t ongoing_txs; > > > > > atomic_t hold_txs; > > > > > + atomic_t queue_stopped; > > > > > > > > Maybe some test_bit()/set_bit() is better there? > > > > > > What do you mean? Shall I change the atomic_t type of queue_stopped? > > > Isn't the atomic_t preferred in this situation? > > > > Actually I re-read the doc and that's right, a regular unsigned long > > Which doc is that? Documentation/atomic_t.txt states [SEMANTICS chapter]: "if you find yourself only using the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all and are doing it wrong." In this case, I was only using atomic_set() and atomic_read(), which are both non-RMW operations. Thanks, Miquèl
diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h index 8b6326aa2d42..a1370e87233e 100644 --- a/include/net/cfg802154.h +++ b/include/net/cfg802154.h @@ -218,6 +218,7 @@ struct wpan_phy { struct mutex queue_lock; atomic_t ongoing_txs; atomic_t hold_txs; + atomic_t queue_stopped; wait_queue_head_t sync_txq; char priv[] __aligned(NETDEV_ALIGN); diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c index ec8d872143ee..a3c9f194c025 100644 --- a/net/mac802154/tx.c +++ b/net/mac802154/tx.c @@ -123,9 +123,13 @@ static int ieee802154_sync_queue(struct ieee802154_local *local) int ieee802154_sync_and_hold_queue(struct ieee802154_local *local) { + int ret; + ieee802154_hold_queue(local); + ret = ieee802154_sync_queue(local); + atomic_set(&local->phy->queue_stopped, 1); - return ieee802154_sync_queue(local); + return ret; } int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) @@ -153,9 +157,19 @@ int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) return ret; } +static bool ieee802154_queue_is_stopped(struct ieee802154_local *local) +{ + return atomic_read(&local->phy->queue_stopped); +} + static netdev_tx_t ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb) { + /* Warn if the net interface tries to transmit frames while the + * ieee802154 core assumes the queue is stopped. + */ + 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 65a9127a41ea..54f05ae88172 100644 --- a/net/mac802154/util.c +++ b/net/mac802154/util.c @@ -29,6 +29,7 @@ static void ieee802154_wake_queue(struct ieee802154_hw *hw) struct ieee802154_sub_if_data *sdata; rcu_read_lock(); + atomic_set(&local->phy->queue_stopped, 0); list_for_each_entry_rcu(sdata, &local->interfaces, list) { if (!sdata->dev) continue;
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. Set an atomic when the queue will remain stopped. Reset this atomic when the queue actually gets restarded. Just check this atomic to know if the transmission is legitimate, warn if it is not. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- include/net/cfg802154.h | 1 + net/mac802154/tx.c | 16 +++++++++++++++- net/mac802154/util.c | 1 + 3 files changed, 17 insertions(+), 1 deletion(-)