| Message ID | 20220517163450.240299-11-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: > > 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 8881b6126b58..f4e7b3fe7cf0 100644 > --- a/include/net/cfg802154.h > +++ b/include/net/cfg802154.h > @@ -218,6 +218,7 @@ struct wpan_phy { > spinlock_t queue_lock; > atomic_t ongoing_txs; > atomic_t hold_txs; > + unsigned long queue_stopped; Can we name it something like state_flags (as phy state flags)? Pretty sure there will be more coming, or internal_flags, no idea... something_flags... > wait_queue_head_t sync_txq; > > char priv[] __aligned(NETDEV_ALIGN); > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > index 6cc4e5c7ba94..e36aca788ea2 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); > + set_bit(0, &local->phy->queue_stopped); > Define the 0 as WPAN_PHY_STATE_QUEUE_STOPPED_BIT or something like that, above wpan_phy. - Alex
aahringo@redhat.com wrote on Tue, 17 May 2022 20:58:19 -0400: > Hi, > > On Tue, May 17, 2022 at 12:35 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. > > > > 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 8881b6126b58..f4e7b3fe7cf0 100644 > > --- a/include/net/cfg802154.h > > +++ b/include/net/cfg802154.h > > @@ -218,6 +218,7 @@ struct wpan_phy { > > spinlock_t queue_lock; > > atomic_t ongoing_txs; > > atomic_t hold_txs; > > + unsigned long queue_stopped; > > Can we name it something like state_flags (as phy state flags)? Pretty > sure there will be more coming, or internal_flags, no idea... > something_flags... 'phy_flags'? Just 'flags', maybe? state_flags seems a bit too specific, but if it's your favorite I don't mind using it. > > > wait_queue_head_t sync_txq; > > > > char priv[] __aligned(NETDEV_ALIGN); > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > > index 6cc4e5c7ba94..e36aca788ea2 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); > > + set_bit(0, &local->phy->queue_stopped); > > > > Define the 0 as WPAN_PHY_STATE_QUEUE_STOPPED_BIT or something like > that, above wpan_phy. Sure. Thanks, Miquèl
Hi, On Wed, May 18, 2022 at 4:55 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:58:19 -0400: > > > Hi, > > > > On Tue, May 17, 2022 at 12:35 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. > > > > > > 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 8881b6126b58..f4e7b3fe7cf0 100644 > > > --- a/include/net/cfg802154.h > > > +++ b/include/net/cfg802154.h > > > @@ -218,6 +218,7 @@ struct wpan_phy { > > > spinlock_t queue_lock; > > > atomic_t ongoing_txs; > > > atomic_t hold_txs; > > > + unsigned long queue_stopped; > > > > Can we name it something like state_flags (as phy state flags)? Pretty > > sure there will be more coming, or internal_flags, no idea... > > something_flags... > > 'phy_flags'? Just 'flags', maybe? make it so. - Alex
aahringo@redhat.com wrote on Wed, 18 May 2022 10:31:55 -0400: > Hi, > > On Wed, May 18, 2022 at 4:55 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:58:19 -0400: > > > > > Hi, > > > > > > On Tue, May 17, 2022 at 12:35 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. > > > > > > > > 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 8881b6126b58..f4e7b3fe7cf0 100644 > > > > --- a/include/net/cfg802154.h > > > > +++ b/include/net/cfg802154.h > > > > @@ -218,6 +218,7 @@ struct wpan_phy { > > > > spinlock_t queue_lock; > > > > atomic_t ongoing_txs; > > > > atomic_t hold_txs; > > > > + unsigned long queue_stopped; > > > > > > Can we name it something like state_flags (as phy state flags)? Pretty > > > sure there will be more coming, or internal_flags, no idea... > > > something_flags... > > > > 'phy_flags'? Just 'flags', maybe? > > make it so. Oh, there is already a flags entry in wpan_phy. I've adjusted the naming to what existed (keeping the _STATE_ prefix) and kept that "flags" entry instead of creating a new one. Thanks, Miquèl
Hi, On Wed, May 18, 2022 at 12:29 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > aahringo@redhat.com wrote on Wed, 18 May 2022 10:31:55 -0400: > > > Hi, > > > > On Wed, May 18, 2022 at 4:55 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:58:19 -0400: > > > > > > > Hi, > > > > > > > > On Tue, May 17, 2022 at 12:35 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. > > > > > > > > > > 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 8881b6126b58..f4e7b3fe7cf0 100644 > > > > > --- a/include/net/cfg802154.h > > > > > +++ b/include/net/cfg802154.h > > > > > @@ -218,6 +218,7 @@ struct wpan_phy { > > > > > spinlock_t queue_lock; > > > > > atomic_t ongoing_txs; > > > > > atomic_t hold_txs; > > > > > + unsigned long queue_stopped; > > > > > > > > Can we name it something like state_flags (as phy state flags)? Pretty > > > > sure there will be more coming, or internal_flags, no idea... > > > > something_flags... > > > > > > 'phy_flags'? Just 'flags', maybe? > > > > make it so. > > Oh, there is already a flags entry in wpan_phy. I've adjusted the > naming to what existed (keeping the _STATE_ prefix) and kept that > "flags" entry instead of creating a new one. > make it so. - Alex
diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h index 8881b6126b58..f4e7b3fe7cf0 100644 --- a/include/net/cfg802154.h +++ b/include/net/cfg802154.h @@ -218,6 +218,7 @@ struct wpan_phy { spinlock_t queue_lock; atomic_t ongoing_txs; atomic_t hold_txs; + unsigned long 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 6cc4e5c7ba94..e36aca788ea2 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); + set_bit(0, &local->phy->queue_stopped); - return ieee802154_sync_queue(local); + return ret; } static int ieee802154_mlme_op_pre(struct ieee802154_local *local) @@ -174,9 +178,19 @@ int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb) return ret; } +static bool ieee802154_queue_is_stopped(struct ieee802154_local *local) +{ + return test_bit(0, &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 cddb42984484..faf12c2135bf 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(); + clear_bit(0, &local->phy->queue_stopped); 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(-)