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

[PATCH wpan-next v2 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

Changes in v2:
* Updated the main tx function error path.
* Added a missing atomic_dec_at_test() call on the hold counter.
* Always called (upon a certain condition) the queue wakeup helper from
  the release queue helper (and similarly in the hold helper) and
  squashed two existing patches in it to simplify the series.
* Introduced a mutex to serialize accesses to the increment/decrement of
  the hold counter and the wake up call.
* Added a warning in case an MLME Tx gets triggered while the device was
  stopped.
* Used the rtnl to ensure the device cannot be stopped while an MLME
  transmission is ongoing.

Changes in v1 since this series got extracted from a bigger change:
* Introduced a new atomic variable to know when the queue is actually
  stopped. So far we only had an atomic to know when the queue was held
  (indicates a transitioning state towards a stopped queue only) and
  another atomic indicating if a transfer was still ongoing at this
  point (used by the wait logic as a condition to wake up).


Miquel Raynal (11):
  net: mac802154: Rename the synchronous xmit worker
  net: mac802154: Rename the main tx_work struct
  net: mac802154: Enhance the error path in the main tx helper
  net: mac802154: Follow the count of ongoing transmissions
  net: mac802154: Bring the hability to hold the transmit queue
  net: mac802154: Create a hot tx 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
  net: mac802154: Add a warning in the hot path
  net: mac802154: Add a warning in the slow path

 include/net/cfg802154.h      |   7 ++
 include/net/mac802154.h      |  27 --------
 net/ieee802154/core.c        |   3 +
 net/mac802154/cfg.c          |   4 +-
 net/mac802154/ieee802154_i.h |  37 +++++++++-
 net/mac802154/main.c         |   2 +-
 net/mac802154/tx.c           | 126 +++++++++++++++++++++++++++++++----
 net/mac802154/util.c         |  67 +++++++++++++++++--
 8 files changed, 220 insertions(+), 53 deletions(-)

-- 
2.27.0

[PATCH wpan-next v2 01/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 1381e6a5e180..d7632c6d225f 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 bd7bdb1219dd..392771bba9dd 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 c829e4a75325..97df5985b830 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 v2 02/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 d7632c6d225f..a8b7b9049f14 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 392771bba9dd..40fab08df24b 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 97df5985b830..a01689ddd547 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->hw);
 
-	/* 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 v2 03/11] net: mac802154: Enhance the error path in the main tx helper

[Miquel Raynal]

Before adding more logic in the error path, let's move the wake queue
call there, rename the default label and create an additional one.

There is no functional change.

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

diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index a01689ddd547..4a46ce8d2ac8 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -65,7 +65,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 				consume_skb(skb);
 				skb = nskb;
 			} else {
-				goto err_tx;
+				goto err_free_skb;
 			}
 		}
 
@@ -84,10 +84,8 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 		unsigned int len = skb->len;
 
 		ret = drv_xmit_async(local, skb);
-		if (ret) {
-			ieee802154_wake_queue(&local->hw);
-			goto err_tx;
-		}
+		if (ret)
+			goto err_wake_netif_queue;
 
 		dev->stats.tx_packets++;
 		dev->stats.tx_bytes += len;
@@ -98,7 +96,9 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 
 	return NETDEV_TX_OK;
 
-err_tx:
+err_wake_netif_queue:
+	ieee802154_wake_queue(&local->hw);
+err_free_skb:
 	kfree_skb(skb);
 	return NETDEV_TX_OK;
 }
-- 
2.27.0

[PATCH wpan-next v2 04/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 4a46ce8d2ac8..33f64ecd96c7 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -44,6 +44,7 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
 err_tx:
 	/* Restart the netif queue on each sub_if_data object. */
 	ieee802154_wake_queue(&local->hw);
+	atomic_dec(&local->phy->ongoing_txs);
 	kfree_skb(skb);
 	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->hw);
+	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
@@ -98,6 +100,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 
 err_wake_netif_queue:
 	ieee802154_wake_queue(&local->hw);
+	atomic_dec(&local->phy->ongoing_txs);
 err_free_skb:
 	kfree_skb(skb);
 	return NETDEV_TX_OK;
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index 9f024d85563b..76dc663e2af4 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -88,6 +88,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);
 
@@ -99,6 +100,7 @@ void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb,
 	local->tx_result = reason;
 	ieee802154_wake_queue(hw);
 	dev_kfree_skb_any(skb);
+	atomic_dec(&hw->phy->ongoing_txs);
 }
 EXPORT_SYMBOL(ieee802154_xmit_error);
 
-- 
2.27.0

[PATCH wpan-next v2 05/11] net: mac802154: Bring the hability to hold the transmit queue

[Miquel Raynal]

Create a hold_txs atomic variable and increment/decrement it when
relevant, ie. when we want to hold the queue or release it: currently
all the "stopped" situations are suitable, but very soon we will more
extensively use this feature for MLME purposes.

Upon release, the atomic counter is decremented and checked. If it is
back to 0, then the netif queue gets woken up. This makes the whole
process fully transparent, provided that all the users of
ieee802154_wake/stop_queue() now call ieee802154_hold/release_queue()
instead.

In no situation individual drivers should call any of these helpers
manually in order to avoid messing with the counters. There are other
functions more suited for this purpose which have been introduced, such
as the _xmit_complete() and _xmit_error() helpers which will handle all
that for them.

One advantage is that, as no more drivers call the stop/wake helpers
directly, we can safely stop exporting them and only declare the
hold/release ones in a header only accessible to the core.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h      |  4 ++-
 include/net/mac802154.h      | 27 --------------------
 net/ieee802154/core.c        |  2 ++
 net/mac802154/cfg.c          |  4 +--
 net/mac802154/ieee802154_i.h | 19 ++++++++++++++
 net/mac802154/tx.c           |  6 ++---
 net/mac802154/util.c         | 48 ++++++++++++++++++++++++++++++------
 7 files changed, 70 insertions(+), 40 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 473ebcb9b155..ad3f438e4583 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -214,8 +214,10 @@ struct wpan_phy {
 	/* the network namespace this phy lives in currently */
 	possible_net_t _net;
 
-	/* Transmission monitoring */
+	/* Transmission monitoring and control */
+	struct mutex queue_lock;
 	atomic_t ongoing_txs;
+	atomic_t hold_txs;
 
 	char priv[] __aligned(NETDEV_ALIGN);
 };
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/ieee802154/core.c b/net/ieee802154/core.c
index de259b5170ab..d81b7301e013 100644
--- a/net/ieee802154/core.c
+++ b/net/ieee802154/core.c
@@ -130,6 +130,8 @@ wpan_phy_new(const struct cfg802154_ops *ops, size_t priv_size)
 
 	init_waitqueue_head(&rdev->dev_wait);
 
+	mutex_init(&rdev->wpan_phy.queue_lock);
+
 	return &rdev->wpan_phy;
 }
 EXPORT_SYMBOL(wpan_phy_new);
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index 1e4a9f74ed43..b51100fd9e3f 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_hold_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_release_queue(local);
 	local->suspended = false;
 	return 0;
 }
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index a8b7b9049f14..0c7ff9e0b632 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -130,6 +130,25 @@ 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_hold_queue - hold ieee802154 queue
+ * @local: main mac object
+ *
+ * Hold a queue by incrementing an atomic counter and requesting the netif
+ * queues to be stopped. The queues cannot be woken up while the counter has not
+ * been reset with as any ieee802154_release_queue() calls as needed.
+ */
+void ieee802154_hold_queue(struct ieee802154_local *local);
+
+/**
+ * ieee802154_release_queue - release ieee802154 queue
+ * @local: main mac object
+ *
+ * Release a queue which is held by decrementing an atomic counter and wake it
+ * up only if the counter reaches 0.
+ */
+void ieee802154_release_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 33f64ecd96c7..6a53c83cf039 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -43,7 +43,7 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
 
 err_tx:
 	/* Restart the netif queue on each sub_if_data object. */
-	ieee802154_wake_queue(&local->hw);
+	ieee802154_release_queue(local);
 	atomic_dec(&local->phy->ongoing_txs);
 	kfree_skb(skb);
 	netdev_dbg(dev, "transmission failed\n");
@@ -75,7 +75,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_hold_queue(local);
 	atomic_inc(&local->phy->ongoing_txs);
 
 	/* Drivers should preferably implement the async callback. In some rare
@@ -99,7 +99,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 	return NETDEV_TX_OK;
 
 err_wake_netif_queue:
-	ieee802154_wake_queue(&local->hw);
+	ieee802154_release_queue(local);
 	atomic_dec(&local->phy->ongoing_txs);
 err_free_skb:
 	kfree_skb(skb);
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index 76dc663e2af4..b629c94cfd1b 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -13,7 +13,17 @@
 /* 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)
+/**
+ * ieee802154_wake_queue - wake ieee802154 queue
+ * @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
+ * 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.
+ */
+static void ieee802154_wake_queue(struct ieee802154_hw *hw)
 {
 	struct ieee802154_local *local = hw_to_local(hw);
 	struct ieee802154_sub_if_data *sdata;
@@ -27,9 +37,18 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL(ieee802154_wake_queue);
 
-void ieee802154_stop_queue(struct ieee802154_hw *hw)
+/**
+ * ieee802154_stop_queue - stop ieee802154 queue
+ * @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
+ * 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.
+ */
+static void ieee802154_stop_queue(struct ieee802154_hw *hw)
 {
 	struct ieee802154_local *local = hw_to_local(hw);
 	struct ieee802154_sub_if_data *sdata;
@@ -43,14 +62,29 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL(ieee802154_stop_queue);
+
+void ieee802154_hold_queue(struct ieee802154_local *local)
+{
+	mutex_lock(&local->phy->queue_lock);
+	ieee802154_stop_queue(&local->hw);
+	atomic_inc(&local->phy->hold_txs);
+	mutex_unlock(&local->phy->queue_lock);
+}
+
+void ieee802154_release_queue(struct ieee802154_local *local)
+{
+	mutex_lock(&local->phy->queue_lock);
+	if (!atomic_dec_and_test(&local->phy->hold_txs))
+		ieee802154_wake_queue(&local->hw);
+	mutex_unlock(&local->phy->queue_lock);
+}
 
 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_release_queue(local);
 
 	return HRTIMER_NORESTART;
 }
@@ -84,7 +118,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_release_queue(local);
 	}
 
 	dev_consume_skb_any(skb);
@@ -98,7 +132,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_release_queue(local);
 	dev_kfree_skb_any(skb);
 	atomic_dec(&hw->phy->ongoing_txs);
 }
-- 
2.27.0

[PATCH wpan-next v2 06/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 6a53c83cf039..607019b8f8ab 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -106,6 +106,12 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 	return NETDEV_TX_OK;
 }
 
+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)
 {
@@ -113,7 +119,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
@@ -135,5 +141,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 v2 07/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         | 14 ++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 0c7ff9e0b632..e34db1d49ef4 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -149,6 +149,18 @@ void ieee802154_hold_queue(struct ieee802154_local *local);
  */
 void ieee802154_release_queue(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 b629c94cfd1b..31b53b3165ec 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -79,6 +79,20 @@ void ieee802154_release_queue(struct ieee802154_local *local)
 	mutex_unlock(&local->phy->queue_lock);
 }
 
+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();
+}
+
 enum hrtimer_restart ieee802154_xmit_ifs_timer(struct hrtimer *timer)
 {
 	struct ieee802154_local *local =
-- 
2.27.0

[PATCH wpan-next v2 08/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          |  2 +-
 net/mac802154/ieee802154_i.h |  1 +
 net/mac802154/tx.c           | 26 ++++++++++++++++++++++++--
 net/mac802154/util.c         |  6 ++++--
 6 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index ad3f438e4583..8b6326aa2d42 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;
+	wait_queue_head_t sync_txq;
 
 	char priv[] __aligned(NETDEV_ALIGN);
 };
diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
index d81b7301e013..f13e3082d988 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);
 
 	mutex_init(&rdev->wpan_phy.queue_lock);
 
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index b51100fd9e3f..93df24f75572 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_hold_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 e34db1d49ef4..a057827fc48a 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 607019b8f8ab..38f74b8b6740 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -44,7 +44,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
 err_tx:
 	/* Restart the netif queue on each sub_if_data object. */
 	ieee802154_release_queue(local);
-	atomic_dec(&local->phy->ongoing_txs);
+	if (!atomic_dec_and_test(&local->phy->ongoing_txs))
+		wake_up(&local->phy->sync_txq);
 	kfree_skb(skb);
 	netdev_dbg(dev, "transmission failed\n");
 }
@@ -100,12 +101,33 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 
 err_wake_netif_queue:
 	ieee802154_release_queue(local);
-	atomic_dec(&local->phy->ongoing_txs);
+	if (!atomic_dec_and_test(&local->phy->ongoing_txs))
+		wake_up(&local->phy->sync_txq);
 err_free_skb:
 	kfree_skb(skb);
 	return NETDEV_TX_OK;
 }
 
+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);
+
+	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 31b53b3165ec..65a9127a41ea 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -136,7 +136,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);
 
@@ -148,7 +149,8 @@ void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb,
 	local->tx_result = reason;
 	ieee802154_release_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 v2 09/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           | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -128,6 +128,31 @@ 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)
+{
+	int ret;
+
+	/* Avoid possible calls to ->ndo_stop() when we asynchronously perform
+	 * MLME transmissions.
+	 */
+	rtnl_lock();
+
+	/* Ensure the device was not stopped, otherwise error out */
+	if (!local->open_count)
+		return -EBUSY;
+
+	ieee802154_sync_and_hold_queue(local);
+
+	ieee802154_tx(local, skb);
+	ret = ieee802154_sync_queue(local);
+
+	ieee802154_release_queue(local);
+
+	rtnl_unlock();
+
+	return ret;
+}
+
 static netdev_tx_t
 ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
 {
-- 
2.27.0

[PATCH wpan-next v2 10/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.

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

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

[Miquel Raynal]

In order to be able to detect possible conflicts between the net
interface core and the ieee802154 core, let's add a warning in the slow
path: we want to be sure that whenever we start an asynchronous MLME
transmission (which can be fully asynchronous) the net core somehow
agrees that this transmission is possible, ie. the device was not
stopped. Warning in this case would allow us to track down more easily
possible issues with the MLME logic if we ever get reports.

Unlike in the hot path, such a situation cannot be handled.

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

diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index a3c9f194c025..d61b076239c3 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -132,6 +132,25 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
 	return ret;
 }
 
+static bool ieee802154_netif_is_down(struct ieee802154_local *local)
+{
+	struct ieee802154_sub_if_data *sdata;
+	bool is_down = false;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!sdata->dev)
+			continue;
+
+		is_down = !(sdata->dev->flags & IFF_UP);
+		if (is_down)
+			break;
+	}
+	rcu_read_unlock();
+
+	return is_down;
+}
+
 int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb)
 {
 	int ret;
@@ -145,6 +164,12 @@ int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb)
 	if (!local->open_count)
 		return -EBUSY;
 
+	/* Warn if the ieee802154 core thinks MLME frames can be sent while the
+	 * net interface expects this cannot happen.
+	 */
+	if (WARN_ON_ONCE(ieee802154_netif_is_down(local)))
+		return -EHOSTDOWN;
+
 	ieee802154_sync_and_hold_queue(local);
 
 	ieee802154_tx(local, skb);
-- 
2.27.0

Re: [PATCH wpan-next v2 05/11] net: mac802154: Bring the hability to hold the transmit queue

[Alexander Aring]

Hi,

On Thu, May 12, 2022 at 10:33 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Create a hold_txs atomic variable and increment/decrement it when
> relevant, ie. when we want to hold the queue or release it: currently
> all the "stopped" situations are suitable, but very soon we will more
> extensively use this feature for MLME purposes.
>
> Upon release, the atomic counter is decremented and checked. If it is
> back to 0, then the netif queue gets woken up. This makes the whole
> process fully transparent, provided that all the users of
> ieee802154_wake/stop_queue() now call ieee802154_hold/release_queue()
> instead.
>
> In no situation individual drivers should call any of these helpers
> manually in order to avoid messing with the counters. There are other
> functions more suited for this purpose which have been introduced, such
> as the _xmit_complete() and _xmit_error() helpers which will handle all
> that for them.
>
> One advantage is that, as no more drivers call the stop/wake helpers
> directly, we can safely stop exporting them and only declare the
> hold/release ones in a header only accessible to the core.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/net/cfg802154.h      |  4 ++-
>  include/net/mac802154.h      | 27 --------------------
>  net/ieee802154/core.c        |  2 ++
>  net/mac802154/cfg.c          |  4 +--
>  net/mac802154/ieee802154_i.h | 19 ++++++++++++++
>  net/mac802154/tx.c           |  6 ++---
>  net/mac802154/util.c         | 48 ++++++++++++++++++++++++++++++------
>  7 files changed, 70 insertions(+), 40 deletions(-)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 473ebcb9b155..ad3f438e4583 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -214,8 +214,10 @@ struct wpan_phy {
>         /* the network namespace this phy lives in currently */
>         possible_net_t _net;
>
> -       /* Transmission monitoring */
> +       /* Transmission monitoring and control */
> +       struct mutex queue_lock;
>         atomic_t ongoing_txs;
> +       atomic_t hold_txs;
>
>         char priv[] __aligned(NETDEV_ALIGN);
>  };
> 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/ieee802154/core.c b/net/ieee802154/core.c
> index de259b5170ab..d81b7301e013 100644
> --- a/net/ieee802154/core.c
> +++ b/net/ieee802154/core.c
> @@ -130,6 +130,8 @@ wpan_phy_new(const struct cfg802154_ops *ops, size_t priv_size)
>
>         init_waitqueue_head(&rdev->dev_wait);
>
> +       mutex_init(&rdev->wpan_phy.queue_lock);
> +
>         return &rdev->wpan_phy;
>  }
>  EXPORT_SYMBOL(wpan_phy_new);
> diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> index 1e4a9f74ed43..b51100fd9e3f 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_hold_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_release_queue(local);
>         local->suspended = false;
>         return 0;
>  }
> diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> index a8b7b9049f14..0c7ff9e0b632 100644
> --- a/net/mac802154/ieee802154_i.h
> +++ b/net/mac802154/ieee802154_i.h
> @@ -130,6 +130,25 @@ 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_hold_queue - hold ieee802154 queue
> + * @local: main mac object
> + *
> + * Hold a queue by incrementing an atomic counter and requesting the netif
> + * queues to be stopped. The queues cannot be woken up while the counter has not
> + * been reset with as any ieee802154_release_queue() calls as needed.
> + */
> +void ieee802154_hold_queue(struct ieee802154_local *local);
> +
> +/**
> + * ieee802154_release_queue - release ieee802154 queue
> + * @local: main mac object
> + *
> + * Release a queue which is held by decrementing an atomic counter and wake it
> + * up only if the counter reaches 0.
> + */
> +void ieee802154_release_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 33f64ecd96c7..6a53c83cf039 100644
> --- a/net/mac802154/tx.c
> +++ b/net/mac802154/tx.c
> @@ -43,7 +43,7 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
>
>  err_tx:
>         /* Restart the netif queue on each sub_if_data object. */
> -       ieee802154_wake_queue(&local->hw);
> +       ieee802154_release_queue(local);
>         atomic_dec(&local->phy->ongoing_txs);
>         kfree_skb(skb);
>         netdev_dbg(dev, "transmission failed\n");
> @@ -75,7 +75,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_hold_queue(local);
>         atomic_inc(&local->phy->ongoing_txs);
>
>         /* Drivers should preferably implement the async callback. In some rare
> @@ -99,7 +99,7 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
>         return NETDEV_TX_OK;
>
>  err_wake_netif_queue:
> -       ieee802154_wake_queue(&local->hw);
> +       ieee802154_release_queue(local);
>         atomic_dec(&local->phy->ongoing_txs);
>  err_free_skb:
>         kfree_skb(skb);
> diff --git a/net/mac802154/util.c b/net/mac802154/util.c
> index 76dc663e2af4..b629c94cfd1b 100644
> --- a/net/mac802154/util.c
> +++ b/net/mac802154/util.c
> @@ -13,7 +13,17 @@
>  /* 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)
> +/**
> + * ieee802154_wake_queue - wake ieee802154 queue
> + * @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
> + * 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.
> + */
> +static void ieee802154_wake_queue(struct ieee802154_hw *hw)
>  {
>         struct ieee802154_local *local = hw_to_local(hw);
>         struct ieee802154_sub_if_data *sdata;
> @@ -27,9 +37,18 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw)
>         }
>         rcu_read_unlock();
>  }
> -EXPORT_SYMBOL(ieee802154_wake_queue);
>
> -void ieee802154_stop_queue(struct ieee802154_hw *hw)
> +/**
> + * ieee802154_stop_queue - stop ieee802154 queue
> + * @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
> + * 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.
> + */
> +static void ieee802154_stop_queue(struct ieee802154_hw *hw)
>  {
>         struct ieee802154_local *local = hw_to_local(hw);
>         struct ieee802154_sub_if_data *sdata;
> @@ -43,14 +62,29 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw)
>         }
>         rcu_read_unlock();
>  }
> -EXPORT_SYMBOL(ieee802154_stop_queue);
> +
> +void ieee802154_hold_queue(struct ieee802154_local *local)
> +{
> +       mutex_lock(&local->phy->queue_lock);
> +       ieee802154_stop_queue(&local->hw);
> +       atomic_inc(&local->phy->hold_txs);
> +       mutex_unlock(&local->phy->queue_lock);
> +}
> +
> +void ieee802154_release_queue(struct ieee802154_local *local)
> +{
> +       mutex_lock(&local->phy->queue_lock);
> +       if (!atomic_dec_and_test(&local->phy->hold_txs))
> +               ieee802154_wake_queue(&local->hw);
> +       mutex_unlock(&local->phy->queue_lock);
> +}
>
>  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_release_queue(local);
>
>         return HRTIMER_NORESTART;
>  }
> @@ -84,7 +118,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_release_queue(local);
>         }
>
>         dev_consume_skb_any(skb);
> @@ -98,7 +132,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_release_queue(local);
>         dev_kfree_skb_any(skb);
>         atomic_dec(&hw->phy->ongoing_txs);

I am pretty sure that will end in a scheduling while atomic warning
with hwsim. If you don't hit it you have the wrong config, you need to
enable such warnings and have the right preemption model setting.
These calls xmit complete/error should even be allowed to be called
from a hardware irq context, however I _think_ we don't have a driver
which currently does that, but the mutex will break stuff here in the
xmit_do() callback of netdev which hwsim is calling it from.

Please check again...

- Alex

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

[Alexander Aring]

Hi,

On Thu, May 12, 2022 at 10:34 AM 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          |  2 +-
>  net/mac802154/ieee802154_i.h |  1 +
>  net/mac802154/tx.c           | 26 ++++++++++++++++++++++++--
>  net/mac802154/util.c         |  6 ++++--
>  6 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index ad3f438e4583..8b6326aa2d42 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;
> +       wait_queue_head_t sync_txq;
>
>         char priv[] __aligned(NETDEV_ALIGN);
>  };
> diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
> index d81b7301e013..f13e3082d988 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);
>
>         mutex_init(&rdev->wpan_phy.queue_lock);
>
> diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> index b51100fd9e3f..93df24f75572 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_hold_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 e34db1d49ef4..a057827fc48a 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 607019b8f8ab..38f74b8b6740 100644
> --- a/net/mac802154/tx.c
> +++ b/net/mac802154/tx.c
> @@ -44,7 +44,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
>  err_tx:
>         /* Restart the netif queue on each sub_if_data object. */
>         ieee802154_release_queue(local);
> -       atomic_dec(&local->phy->ongoing_txs);
> +       if (!atomic_dec_and_test(&local->phy->ongoing_txs))
> +               wake_up(&local->phy->sync_txq);
>         kfree_skb(skb);
>         netdev_dbg(dev, "transmission failed\n");
>  }
> @@ -100,12 +101,33 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
>
>  err_wake_netif_queue:
>         ieee802154_release_queue(local);
> -       atomic_dec(&local->phy->ongoing_txs);
> +       if (!atomic_dec_and_test(&local->phy->ongoing_txs))
> +               wake_up(&local->phy->sync_txq);
>  err_free_skb:
>         kfree_skb(skb);
>         return NETDEV_TX_OK;
>  }
>
> +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);

I am curious, why this extra hold, release here?

- Alex

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

[Alexander Aring]

Hi,

On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> 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           | 25 +++++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> --- a/net/mac802154/tx.c
> +++ b/net/mac802154/tx.c
> @@ -128,6 +128,31 @@ 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)
> +{
> +       int ret;
> +
> +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> +        * MLME transmissions.
> +        */
> +       rtnl_lock();

I think we should make an ASSERT_RTNL() here, the lock needs to be
earlier than that over the whole MLME op. MLME can trigger more than
one message, the whole sync_hold/release queue should be earlier than
that... in my opinion is it not right to allow other messages so far
an MLME op is going on? I am not sure what the standard says to this,
but I think it should be stopped the whole time? All those sequence
diagrams show only some specific frames, also remember that on the
receive side we drop all other frames if MLME op (e.g. scan) is going
on?

- Alex

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

[Alexander Aring]

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

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

[Alexander Aring]

Hi,

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

Is there not a helper for this flag?

- Alex

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

[Alexander Aring]

Hi,

On Sun, May 15, 2022 at 6:28 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > 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           | 25 +++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -128,6 +128,31 @@ 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)
> > +{
> > +       int ret;
> > +
> > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > +        * MLME transmissions.
> > +        */
> > +       rtnl_lock();
>
> I think we should make an ASSERT_RTNL() here, the lock needs to be
> earlier than that over the whole MLME op. MLME can trigger more than
> one message, the whole sync_hold/release queue should be earlier than
> that... in my opinion is it not right to allow other messages so far
> an MLME op is going on? I am not sure what the standard says to this,
> but I think it should be stopped the whole time? All those sequence
> diagrams show only some specific frames, also remember that on the
> receive side we drop all other frames if MLME op (e.g. scan) is going
> on?

Maybe some mlme_op_pre(), ... mlme_tx(), ..., mlme_tx(), ...,
mlme_op_post() handling?

- Alex

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

[Alexander Aring]

Hi,

On Sun, May 15, 2022 at 6:28 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > 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           | 25 +++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -128,6 +128,31 @@ 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)
> > +{
> > +       int ret;
> > +
> > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > +        * MLME transmissions.
> > +        */
> > +       rtnl_lock();
>
> I think we should make an ASSERT_RTNL() here, the lock needs to be
> earlier than that over the whole MLME op. MLME can trigger more than

not over the whole MLME_op, that's terrible to hold the rtnl lock so
long... so I think this is fine that some netdev call will interfere
with this transmission.
So forget about the ASSERT_RTNL() here, it's fine (I hope).

> one message, the whole sync_hold/release queue should be earlier than
> that... in my opinion is it not right to allow other messages so far
> an MLME op is going on? I am not sure what the standard says to this,
> but I think it should be stopped the whole time? All those sequence

Whereas the stop of the netdev queue makes sense for the whole mlme-op
(in my opinion).

- Alex

Re: [PATCH wpan-next v2 05/11] net: mac802154: Bring the hability to hold the transmit queue

[Miquel Raynal]

Hi Alex,

> > @@ -84,7 +118,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_release_queue(local);
> >         }
> >
> >         dev_consume_skb_any(skb);
> > @@ -98,7 +132,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_release_queue(local);
> >         dev_kfree_skb_any(skb);
> >         atomic_dec(&hw->phy->ongoing_txs);  
> 
> I am pretty sure that will end in a scheduling while atomic warning
> with hwsim. If you don't hit it you have the wrong config, you need to
> enable such warnings and have the right preemption model setting.

I was using the "desktop" kernel preemption model (voluntary), I've
switched to CONFIG_PREEMPT ("Preemptible kernel (Low-latency)"),
and enabled CONFIG_DEBUG_ATOMIC_SLEEP. You are right that we should use
a spinlock instead of a mutex here. However I don't think disabling
IRQs is necessary, so I'll switch to spin_(un)lock() calls.

> These calls xmit complete/error should even be allowed to be called
> from a hardware irq context, however I _think_ we don't have a driver
> which currently does that, but the mutex will break stuff here in the
> xmit_do() callback of netdev which hwsim is calling it from.
> 
> Please check again...

Will perform a new set of test runs with the new configuration, yes.

Thanks,
Miquèl

Re: [PATCH wpan-next v2 05/11] net: mac802154: Bring the hability to hold the transmit queue

[Alexander Aring]

Hi,

On Tue, May 17, 2022 at 5:28 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alex,
>
> > > @@ -84,7 +118,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_release_queue(local);
> > >         }
> > >
> > >         dev_consume_skb_any(skb);
> > > @@ -98,7 +132,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_release_queue(local);
> > >         dev_kfree_skb_any(skb);
> > >         atomic_dec(&hw->phy->ongoing_txs);
> >
> > I am pretty sure that will end in a scheduling while atomic warning
> > with hwsim. If you don't hit it you have the wrong config, you need to
> > enable such warnings and have the right preemption model setting.
>
> I was using the "desktop" kernel preemption model (voluntary), I've
> switched to CONFIG_PREEMPT ("Preemptible kernel (Low-latency)"),
> and enabled CONFIG_DEBUG_ATOMIC_SLEEP. You are right that we should use
> a spinlock instead of a mutex here. However I don't think disabling
> IRQs is necessary, so I'll switch to spin_(un)lock() calls.
>

In my opinion it's necessary for the ifs hrtimer. Normal
spin_lock/unlock is not the right fit here.

- Alex

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

[Miquel Raynal]

Hi Alex,

aahringo@redhat.com wrote on Sun, 15 May 2022 18:23:04 -0400:

> Hi,
> 
> On Thu, May 12, 2022 at 10:34 AM 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          |  2 +-
> >  net/mac802154/ieee802154_i.h |  1 +
> >  net/mac802154/tx.c           | 26 ++++++++++++++++++++++++--
> >  net/mac802154/util.c         |  6 ++++--
> >  6 files changed, 32 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > index ad3f438e4583..8b6326aa2d42 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;
> > +       wait_queue_head_t sync_txq;
> >
> >         char priv[] __aligned(NETDEV_ALIGN);
> >  };
> > diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
> > index d81b7301e013..f13e3082d988 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);
> >
> >         mutex_init(&rdev->wpan_phy.queue_lock);
> >
> > diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> > index b51100fd9e3f..93df24f75572 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_hold_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 e34db1d49ef4..a057827fc48a 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 607019b8f8ab..38f74b8b6740 100644
> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -44,7 +44,8 @@ void ieee802154_xmit_sync_worker(struct work_struct *work)
> >  err_tx:
> >         /* Restart the netif queue on each sub_if_data object. */
> >         ieee802154_release_queue(local);
> > -       atomic_dec(&local->phy->ongoing_txs);
> > +       if (!atomic_dec_and_test(&local->phy->ongoing_txs))
> > +               wake_up(&local->phy->sync_txq);
> >         kfree_skb(skb);
> >         netdev_dbg(dev, "transmission failed\n");
> >  }
> > @@ -100,12 +101,33 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
> >
> >  err_wake_netif_queue:
> >         ieee802154_release_queue(local);
> > -       atomic_dec(&local->phy->ongoing_txs);
> > +       if (!atomic_dec_and_test(&local->phy->ongoing_txs))
> > +               wake_up(&local->phy->sync_txq);
> >  err_free_skb:
> >         kfree_skb(skb);
> >         return NETDEV_TX_OK;
> >  }
> >
> > +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);  
> 
> I am curious, why this extra hold, release here?

My idea was:
- stop the queue
- increment the hold counter to be sure the queue does not get
  restarted asynchronously
- wait for the last transmission to finish
- decrement the hold counter
- restart the queue if the hold counter is null

What is bothering you with it? Without the hold we cannot be sure that
an asynchronous event will not restart the queue and possibly fail our
logic.

Thanks,
Miquèl

Re: [PATCH wpan-next v2 05/11] net: mac802154: Bring the hability to hold the transmit queue

[Miquel Raynal]

aahringo@redhat.com wrote on Tue, 17 May 2022 09:19:29 -0400:

> Hi,
> 
> On Tue, May 17, 2022 at 5:28 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alex,
> >  
> > > > @@ -84,7 +118,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_release_queue(local);
> > > >         }
> > > >
> > > >         dev_consume_skb_any(skb);
> > > > @@ -98,7 +132,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_release_queue(local);
> > > >         dev_kfree_skb_any(skb);
> > > >         atomic_dec(&hw->phy->ongoing_txs);  
> > >
> > > I am pretty sure that will end in a scheduling while atomic warning
> > > with hwsim. If you don't hit it you have the wrong config, you need to
> > > enable such warnings and have the right preemption model setting.  
> >
> > I was using the "desktop" kernel preemption model (voluntary), I've
> > switched to CONFIG_PREEMPT ("Preemptible kernel (Low-latency)"),
> > and enabled CONFIG_DEBUG_ATOMIC_SLEEP. You are right that we should use
> > a spinlock instead of a mutex here. However I don't think disabling
> > IRQs is necessary, so I'll switch to spin_(un)lock() calls.
> >  
> 
> In my opinion it's necessary for the ifs hrtimer. Normal
> spin_lock/unlock is not the right fit here.

You're right, I forgot about hrtimers.

Thanks,
Miquèl

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

[Miquel Raynal]

aahringo@redhat.com wrote on Sun, 15 May 2022 19:03:53 -0400:

> Hi,
> 
> On Sun, May 15, 2022 at 6:28 PM Alexander Aring <aahringo@redhat.com> wrote:
> >
> > Hi,
> >
> > On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> > >
> > > 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           | 25 +++++++++++++++++++++++++
> > >  2 files changed, 26 insertions(+)
> > >
> > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> > > --- a/net/mac802154/tx.c
> > > +++ b/net/mac802154/tx.c
> > > @@ -128,6 +128,31 @@ 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)
> > > +{
> > > +       int ret;
> > > +
> > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > +        * MLME transmissions.
> > > +        */
> > > +       rtnl_lock();  
> >
> > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > earlier than that over the whole MLME op. MLME can trigger more than  
> 
> not over the whole MLME_op, that's terrible to hold the rtnl lock so
> long... so I think this is fine that some netdev call will interfere
> with this transmission.
> So forget about the ASSERT_RTNL() here, it's fine (I hope).
> 
> > one message, the whole sync_hold/release queue should be earlier than
> > that... in my opinion is it not right to allow other messages so far
> > an MLME op is going on? I am not sure what the standard says to this,
> > but I think it should be stopped the whole time? All those sequence  
> 
> Whereas the stop of the netdev queue makes sense for the whole mlme-op
> (in my opinion).

I might still implement an MLME pre/post helper and do the queue
hold/release calls there, while only taking the rtnl from the _tx.

And I might create an mlme_tx_one() which does the pre/post calls as
well.

Would something like this fit?

Thanks,
Miquèl

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

[Miquel Raynal]

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?

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

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Sun, 15 May 2022 18:30:28 -0400:

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

I was surprised that nobody cared enough about that information to
create a helper. Then I grepped and figured out I was not the first to
to do that...

$ git grep "flags & IFF_UP" | wc -l
289

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

[Miquel Raynal]

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

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

[Alexander Aring]

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

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

[Alexander Aring]

Hi,

On Tue, May 17, 2022 at 9:30 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>
> aahringo@redhat.com wrote on Sun, 15 May 2022 19:03:53 -0400:
>
> > Hi,
> >
> > On Sun, May 15, 2022 at 6:28 PM Alexander Aring <aahringo@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:
> > > >
> > > > 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           | 25 +++++++++++++++++++++++++
> > > >  2 files changed, 26 insertions(+)
> > > >
> > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > > index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> > > > --- a/net/mac802154/tx.c
> > > > +++ b/net/mac802154/tx.c
> > > > @@ -128,6 +128,31 @@ 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)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > > +        * MLME transmissions.
> > > > +        */
> > > > +       rtnl_lock();
> > >
> > > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > > earlier than that over the whole MLME op. MLME can trigger more than
> >
> > not over the whole MLME_op, that's terrible to hold the rtnl lock so
> > long... so I think this is fine that some netdev call will interfere
> > with this transmission.
> > So forget about the ASSERT_RTNL() here, it's fine (I hope).
> >
> > > one message, the whole sync_hold/release queue should be earlier than
> > > that... in my opinion is it not right to allow other messages so far
> > > an MLME op is going on? I am not sure what the standard says to this,
> > > but I think it should be stopped the whole time? All those sequence
> >
> > Whereas the stop of the netdev queue makes sense for the whole mlme-op
> > (in my opinion).
>
> I might still implement an MLME pre/post helper and do the queue
> hold/release calls there, while only taking the rtnl from the _tx.
>
> And I might create an mlme_tx_one() which does the pre/post calls as
> well.
>
> Would something like this fit?

I think so, I've heard for some transceiver types a scan operation can
take hours... but I guess whoever triggers that scan in such an
environment knows that it has some "side-effects"...

- Alex

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

[Miquel Raynal]

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

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

[Miquel Raynal]

aahringo@redhat.com wrote on Tue, 17 May 2022 21:14:03 -0400:

> Hi,
> 
> On Tue, May 17, 2022 at 9:30 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> >
> > aahringo@redhat.com wrote on Sun, 15 May 2022 19:03:53 -0400:
> >  
> > > Hi,
> > >
> > > On Sun, May 15, 2022 at 6:28 PM Alexander Aring <aahringo@redhat.com> wrote:  
> > > >
> > > > Hi,
> > > >
> > > > On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
> > > > <miquel.raynal@bootlin.com> wrote:  
> > > > >
> > > > > 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           | 25 +++++++++++++++++++++++++
> > > > >  2 files changed, 26 insertions(+)
> > > > >
> > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > > > index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> > > > > --- a/net/mac802154/tx.c
> > > > > +++ b/net/mac802154/tx.c
> > > > > @@ -128,6 +128,31 @@ 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)
> > > > > +{
> > > > > +       int ret;
> > > > > +
> > > > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > > > +        * MLME transmissions.
> > > > > +        */
> > > > > +       rtnl_lock();  
> > > >
> > > > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > > > earlier than that over the whole MLME op. MLME can trigger more than  
> > >
> > > not over the whole MLME_op, that's terrible to hold the rtnl lock so
> > > long... so I think this is fine that some netdev call will interfere
> > > with this transmission.
> > > So forget about the ASSERT_RTNL() here, it's fine (I hope).
> > >  
> > > > one message, the whole sync_hold/release queue should be earlier than
> > > > that... in my opinion is it not right to allow other messages so far
> > > > an MLME op is going on? I am not sure what the standard says to this,
> > > > but I think it should be stopped the whole time? All those sequence  
> > >
> > > Whereas the stop of the netdev queue makes sense for the whole mlme-op
> > > (in my opinion).  
> >
> > I might still implement an MLME pre/post helper and do the queue
> > hold/release calls there, while only taking the rtnl from the _tx.
> >
> > And I might create an mlme_tx_one() which does the pre/post calls as
> > well.
> >
> > Would something like this fit?  
> 
> I think so, I've heard for some transceiver types a scan operation can
> take hours... but I guess whoever triggers that scan in such an
> environment knows that it has some "side-effects"...

Yeah, a scan requires the data queue to be stopped and all incoming
packets to be dropped (others than beacons, ofc), so users must be
aware of this limitation.

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

[Alexander Aring]

Hi,

On Wed, May 18, 2022 at 6:12 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>
> aahringo@redhat.com wrote on Tue, 17 May 2022 21:14:03 -0400:
>
> > Hi,
> >
> > On Tue, May 17, 2022 at 9:30 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > >
> > > aahringo@redhat.com wrote on Sun, 15 May 2022 19:03:53 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Sun, May 15, 2022 at 6:28 PM Alexander Aring <aahringo@redhat.com> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
> > > > > <miquel.raynal@bootlin.com> wrote:
> > > > > >
> > > > > > 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           | 25 +++++++++++++++++++++++++
> > > > > >  2 files changed, 26 insertions(+)
> > > > > >
> > > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > > > > index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> > > > > > --- a/net/mac802154/tx.c
> > > > > > +++ b/net/mac802154/tx.c
> > > > > > @@ -128,6 +128,31 @@ 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)
> > > > > > +{
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > > > > +        * MLME transmissions.
> > > > > > +        */
> > > > > > +       rtnl_lock();
> > > > >
> > > > > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > > > > earlier than that over the whole MLME op. MLME can trigger more than
> > > >
> > > > not over the whole MLME_op, that's terrible to hold the rtnl lock so
> > > > long... so I think this is fine that some netdev call will interfere
> > > > with this transmission.
> > > > So forget about the ASSERT_RTNL() here, it's fine (I hope).
> > > >
> > > > > one message, the whole sync_hold/release queue should be earlier than
> > > > > that... in my opinion is it not right to allow other messages so far
> > > > > an MLME op is going on? I am not sure what the standard says to this,
> > > > > but I think it should be stopped the whole time? All those sequence
> > > >
> > > > Whereas the stop of the netdev queue makes sense for the whole mlme-op
> > > > (in my opinion).
> > >
> > > I might still implement an MLME pre/post helper and do the queue
> > > hold/release calls there, while only taking the rtnl from the _tx.
> > >
> > > And I might create an mlme_tx_one() which does the pre/post calls as
> > > well.
> > >
> > > Would something like this fit?
> >
> > I think so, I've heard for some transceiver types a scan operation can
> > take hours... but I guess whoever triggers that scan in such an
> > environment knows that it has some "side-effects"...
>
> Yeah, a scan requires the data queue to be stopped and all incoming
> packets to be dropped (others than beacons, ofc), so users must be
> aware of this limitation.

I think there is a real problem about how the user can synchronize the
start of a scan and be sure that at this point everything was
transmitted, we might need to real "flush" the queue. Your naming
"flush" is also wrong, It will flush the framebuffer(s) of the
transceivers but not the netdev queue... and we probably should flush
the netdev queue before starting mlme-op... this is something to add
in the mlme_op_pre() function.

- Alex

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

[Miquel Raynal]

alex.aring@gmail.com wrote on Wed, 18 May 2022 08:05:46 -0400:

> Hi,
> 
> On Wed, May 18, 2022 at 6:12 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> >
> > aahringo@redhat.com wrote on Tue, 17 May 2022 21:14:03 -0400:
> >  
> > > Hi,
> > >
> > > On Tue, May 17, 2022 at 9:30 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > >
> > > > aahringo@redhat.com wrote on Sun, 15 May 2022 19:03:53 -0400:
> > > >  
> > > > > Hi,
> > > > >
> > > > > On Sun, May 15, 2022 at 6:28 PM Alexander Aring <aahringo@redhat.com> wrote:  
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
> > > > > > <miquel.raynal@bootlin.com> wrote:  
> > > > > > >
> > > > > > > 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           | 25 +++++++++++++++++++++++++
> > > > > > >  2 files changed, 26 insertions(+)
> > > > > > >
> > > > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > > > > > index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> > > > > > > --- a/net/mac802154/tx.c
> > > > > > > +++ b/net/mac802154/tx.c
> > > > > > > @@ -128,6 +128,31 @@ 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)
> > > > > > > +{
> > > > > > > +       int ret;
> > > > > > > +
> > > > > > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > > > > > +        * MLME transmissions.
> > > > > > > +        */
> > > > > > > +       rtnl_lock();  
> > > > > >
> > > > > > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > > > > > earlier than that over the whole MLME op. MLME can trigger more than  
> > > > >
> > > > > not over the whole MLME_op, that's terrible to hold the rtnl lock so
> > > > > long... so I think this is fine that some netdev call will interfere
> > > > > with this transmission.
> > > > > So forget about the ASSERT_RTNL() here, it's fine (I hope).
> > > > >  
> > > > > > one message, the whole sync_hold/release queue should be earlier than
> > > > > > that... in my opinion is it not right to allow other messages so far
> > > > > > an MLME op is going on? I am not sure what the standard says to this,
> > > > > > but I think it should be stopped the whole time? All those sequence  
> > > > >
> > > > > Whereas the stop of the netdev queue makes sense for the whole mlme-op
> > > > > (in my opinion).  
> > > >
> > > > I might still implement an MLME pre/post helper and do the queue
> > > > hold/release calls there, while only taking the rtnl from the _tx.
> > > >
> > > > And I might create an mlme_tx_one() which does the pre/post calls as
> > > > well.
> > > >
> > > > Would something like this fit?  
> > >
> > > I think so, I've heard for some transceiver types a scan operation can
> > > take hours... but I guess whoever triggers that scan in such an
> > > environment knows that it has some "side-effects"...  
> >
> > Yeah, a scan requires the data queue to be stopped and all incoming
> > packets to be dropped (others than beacons, ofc), so users must be
> > aware of this limitation.  
> 
> I think there is a real problem about how the user can synchronize the
> start of a scan and be sure that at this point everything was
> transmitted, we might need to real "flush" the queue. Your naming
> "flush" is also wrong, It will flush the framebuffer(s) of the
> transceivers but not the netdev queue... and we probably should flush
> the netdev queue before starting mlme-op... this is something to add
> in the mlme_op_pre() function.

Is it even possible? This requires waiting for the netdev queue to be
empty before stopping it, but if users constantly flood the transceiver
with data packets this might "never" happen.

And event thought we might accept this situation, I don't know how to
check the emptiness of the netif queue. Any inputs?

Thanks,
Miquèl

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

[Alexander Aring]

Hi,

On Wed, May 18, 2022 at 8:37 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>
> alex.aring@gmail.com wrote on Wed, 18 May 2022 08:05:46 -0400:
>
> > Hi,
> >
> > On Wed, May 18, 2022 at 6:12 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > >
> > > aahringo@redhat.com wrote on Tue, 17 May 2022 21:14:03 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Tue, May 17, 2022 at 9:30 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > >
> > > > > aahringo@redhat.com wrote on Sun, 15 May 2022 19:03:53 -0400:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Sun, May 15, 2022 at 6:28 PM Alexander Aring <aahringo@redhat.com> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Thu, May 12, 2022 at 10:34 AM Miquel Raynal
> > > > > > > <miquel.raynal@bootlin.com> wrote:
> > > > > > > >
> > > > > > > > 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           | 25 +++++++++++++++++++++++++
> > > > > > > >  2 files changed, 26 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > > > > > > index a057827fc48a..f8b374810a11 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 38f74b8b6740..ec8d872143ee 100644
> > > > > > > > --- a/net/mac802154/tx.c
> > > > > > > > +++ b/net/mac802154/tx.c
> > > > > > > > @@ -128,6 +128,31 @@ 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)
> > > > > > > > +{
> > > > > > > > +       int ret;
> > > > > > > > +
> > > > > > > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > > > > > > +        * MLME transmissions.
> > > > > > > > +        */
> > > > > > > > +       rtnl_lock();
> > > > > > >
> > > > > > > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > > > > > > earlier than that over the whole MLME op. MLME can trigger more than
> > > > > >
> > > > > > not over the whole MLME_op, that's terrible to hold the rtnl lock so
> > > > > > long... so I think this is fine that some netdev call will interfere
> > > > > > with this transmission.
> > > > > > So forget about the ASSERT_RTNL() here, it's fine (I hope).
> > > > > >
> > > > > > > one message, the whole sync_hold/release queue should be earlier than
> > > > > > > that... in my opinion is it not right to allow other messages so far
> > > > > > > an MLME op is going on? I am not sure what the standard says to this,
> > > > > > > but I think it should be stopped the whole time? All those sequence
> > > > > >
> > > > > > Whereas the stop of the netdev queue makes sense for the whole mlme-op
> > > > > > (in my opinion).
> > > > >
> > > > > I might still implement an MLME pre/post helper and do the queue
> > > > > hold/release calls there, while only taking the rtnl from the _tx.
> > > > >
> > > > > And I might create an mlme_tx_one() which does the pre/post calls as
> > > > > well.
> > > > >
> > > > > Would something like this fit?
> > > >
> > > > I think so, I've heard for some transceiver types a scan operation can
> > > > take hours... but I guess whoever triggers that scan in such an
> > > > environment knows that it has some "side-effects"...
> > >
> > > Yeah, a scan requires the data queue to be stopped and all incoming
> > > packets to be dropped (others than beacons, ofc), so users must be
> > > aware of this limitation.
> >
> > I think there is a real problem about how the user can synchronize the
> > start of a scan and be sure that at this point everything was
> > transmitted, we might need to real "flush" the queue. Your naming
> > "flush" is also wrong, It will flush the framebuffer(s) of the
> > transceivers but not the netdev queue... and we probably should flush
> > the netdev queue before starting mlme-op... this is something to add
> > in the mlme_op_pre() function.
>
> Is it even possible? This requires waiting for the netdev queue to be
> empty before stopping it, but if users constantly flood the transceiver
> with data packets this might "never" happen.
>

Nothing is impossible, just maybe nobody thought about that. Sure
putting more into the queue should be forbidden but what's inside
should be "flushed". Currently we make a hard cut, there is no way
that the user knows what's sent or not BUT that is the case for
xmit_do() anyway, it's not reliable... people need to have the right
upper layer protocol. However I think we could run into problems if we
especially have features like waiting for the socket error queue to
know if e.g. an ack was received or not.

> And event thought we might accept this situation, I don't know how to
> check the emptiness of the netif queue. Any inputs?

Don't think about it, I see a practical issue here which I keep in my mind.

- Alex

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

[Miquel Raynal]

Hi Alexander,

> > > > > > > > > +int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb)
> > > > > > > > > +{
> > > > > > > > > +       int ret;
> > > > > > > > > +
> > > > > > > > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > > > > > > > +        * MLME transmissions.
> > > > > > > > > +        */
> > > > > > > > > +       rtnl_lock();  
> > > > > > > >
> > > > > > > > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > > > > > > > earlier than that over the whole MLME op. MLME can trigger more than  
> > > > > > >
> > > > > > > not over the whole MLME_op, that's terrible to hold the rtnl lock so
> > > > > > > long... so I think this is fine that some netdev call will interfere
> > > > > > > with this transmission.
> > > > > > > So forget about the ASSERT_RTNL() here, it's fine (I hope).
> > > > > > >  
> > > > > > > > one message, the whole sync_hold/release queue should be earlier than
> > > > > > > > that... in my opinion is it not right to allow other messages so far
> > > > > > > > an MLME op is going on? I am not sure what the standard says to this,
> > > > > > > > but I think it should be stopped the whole time? All those sequence  
> > > > > > >
> > > > > > > Whereas the stop of the netdev queue makes sense for the whole mlme-op
> > > > > > > (in my opinion).  
> > > > > >
> > > > > > I might still implement an MLME pre/post helper and do the queue
> > > > > > hold/release calls there, while only taking the rtnl from the _tx.
> > > > > >
> > > > > > And I might create an mlme_tx_one() which does the pre/post calls as
> > > > > > well.
> > > > > >
> > > > > > Would something like this fit?  
> > > > >
> > > > > I think so, I've heard for some transceiver types a scan operation can
> > > > > take hours... but I guess whoever triggers that scan in such an
> > > > > environment knows that it has some "side-effects"...  
> > > >
> > > > Yeah, a scan requires the data queue to be stopped and all incoming
> > > > packets to be dropped (others than beacons, ofc), so users must be
> > > > aware of this limitation.  
> > >
> > > I think there is a real problem about how the user can synchronize the
> > > start of a scan and be sure that at this point everything was
> > > transmitted, we might need to real "flush" the queue. Your naming
> > > "flush" is also wrong, It will flush the framebuffer(s) of the
> > > transceivers but not the netdev queue... and we probably should flush
> > > the netdev queue before starting mlme-op... this is something to add
> > > in the mlme_op_pre() function.  
> >
> > Is it even possible? This requires waiting for the netdev queue to be
> > empty before stopping it, but if users constantly flood the transceiver
> > with data packets this might "never" happen.
> >  
> 
> Nothing is impossible, just maybe nobody thought about that. Sure
> putting more into the queue should be forbidden but what's inside
> should be "flushed". Currently we make a hard cut, there is no way
> that the user knows what's sent or not BUT that is the case for
> xmit_do() anyway, it's not reliable... people need to have the right
> upper layer protocol. However I think we could run into problems if we
> especially have features like waiting for the socket error queue to
> know if e.g. an ack was received or not.

Looking at net/core/dev.c I don't see the issue anymore, let me try to
explain: as far as I understand the net device queue is a very
conceptual "queue" which only has a reality if the underlying layer
really implements the concept of a queue. To be more precise, at the
netdev level itself, there is a HARD_TX_LOCK() call which serializes
the ->ndo_start_xmit() calls, but whatever entered the
->ndo_start_xmit() hook _will_ be handled by the lower layer and is not
in any "waiting" state at the net core level.

In practice, the IEEE 802.15.4 core treats all packets immediately and
do not really bother "queuing" them like if there was a "waiting"
state. So all messages that the userspace expected to be send (which
did not return NETDEV_TX_BUSY) at the moment where we decide to stop
data transmissions will be processed.

If several frames had to be transmitted to the IEEE 802.15.4 core and
they all passed the netdev "queuing" mechanism, then they will be
forwarded to the tranceivers thanks to the wait_event(!ongoing_txs) and
only after we declare the queue sync'ed.

For me there is no hard cut.

> > And event thought we might accept this situation, I don't know how to
> > check the emptiness of the netif queue. Any inputs?  
> 
> Don't think about it, I see a practical issue here which I keep in my mind.
> 
> - Alex
> 


Thanks,
Miquèl

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

[Alexander Aring]

Hi,

On Wed, May 18, 2022 at 12:12 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> > > > > > > > > > +int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb)
> > > > > > > > > > +{
> > > > > > > > > > +       int ret;
> > > > > > > > > > +
> > > > > > > > > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > > > > > > > > +        * MLME transmissions.
> > > > > > > > > > +        */
> > > > > > > > > > +       rtnl_lock();
> > > > > > > > >
> > > > > > > > > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > > > > > > > > earlier than that over the whole MLME op. MLME can trigger more than
> > > > > > > >
> > > > > > > > not over the whole MLME_op, that's terrible to hold the rtnl lock so
> > > > > > > > long... so I think this is fine that some netdev call will interfere
> > > > > > > > with this transmission.
> > > > > > > > So forget about the ASSERT_RTNL() here, it's fine (I hope).
> > > > > > > >
> > > > > > > > > one message, the whole sync_hold/release queue should be earlier than
> > > > > > > > > that... in my opinion is it not right to allow other messages so far
> > > > > > > > > an MLME op is going on? I am not sure what the standard says to this,
> > > > > > > > > but I think it should be stopped the whole time? All those sequence
> > > > > > > >
> > > > > > > > Whereas the stop of the netdev queue makes sense for the whole mlme-op
> > > > > > > > (in my opinion).
> > > > > > >
> > > > > > > I might still implement an MLME pre/post helper and do the queue
> > > > > > > hold/release calls there, while only taking the rtnl from the _tx.
> > > > > > >
> > > > > > > And I might create an mlme_tx_one() which does the pre/post calls as
> > > > > > > well.
> > > > > > >
> > > > > > > Would something like this fit?
> > > > > >
> > > > > > I think so, I've heard for some transceiver types a scan operation can
> > > > > > take hours... but I guess whoever triggers that scan in such an
> > > > > > environment knows that it has some "side-effects"...
> > > > >
> > > > > Yeah, a scan requires the data queue to be stopped and all incoming
> > > > > packets to be dropped (others than beacons, ofc), so users must be
> > > > > aware of this limitation.
> > > >
> > > > I think there is a real problem about how the user can synchronize the
> > > > start of a scan and be sure that at this point everything was
> > > > transmitted, we might need to real "flush" the queue. Your naming
> > > > "flush" is also wrong, It will flush the framebuffer(s) of the
> > > > transceivers but not the netdev queue... and we probably should flush
> > > > the netdev queue before starting mlme-op... this is something to add
> > > > in the mlme_op_pre() function.
> > >
> > > Is it even possible? This requires waiting for the netdev queue to be
> > > empty before stopping it, but if users constantly flood the transceiver
> > > with data packets this might "never" happen.
> > >
> >
> > Nothing is impossible, just maybe nobody thought about that. Sure
> > putting more into the queue should be forbidden but what's inside
> > should be "flushed". Currently we make a hard cut, there is no way
> > that the user knows what's sent or not BUT that is the case for
> > xmit_do() anyway, it's not reliable... people need to have the right
> > upper layer protocol. However I think we could run into problems if we
> > especially have features like waiting for the socket error queue to
> > know if e.g. an ack was received or not.
>
> Looking at net/core/dev.c I don't see the issue anymore, let me try to
> explain: as far as I understand the net device queue is a very
> conceptual "queue" which only has a reality if the underlying layer
> really implements the concept of a queue. To be more precise, at the
> netdev level itself, there is a HARD_TX_LOCK() call which serializes
> the ->ndo_start_xmit() calls, but whatever entered the
> ->ndo_start_xmit() hook _will_ be handled by the lower layer and is not
> in any "waiting" state at the net core level.
>
> In practice, the IEEE 802.15.4 core treats all packets immediately and
> do not really bother "queuing" them like if there was a "waiting"
> state. So all messages that the userspace expected to be send (which
> did not return NETDEV_TX_BUSY) at the moment where we decide to stop
> data transmissions will be processed.
>
> If several frames had to be transmitted to the IEEE 802.15.4 core and
> they all passed the netdev "queuing" mechanism, then they will be
> forwarded to the tranceivers thanks to the wait_event(!ongoing_txs) and
> only after we declare the queue sync'ed.
>
> For me there is no hard cut.

In my opinion there is definitely in case of a wpan interface a queue
handling right above xmit_do() which is in a "works for now" state.
Your queue flush function will not flush any queue, as I said it's
flushing the transceivers framebuffer at the starting point of
xmit_do() call and you should change your comments/function names to
describe this behaviour.

- Alex

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

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Wed, 18 May 2022 21:51:36 -0400:

> Hi,
> 
> On Wed, May 18, 2022 at 12:12 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >  
> > > > > > > > > > > +int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb)
> > > > > > > > > > > +{
> > > > > > > > > > > +       int ret;
> > > > > > > > > > > +
> > > > > > > > > > > +       /* Avoid possible calls to ->ndo_stop() when we asynchronously perform
> > > > > > > > > > > +        * MLME transmissions.
> > > > > > > > > > > +        */
> > > > > > > > > > > +       rtnl_lock();  
> > > > > > > > > >
> > > > > > > > > > I think we should make an ASSERT_RTNL() here, the lock needs to be
> > > > > > > > > > earlier than that over the whole MLME op. MLME can trigger more than  
> > > > > > > > >
> > > > > > > > > not over the whole MLME_op, that's terrible to hold the rtnl lock so
> > > > > > > > > long... so I think this is fine that some netdev call will interfere
> > > > > > > > > with this transmission.
> > > > > > > > > So forget about the ASSERT_RTNL() here, it's fine (I hope).
> > > > > > > > >  
> > > > > > > > > > one message, the whole sync_hold/release queue should be earlier than
> > > > > > > > > > that... in my opinion is it not right to allow other messages so far
> > > > > > > > > > an MLME op is going on? I am not sure what the standard says to this,
> > > > > > > > > > but I think it should be stopped the whole time? All those sequence  
> > > > > > > > >
> > > > > > > > > Whereas the stop of the netdev queue makes sense for the whole mlme-op
> > > > > > > > > (in my opinion).  
> > > > > > > >
> > > > > > > > I might still implement an MLME pre/post helper and do the queue
> > > > > > > > hold/release calls there, while only taking the rtnl from the _tx.
> > > > > > > >
> > > > > > > > And I might create an mlme_tx_one() which does the pre/post calls as
> > > > > > > > well.
> > > > > > > >
> > > > > > > > Would something like this fit?  
> > > > > > >
> > > > > > > I think so, I've heard for some transceiver types a scan operation can
> > > > > > > take hours... but I guess whoever triggers that scan in such an
> > > > > > > environment knows that it has some "side-effects"...  
> > > > > >
> > > > > > Yeah, a scan requires the data queue to be stopped and all incoming
> > > > > > packets to be dropped (others than beacons, ofc), so users must be
> > > > > > aware of this limitation.  
> > > > >
> > > > > I think there is a real problem about how the user can synchronize the
> > > > > start of a scan and be sure that at this point everything was
> > > > > transmitted, we might need to real "flush" the queue. Your naming
> > > > > "flush" is also wrong, It will flush the framebuffer(s) of the
> > > > > transceivers but not the netdev queue... and we probably should flush
> > > > > the netdev queue before starting mlme-op... this is something to add
> > > > > in the mlme_op_pre() function.  
> > > >
> > > > Is it even possible? This requires waiting for the netdev queue to be
> > > > empty before stopping it, but if users constantly flood the transceiver
> > > > with data packets this might "never" happen.
> > > >  
> > >
> > > Nothing is impossible, just maybe nobody thought about that. Sure
> > > putting more into the queue should be forbidden but what's inside
> > > should be "flushed". Currently we make a hard cut, there is no way
> > > that the user knows what's sent or not BUT that is the case for
> > > xmit_do() anyway, it's not reliable... people need to have the right
> > > upper layer protocol. However I think we could run into problems if we
> > > especially have features like waiting for the socket error queue to
> > > know if e.g. an ack was received or not.  
> >
> > Looking at net/core/dev.c I don't see the issue anymore, let me try to
> > explain: as far as I understand the net device queue is a very
> > conceptual "queue" which only has a reality if the underlying layer
> > really implements the concept of a queue. To be more precise, at the
> > netdev level itself, there is a HARD_TX_LOCK() call which serializes
> > the ->ndo_start_xmit() calls, but whatever entered the  
> > ->ndo_start_xmit() hook _will_ be handled by the lower layer and is not  
> > in any "waiting" state at the net core level.
> >
> > In practice, the IEEE 802.15.4 core treats all packets immediately and
> > do not really bother "queuing" them like if there was a "waiting"
> > state. So all messages that the userspace expected to be send (which
> > did not return NETDEV_TX_BUSY) at the moment where we decide to stop
> > data transmissions will be processed.
> >
> > If several frames had to be transmitted to the IEEE 802.15.4 core and
> > they all passed the netdev "queuing" mechanism, then they will be
> > forwarded to the tranceivers thanks to the wait_event(!ongoing_txs) and
> > only after we declare the queue sync'ed.
> >
> > For me there is no hard cut.  
> 
> In my opinion there is definitely in case of a wpan interface a queue
> handling right above xmit_do() which is in a "works for now" state.
> Your queue flush function will not flush any queue, as I said it's
> flushing the transceivers framebuffer at the starting point of
> xmit_do() call and you should change your comments/function names to
> describe this behaviour.

I see we are still discussing the v2 here but I assume the v3 naming
might already be better, I am not flushing anymore but I "{sync,sync and
hold} the ieee802154 queue", which is hopefully closer to the reality.

The fact is that we have no way of knowing what happens at an higher
level (for the best) and if the user ever decides to perform a scan,
any data left in the upper layers will be frozen there for a moment, I
would say it is the responsibility of the user to act on the upper
layers to sync there first.

I'll send the v4 with all your other comments addressed, let's
continue discussing there if needed.

Thanks,
Miquèl