[PATCH wpan-next 00/11] ieee802154: Synchronous Tx support

[PATCH wpan-next 00/11] ieee802154: Synchronous Tx support

[Miquel Raynal]

Hello,

New series bringing support for that famous synchronous Tx API for MLME
commands.

MLME commands will be used for during scan operations.

We need to be able to be sure that all transfers finished and that no
transfer will be queued for a short moment.

Cheers,
Miquèl

Miquel Raynal (11):
  net: mac802154: Stop exporting ieee802154_wake/stop_queue()
  net: mac802154: Change the wake/stop queue prototypes
  net: mac802154: Rename the synchronous xmit worker
  net: mac802154: Rename the main tx_work struct
  net: mac802154: Follow the count of ongoing transmissions
  net: mac802154: Hold the transmit queue when relevant
  net: mac802154: Create a hot tx path
  net: mac802154: Add a warning in the hot path
  net: mac802154: Introduce a helper to disable the queue
  net: mac802154: Introduce a tx queue flushing mechanism
  net: mac802154: Introduce a synchronous API for MLME commands

 include/net/cfg802154.h      |  5 ++
 include/net/mac802154.h      | 27 -----------
 net/ieee802154/core.c        |  1 +
 net/mac802154/cfg.c          |  5 +-
 net/mac802154/ieee802154_i.h | 74 +++++++++++++++++++++++++++++-
 net/mac802154/main.c         |  2 +-
 net/mac802154/tx.c           | 88 ++++++++++++++++++++++++++++++++----
 net/mac802154/util.c         | 55 ++++++++++++++++++----
 8 files changed, 207 insertions(+), 50 deletions(-)

-- 
2.27.0

[PATCH wpan-next 01/11] net: mac802154: Stop exporting ieee802154_wake/stop_queue()

[Miquel Raynal]

Individual drivers do not necessarily need to call these helpers
manually. There are other functions, more suited for this purpose, that
will do that for them. The advantage is that, as no more drivers call
these, it eases the tracking of the ongoing transfers that we are about
to introduce while keeping the possibility to bypass thse counters from
core code.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/mac802154.h      | 27 ---------------------------
 net/mac802154/ieee802154_i.h | 24 ++++++++++++++++++++++++
 net/mac802154/util.c         |  2 --
 3 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index bdac0ddbdcdb..357d25ef627a 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -460,33 +460,6 @@ void ieee802154_unregister_hw(struct ieee802154_hw *hw);
  */
 void ieee802154_rx_irqsafe(struct ieee802154_hw *hw, struct sk_buff *skb,
 			   u8 lqi);
-/**
- * ieee802154_wake_queue - wake ieee802154 queue
- * @hw: pointer as obtained from ieee802154_alloc_hw().
- *
- * Tranceivers usually have either one transmit framebuffer or one framebuffer
- * for both transmitting and receiving. Hence, the core currently only handles
- * one frame at a time for each phy, which means we had to stop the queue to
- * avoid new skb to come during the transmission. The queue then needs to be
- * woken up after the operation.
- *
- * Drivers should use this function instead of netif_wake_queue.
- */
-void ieee802154_wake_queue(struct ieee802154_hw *hw);
-
-/**
- * ieee802154_stop_queue - stop ieee802154 queue
- * @hw: pointer as obtained from ieee802154_alloc_hw().
- *
- * Tranceivers usually have either one transmit framebuffer or one framebuffer
- * for both transmitting and receiving. Hence, the core currently only handles
- * one frame at a time for each phy, which means we need to tell upper layers to
- * stop giving us new skbs while we are busy with the transmitted one. The queue
- * must then be stopped before transmitting.
- *
- * Drivers should use this function instead of netif_stop_queue.
- */
-void ieee802154_stop_queue(struct ieee802154_hw *hw);
 
 /**
  * ieee802154_xmit_complete - frame transmission complete
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 1381e6a5e180..83f5ccd1ca0f 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -130,6 +130,30 @@ netdev_tx_t
 ieee802154_subif_start_xmit(struct sk_buff *skb, struct net_device *dev);
 enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer);
 
+/**
+ * ieee802154_wake_queue - wake ieee802154 queue
+ * @hw: pointer as obtained from ieee802154_alloc_hw().
+ *
+ * Tranceivers usually have either one transmit framebuffer or one framebuffer
+ * for both transmitting and receiving. Hence, the core currently only handles
+ * one frame at a time for each phy, which means we had to stop the queue to
+ * avoid new skb to come during the transmission. The queue then needs to be
+ * woken up after the operation.
+ */
+void ieee802154_wake_queue(struct ieee802154_hw *hw);
+
+/**
+ * ieee802154_stop_queue - stop ieee802154 queue
+ * @hw: pointer as obtained from ieee802154_alloc_hw().
+ *
+ * Tranceivers usually have either one transmit framebuffer or one framebuffer
+ * for both transmitting and receiving. Hence, the core currently only handles
+ * one frame at a time for each phy, which means we need to tell upper layers to
+ * stop giving us new skbs while we are busy with the transmitted one. The queue
+ * must then be stopped before transmitting.
+ */
+void ieee802154_stop_queue(struct ieee802154_hw *hw);
+
 /* MIB callbacks */
 void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan);
 
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index 9f024d85563b..15a46b56d85a 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -27,7 +27,6 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL(ieee802154_wake_queue);
 
 void ieee802154_stop_queue(struct ieee802154_hw *hw)
 {
@@ -43,7 +42,6 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL(ieee802154_stop_queue);
 
 enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer)
 {
-- 
2.27.0

[PATCH wpan-next 02/11] net: mac802154: Change the wake/stop queue prototypes

[Miquel Raynal]

Currently the pointer returned by *_alloc_hw() is used for these
helpers, while actually all the callers have a *local pointer available
and anyway this local pointer is going to be derived inside both
helpers. We will soon add more helpers like these so let's change the
prototype right now to improve the harmony in this file.

There is no functional change.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/cfg.c          |  4 ++--
 net/mac802154/ieee802154_i.h |  8 ++++----
 net/mac802154/tx.c           |  6 +++---
 net/mac802154/util.c         | 12 +++++-------
 4 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index 1e4a9f74ed43..dafe02548161 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -46,7 +46,7 @@ static int ieee802154_suspend(struct wpan_phy *wpan_phy)
 	if (!local->open_count)
 		goto suspend;
 
-	ieee802154_stop_queue(&local->hw);
+	ieee802154_stop_queue(local);
 	synchronize_net();
 
 	/* stop hardware - this must stop RX */
@@ -72,7 +72,7 @@ static int ieee802154_resume(struct wpan_phy *wpan_phy)
 		return ret;
 
 wake_up:
-	ieee802154_wake_queue(&local->hw);
+	ieee802154_wake_queue(local);
 	local->suspended = false;
 	return 0;
 }
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 83f5ccd1ca0f..6652445a1147 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -132,7 +132,7 @@ enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer);
 
 /**
  * ieee802154_wake_queue - wake ieee802154 queue
- * @hw: pointer as obtained from ieee802154_alloc_hw().
+ * @local: main mac object
  *
  * Tranceivers usually have either one transmit framebuffer or one framebuffer
  * for both transmitting and receiving. Hence, the core currently only handles
@@ -140,11 +140,11 @@ enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer);
  * avoid new skb to come during the transmission. The queue then needs to be
  * woken up after the operation.
  */
-void ieee802154_wake_queue(struct ieee802154_hw *hw);
+void ieee802154_wake_queue(struct ieee802154_local *local);
 
 /**
  * ieee802154_stop_queue - stop ieee802154 queue
- * @hw: pointer as obtained from ieee802154_alloc_hw().
+ * @local: main mac object
  *
  * Tranceivers usually have either one transmit framebuffer or one framebuffer
  * for both transmitting and receiving. Hence, the core currently only handles
@@ -152,7 +152,7 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw);
  * stop giving us new skbs while we are busy with the transmitted one. The queue
  * must then be stopped before transmitting.
  */
-void ieee802154_stop_queue(struct ieee802154_hw *hw);
+void ieee802154_stop_queue(struct ieee802154_local *local);
 
 /* MIB callbacks */
 void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan);
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index c829e4a75325..c5befaca5366 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -43,7 +43,7 @@ void ieee802154_xmit_worker(struct work_struct *work)
 
 err_tx:
 	/* Restart the netif queue on each sub_if_data object. */
-	ieee802154_wake_queue(&local->hw);
+	ieee802154_wake_queue(local);
 	kfree_skb(skb);
 	netdev_dbg(dev, "transmission failed\n");
 }
@@ -74,7 +74,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 	}
 
 	/* Stop the netif queue on each sub_if_data object. */
-	ieee802154_stop_queue(&local->hw);
+	ieee802154_stop_queue(local);
 
 	/* async is priority, otherwise sync is fallback */
 	if (local->ops->xmit_async) {
@@ -82,7 +82,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 
 		ret = drv_xmit_async(local, skb);
 		if (ret) {
-			ieee802154_wake_queue(&local->hw);
+			ieee802154_wake_queue(local);
 			goto err_tx;
 		}
 
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index 15a46b56d85a..6ded390f0132 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -13,9 +13,8 @@
 /* privid for wpan_phys to determine whether they belong to us or not */
 const void *const mac802154_wpan_phy_privid = &mac802154_wpan_phy_privid;
 
-void ieee802154_wake_queue(struct ieee802154_hw *hw)
+void ieee802154_wake_queue(struct ieee802154_local *local)
 {
-	struct ieee802154_local *local = hw_to_local(hw);
 	struct ieee802154_sub_if_data *sdata;
 
 	rcu_read_lock();
@@ -28,9 +27,8 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw)
 	rcu_read_unlock();
 }
 
-void ieee802154_stop_queue(struct ieee802154_hw *hw)
+void ieee802154_stop_queue(struct ieee802154_local *local)
 {
-	struct ieee802154_local *local = hw_to_local(hw);
 	struct ieee802154_sub_if_data *sdata;
 
 	rcu_read_lock();
@@ -48,7 +46,7 @@ enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer)
 	struct ieee802154_local *local =
 		container_of(timer, struct ieee802154_local, ifs_timer);
 
-	ieee802154_wake_queue(&local->hw);
+	ieee802154_wake_queue(local);
 
 	return HRTIMER_NORESTART;
 }
@@ -82,7 +80,7 @@ void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb,
 				      hw->phy->sifs_period * NSEC_PER_USEC,
 				      HRTIMER_MODE_REL);
 	} else {
-		ieee802154_wake_queue(hw);
+		ieee802154_wake_queue(local);
 	}
 
 	dev_consume_skb_any(skb);
@@ -95,7 +93,7 @@ void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb,
 	struct ieee802154_local *local = hw_to_local(hw);
 
 	local->tx_result = reason;
-	ieee802154_wake_queue(hw);
+	ieee802154_wake_queue(local);
 	dev_kfree_skb_any(skb);
 }
 EXPORT_SYMBOL(ieee802154_xmit_error);
-- 
2.27.0

[PATCH wpan-next 03/11] net: mac802154: Rename the synchronous xmit worker

[Miquel Raynal]

There are currently two driver hooks: one is synchronous, the other is
not. We cannot rely on driver implementations to provide a synchronous
API (which is related to the bus medium more than a wish to have a
synchronized implementation) so we are going to introduce a sync API
above any kind of driver transmit function. In order to clarify what
this worker is for (synchronous driver implementation), let's rename it
so that people don't get bothered by the fact that their driver does not
make use of the "xmit worker" which is a too generic name.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/ieee802154_i.h | 2 +-
 net/mac802154/main.c         | 2 +-
 net/mac802154/tx.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 6652445a1147..d27e9fe363b6 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -123,7 +123,7 @@ ieee802154_sdata_running(struct ieee802154_sub_if_data *sdata)
 extern struct ieee802154_mlme_ops mac802154_mlme_wpan;
 
 void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb);
-void ieee802154_xmit_worker(struct work_struct *work);
+void ieee802154_xmit_sync_worker(struct work_struct *work);
 netdev_tx_t
 ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev);
 netdev_tx_t
diff --git a/net/mac802154/main.c b/net/mac802154/main.c
index 5546ef86e231..13c6b3cd0429 100644
--- a/net/mac802154/main.c
+++ b/net/mac802154/main.c
@@ -95,7 +95,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
 
 	skb_queue_head_init(&local->skb_queue);
 
-	INIT_WORK(&local->tx_work, ieee802154_xmit_worker);
+	INIT_WORK(&local->tx_work, ieee802154_xmit_sync_worker);
 
 	/* init supported flags with 802.15.4 default ranges */
 	phy->supported.max_minbe = 8;
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index c5befaca5366..0ebe39169309 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -22,7 +22,7 @@
 #include "ieee802154_i.h"
 #include "driver-ops.h"
 
-void ieee802154_xmit_worker(struct work_struct *work)
+void ieee802154_xmit_sync_worker(struct work_struct *work)
 {
 	struct ieee802154_local *local =
 		container_of(work, struct ieee802154_local, tx_work);
-- 
2.27.0

[PATCH wpan-next 04/11] net: mac802154: Rename the main tx_work struct

[Miquel Raynal]

This entry is dedicated to synchronous transmissions done by drivers
without async hook. Make this clearer that this is not a work that any
driver can use by at least prefixing it with "sync_". While at it, let's
enhance the comment explaining why we choose one or the other.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/ieee802154_i.h | 2 +-
 net/mac802154/main.c         | 2 +-
 net/mac802154/tx.c           | 9 ++++++---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index d27e9fe363b6..3f59a291b481 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -55,7 +55,7 @@ struct ieee802154_local {
 	struct sk_buff_head skb_queue;
 
 	struct sk_buff *tx_skb;
-	struct work_struct tx_work;
+	struct work_struct sync_tx_work;
 	/* A negative Linux error code or a null/positive MLME error status */
 	int tx_result;
 };
diff --git a/net/mac802154/main.c b/net/mac802154/main.c
index 13c6b3cd0429..46258c6d430f 100644
--- a/net/mac802154/main.c
+++ b/net/mac802154/main.c
@@ -95,7 +95,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
 
 	skb_queue_head_init(&local->skb_queue);
 
-	INIT_WORK(&local->tx_work, ieee802154_xmit_sync_worker);
+	INIT_WORK(&local->sync_tx_work, ieee802154_xmit_sync_worker);
 
 	/* init supported flags with 802.15.4 default ranges */
 	phy->supported.max_minbe = 8;
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 0ebe39169309..6d559f96664d 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -25,7 +25,7 @@
 void ieee802154_xmit_sync_worker(struct work_struct *work)
 {
 	struct ieee802154_local *local =
-		container_of(work, struct ieee802154_local, tx_work);
+		container_of(work, struct ieee802154_local, sync_tx_work);
 	struct sk_buff *skb = local->tx_skb;
 	struct net_device *dev = skb->dev;
 	int res;
@@ -76,7 +76,10 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 	/* Stop the netif queue on each sub_if_data object. */
 	ieee802154_stop_queue(local);
 
-	/* async is priority, otherwise sync is fallback */
+	/* Drivers should preferably implement the async callback. In some rare
+	 * cases they only provide a sync callback which we will use as a
+	 * fallback.
+	 */
 	if (local->ops->xmit_async) {
 		unsigned int len = skb->len;
 
@@ -90,7 +93,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 		dev->stats.tx_bytes += len;
 	} else {
 		local->tx_skb = skb;
-		queue_work(local->workqueue, &local->tx_work);
+		queue_work(local->workqueue, &local->sync_tx_work);
 	}
 
 	return NETDEV_TX_OK;
-- 
2.27.0

[PATCH wpan-next 05/11] net: mac802154: Follow the count of ongoing transmissions

[Miquel Raynal]

In order to create a synchronous API for MLME command purposes, we need
to be able to track the end of the ongoing transmissions. Let's
introduce an atomic variable which is incremented when a transmission
starts and decremented when relevant so that we know at any moment
whether there is an ongoing transmission.

The counter gets decremented in the following situations:
- The operation is asynchronous and there was a failure during the
  offloading process.
- The operation is synchronous and the synchronous operation failed.
- The operation finished, either successfully or not.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h | 3 +++
 net/mac802154/tx.c      | 3 +++
 net/mac802154/util.c    | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 85f9e8417688..473ebcb9b155 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -214,6 +214,9 @@ struct wpan_phy {
 	/* the network namespace this phy lives in currently */
 	possible_net_t _net;
 
+	/* Transmission monitoring */
+	atomic_t ongoing_txs;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 6d559f96664d..8c0bad7796ba 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -45,6 +45,7 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
 	/* Restart the netif queue on each sub_if_data object. */
 	ieee802154_wake_queue(local);
 	kfree_skb(skb);
+	atomic_dec(&local->phy->ongoing_txs);
 	netdev_dbg(dev, "transmission failed\n");
 }
 
@@ -75,6 +76,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 
 	/* Stop the netif queue on each sub_if_data object. */
 	ieee802154_stop_queue(local);
+	atomic_inc(&local->phy->ongoing_txs);
 
 	/* Drivers should preferably implement the async callback. In some rare
 	 * cases they only provide a sync callback which we will use as a
@@ -99,6 +101,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 	return NETDEV_TX_OK;
 
 err_tx:
+	atomic_dec(&local->phy->ongoing_txs);
 	kfree_skb(skb);
 	return NETDEV_TX_OK;
 }
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index 6ded390f0132..4bcc9cd2eb9d 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -84,6 +84,7 @@ void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb,
 	}
 
 	dev_consume_skb_any(skb);
+	atomic_dec(&hw->phy->ongoing_txs);
 }
 EXPORT_SYMBOL(ieee802154_xmit_complete);
 
@@ -95,6 +96,7 @@ void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb,
 	local->tx_result = reason;
 	ieee802154_wake_queue(local);
 	dev_kfree_skb_any(skb);
+	atomic_dec(&hw->phy->ongoing_txs);
 }
 EXPORT_SYMBOL(ieee802154_xmit_error);
 
-- 
2.27.0

[PATCH wpan-next 06/11] net: mac802154: Hold the transmit queue when relevant

[Miquel Raynal]

Let's create a hold_txs atomic variable and increment/decrement it when
relevant. Currently we can use it during a suspend. Very soon we will
also use this feature during scans.

When the variable is incremented, any further wake up call will be
skipped until the variable gets decremented back.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h      |  3 ++-
 net/mac802154/cfg.c          |  2 ++
 net/mac802154/ieee802154_i.h | 24 ++++++++++++++++++++++++
 net/mac802154/tx.c           | 15 +++++++++++++++
 net/mac802154/util.c         |  3 +++
 5 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 473ebcb9b155..043d8e4359e7 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -214,8 +214,9 @@ struct wpan_phy {
 	/* the network namespace this phy lives in currently */
 	possible_net_t _net;
 
-	/* Transmission monitoring */
+	/* Transmission monitoring and control */
 	atomic_t ongoing_txs;
+	atomic_t hold_txs;
 
 	char priv[] __aligned(NETDEV_ALIGN);
 };
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index dafe02548161..0540a2b014d2 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -46,6 +46,7 @@ static int ieee802154_suspend(struct wpan_phy *wpan_phy)
 	if (!local->open_count)
 		goto suspend;
 
+	ieee802154_hold_queue(local);
 	ieee802154_stop_queue(local);
 	synchronize_net();
 
@@ -72,6 +73,7 @@ static int ieee802154_resume(struct wpan_phy *wpan_phy)
 		return ret;
 
 wake_up:
+	ieee802154_release_queue(local);
 	ieee802154_wake_queue(local);
 	local->suspended = false;
 	return 0;
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 3f59a291b481..b55fdefb0b34 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -142,6 +142,30 @@ enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer);
  */
 void ieee802154_wake_queue(struct ieee802154_local *local);
 
+/**
+ * ieee802154_hold_queue - hold ieee802154 queue
+ * @local: main mac object
+ *
+ * Hold a queue, this queue cannot be woken up while this is active.
+ */
+void ieee802154_hold_queue(struct ieee802154_local *local);
+
+/**
+ * ieee802154_release_queue - release ieee802154 queue
+ * @local: main mac object
+ *
+ * Release a queue which is held. The queue can now be woken up.
+ */
+void ieee802154_release_queue(struct ieee802154_local *local);
+
+/**
+ * ieee802154_queue_is_held - checks whether the ieee802154 queue is held
+ * @local: main mac object
+ *
+ * Checks whether the queue is currently held.
+ */
+bool ieee802154_queue_is_held(struct ieee802154_local *local);
+
 /**
  * ieee802154_stop_queue - stop ieee802154 queue
  * @local: main mac object
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 8c0bad7796ba..d088aa8119e8 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 	return NETDEV_TX_OK;
 }
 
+void ieee802154_hold_queue(struct ieee802154_local *local)
+{
+	atomic_inc(&local->phy->hold_txs);
+}
+
+void ieee802154_release_queue(struct ieee802154_local *local)
+{
+	atomic_dec(&local->phy->hold_txs);
+}
+
+bool ieee802154_queue_is_held(struct ieee802154_local *local)
+{
+	return atomic_read(&local->phy->hold_txs);
+}
+
 netdev_tx_t
 ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index 4bcc9cd2eb9d..847e0864b575 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -17,6 +17,9 @@ void ieee802154_wake_queue(struct ieee802154_local *local)
 {
 	struct ieee802154_sub_if_data *sdata;
 
+	if (ieee802154_queue_is_held(local))
+		return;
+
 	rcu_read_lock();
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 		if (!sdata->dev)
-- 
2.27.0

[PATCH wpan-next 07/11] net: mac802154: Create a hot tx path

[Miquel Raynal]

Let's rename the current Tx path to show that this is the "hot" Tx
path. We will soon introduce a slower Tx path for MLME commands.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/tx.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index d088aa8119e8..a8a83f0167bf 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -121,6 +121,12 @@ bool ieee802154_queue_is_held(struct ieee802154_local *local)
 	return atomic_read(&local->phy->hold_txs);
 }
 
+static netdev_tx_t
+ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
+{
+	return ieee802154_tx(local, skb);
+}
+
 netdev_tx_t
 ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -128,7 +134,7 @@ ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	skb->skb_iif = dev->ifindex;
 
-	return ieee802154_tx(sdata->local, skb);
+	return ieee802154_hot_tx(sdata->local, skb);
 }
 
 netdev_tx_t
@@ -150,5 +156,5 @@ ieee802154_subif_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	skb->skb_iif = dev->ifindex;
 
-	return ieee802154_tx(sdata->local, skb);
+	return ieee802154_hot_tx(sdata->local, skb);
 }
-- 
2.27.0

[PATCH wpan-next 08/11] net: mac802154: Add a warning in the hot path

[Miquel Raynal]

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@bootlin.com>
---
 net/mac802154/ieee802154_i.h |  8 ++++++++
 net/mac802154/tx.c           |  2 ++
 net/mac802154/util.c         | 18 ++++++++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index b55fdefb0b34..cb61a4abaf37 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -178,6 +178,14 @@ bool ieee802154_queue_is_held(struct ieee802154_local *local);
  */
 void ieee802154_stop_queue(struct ieee802154_local *local);
 
+/**
+ * ieee802154_queue_is_stopped - check whether ieee802154 queue was stopped
+ * @local: main mac object
+ *
+ * Goes through all the interfaces and indicates if they are all stopped or not.
+ */
+bool ieee802154_queue_is_stopped(struct ieee802154_local *local);
+
 /* MIB callbacks */
 void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan);
 
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;
+}
+
 enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer)
 {
 	struct ieee802154_local *local =
-- 
2.27.0

[PATCH wpan-next 09/11] net: mac802154: Introduce a helper to disable the queue

[Miquel Raynal]

Sometimes calling the stop queue helper is not enough because it does
not hold any lock. In order to be safe and avoid racy situations when
trying to (soon) sync the Tx queue, for instance before sending an MLME
frame, let's now introduce an helper which actually hold the necessary
locks when doing so.

Suggested-by: Alexander Aring <alex.aring@gmail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/ieee802154_i.h | 12 ++++++++++++
 net/mac802154/util.c         | 15 +++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index cb61a4abaf37..c686a027555a 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -186,6 +186,18 @@ void ieee802154_stop_queue(struct ieee802154_local *local);
  */
 bool ieee802154_queue_is_stopped(struct ieee802154_local *local);
 
+/**
+ * ieee802154_disable_queue - disable ieee802154 queue
+ * @local: main mac object
+ *
+ * When trying to sync the Tx queue, we cannot just stop the queue
+ * (which is basically a bit being set without proper lock handling)
+ * because it would be racy. We actually need to call netif_tx_disable()
+ * instead, which is done by this helper. Restarting the queue can
+ * however still be done with a regular wake call.
+ */
+void ieee802154_disable_queue(struct ieee802154_local *local);
+
 /* MIB callbacks */
 void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan);
 
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index cfd17a7db532..3e2b157b34b1 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -62,6 +62,21 @@ bool ieee802154_queue_is_stopped(struct ieee802154_local *local)
 	return stopped;
 }
 
+void ieee802154_disable_queue(struct ieee802154_local *local)
+{
+        struct ieee802154_sub_if_data *sdata;
+
+        rcu_read_lock();
+        list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+                if (!sdata->dev)
+                        continue;
+
+		netif_tx_disable(sdata->dev);
+        }
+        rcu_read_unlock();
+}
+EXPORT_SYMBOL(ieee802154_disable_queue);
+
 enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer)
 {
 	struct ieee802154_local *local =
-- 
2.27.0

[PATCH wpan-next 10/11] net: mac802154: Introduce a tx queue flushing mechanism

[Miquel Raynal]

Right now we are able to stop a queue but we have no indication if a
transmission is ongoing or not.

Thanks to recent additions, we can track the number of ongoing
transmissions so we know if the last transmission is over. Adding on top
of it an internal wait queue also allows to be woken up asynchronously
when this happens. If, beforehands, we marked the queue to be held and
stopped it, we end up flushing and stopping the tx queue.

Thanks to this feature, we will soon be able to introduce a synchronous
transmit API.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h      |  1 +
 net/ieee802154/core.c        |  1 +
 net/mac802154/cfg.c          |  3 +--
 net/mac802154/ieee802154_i.h |  1 +
 net/mac802154/tx.c           | 24 +++++++++++++++++++++++-
 net/mac802154/util.c         |  7 +++++--
 6 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 043d8e4359e7..0d385a214da3 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -217,6 +217,7 @@ struct wpan_phy {
 	/* Transmission monitoring and control */
 	atomic_t ongoing_txs;
 	atomic_t hold_txs;
+	wait_queue_head_t sync_txq;
 
 	char priv[] __aligned(NETDEV_ALIGN);
 };
diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
index de259b5170ab..0953cacafbff 100644
--- a/net/ieee802154/core.c
+++ b/net/ieee802154/core.c
@@ -129,6 +129,7 @@ wpan_phy_new(const struct cfg802154_ops *ops, size_t priv_size)
 	wpan_phy_net_set(&rdev->wpan_phy, &init_net);
 
 	init_waitqueue_head(&rdev->dev_wait);
+	init_waitqueue_head(&rdev->wpan_phy.sync_txq);
 
 	return &rdev->wpan_phy;
 }
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index 0540a2b014d2..c17e38bef425 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -46,8 +46,7 @@ static int ieee802154_suspend(struct wpan_phy *wpan_phy)
 	if (!local->open_count)
 		goto suspend;
 
-	ieee802154_hold_queue(local);
-	ieee802154_stop_queue(local);
+	ieee802154_sync_and_hold_queue(local);
 	synchronize_net();
 
 	/* stop hardware - this must stop RX */
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index c686a027555a..c87c7fa04435 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -124,6 +124,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan;
 
 void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb);
 void ieee802154_xmit_sync_worker(struct work_struct *work);
+int ieee802154_sync_and_hold_queue(struct ieee802154_local *local);
 netdev_tx_t
 ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev);
 netdev_tx_t
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 021dddfea542..5f94973b57e4 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -45,7 +45,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
 	/* Restart the netif queue on each sub_if_data object. */
 	ieee802154_wake_queue(local);
 	kfree_skb(skb);
-	atomic_dec(&local->phy->ongoing_txs);
+	if (!atomic_dec_and_test(&local->phy->ongoing_txs))
+		wake_up(&local->phy->sync_txq);
 	netdev_dbg(dev, "transmission failed\n");
 }
 
@@ -121,6 +122,27 @@ bool ieee802154_queue_is_held(struct ieee802154_local *local)
 	return atomic_read(&local->phy->hold_txs);
 }
 
+static int ieee802154_sync_queue(struct ieee802154_local *local)
+{
+	int ret;
+
+	ieee802154_hold_queue(local);
+	ieee802154_disable_queue(local);
+	wait_event(local->phy->sync_txq, !atomic_read(&local->phy->ongoing_txs));
+	ret = local->tx_result;
+	ieee802154_release_queue(local);
+	ieee802154_wake_queue(local);
+
+	return ret;
+}
+
+int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
+{
+	ieee802154_hold_queue(local);
+
+	return ieee802154_sync_queue(local);
+}
+
 static netdev_tx_t
 ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
 {
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index 3e2b157b34b1..056f8c4e6bcd 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -120,7 +120,8 @@ void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb,
 	}
 
 	dev_consume_skb_any(skb);
-	atomic_dec(&hw->phy->ongoing_txs);
+	if (!atomic_dec_and_test(&hw->phy->ongoing_txs))
+		wake_up(&hw->phy->sync_txq);
 }
 EXPORT_SYMBOL(ieee802154_xmit_complete);
 
@@ -132,7 +133,9 @@ void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb,
 	local->tx_result = reason;
 	ieee802154_wake_queue(local);
 	dev_kfree_skb_any(skb);
-	atomic_dec(&hw->phy->ongoing_txs);
+
+	if (!atomic_dec_and_test(&hw->phy->ongoing_txs))
+		wake_up(&hw->phy->sync_txq);
 }
 EXPORT_SYMBOL(ieee802154_xmit_error);
 
-- 
2.27.0

[PATCH wpan-next 11/11] net: mac802154: Introduce a synchronous API for MLME commands

[Miquel Raynal]

This is the slow path, we need to wait for each command to be processed
before continuing so let's introduce an helper which does the
transmission and blocks until it gets notified of its asynchronous
completion. This helper is going to be used when introducing scan
support.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/ieee802154_i.h |  1 +
 net/mac802154/tx.c           | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index c87c7fa04435..8aa8d0dd9c41 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -125,6 +125,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan;
 void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb);
 void ieee802154_xmit_sync_worker(struct work_struct *work);
 int ieee802154_sync_and_hold_queue(struct ieee802154_local *local);
+int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb);
 netdev_tx_t
 ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev);
 netdev_tx_t
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 5f94973b57e4..17244293c59a 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -143,6 +143,25 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
 	return ieee802154_sync_queue(local);
 }
 
+int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb)
+{
+	bool queue_held = ieee802154_queue_is_held(local);
+	int ret;
+
+	if (!queue_held)
+		ieee802154_sync_and_hold_queue(local);
+
+	ieee802154_tx(local, skb);
+	ret = ieee802154_sync_queue(local);
+
+	if (!queue_held)
+		ieee802154_release_queue(local);
+
+	ieee802154_wake_queue(local);
+
+	return ret;
+}
+
 static netdev_tx_t
 ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
 {
-- 
2.27.0

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

[Alexander Aring]

Hi,

On Wed, Apr 27, 2022 at 12:47 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.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  net/mac802154/ieee802154_i.h |  8 ++++++++
>  net/mac802154/tx.c           |  2 ++
>  net/mac802154/util.c         | 18 ++++++++++++++++++
>  3 files changed, 28 insertions(+)
>
> diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> index b55fdefb0b34..cb61a4abaf37 100644
> --- a/net/mac802154/ieee802154_i.h
> +++ b/net/mac802154/ieee802154_i.h
> @@ -178,6 +178,14 @@ bool ieee802154_queue_is_held(struct ieee802154_local *local);
>   */
>  void ieee802154_stop_queue(struct ieee802154_local *local);
>
> +/**
> + * ieee802154_queue_is_stopped - check whether ieee802154 queue was stopped
> + * @local: main mac object
> + *
> + * Goes through all the interfaces and indicates if they are all stopped or not.
> + */
> +bool ieee802154_queue_is_stopped(struct ieee802154_local *local);
> +
>  /* MIB callbacks */
>  void mac802154_dev_set_page_channel(struct net_device *dev, u8 page, u8 chan);
>
> 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. btw: if it's hit your if
branch the first time you can break?

I am not done with the review, this is just what I see now and we can
discuss that. Please be patient.

- Alex

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

[Miquel Raynal]

Hi Alexander,

alex.aring@gmail.com wrote on Wed, 27 Apr 2022 14:01:25 -0400:

> Hi,
> 
> On Wed, Apr 27, 2022 at 12:47 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.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---

[...]

> > 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

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

[Alexander Aring]

Hi,

On Thu, Apr 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@gmail.com wrote on Wed, 27 Apr 2022 14:01:25 -0400:
>
> > Hi,
> >
> > On Wed, Apr 27, 2022 at 12:47 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.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
>
> [...]
>
> > > 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.
>

I am thinking about this patch series... and I think it still has bugs
or at least it's easy to have bugs when the context is not right
prepared to call a synchronized transmission. We leave here the netdev
state machine world for transmit vs e.g. start/stop netif callback...
We have a warning here if there is a core netif xmit callback running
when 802.15.4 thinks it shouldn't (because we take control of it) but
I also think about a kind of the other way around. A warning if
802.15.4 transmits something but the netdev core logic "thinks" it
shouldn't.

That requires some checks (probably from netcore functionality) to
check if we call a 802.15.4 sync xmit but netif core already called
stop() callback. The last stop() callback - means the driver_ops
stop() callback was called, we have some "open_count" counter there
which MUST be incremented before doing any looping of one or several
sync transmissions. All I can say is if we call xmit() but the driver
is in stop() state... it will break things.

My concern is also here that e.g. calling netif down or device
suspend() are only two examples I have in my mind right now. I don't
know all cases which can occur, that's why we should introduce another
WARN_ON_ONCE() for the case that 802.15.4 transmits something but we
are in a state where we can't transmit something according to netif
state (driver ops called stop()).

Can you add such a check as well? And please keep in mind to increment
the open count when implementing MLME-ops (or at least handle it
somehow), otherwise I guess it's easy to hit the warning. If another
user reports warnings and tells us what they did we might know more
other "cases" to fix.

There should maybe be an option in hwsim to delay a transmission
completion and such cases can be tested...

- Alex

Re: [PATCH wpan-next 10/11] net: mac802154: Introduce a tx queue flushing mechanism

[Alexander Aring]

Hi,

On Wed, Apr 27, 2022 at 12:55 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Right now we are able to stop a queue but we have no indication if a
> transmission is ongoing or not.
>
> Thanks to recent additions, we can track the number of ongoing
> transmissions so we know if the last transmission is over. Adding on top
> of it an internal wait queue also allows to be woken up asynchronously
> when this happens. If, beforehands, we marked the queue to be held and
> stopped it, we end up flushing and stopping the tx queue.
>
> Thanks to this feature, we will soon be able to introduce a synchronous
> transmit API.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/net/cfg802154.h      |  1 +
>  net/ieee802154/core.c        |  1 +
>  net/mac802154/cfg.c          |  3 +--
>  net/mac802154/ieee802154_i.h |  1 +
>  net/mac802154/tx.c           | 24 +++++++++++++++++++++++-
>  net/mac802154/util.c         |  7 +++++--
>  6 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 043d8e4359e7..0d385a214da3 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -217,6 +217,7 @@ struct wpan_phy {
>         /* Transmission monitoring and control */
>         atomic_t ongoing_txs;
>         atomic_t hold_txs;
> +       wait_queue_head_t sync_txq;
>
>         char priv[] __aligned(NETDEV_ALIGN);
>  };
> diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
> index de259b5170ab..0953cacafbff 100644
> --- a/net/ieee802154/core.c
> +++ b/net/ieee802154/core.c
> @@ -129,6 +129,7 @@ wpan_phy_new(const struct cfg802154_ops *ops, size_t priv_size)
>         wpan_phy_net_set(&rdev->wpan_phy, &init_net);
>
>         init_waitqueue_head(&rdev->dev_wait);
> +       init_waitqueue_head(&rdev->wpan_phy.sync_txq);
>
>         return &rdev->wpan_phy;
>  }
> diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> index 0540a2b014d2..c17e38bef425 100644
> --- a/net/mac802154/cfg.c
> +++ b/net/mac802154/cfg.c
> @@ -46,8 +46,7 @@ static int ieee802154_suspend(struct wpan_phy *wpan_phy)
>         if (!local->open_count)
>                 goto suspend;
>
> -       ieee802154_hold_queue(local);
> -       ieee802154_stop_queue(local);
> +       ieee802154_sync_and_hold_queue(local);
>         synchronize_net();
>
>         /* stop hardware - this must stop RX */
> diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> index c686a027555a..c87c7fa04435 100644
> --- a/net/mac802154/ieee802154_i.h
> +++ b/net/mac802154/ieee802154_i.h
> @@ -124,6 +124,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan;
>
>  void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb);
>  void ieee802154_xmit_sync_worker(struct work_struct *work);
> +int ieee802154_sync_and_hold_queue(struct ieee802154_local *local);
>  netdev_tx_t
>  ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev);
>  netdev_tx_t
> diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> index 021dddfea542..5f94973b57e4 100644
> --- a/net/mac802154/tx.c
> +++ b/net/mac802154/tx.c
> @@ -45,7 +45,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
>         /* Restart the netif queue on each sub_if_data object. */
>         ieee802154_wake_queue(local);
>         kfree_skb(skb);
> -       atomic_dec(&local->phy->ongoing_txs);
> +       if (!atomic_dec_and_test(&local->phy->ongoing_txs))
> +               wake_up(&local->phy->sync_txq);
>         netdev_dbg(dev, "transmission failed\n");
>  }

There is a missing handling of dec() wake_up() in ieee802154_tx()
"err_tx" label.

- Alex

Re: [PATCH wpan-next 06/11] net: mac802154: Hold the transmit queue when relevant

[Alexander Aring]

Hi,

On Wed, Apr 27, 2022 at 12:54 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Let's create a hold_txs atomic variable and increment/decrement it when
> relevant. Currently we can use it during a suspend. Very soon we will
> also use this feature during scans.
>
> When the variable is incremented, any further wake up call will be
> skipped until the variable gets decremented back.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/net/cfg802154.h      |  3 ++-
>  net/mac802154/cfg.c          |  2 ++
>  net/mac802154/ieee802154_i.h | 24 ++++++++++++++++++++++++
>  net/mac802154/tx.c           | 15 +++++++++++++++
>  net/mac802154/util.c         |  3 +++
>  5 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 473ebcb9b155..043d8e4359e7 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -214,8 +214,9 @@ struct wpan_phy {
>         /* the network namespace this phy lives in currently */
>         possible_net_t _net;
>
> -       /* Transmission monitoring */
> +       /* Transmission monitoring and control */
>         atomic_t ongoing_txs;
> +       atomic_t hold_txs;
>
>         char priv[] __aligned(NETDEV_ALIGN);
>  };
> diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> index dafe02548161..0540a2b014d2 100644
> --- a/net/mac802154/cfg.c
> +++ b/net/mac802154/cfg.c
> @@ -46,6 +46,7 @@ static int ieee802154_suspend(struct wpan_phy *wpan_phy)
>         if (!local->open_count)
>                 goto suspend;
>
> +       ieee802154_hold_queue(local);
>         ieee802154_stop_queue(local);
>         synchronize_net();
>
> @@ -72,6 +73,7 @@ static int ieee802154_resume(struct wpan_phy *wpan_phy)
>                 return ret;
>
>  wake_up:
> +       ieee802154_release_queue(local);
>         ieee802154_wake_queue(local);
>         local->suspended = false;
>         return 0;
> diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> index 3f59a291b481..b55fdefb0b34 100644
> --- a/net/mac802154/ieee802154_i.h
> +++ b/net/mac802154/ieee802154_i.h
> @@ -142,6 +142,30 @@ enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer);
>   */
>  void ieee802154_wake_queue(struct ieee802154_local *local);
>
> +/**
> + * ieee802154_hold_queue - hold ieee802154 queue
> + * @local: main mac object
> + *
> + * Hold a queue, this queue cannot be woken up while this is active.
> + */
> +void ieee802154_hold_queue(struct ieee802154_local *local);
> +
> +/**
> + * ieee802154_release_queue - release ieee802154 queue
> + * @local: main mac object
> + *
> + * Release a queue which is held. The queue can now be woken up.
> + */
> +void ieee802154_release_queue(struct ieee802154_local *local);
> +
> +/**
> + * ieee802154_queue_is_held - checks whether the ieee802154 queue is held
> + * @local: main mac object
> + *
> + * Checks whether the queue is currently held.
> + */
> +bool ieee802154_queue_is_held(struct ieee802154_local *local);
> +
>  /**
>   * ieee802154_stop_queue - stop ieee802154 queue
>   * @local: main mac object
> diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> index 8c0bad7796ba..d088aa8119e8 100644
> --- a/net/mac802154/tx.c
> +++ b/net/mac802154/tx.c
> @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
>         return NETDEV_TX_OK;
>  }
>
> +void ieee802154_hold_queue(struct ieee802154_local *local)
> +{
> +       atomic_inc(&local->phy->hold_txs);
> +}
> +
> +void ieee802154_release_queue(struct ieee802154_local *local)
> +{
> +       atomic_dec(&local->phy->hold_txs);
> +}
> +
> +bool ieee802154_queue_is_held(struct ieee802154_local *local)
> +{
> +       return atomic_read(&local->phy->hold_txs);
> +}

I am not getting this, should the release_queue() function not do
something like:

if (atomic_dec_and_test(hold_txs))
      ieee802154_wake_queue(local);

I think we don't need the test of "ieee802154_queue_is_held()" here,
then we need to replace all stop_queue/wake_queue with hold and
release?

- Alex

Re: [PATCH wpan-next 10/11] net: mac802154: Introduce a tx queue flushing mechanism

[Miquel Raynal]

Hi Alex,

> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -45,7 +45,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
> >         /* Restart the netif queue on each sub_if_data object. */
> >         ieee802154_wake_queue(local);
> >         kfree_skb(skb);
> > -       atomic_dec(&local->phy->ongoing_txs);
> > +       if (!atomic_dec_and_test(&local->phy->ongoing_txs))
> > +               wake_up(&local->phy->sync_txq);
> >         netdev_dbg(dev, "transmission failed\n");
> >  }  
> 
> There is a missing handling of dec() wake_up() in ieee802154_tx()
> "err_tx" label.

Good catch. Fixed.

Thanks,
Miquèl

Re: [PATCH wpan-next 06/11] net: mac802154: Hold the transmit queue when relevant

[Miquel Raynal]

Hi Alex,

> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
> >         return NETDEV_TX_OK;
> >  }
> >
> > +void ieee802154_hold_queue(struct ieee802154_local *local)
> > +{
> > +       atomic_inc(&local->phy->hold_txs);
> > +}
> > +
> > +void ieee802154_release_queue(struct ieee802154_local *local)
> > +{
> > +       atomic_dec(&local->phy->hold_txs);
> > +}
> > +
> > +bool ieee802154_queue_is_held(struct ieee802154_local *local)
> > +{
> > +       return atomic_read(&local->phy->hold_txs);
> > +}  
> 
> I am not getting this, should the release_queue() function not do
> something like:
> 
> if (atomic_dec_and_test(hold_txs))
>       ieee802154_wake_queue(local);
> 
> I think we don't need the test of "ieee802154_queue_is_held()" here,
> then we need to replace all stop_queue/wake_queue with hold and
> release?

That's actually a good idea. I've implemented it and it looks nice too.
I'll clean this up and share a new version with:
- The wake call checked everytime hold_txs gets decremented
- The removal of the _queue_is_held() helper
- _wake/stop_queue() turned static
- _hold/release_queue() used everywhere

Thanks,
Miquèl

Re: [PATCH wpan-next 06/11] net: mac802154: Hold the transmit queue when relevant

[Alexander Aring]

Hi,

On Tue, May 10, 2022 at 10:52 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Alex,
>
> > > --- a/net/mac802154/tx.c
> > > +++ b/net/mac802154/tx.c
> > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
> > >         return NETDEV_TX_OK;
> > >  }
> > >
> > > +void ieee802154_hold_queue(struct ieee802154_local *local)
> > > +{
> > > +       atomic_inc(&local->phy->hold_txs);
> > > +}
> > > +
> > > +void ieee802154_release_queue(struct ieee802154_local *local)
> > > +{
> > > +       atomic_dec(&local->phy->hold_txs);
> > > +}
> > > +
> > > +bool ieee802154_queue_is_held(struct ieee802154_local *local)
> > > +{
> > > +       return atomic_read(&local->phy->hold_txs);
> > > +}
> >
> > I am not getting this, should the release_queue() function not do
> > something like:
> >
> > if (atomic_dec_and_test(hold_txs))
> >       ieee802154_wake_queue(local);
> >
> > I think we don't need the test of "ieee802154_queue_is_held()" here,
> > then we need to replace all stop_queue/wake_queue with hold and
> > release?
>
> That's actually a good idea. I've implemented it and it looks nice too.
> I'll clean this up and share a new version with:
> - The wake call checked everytime hold_txs gets decremented
> - The removal of the _queue_is_held() helper
> - _wake/stop_queue() turned static
> - _hold/release_queue() used everywhere
>

I think there is also a lock necessary for atomic inc/dec hitting zero
and the stop/wake call afterwards... ,there are also a lot of
optimization techniques to only hold the lock for hitting zero cases
in such areas. However we will see...

- Alex

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

[Miquel Raynal]

Hi Alexander,

alex.aring@gmail.com wrote on Sun, 1 May 2022 20:21:18 -0400:

> Hi,
> 
> On Thu, Apr 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > alex.aring@gmail.com wrote on Wed, 27 Apr 2022 14:01:25 -0400:
> >  
> > > Hi,
> > >
> > > On Wed, Apr 27, 2022 at 12:47 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.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---  
> >
> > [...]
> >  
> > > > 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.
> >  
> 
> I am thinking about this patch series... and I think it still has bugs
> or at least it's easy to have bugs when the context is not right
> prepared to call a synchronized transmission. We leave here the netdev
> state machine world for transmit vs e.g. start/stop netif callback...
> We have a warning here if there is a core netif xmit callback running
> when 802.15.4 thinks it shouldn't (because we take control of it) but
> I also think about a kind of the other way around. A warning if
> 802.15.4 transmits something but the netdev core logic "thinks" it
> shouldn't.
> 
> That requires some checks (probably from netcore functionality) to
> check if we call a 802.15.4 sync xmit but netif core already called
> stop() callback. The last stop() callback - means the driver_ops
> stop() callback was called, we have some "open_count" counter there
> which MUST be incremented before doing any looping of one or several
> sync transmissions. All I can say is if we call xmit() but the driver
> is in stop() state... it will break things.
> 
> My concern is also here that e.g. calling netif down or device
> suspend() are only two examples I have in my mind right now. I don't
> know all cases which can occur, that's why we should introduce another
> WARN_ON_ONCE() for the case that 802.15.4 transmits something but we
> are in a state where we can't transmit something according to netif
> state (driver ops called stop()).
> 
> Can you add such a check as well? 

That is a good idea, I have added such a check: if the interface is
supposed to be down I'll warn and return because I don't think there is
much we can do in this situation besides avoiding trying to transmit
anything.

> And please keep in mind to increment
> the open count when implementing MLME-ops (or at least handle it
> somehow), otherwise I guess it's easy to hit the warning. If another
> user reports warnings and tells us what they did we might know more
> other "cases" to fix.

I don't think incrementing the open_count counter is the right solution
here just because the stop call is not supposed to fail and has no
straightforward ways to be deferred. In particular, just keeping the
open_count incremented will just avoid the actual driver stop operation
to be executed and the core will not notice it.

I came out with another solution: acquiring the rtnl when performing a
MLME Tx operation to serialize these operations. We can easily have a
version which just checks the rtnl was acquired as well for situations
when the MLME operations are called by eg. the nl layer (and thus, with
the rtnl lock taken automatically).

Thanks,
Miquèl

Re: [PATCH wpan-next 06/11] net: mac802154: Hold the transmit queue when relevant

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Wed, 11 May 2022 09:09:40 -0400:

> Hi,
> 
> On Tue, May 10, 2022 at 10:52 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alex,
> >  
> > > > --- a/net/mac802154/tx.c
> > > > +++ b/net/mac802154/tx.c
> > > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
> > > >         return NETDEV_TX_OK;
> > > >  }
> > > >
> > > > +void ieee802154_hold_queue(struct ieee802154_local *local)
> > > > +{
> > > > +       atomic_inc(&local->phy->hold_txs);
> > > > +}
> > > > +
> > > > +void ieee802154_release_queue(struct ieee802154_local *local)
> > > > +{
> > > > +       atomic_dec(&local->phy->hold_txs);
> > > > +}
> > > > +
> > > > +bool ieee802154_queue_is_held(struct ieee802154_local *local)
> > > > +{
> > > > +       return atomic_read(&local->phy->hold_txs);
> > > > +}  
> > >
> > > I am not getting this, should the release_queue() function not do
> > > something like:
> > >
> > > if (atomic_dec_and_test(hold_txs))
> > >       ieee802154_wake_queue(local);
> > >
> > > I think we don't need the test of "ieee802154_queue_is_held()" here,
> > > then we need to replace all stop_queue/wake_queue with hold and
> > > release?  
> >
> > That's actually a good idea. I've implemented it and it looks nice too.
> > I'll clean this up and share a new version with:
> > - The wake call checked everytime hold_txs gets decremented
> > - The removal of the _queue_is_held() helper
> > - _wake/stop_queue() turned static
> > - _hold/release_queue() used everywhere
> >  
> 
> I think there is also a lock necessary for atomic inc/dec hitting zero
> and the stop/wake call afterwards...

Mmmh that is true, it can race. I've introduced a mutex (I think it's
safe but it can be turned into a spinlock if proven necessary) to
secure these increment/decrement+wakeup operations.

> ,there are also a lot of
> optimization techniques to only hold the lock for hitting zero cases
> in such areas. However we will see...

I am not aware of technical solutions to avoid the locking in these
cases, what do you have in mind? Otherwise I propose just to come up
with a working and hopefully solid solution and then we'll see how to
optimize.

Thanks,
Miquèl

Re: [PATCH wpan-next 06/11] net: mac802154: Hold the transmit queue when relevant

[Alexander Aring]

Hi,

On Thu, May 12, 2022 at 10:33 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Wed, 11 May 2022 09:09:40 -0400:
>
> > Hi,
> >
> > On Tue, May 10, 2022 at 10:52 AM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alex,
> > >
> > > > > --- a/net/mac802154/tx.c
> > > > > +++ b/net/mac802154/tx.c
> > > > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
> > > > >         return NETDEV_TX_OK;
> > > > >  }
> > > > >
> > > > > +void ieee802154_hold_queue(struct ieee802154_local *local)
> > > > > +{
> > > > > +       atomic_inc(&local->phy->hold_txs);
> > > > > +}
> > > > > +
> > > > > +void ieee802154_release_queue(struct ieee802154_local *local)
> > > > > +{
> > > > > +       atomic_dec(&local->phy->hold_txs);
> > > > > +}
> > > > > +
> > > > > +bool ieee802154_queue_is_held(struct ieee802154_local *local)
> > > > > +{
> > > > > +       return atomic_read(&local->phy->hold_txs);
> > > > > +}
> > > >
> > > > I am not getting this, should the release_queue() function not do
> > > > something like:
> > > >
> > > > if (atomic_dec_and_test(hold_txs))
> > > >       ieee802154_wake_queue(local);
> > > >
> > > > I think we don't need the test of "ieee802154_queue_is_held()" here,
> > > > then we need to replace all stop_queue/wake_queue with hold and
> > > > release?
> > >
> > > That's actually a good idea. I've implemented it and it looks nice too.
> > > I'll clean this up and share a new version with:
> > > - The wake call checked everytime hold_txs gets decremented
> > > - The removal of the _queue_is_held() helper
> > > - _wake/stop_queue() turned static
> > > - _hold/release_queue() used everywhere
> > >
> >
> > I think there is also a lock necessary for atomic inc/dec hitting zero
> > and the stop/wake call afterwards...
>
> Mmmh that is true, it can race. I've introduced a mutex (I think it's
> safe but it can be turned into a spinlock if proven necessary) to
> secure these increment/decrement+wakeup operations.
>

be aware that you might call these functions from different contexts,
test your patches with PROVE_LOCKING enabled.

> > ,there are also a lot of
> > optimization techniques to only hold the lock for hitting zero cases
> > in such areas. However we will see...
>
> I am not aware of technical solutions to avoid the locking in these
> cases, what do you have in mind? Otherwise I propose just to come up
> with a working and hopefully solid solution and then we'll see how to
> optimize.

Yes, it's not so important...

- Alex

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

[Alexander Aring]

Hi,

On Thu, May 12, 2022 at 10:33 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@gmail.com wrote on Sun, 1 May 2022 20:21:18 -0400:
>
> > Hi,
> >
> > On Thu, Apr 28, 2022 at 3:58 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > alex.aring@gmail.com wrote on Wed, 27 Apr 2022 14:01:25 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Wed, Apr 27, 2022 at 12:47 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.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > ---
> > >
> > > [...]
> > >
> > > > > 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.
> > >
> >
> > I am thinking about this patch series... and I think it still has bugs
> > or at least it's easy to have bugs when the context is not right
> > prepared to call a synchronized transmission. We leave here the netdev
> > state machine world for transmit vs e.g. start/stop netif callback...
> > We have a warning here if there is a core netif xmit callback running
> > when 802.15.4 thinks it shouldn't (because we take control of it) but
> > I also think about a kind of the other way around. A warning if
> > 802.15.4 transmits something but the netdev core logic "thinks" it
> > shouldn't.
> >
> > That requires some checks (probably from netcore functionality) to
> > check if we call a 802.15.4 sync xmit but netif core already called
> > stop() callback. The last stop() callback - means the driver_ops
> > stop() callback was called, we have some "open_count" counter there
> > which MUST be incremented before doing any looping of one or several
> > sync transmissions. All I can say is if we call xmit() but the driver
> > is in stop() state... it will break things.
> >
> > My concern is also here that e.g. calling netif down or device
> > suspend() are only two examples I have in my mind right now. I don't
> > know all cases which can occur, that's why we should introduce another
> > WARN_ON_ONCE() for the case that 802.15.4 transmits something but we
> > are in a state where we can't transmit something according to netif
> > state (driver ops called stop()).
> >
> > Can you add such a check as well?
>
> That is a good idea, I have added such a check: if the interface is
> supposed to be down I'll warn and return because I don't think there is
> much we can do in this situation besides avoiding trying to transmit
> anything.
>

ok...

> > And please keep in mind to increment
> > the open count when implementing MLME-ops (or at least handle it
> > somehow), otherwise I guess it's easy to hit the warning. If another
> > user reports warnings and tells us what they did we might know more
> > other "cases" to fix.
>
> I don't think incrementing the open_count counter is the right solution
> here just because the stop call is not supposed to fail and has no
> straightforward ways to be deferred. In particular, just keeping the
> open_count incremented will just avoid the actual driver stop operation
> to be executed and the core will not notice it.
>

the stop callback can sleep, it's the job of the driver to synchronize
it somehow with the transceiver state.

> I came out with another solution: acquiring the rtnl when performing a
> MLME Tx operation to serialize these operations. We can easily have a
> version which just checks the rtnl was acquired as well for situations
> when the MLME operations are called by eg. the nl layer (and thus, with
> the rtnl lock taken automatically).

The rtnl lock needs definitely to be held during such operation.

- Alex

Re: [PATCH wpan-next 06/11] net: mac802154: Hold the transmit queue when relevant

[Miquel Raynal]

Hi Alex,

aahringo@redhat.com wrote on Thu, 12 May 2022 10:44:35 -0400:

> Hi,
> 
> On Thu, May 12, 2022 at 10:33 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Wed, 11 May 2022 09:09:40 -0400:
> >  
> > > Hi,
> > >
> > > On Tue, May 10, 2022 at 10:52 AM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Hi Alex,
> > > >  
> > > > > > --- a/net/mac802154/tx.c
> > > > > > +++ b/net/mac802154/tx.c
> > > > > > @@ -106,6 +106,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
> > > > > >         return NETDEV_TX_OK;
> > > > > >  }
> > > > > >
> > > > > > +void ieee802154_hold_queue(struct ieee802154_local *local)
> > > > > > +{
> > > > > > +       atomic_inc(&local->phy->hold_txs);
> > > > > > +}
> > > > > > +
> > > > > > +void ieee802154_release_queue(struct ieee802154_local *local)
> > > > > > +{
> > > > > > +       atomic_dec(&local->phy->hold_txs);
> > > > > > +}
> > > > > > +
> > > > > > +bool ieee802154_queue_is_held(struct ieee802154_local *local)
> > > > > > +{
> > > > > > +       return atomic_read(&local->phy->hold_txs);
> > > > > > +}  
> > > > >
> > > > > I am not getting this, should the release_queue() function not do
> > > > > something like:
> > > > >
> > > > > if (atomic_dec_and_test(hold_txs))
> > > > >       ieee802154_wake_queue(local);
> > > > >
> > > > > I think we don't need the test of "ieee802154_queue_is_held()" here,
> > > > > then we need to replace all stop_queue/wake_queue with hold and
> > > > > release?  
> > > >
> > > > That's actually a good idea. I've implemented it and it looks nice too.
> > > > I'll clean this up and share a new version with:
> > > > - The wake call checked everytime hold_txs gets decremented
> > > > - The removal of the _queue_is_held() helper
> > > > - _wake/stop_queue() turned static
> > > > - _hold/release_queue() used everywhere
> > > >  
> > >
> > > I think there is also a lock necessary for atomic inc/dec hitting zero
> > > and the stop/wake call afterwards...  
> >
> > Mmmh that is true, it can race. I've introduced a mutex (I think it's
> > safe but it can be turned into a spinlock if proven necessary) to
> > secure these increment/decrement+wakeup operations.
> >  
> 
> be aware that you might call these functions from different contexts,
> test your patches with PROVE_LOCKING enabled.

Right, I've added it to my .config, let's see what it tells me.

> > > ,there are also a lot of
> > > optimization techniques to only hold the lock for hitting zero cases
> > > in such areas. However we will see...  
> >
> > I am not aware of technical solutions to avoid the locking in these
> > cases, what do you have in mind? Otherwise I propose just to come up
> > with a working and hopefully solid solution and then we'll see how to
> > optimize.  
> 
> Yes, it's not so important...
> 
> - Alex
> 

Thanks,
Miquèl