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

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

[Miquel Raynal]

Hello,

This series brings support for that famous synchronous Tx API for MLME
commands.

MLME commands will be used during scan operations. In this situation,
we need to be sure that all transfers finished and that no transfer
will be queued for a short moment.

Cheers,
Miquèl

Changes in v3:
* Tested with lockdep enabled, a more aggressive preemption level and
  the sleeping while atomic warnings enabled.
* Changed the hold/release queue mutex into a spinlock.
* Split the mlme_tx function into three, one to hold the queue, then
  another part that does takes the rtnl and has the real content, and a
  last helper to release the queue.
* Fixed the warning condition in the slow path.
* Used an unsigned long and test/set_bit helpers to follow the queue
  state instead of an atomic_t.

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 ability 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      |   9 ++-
 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           | 147 +++++++++++++++++++++++++++++++----
 net/mac802154/util.c         |  71 +++++++++++++++--
 8 files changed, 246 insertions(+), 54 deletions(-)

-- 
2.34.1

[PATCH wpan-next v3 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.34.1

[PATCH wpan-next v3 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.34.1

[PATCH wpan-next v3 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.34.1

[PATCH wpan-next v3 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.34.1

[PATCH wpan-next v3 05/11] net: mac802154: Bring the ability 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      |  6 +++--
 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         | 52 +++++++++++++++++++++++++++++++-----
 7 files changed, 75 insertions(+), 41 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 473ebcb9b155..7a191418f258 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -11,7 +11,7 @@
 
 #include <linux/ieee802154.h>
 #include <linux/netdevice.h>
-#include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <linux/bug.h>
 
 #include <net/nl802154.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 */
+	spinlock_t 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..47a4de6df88b 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);
 
+	spin_lock_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..6176cc40df91 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,33 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw)
 	}
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL(ieee802154_stop_queue);
+
+void ieee802154_hold_queue(struct ieee802154_local *local)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&local->phy->queue_lock, flags);
+	ieee802154_stop_queue(&local->hw);
+	atomic_inc(&local->phy->hold_txs);
+	spin_unlock_irqrestore(&local->phy->queue_lock, flags);
+}
+
+void ieee802154_release_queue(struct ieee802154_local *local)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&local->phy->queue_lock, flags);
+	if (!atomic_dec_and_test(&local->phy->hold_txs))
+		ieee802154_wake_queue(&local->hw);
+	spin_unlock_irqrestore(&local->phy->queue_lock, flags);
+}
 
 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 +122,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 +136,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.34.1

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

[Alexander Aring]

Hi,

On Tue, May 17, 2022 at 12:35 PM 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      |  6 +++--
>  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         | 52 +++++++++++++++++++++++++++++++-----
>  7 files changed, 75 insertions(+), 41 deletions(-)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 473ebcb9b155..7a191418f258 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -11,7 +11,7 @@
>
>  #include <linux/ieee802154.h>
>  #include <linux/netdevice.h>
> -#include <linux/mutex.h>
> +#include <linux/spinlock.h>
>  #include <linux/bug.h>
>
>  #include <net/nl802154.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 */
> +       spinlock_t 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..47a4de6df88b 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);
>
> +       spin_lock_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..6176cc40df91 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,33 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw)
>         }
>         rcu_read_unlock();
>  }
> -EXPORT_SYMBOL(ieee802154_stop_queue);
> +
> +void ieee802154_hold_queue(struct ieee802154_local *local)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&local->phy->queue_lock, flags);
> +       ieee802154_stop_queue(&local->hw);
> +       atomic_inc(&local->phy->hold_txs);
> +       spin_unlock_irqrestore(&local->phy->queue_lock, flags);
> +}

I think that works, but I would expect something like:

if (!atomic_fetch_inc(hold_txs))
      ieee802154_stop_queue(&local->hw);

- Alex

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

[Miquel Raynal]

Hi Alex,

aahringo@redhat.com wrote on Tue, 17 May 2022 20:37:38 -0400:

> Hi,
> 
> On Tue, May 17, 2022 at 12:35 PM 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      |  6 +++--
> >  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         | 52 +++++++++++++++++++++++++++++++-----
> >  7 files changed, 75 insertions(+), 41 deletions(-)
> >
> > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > index 473ebcb9b155..7a191418f258 100644
> > --- a/include/net/cfg802154.h
> > +++ b/include/net/cfg802154.h
> > @@ -11,7 +11,7 @@
> >
> >  #include <linux/ieee802154.h>
> >  #include <linux/netdevice.h>
> > -#include <linux/mutex.h>
> > +#include <linux/spinlock.h>
> >  #include <linux/bug.h>
> >
> >  #include <net/nl802154.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 */
> > +       spinlock_t 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..47a4de6df88b 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);
> >
> > +       spin_lock_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..6176cc40df91 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,33 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw)
> >         }
> >         rcu_read_unlock();
> >  }
> > -EXPORT_SYMBOL(ieee802154_stop_queue);
> > +
> > +void ieee802154_hold_queue(struct ieee802154_local *local)
> > +{
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&local->phy->queue_lock, flags);
> > +       ieee802154_stop_queue(&local->hw);
> > +       atomic_inc(&local->phy->hold_txs);
> > +       spin_unlock_irqrestore(&local->phy->queue_lock, flags);
> > +}  
> 
> I think that works, but I would expect something like:
> 
> if (!atomic_fetch_inc(hold_txs))
>       ieee802154_stop_queue(&local->hw);

I didn't know about the _fetch_ alternative, that looks much better
this way!

[PATCH wpan-next v3 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.34.1

[PATCH wpan-next v3 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 6176cc40df91..02645f57fc2a 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -83,6 +83,20 @@ void ieee802154_release_queue(struct ieee802154_local *local)
 	spin_unlock_irqrestore(&local->phy->queue_lock, flags);
 }
 
+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.34.1

[PATCH wpan-next v3 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 7a191418f258..8881b6126b58 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -218,6 +218,7 @@ struct wpan_phy {
 	spinlock_t queue_lock;
 	atomic_t ongoing_txs;
 	atomic_t hold_txs;
+	wait_queue_head_t sync_txq;
 
 	char priv[] __aligned(NETDEV_ALIGN);
 };
diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
index 47a4de6df88b..57546e07e06a 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);
 
 	spin_lock_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 02645f57fc2a..cddb42984484 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -140,7 +140,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);
 
@@ -152,7 +153,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.34.1

[PATCH wpan-next v3 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           | 46 ++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index a057827fc48a..b42c6ac789f5 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_one(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..6cc4e5c7ba94 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
 	return ieee802154_sync_queue(local);
 }
 
+static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
+{
+	return ieee802154_sync_and_hold_queue(local);
+}
+
+static 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_tx(local, skb);
+	ret = ieee802154_sync_queue(local);
+
+	rtnl_unlock();
+
+	return ret;
+}
+
+static void ieee802154_mlme_op_post(struct ieee802154_local *local)
+{
+	ieee802154_release_queue(local);
+}
+
+int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb)
+{
+	int ret;
+
+	ret = ieee802154_mlme_op_pre(local);
+	if (ret)
+		return ret;
+
+	ret = ieee802154_mlme_tx(local, skb);
+
+	ieee802154_mlme_op_post(local);
+
+	return ret;
+}
+
 static netdev_tx_t
 ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
 {
-- 
2.34.1

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

[Alexander Aring]

Hi,

On Tue, May 17, 2022 at 12:35 PM 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           | 46 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> index a057827fc48a..b42c6ac789f5 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_one(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..6cc4e5c7ba94 100644
> --- a/net/mac802154/tx.c
> +++ b/net/mac802154/tx.c
> @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
>         return ieee802154_sync_queue(local);
>  }
>
> +static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
> +{
> +       return ieee802154_sync_and_hold_queue(local);
> +}
> +
> +static 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;
> +

No -EBUSY here, use ?-ENETDOWN?. You forgot rtnl_unlock() here.

- Alex

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

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400:

> Hi,
> 
> On Tue, May 17, 2022 at 12:35 PM 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           | 46 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 47 insertions(+)
> >
> > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > index a057827fc48a..b42c6ac789f5 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_one(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..6cc4e5c7ba94 100644
> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
> >         return ieee802154_sync_queue(local);
> >  }
> >
> > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
> > +{
> > +       return ieee802154_sync_and_hold_queue(local);
> > +}
> > +
> > +static 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;
> > +  
> 
> No -EBUSY here, use ?-ENETDOWN?.

Isn't it strange to return "Network is down" while we try to stop the
device but fail to do so because, actually, it is still being used?

> You forgot rtnl_unlock() here.

I've blindly added those rtnl calls, I need to go through all of them
once again and ensure all the error path release the lock.

Thanks,
Miquèl

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

[Alexander Aring]

Hi,

On Wed, May 18, 2022 at 4:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400:
>
> > Hi,
> >
> > On Tue, May 17, 2022 at 12:35 PM 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           | 46 ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 47 insertions(+)
> > >
> > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > index a057827fc48a..b42c6ac789f5 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_one(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..6cc4e5c7ba94 100644
> > > --- a/net/mac802154/tx.c
> > > +++ b/net/mac802154/tx.c
> > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
> > >         return ieee802154_sync_queue(local);
> > >  }
> > >
> > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
> > > +{
> > > +       return ieee802154_sync_and_hold_queue(local);
> > > +}
> > > +
> > > +static 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;
> > > +
> >
> > No -EBUSY here, use ?-ENETDOWN?.
>
> Isn't it strange to return "Network is down" while we try to stop the
> device but fail to do so because, actually, it is still being used?
>

you are right. Maybe -EPERM, in a sense of whether the netdev state
allows it or not.

- Alex

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

[Alexander Aring]

Hi,

On Wed, May 18, 2022 at 7:59 AM Alexander Aring <alex.aring@gmail.com> wrote:
>
> Hi,
>
> On Wed, May 18, 2022 at 4:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400:
> >
> > > Hi,
> > >
> > > On Tue, May 17, 2022 at 12:35 PM 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           | 46 ++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 47 insertions(+)
> > > >
> > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > > index a057827fc48a..b42c6ac789f5 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_one(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..6cc4e5c7ba94 100644
> > > > --- a/net/mac802154/tx.c
> > > > +++ b/net/mac802154/tx.c
> > > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
> > > >         return ieee802154_sync_queue(local);
> > > >  }
> > > >
> > > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
> > > > +{
> > > > +       return ieee802154_sync_and_hold_queue(local);
> > > > +}
> > > > +
> > > > +static 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;
> > > > +
> > >
> > > No -EBUSY here, use ?-ENETDOWN?.
> >
> > Isn't it strange to return "Network is down" while we try to stop the
> > device but fail to do so because, actually, it is still being used?
> >
>
> you are right. Maybe -EPERM, in a sense of whether the netdev state
> allows it or not.

or maybe not, if this is the error the user gets by running iwpan. The
problem I have with -EBUSY is that it indicates some resource is being
used and will be solved at some time. Especially in a transmit
function e.g. framebuffer.

- Alex

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

[Miquel Raynal]

alex.aring@gmail.com wrote on Wed, 18 May 2022 07:59:37 -0400:

> Hi,
> 
> On Wed, May 18, 2022 at 4:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400:
> >  
> > > Hi,
> > >
> > > On Tue, May 17, 2022 at 12:35 PM 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           | 46 ++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 47 insertions(+)
> > > >
> > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > > index a057827fc48a..b42c6ac789f5 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_one(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..6cc4e5c7ba94 100644
> > > > --- a/net/mac802154/tx.c
> > > > +++ b/net/mac802154/tx.c
> > > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
> > > >         return ieee802154_sync_queue(local);
> > > >  }
> > > >
> > > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
> > > > +{
> > > > +       return ieee802154_sync_and_hold_queue(local);
> > > > +}
> > > > +
> > > > +static 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;
> > > > +  
> > >
> > > No -EBUSY here, use ?-ENETDOWN?.  
> >
> > Isn't it strange to return "Network is down" while we try to stop the
> > device but fail to do so because, actually, it is still being used?
> >  
> 
> you are right. Maybe -EPERM, in a sense of whether the netdev state
> allows it or not.

Actually you were right in your fist review, "!open_count" means
that the net iface is down, so returning -ENETDOWN is fine, I believe.

Thanks,
Miquèl

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

[Alexander Aring]

Hi,

On Wed, May 18, 2022 at 8:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>
> alex.aring@gmail.com wrote on Wed, 18 May 2022 07:59:37 -0400:
>
> > Hi,
> >
> > On Wed, May 18, 2022 at 4:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Tue, May 17, 2022 at 12:35 PM 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           | 46 ++++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 47 insertions(+)
> > > > >
> > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
> > > > > index a057827fc48a..b42c6ac789f5 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_one(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..6cc4e5c7ba94 100644
> > > > > --- a/net/mac802154/tx.c
> > > > > +++ b/net/mac802154/tx.c
> > > > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
> > > > >         return ieee802154_sync_queue(local);
> > > > >  }
> > > > >
> > > > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
> > > > > +{
> > > > > +       return ieee802154_sync_and_hold_queue(local);
> > > > > +}
> > > > > +
> > > > > +static 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;
> > > > > +
> > > >
> > > > No -EBUSY here, use ?-ENETDOWN?.
> > >
> > > Isn't it strange to return "Network is down" while we try to stop the
> > > device but fail to do so because, actually, it is still being used?
> > >
> >
> > you are right. Maybe -EPERM, in a sense of whether the netdev state
> > allows it or not.
>
> Actually you were right in your fist review, "!open_count" means
> that the net iface is down, so returning -ENETDOWN is fine, I believe.
>
ah yes, you confused me!

- Alex

[PATCH wpan-next v3 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 8881b6126b58..f4e7b3fe7cf0 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -218,6 +218,7 @@ struct wpan_phy {
 	spinlock_t queue_lock;
 	atomic_t ongoing_txs;
 	atomic_t hold_txs;
+	unsigned long queue_stopped;
 	wait_queue_head_t sync_txq;
 
 	char priv[] __aligned(NETDEV_ALIGN);
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 6cc4e5c7ba94..e36aca788ea2 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -123,9 +123,13 @@ static int ieee802154_sync_queue(struct ieee802154_local *local)
 
 int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
 {
+	int ret;
+
 	ieee802154_hold_queue(local);
+	ret = ieee802154_sync_queue(local);
+	set_bit(0, &local->phy->queue_stopped);
 
-	return ieee802154_sync_queue(local);
+	return ret;
 }
 
 static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
@@ -174,9 +178,19 @@ int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb)
 	return ret;
 }
 
+static bool ieee802154_queue_is_stopped(struct ieee802154_local *local)
+{
+	return test_bit(0, &local->phy->queue_stopped);
+}
+
 static netdev_tx_t
 ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb)
 {
+	/* Warn if the net interface tries to transmit frames while the
+	 * ieee802154 core assumes the queue is stopped.
+	 */
+	WARN_ON_ONCE(ieee802154_queue_is_stopped(local));
+
 	return ieee802154_tx(local, skb);
 }
 
diff --git a/net/mac802154/util.c b/net/mac802154/util.c
index cddb42984484..faf12c2135bf 100644
--- a/net/mac802154/util.c
+++ b/net/mac802154/util.c
@@ -29,6 +29,7 @@ static void ieee802154_wake_queue(struct ieee802154_hw *hw)
 	struct ieee802154_sub_if_data *sdata;
 
 	rcu_read_lock();
+	clear_bit(0, &local->phy->queue_stopped);
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 		if (!sdata->dev)
 			continue;
-- 
2.34.1

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

[Alexander Aring]

Hi,

On Tue, May 17, 2022 at 12:35 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> We should never start a transmission after the queue has been stopped.
>
> But because it might work we don't kill the function here but rather
> warn loudly the user that something is wrong.
>
> Set an atomic when the queue will remain stopped. Reset this atomic when
> the queue actually gets restarded. Just check this atomic to know if the
> transmission is legitimate, warn if it is not.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/net/cfg802154.h |  1 +
>  net/mac802154/tx.c      | 16 +++++++++++++++-
>  net/mac802154/util.c    |  1 +
>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 8881b6126b58..f4e7b3fe7cf0 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -218,6 +218,7 @@ struct wpan_phy {
>         spinlock_t queue_lock;
>         atomic_t ongoing_txs;
>         atomic_t hold_txs;
> +       unsigned long queue_stopped;

Can we name it something like state_flags (as phy state flags)? Pretty
sure there will be more coming, or internal_flags, no idea...
something_flags...

>         wait_queue_head_t sync_txq;
>
>         char priv[] __aligned(NETDEV_ALIGN);
> diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> index 6cc4e5c7ba94..e36aca788ea2 100644
> --- a/net/mac802154/tx.c
> +++ b/net/mac802154/tx.c
> @@ -123,9 +123,13 @@ static int ieee802154_sync_queue(struct ieee802154_local *local)
>
>  int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
>  {
> +       int ret;
> +
>         ieee802154_hold_queue(local);
> +       ret = ieee802154_sync_queue(local);
> +       set_bit(0, &local->phy->queue_stopped);
>

Define the 0 as WPAN_PHY_STATE_QUEUE_STOPPED_BIT or something like
that, above wpan_phy.

- Alex

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

[Miquel Raynal]

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

> Hi,
> 
> On Tue, May 17, 2022 at 12:35 PM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > We should never start a transmission after the queue has been stopped.
> >
> > But because it might work we don't kill the function here but rather
> > warn loudly the user that something is wrong.
> >
> > Set an atomic when the queue will remain stopped. Reset this atomic when
> > the queue actually gets restarded. Just check this atomic to know if the
> > transmission is legitimate, warn if it is not.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/net/cfg802154.h |  1 +
> >  net/mac802154/tx.c      | 16 +++++++++++++++-
> >  net/mac802154/util.c    |  1 +
> >  3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > index 8881b6126b58..f4e7b3fe7cf0 100644
> > --- a/include/net/cfg802154.h
> > +++ b/include/net/cfg802154.h
> > @@ -218,6 +218,7 @@ struct wpan_phy {
> >         spinlock_t queue_lock;
> >         atomic_t ongoing_txs;
> >         atomic_t hold_txs;
> > +       unsigned long queue_stopped;  
> 
> Can we name it something like state_flags (as phy state flags)? Pretty
> sure there will be more coming, or internal_flags, no idea...
> something_flags...

'phy_flags'? Just 'flags', maybe?

state_flags seems a bit too specific, but if it's your favorite I don't
mind using it.

> 
> >         wait_queue_head_t sync_txq;
> >
> >         char priv[] __aligned(NETDEV_ALIGN);
> > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> > index 6cc4e5c7ba94..e36aca788ea2 100644
> > --- a/net/mac802154/tx.c
> > +++ b/net/mac802154/tx.c
> > @@ -123,9 +123,13 @@ static int ieee802154_sync_queue(struct ieee802154_local *local)
> >
> >  int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
> >  {
> > +       int ret;
> > +
> >         ieee802154_hold_queue(local);
> > +       ret = ieee802154_sync_queue(local);
> > +       set_bit(0, &local->phy->queue_stopped);
> >  
> 
> Define the 0 as WPAN_PHY_STATE_QUEUE_STOPPED_BIT or something like
> that, above wpan_phy.

Sure.

Thanks,
Miquèl

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

[Alexander Aring]

Hi,

On Wed, May 18, 2022 at 4:55 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>
> aahringo@redhat.com wrote on Tue, 17 May 2022 20:58:19 -0400:
>
> > Hi,
> >
> > On Tue, May 17, 2022 at 12:35 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:
> > >
> > > We should never start a transmission after the queue has been stopped.
> > >
> > > But because it might work we don't kill the function here but rather
> > > warn loudly the user that something is wrong.
> > >
> > > Set an atomic when the queue will remain stopped. Reset this atomic when
> > > the queue actually gets restarded. Just check this atomic to know if the
> > > transmission is legitimate, warn if it is not.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  include/net/cfg802154.h |  1 +
> > >  net/mac802154/tx.c      | 16 +++++++++++++++-
> > >  net/mac802154/util.c    |  1 +
> > >  3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > index 8881b6126b58..f4e7b3fe7cf0 100644
> > > --- a/include/net/cfg802154.h
> > > +++ b/include/net/cfg802154.h
> > > @@ -218,6 +218,7 @@ struct wpan_phy {
> > >         spinlock_t queue_lock;
> > >         atomic_t ongoing_txs;
> > >         atomic_t hold_txs;
> > > +       unsigned long queue_stopped;
> >
> > Can we name it something like state_flags (as phy state flags)? Pretty
> > sure there will be more coming, or internal_flags, no idea...
> > something_flags...
>
> 'phy_flags'? Just 'flags', maybe?

make it so.

- Alex

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

[Miquel Raynal]

aahringo@redhat.com wrote on Wed, 18 May 2022 10:31:55 -0400:

> Hi,
> 
> On Wed, May 18, 2022 at 4:55 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> >
> > aahringo@redhat.com wrote on Tue, 17 May 2022 20:58:19 -0400:
> >  
> > > Hi,
> > >
> > > On Tue, May 17, 2022 at 12:35 PM Miquel Raynal
> > > <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > We should never start a transmission after the queue has been stopped.
> > > >
> > > > But because it might work we don't kill the function here but rather
> > > > warn loudly the user that something is wrong.
> > > >
> > > > Set an atomic when the queue will remain stopped. Reset this atomic when
> > > > the queue actually gets restarded. Just check this atomic to know if the
> > > > transmission is legitimate, warn if it is not.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  include/net/cfg802154.h |  1 +
> > > >  net/mac802154/tx.c      | 16 +++++++++++++++-
> > > >  net/mac802154/util.c    |  1 +
> > > >  3 files changed, 17 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > > index 8881b6126b58..f4e7b3fe7cf0 100644
> > > > --- a/include/net/cfg802154.h
> > > > +++ b/include/net/cfg802154.h
> > > > @@ -218,6 +218,7 @@ struct wpan_phy {
> > > >         spinlock_t queue_lock;
> > > >         atomic_t ongoing_txs;
> > > >         atomic_t hold_txs;
> > > > +       unsigned long queue_stopped;  
> > >
> > > Can we name it something like state_flags (as phy state flags)? Pretty
> > > sure there will be more coming, or internal_flags, no idea...
> > > something_flags...  
> >
> > 'phy_flags'? Just 'flags', maybe?  
> 
> make it so.

Oh, there is already a flags entry in wpan_phy. I've adjusted the
naming to what existed (keeping the _STATE_ prefix) and kept that
"flags" entry instead of creating a new one.

Thanks,
Miquèl

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

[Alexander Aring]

Hi,

On Wed, May 18, 2022 at 12:29 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
>
> aahringo@redhat.com wrote on Wed, 18 May 2022 10:31:55 -0400:
>
> > Hi,
> >
> > On Wed, May 18, 2022 at 4:55 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > >
> > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:58:19 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Tue, May 17, 2022 at 12:35 PM Miquel Raynal
> > > > <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > We should never start a transmission after the queue has been stopped.
> > > > >
> > > > > But because it might work we don't kill the function here but rather
> > > > > warn loudly the user that something is wrong.
> > > > >
> > > > > Set an atomic when the queue will remain stopped. Reset this atomic when
> > > > > the queue actually gets restarded. Just check this atomic to know if the
> > > > > transmission is legitimate, warn if it is not.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > ---
> > > > >  include/net/cfg802154.h |  1 +
> > > > >  net/mac802154/tx.c      | 16 +++++++++++++++-
> > > > >  net/mac802154/util.c    |  1 +
> > > > >  3 files changed, 17 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> > > > > index 8881b6126b58..f4e7b3fe7cf0 100644
> > > > > --- a/include/net/cfg802154.h
> > > > > +++ b/include/net/cfg802154.h
> > > > > @@ -218,6 +218,7 @@ struct wpan_phy {
> > > > >         spinlock_t queue_lock;
> > > > >         atomic_t ongoing_txs;
> > > > >         atomic_t hold_txs;
> > > > > +       unsigned long queue_stopped;
> > > >
> > > > Can we name it something like state_flags (as phy state flags)? Pretty
> > > > sure there will be more coming, or internal_flags, no idea...
> > > > something_flags...
> > >
> > > 'phy_flags'? Just 'flags', maybe?
> >
> > make it so.
>
> Oh, there is already a flags entry in wpan_phy. I've adjusted the
> naming to what existed (keeping the _STATE_ prefix) and kept that
> "flags" entry instead of creating a new one.
>
make it so.

- Alex

[PATCH wpan-next v3 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 e36aca788ea2..53a8be822e33 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -132,6 +132,25 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local)
 	return ret;
 }
 
+static bool ieee802154_netif_is_down(struct ieee802154_local *local)
+{
+	struct ieee802154_sub_if_data *sdata;
+	bool is_down = true;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!sdata->dev)
+			continue;
+
+		is_down = !(sdata->dev->flags & IFF_UP);
+		if (is_down)
+			break;
+	}
+	rcu_read_unlock();
+
+	return is_down;
+}
+
 static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
 {
 	return ieee802154_sync_and_hold_queue(local);
@@ -150,6 +169,12 @@ static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *sk
 	if (!local->open_count)
 		return -EBUSY;
 
+	/* Warn if the ieee802154 core thinks MLME frames can be sent while the
+	 * net interface expects this cannot happen.
+	 */
+	if (WARN_ON_ONCE(ieee802154_netif_is_down(local)))
+		return -EHOSTDOWN;
+
 	ieee802154_tx(local, skb);
 	ret = ieee802154_sync_queue(local);
 
-- 
2.34.1

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

[Alexander Aring]

Hi,

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

I thought that the helper would be "netif_running()". It seems there
are multiple ways to check if an interface is up.

> +       }
> +       rcu_read_unlock();
> +
> +       return is_down;
> +}
> +
>  static int ieee802154_mlme_op_pre(struct ieee802154_local *local)
>  {
>         return ieee802154_sync_and_hold_queue(local);
> @@ -150,6 +169,12 @@ static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *sk
>         if (!local->open_count)
>                 return -EBUSY;
>
> +       /* Warn if the ieee802154 core thinks MLME frames can be sent while the
> +        * net interface expects this cannot happen.
> +        */
> +       if (WARN_ON_ONCE(ieee802154_netif_is_down(local)))
> +               return -EHOSTDOWN;

maybe also ENETDOWN? Also there is a missing rtnl_unlock().

- Alex

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

[Miquel Raynal]

Hi Alex,

aahringo@redhat.com wrote on Tue, 17 May 2022 20:52:35 -0400:

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

Looking at net/core/dev.c shows that netif_running() may indicate a
transitory state (the interface was requested to be up or down) while
the IFF_UP flag really tells when the operation is done and the
interface actually is up or down.

Anyway they are both updated atomically wrt the rtnl lock. The sooner
the better, so let's check the interface state with netif_running().


[...]

> >
> > +       /* Warn if the ieee802154 core thinks MLME frames can be sent while the
> > +        * net interface expects this cannot happen.
> > +        */
> > +       if (WARN_ON_ONCE(ieee802154_netif_is_down(local)))
> > +               return -EHOSTDOWN;  
> 
> maybe also ENETDOWN?

Sure

> Also there is a missing rtnl_unlock().

Duly noted.

Thanks,
Miquèl