[PATCH wpan/next v3 0/9] net: ieee802154: Support scanning/beaconing

[PATCH wpan/next v3 0/9] net: ieee802154: Support scanning/beaconing

[Miquel Raynal]

Hello,

A third version of this series, dropping the scan patches for now
because before we need to settle on the filtering topic and the
coordinator interface topic. Here is just the filtering part, I've
integrated Alexander's patches, as well as the atusb fix. Once this is
merge there are a few coordinator-related patches, and finally the
scan.

Thanks,
Miquèl

Changes in v3:
* Full rework of the way the promiscuous mode is handled, with new
  filtering levels, one per-phy and the actual one on the device.
* Dropped all the manual acking, everything is happenging on hardware.
* Better handling of the Acks in atusb to report the trac status.


Alexander Aring (3):
  net: mac802154: move receive parameters above start
  net: mac802154: set filter at drv_start()
  ieee802154: atusb: add support for trac feature

Miquel Raynal (6):
  net: mac802154: Introduce filtering levels
  net: mac802154: Don't limit the FILTER_NONE level to monitors
  net: mac802154: Drop IEEE802154_HW_RX_DROP_BAD_CKSUM
  net: mac802154: Add promiscuous software filtering
  net: mac802154: Ensure proper scan-level filtering
  net: mac802154: Ensure proper general purpose frame filtering

 drivers/net/ieee802154/atusb.c           |  33 ++-
 drivers/net/ieee802154/mac802154_hwsim.c |   3 +-
 include/linux/ieee802154.h               |  24 ++
 include/net/cfg802154.h                  |   7 +-
 include/net/ieee802154_netdev.h          |   8 +
 include/net/mac802154.h                  |   4 -
 net/mac802154/cfg.c                      |   3 +-
 net/mac802154/driver-ops.h               | 284 ++++++++++++++---------
 net/mac802154/ieee802154_i.h             |   4 +
 net/mac802154/iface.c                    |  44 ++--
 net/mac802154/rx.c                       | 129 +++++++++-
 11 files changed, 388 insertions(+), 155 deletions(-)

-- 
2.34.1

[PATCH wpan/next v3 1/9] net: mac802154: Introduce filtering levels

[Miquel Raynal]

The 802154 specification details several filtering levels in which the
PHY and the MAC could be. The amount of filtering will vary if they are
in promiscuous mode or in scanning mode. Otherwise they are expected to
do some very basic checks, such as enforcing the frame validity. Either
the PHY is able to do so, and the MAC has nothing to do, or the PHY has
a lower filtering level than expected and the MAC should take over.

For now we just define these levels in an enumeration.

In a second time, we will add a per-PHY parameter showing the expected
filtering level as well as a per device current filtering level, and
will initialize all these fields.

In a third time, we will use them to apply more filtering by software
when the PHY is limited.

Indeed, if the drivers know they cannot reach the requested level of
filtering, they will overwrite the "current filtering" parameter so that
it reflects what they do. Then, in the core, the expected filtering
level will be used to decide whether some additional software processing
is needed or not.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/ieee802154.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
index f1f9412b6ac6..0303eb84d596 100644
--- a/include/linux/ieee802154.h
+++ b/include/linux/ieee802154.h
@@ -276,6 +276,30 @@ enum {
 	IEEE802154_SYSTEM_ERROR = 0xff,
 };
 
+/**
+ * enum ieee802154_filtering_level - Filtering levels applicable to a PHY
+ *
+ * @IEEE802154_FILTERING_NONE: No filtering at all, what is received is
+ *	forwarded to the softMAC
+ * @IEEE802154_FILTERING_1_FCS: First filtering level, frames with an invalid
+ *	FCS should be dropped
+ * @IEEE802154_FILTERING_2_PROMISCUOUS: Second filtering level, promiscuous
+ *	mode as described in the spec, identical in terms of filtering to the
+ *	level one on PHY side, but at the MAC level the frame should be
+ *	forwarded to the upper layer directly
+ * @IEEE802154_FILTERING_3_SCAN: Third filtering level, scan related, where
+ *	only beacons must be processed, all remaining traffic gets dropped
+ * @IEEE802154_FILTERING_4_FRAME_FIELDS: Fourth filtering level actually
+ *	enforcing the validity of the content of the frame with various checks
+ */
+enum ieee802154_filtering_level {
+	IEEE802154_FILTERING_NONE,
+	IEEE802154_FILTERING_1_FCS,
+	IEEE802154_FILTERING_2_PROMISCUOUS,
+	IEEE802154_FILTERING_3_SCAN,
+	IEEE802154_FILTERING_4_FRAME_FIELDS,
+};
+
 /* frame control handling */
 #define IEEE802154_FCTL_FTYPE		0x0003
 #define IEEE802154_FCTL_ACKREQ		0x0020
-- 
2.34.1

[PATCH wpan/next v3 2/9] net: mac802154: move receive parameters above start

[Miquel Raynal]

From: Alexander Aring <aahringo@redhat.com>

This patch moves all receive parameters above the drv_start()
functionality to make it accessibile in the drv_start() function.

Signed-off-by: Alexander Aring <aahringo@redhat.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/mac802154/driver-ops.h | 210 ++++++++++++++++++-------------------
 1 file changed, 105 insertions(+), 105 deletions(-)

diff --git a/net/mac802154/driver-ops.h b/net/mac802154/driver-ops.h
index d23f0db98015..c9d54088a567 100644
--- a/net/mac802154/driver-ops.h
+++ b/net/mac802154/driver-ops.h
@@ -24,6 +24,111 @@ drv_xmit_sync(struct ieee802154_local *local, struct sk_buff *skb)
 	return local->ops->xmit_sync(&local->hw, skb);
 }
 
+static inline int drv_set_pan_id(struct ieee802154_local *local, __le16 pan_id)
+{
+	struct ieee802154_hw_addr_filt filt;
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_hw_addr_filt) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	filt.pan_id = pan_id;
+
+	trace_802154_drv_set_pan_id(local, pan_id);
+	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
+					    IEEE802154_AFILT_PANID_CHANGED);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
+static inline int
+drv_set_extended_addr(struct ieee802154_local *local, __le64 extended_addr)
+{
+	struct ieee802154_hw_addr_filt filt;
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_hw_addr_filt) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	filt.ieee_addr = extended_addr;
+
+	trace_802154_drv_set_extended_addr(local, extended_addr);
+	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
+					    IEEE802154_AFILT_IEEEADDR_CHANGED);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
+static inline int
+drv_set_short_addr(struct ieee802154_local *local, __le16 short_addr)
+{
+	struct ieee802154_hw_addr_filt filt;
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_hw_addr_filt) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	filt.short_addr = short_addr;
+
+	trace_802154_drv_set_short_addr(local, short_addr);
+	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
+					    IEEE802154_AFILT_SADDR_CHANGED);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
+static inline int
+drv_set_pan_coord(struct ieee802154_local *local, bool is_coord)
+{
+	struct ieee802154_hw_addr_filt filt;
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_hw_addr_filt) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	filt.pan_coord = is_coord;
+
+	trace_802154_drv_set_pan_coord(local, is_coord);
+	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
+					    IEEE802154_AFILT_PANC_CHANGED);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
+static inline int
+drv_set_promiscuous_mode(struct ieee802154_local *local, bool on)
+{
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->set_promiscuous_mode) {
+		WARN_ON(1);
+		return -EOPNOTSUPP;
+	}
+
+	trace_802154_drv_set_promiscuous_mode(local, on);
+	ret = local->ops->set_promiscuous_mode(&local->hw, on);
+	trace_802154_drv_return_int(local, ret);
+	return ret;
+}
+
 static inline int drv_start(struct ieee802154_local *local)
 {
 	int ret;
@@ -138,93 +243,6 @@ drv_set_cca_ed_level(struct ieee802154_local *local, s32 mbm)
 	return ret;
 }
 
-static inline int drv_set_pan_id(struct ieee802154_local *local, __le16 pan_id)
-{
-	struct ieee802154_hw_addr_filt filt;
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_hw_addr_filt) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	filt.pan_id = pan_id;
-
-	trace_802154_drv_set_pan_id(local, pan_id);
-	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
-					    IEEE802154_AFILT_PANID_CHANGED);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
-static inline int
-drv_set_extended_addr(struct ieee802154_local *local, __le64 extended_addr)
-{
-	struct ieee802154_hw_addr_filt filt;
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_hw_addr_filt) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	filt.ieee_addr = extended_addr;
-
-	trace_802154_drv_set_extended_addr(local, extended_addr);
-	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
-					    IEEE802154_AFILT_IEEEADDR_CHANGED);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
-static inline int
-drv_set_short_addr(struct ieee802154_local *local, __le16 short_addr)
-{
-	struct ieee802154_hw_addr_filt filt;
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_hw_addr_filt) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	filt.short_addr = short_addr;
-
-	trace_802154_drv_set_short_addr(local, short_addr);
-	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
-					    IEEE802154_AFILT_SADDR_CHANGED);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
-static inline int
-drv_set_pan_coord(struct ieee802154_local *local, bool is_coord)
-{
-	struct ieee802154_hw_addr_filt filt;
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_hw_addr_filt) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	filt.pan_coord = is_coord;
-
-	trace_802154_drv_set_pan_coord(local, is_coord);
-	ret = local->ops->set_hw_addr_filt(&local->hw, &filt,
-					    IEEE802154_AFILT_PANC_CHANGED);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
 static inline int
 drv_set_csma_params(struct ieee802154_local *local, u8 min_be, u8 max_be,
 		    u8 max_csma_backoffs)
@@ -264,22 +282,4 @@ drv_set_max_frame_retries(struct ieee802154_local *local, s8 max_frame_retries)
 	return ret;
 }
 
-static inline int
-drv_set_promiscuous_mode(struct ieee802154_local *local, bool on)
-{
-	int ret;
-
-	might_sleep();
-
-	if (!local->ops->set_promiscuous_mode) {
-		WARN_ON(1);
-		return -EOPNOTSUPP;
-	}
-
-	trace_802154_drv_set_promiscuous_mode(local, on);
-	ret = local->ops->set_promiscuous_mode(&local->hw, on);
-	trace_802154_drv_return_int(local, ret);
-	return ret;
-}
-
 #endif /* __MAC802154_DRIVER_OPS */
-- 
2.34.1

[PATCH wpan/next v3 3/9] net: mac802154: set filter at drv_start()

[Miquel Raynal]

From: Alexander Aring <aahringo@redhat.com>

The current filtering level is set on the first interface up on a wpan
phy. If we support scan functionality we need to change the filtering
level on the fly on an operational phy and switching back again.

This patch will move the receive mode parameter e.g. address filter and
promiscuous mode to the drv_start() functionality to allow changing the
receive mode on an operational phy not on first ifup only. In future this
should be handled on driver layer because each hardware has it's own way
to enter a specific filtering level. However this should offer to switch
to mode IEEE802154_FILTERING_NONE and back to
IEEE802154_FILTERING_4_FRAME_FIELDS.

Only IEEE802154_FILTERING_4_FRAME_FIELDS and IEEE802154_FILTERING_NONE
are somewhat supported by current hardware. All other filtering levels
can be supported in future but will end in IEEE802154_FILTERING_NONE as
the receive part can kind of "emulate" those receive paths by doing
additional filtering routines.

Signed-off-by: Alexander Aring <aahringo@redhat.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/cfg802154.h      |  7 +++-
 net/mac802154/cfg.c          |  3 +-
 net/mac802154/driver-ops.h   | 74 +++++++++++++++++++++++++++++++++++-
 net/mac802154/ieee802154_i.h |  4 ++
 net/mac802154/iface.c        | 44 ++++++++-------------
 net/mac802154/rx.c           |  7 +++-
 6 files changed, 106 insertions(+), 33 deletions(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 04b996895fc1..7129b404871e 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -223,6 +223,11 @@ struct wpan_phy {
 	atomic_t hold_txs;
 	wait_queue_head_t sync_txq;
 
+	/* Current filtering level on reception.
+	 * Only allowed to be changed if phy is not operational.
+	 */
+	enum ieee802154_filtering_level filtering;
+
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
@@ -374,8 +379,6 @@ struct wpan_dev {
 
 	bool lbt;
 
-	bool promiscuous_mode;
-
 	/* fallback for acknowledgment bit setting */
 	bool ackreq;
 };
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index 93df24f75572..363d465f816a 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -67,7 +67,8 @@ static int ieee802154_resume(struct wpan_phy *wpan_phy)
 		goto wake_up;
 
 	/* restart hardware */
-	ret = drv_start(local);
+	ret = drv_start(local, local->phy->filtering,
+			&local->addr_filt);
 	if (ret)
 		return ret;
 
diff --git a/net/mac802154/driver-ops.h b/net/mac802154/driver-ops.h
index c9d54088a567..1cace614ba11 100644
--- a/net/mac802154/driver-ops.h
+++ b/net/mac802154/driver-ops.h
@@ -129,12 +129,84 @@ drv_set_promiscuous_mode(struct ieee802154_local *local, bool on)
 	return ret;
 }
 
-static inline int drv_start(struct ieee802154_local *local)
+static inline int drv_start(struct ieee802154_local *local,
+			    enum ieee802154_filtering_level level,
+			    const struct ieee802154_hw_addr_filt *addr_filt)
 {
 	int ret;
 
 	might_sleep();
 
+	/* setup receive mode parameters e.g. address mode */
+	if (local->hw.flags & IEEE802154_HW_AFILT) {
+		ret = drv_set_pan_id(local, addr_filt->pan_id);
+		if (ret < 0)
+			return ret;
+
+		ret = drv_set_short_addr(local, addr_filt->short_addr);
+		if (ret < 0)
+			return ret;
+
+		ret = drv_set_extended_addr(local, addr_filt->ieee_addr);
+		if (ret < 0)
+			return ret;
+	}
+
+	switch (level) {
+	case IEEE802154_FILTERING_NONE:
+		fallthrough;
+	case IEEE802154_FILTERING_1_FCS:
+		fallthrough;
+	case IEEE802154_FILTERING_2_PROMISCUOUS:
+		/* TODO requires a different receive mode setup e.g.
+		 * at86rf233 hardware.
+		 */
+		fallthrough;
+	case IEEE802154_FILTERING_3_SCAN:
+		if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
+			ret = drv_set_promiscuous_mode(local, true);
+			if (ret < 0)
+				return ret;
+		} else {
+			return -EOPNOTSUPP;
+		}
+
+		/* may requested other filtering but for now
+		 * all hardware can support
+		 * IEEE802154_FILTERING_NONE only which is a kind of
+		 * "compatible" mode we just need to filter more in
+		 * mac802154 receive path.
+		 *
+		 * TODO: move to driver and hardware may return more
+		 * higher level filter if supported. Hardware may
+		 * require also a different order how register are
+		 * set which could currently be buggy, so all receive
+		 * parameter need to be moved to the start() callback
+		 * and let the driver go into the mode before it will
+		 * turn on receive handling.
+		 */
+		local->phy->filtering = IEEE802154_FILTERING_NONE;
+		break;
+	case IEEE802154_FILTERING_4_FRAME_FIELDS:
+		/* not error out here for now as if hw doesn't support
+		 * IEEE802154_HW_PROMISCUOUS it's in
+		 * IEEE802154_FILTERING_4_FRAME_FIELDS
+		 * we have only a problem if some user switches and do not
+		 * support IEEE802154_HW_PROMISCUOUS.
+		 */
+		if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
+			ret = drv_set_promiscuous_mode(local, false);
+			if (ret < 0)
+				return ret;
+		}
+
+		local->phy->filtering = IEEE802154_FILTERING_4_FRAME_FIELDS;
+		break;
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
 	trace_802154_drv_start(local);
 	local->started = true;
 	smp_mb();
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h
index 010365a6364e..6c6da0d624e0 100644
--- a/net/mac802154/ieee802154_i.h
+++ b/net/mac802154/ieee802154_i.h
@@ -26,6 +26,8 @@ struct ieee802154_local {
 	struct ieee802154_hw hw;
 	const struct ieee802154_ops *ops;
 
+	/* hardware address filter */
+	struct ieee802154_hw_addr_filt addr_filt;
 	/* ieee802154 phy */
 	struct wpan_phy *phy;
 
@@ -82,6 +84,8 @@ struct ieee802154_sub_if_data {
 	struct ieee802154_local *local;
 	struct net_device *dev;
 
+	enum ieee802154_filtering_level required_filtering;
+
 	unsigned long state;
 	char name[IFNAMSIZ];
 
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 500ed1b81250..bf95e710becc 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -147,25 +147,12 @@ static int ieee802154_setup_hw(struct ieee802154_sub_if_data *sdata)
 	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
 	int ret;
 
-	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
-		ret = drv_set_promiscuous_mode(local,
-					       wpan_dev->promiscuous_mode);
-		if (ret < 0)
-			return ret;
-	}
+	local->phy->filtering = sdata->required_filtering;
 
 	if (local->hw.flags & IEEE802154_HW_AFILT) {
-		ret = drv_set_pan_id(local, wpan_dev->pan_id);
-		if (ret < 0)
-			return ret;
-
-		ret = drv_set_extended_addr(local, wpan_dev->extended_addr);
-		if (ret < 0)
-			return ret;
-
-		ret = drv_set_short_addr(local, wpan_dev->short_addr);
-		if (ret < 0)
-			return ret;
+		local->addr_filt.pan_id = wpan_dev->pan_id;
+		local->addr_filt.ieee_addr = wpan_dev->extended_addr;
+		local->addr_filt.short_addr = wpan_dev->short_addr;
 	}
 
 	if (local->hw.flags & IEEE802154_HW_LBT) {
@@ -206,7 +193,8 @@ static int mac802154_slave_open(struct net_device *dev)
 		if (res)
 			goto err;
 
-		res = drv_start(local);
+		res = drv_start(local, local->phy->filtering,
+				&local->addr_filt);
 		if (res)
 			goto err;
 	}
@@ -223,15 +211,16 @@ static int mac802154_slave_open(struct net_device *dev)
 
 static int
 ieee802154_check_mac_settings(struct ieee802154_local *local,
-			      struct wpan_dev *wpan_dev,
-			      struct wpan_dev *nwpan_dev)
+			      struct ieee802154_sub_if_data *sdata,
+			      struct ieee802154_sub_if_data *nsdata)
 {
+	struct wpan_dev *nwpan_dev = &nsdata->wpan_dev;
+	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+
 	ASSERT_RTNL();
 
-	if (local->hw.flags & IEEE802154_HW_PROMISCUOUS) {
-		if (wpan_dev->promiscuous_mode != nwpan_dev->promiscuous_mode)
-			return -EBUSY;
-	}
+	if (sdata->required_filtering != nsdata->required_filtering)
+		return -EBUSY;
 
 	if (local->hw.flags & IEEE802154_HW_AFILT) {
 		if (wpan_dev->pan_id != nwpan_dev->pan_id ||
@@ -285,8 +274,7 @@ ieee802154_check_concurrent_iface(struct ieee802154_sub_if_data *sdata,
 			/* check all phy mac sublayer settings are the same.
 			 * We have only one phy, different values makes trouble.
 			 */
-			ret = ieee802154_check_mac_settings(local, wpan_dev,
-							    &nsdata->wpan_dev);
+			ret = ieee802154_check_mac_settings(local, sdata, nsdata);
 			if (ret < 0)
 				return ret;
 		}
@@ -586,7 +574,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 		sdata->dev->priv_destructor = mac802154_wpan_free;
 		sdata->dev->netdev_ops = &mac802154_wpan_ops;
 		sdata->dev->ml_priv = &mac802154_mlme_wpan;
-		wpan_dev->promiscuous_mode = false;
+		sdata->required_filtering = IEEE802154_FILTERING_4_FRAME_FIELDS;
 		wpan_dev->header_ops = &ieee802154_header_ops;
 
 		mutex_init(&sdata->sec_mtx);
@@ -600,7 +588,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 	case NL802154_IFTYPE_MONITOR:
 		sdata->dev->needs_free_netdev = true;
 		sdata->dev->netdev_ops = &mac802154_monitor_ops;
-		wpan_dev->promiscuous_mode = true;
+		sdata->required_filtering = IEEE802154_FILTERING_NONE;
 		break;
 	default:
 		BUG();
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index c0fd8d0c7f03..369ffd800abf 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -273,7 +273,12 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
 	/* Check if transceiver doesn't validate the checksum.
 	 * If not we validate the checksum here.
 	 */
-	if (local->hw.flags & IEEE802154_HW_RX_DROP_BAD_CKSUM) {
+	/* TODO do whatever you want here if necessary to filter by
+	 * check on IEEE802154_FILTERING_NONE. And upcomming receive
+	 * path in which state the phy is e.g. scanning.
+	 */
+	if (local->hw.flags & IEEE802154_HW_RX_DROP_BAD_CKSUM ||
+	    local->phy->filtering == IEEE802154_FILTERING_NONE) {
 		crc = crc_ccitt(0, skb->data, skb->len);
 		if (crc) {
 			rcu_read_unlock();
-- 
2.34.1

[PATCH wpan/next v3 4/9] net: mac802154: Don't limit the FILTER_NONE level to monitors

[Miquel Raynal]

Historically, only monitors were using promiscuous mode and this
promiscuous mode was not the one from the spec but actually implied no
filtering at all (sniffers). Now that we have a more fine grained
approach, we can ensure all interfaces which would be expecting no
filter at all could get it without being a monitor interface.

Having this in place will allow us to clarify the additional software
checks compared to the hardware capabilities.

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

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 369ffd800abf..26df79911f3e 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -205,10 +205,10 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
 	}
 
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
-		if (sdata->wpan_dev.iftype != NL802154_IFTYPE_NODE)
+		if (!ieee802154_sdata_running(sdata))
 			continue;
 
-		if (!ieee802154_sdata_running(sdata))
+		if (sdata->required_filtering == IEEE802154_FILTERING_NONE)
 			continue;
 
 		ieee802154_subif_frame(sdata, skb, &hdr);
@@ -231,10 +231,10 @@ ieee802154_monitors_rx(struct ieee802154_local *local, struct sk_buff *skb)
 	skb->protocol = htons(ETH_P_IEEE802154);
 
 	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
-		if (sdata->wpan_dev.iftype != NL802154_IFTYPE_MONITOR)
+		if (!ieee802154_sdata_running(sdata))
 			continue;
 
-		if (!ieee802154_sdata_running(sdata))
+		if (sdata->required_filtering > IEEE802154_FILTERING_NONE)
 			continue;
 
 		skb2 = skb_clone(skb, GFP_ATOMIC);
-- 
2.34.1

[PATCH wpan/next v3 5/9] net: mac802154: Drop IEEE802154_HW_RX_DROP_BAD_CKSUM

[Miquel Raynal]

This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to
reflect the fact that it would not validate the checksum (FCS). In other
words, the filtering level of hwsim is always "NONE" while the core
expects it to be higher.

Now that we have access to real filtering levels, we can actually use
them and always enforce the "NONE" level in hwsim. This case is already
correctly handled in the receive so we can drop the flag.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/mac802154_hwsim.c | 3 ++-
 include/net/mac802154.h                  | 4 ----
 net/mac802154/rx.c                       | 7 ++-----
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
index 38c217bd7c82..d18a1391b61f 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -148,6 +148,7 @@ static int hwsim_hw_start(struct ieee802154_hw *hw)
 	struct hwsim_phy *phy = hw->priv;
 
 	phy->suspended = false;
+	hw->phy->filtering = IEEE802154_FILTERING_NONE;
 	return 0;
 }
 
@@ -791,7 +792,7 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
 	phy->idx = idx;
 	INIT_LIST_HEAD(&phy->edges);
 
-	hw->flags = IEEE802154_HW_PROMISCUOUS | IEEE802154_HW_RX_DROP_BAD_CKSUM;
+	hw->flags = IEEE802154_HW_PROMISCUOUS;
 	hw->parent = dev;
 
 	err = ieee802154_register_hw(hw);
diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index 357d25ef627a..4a3a9de9da73 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -111,9 +111,6 @@ struct ieee802154_hw {
  *	promiscuous mode setting.
  *
  * @IEEE802154_HW_RX_OMIT_CKSUM: Indicates that receiver omits FCS.
- *
- * @IEEE802154_HW_RX_DROP_BAD_CKSUM: Indicates that receiver will not filter
- *	frames with bad checksum.
  */
 enum ieee802154_hw_flags {
 	IEEE802154_HW_TX_OMIT_CKSUM	= BIT(0),
@@ -123,7 +120,6 @@ enum ieee802154_hw_flags {
 	IEEE802154_HW_AFILT		= BIT(4),
 	IEEE802154_HW_PROMISCUOUS	= BIT(5),
 	IEEE802154_HW_RX_OMIT_CKSUM	= BIT(6),
-	IEEE802154_HW_RX_DROP_BAD_CKSUM	= BIT(7),
 };
 
 /* Indicates that receiver omits FCS and xmitter will add FCS on it's own. */
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 26df79911f3e..bd1a92fceef7 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -270,15 +270,12 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
 
 	ieee802154_monitors_rx(local, skb);
 
-	/* Check if transceiver doesn't validate the checksum.
-	 * If not we validate the checksum here.
-	 */
+	/* Level 1 filtering: Check the FCS by software when relevant */
 	/* TODO do whatever you want here if necessary to filter by
 	 * check on IEEE802154_FILTERING_NONE. And upcomming receive
 	 * path in which state the phy is e.g. scanning.
 	 */
-	if (local->hw.flags & IEEE802154_HW_RX_DROP_BAD_CKSUM ||
-	    local->phy->filtering == IEEE802154_FILTERING_NONE) {
+	if (local->hw.phy->filtering == IEEE802154_FILTERING_NONE) {
 		crc = crc_ccitt(0, skb->data, skb->len);
 		if (crc) {
 			rcu_read_unlock();
-- 
2.34.1

[PATCH wpan/next v3 6/9] net: mac802154: Add promiscuous software filtering

[Miquel Raynal]

Currently, the promiscuous mode was not as open as it should. It was not
a big deal because until now promiscuous modes were only used on monitor
interfaces, which would never go this far in the filtering. But as we
might now use this promiscuous mode with NODEs or COORDs, it becomes
necessary to really forward the packets to the upper layers without
additional filtering when relevant. Let's add the necessary logic to
handle this situation.

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

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index bd1a92fceef7..8a8c5a4a2f28 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -196,10 +196,31 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
 	int ret;
 	struct ieee802154_sub_if_data *sdata;
 	struct ieee802154_hdr hdr;
+	struct sk_buff *skb2;
 
+	/* Level 2 filtering: Avoid further processing in IEEE 802.15.4 promiscuous modes */
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!ieee802154_sdata_running(sdata))
+			continue;
+
+		if (sdata->required_filtering < IEEE802154_FILTERING_1_FCS ||
+		    sdata->required_filtering > IEEE802154_FILTERING_2_PROMISCUOUS)
+			continue;
+
+		skb2 = skb_clone(skb, GFP_ATOMIC);
+		if (skb2) {
+			skb2->dev = sdata->dev;
+			ieee802154_deliver_skb(skb2);
+
+			sdata->dev->stats.rx_packets++;
+			sdata->dev->stats.rx_bytes += skb->len;
+		}
+	}
+
+	/* Common filtering between level 3 and 4: frame headers validity */
 	ret = ieee802154_parse_frame_start(skb, &hdr);
 	if (ret) {
-		pr_debug("got invalid frame\n");
+		dev_dbg(&sdata->dev->dev, "invalid frame headers\n");
 		kfree_skb(skb);
 		return;
 	}
@@ -208,7 +229,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
 		if (!ieee802154_sdata_running(sdata))
 			continue;
 
-		if (sdata->required_filtering == IEEE802154_FILTERING_NONE)
+		if (sdata->required_filtering <= IEEE802154_FILTERING_2_PROMISCUOUS)
 			continue;
 
 		ieee802154_subif_frame(sdata, skb, &hdr);
-- 
2.34.1

[PATCH wpan/next v3 7/9] net: mac802154: Ensure proper scan-level filtering

[Miquel Raynal]

We now have a fine grained filtering information so let's ensure proper
filtering in scan mode, which means that only beacons are processed.

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

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 8a8c5a4a2f28..c43289c0fdd7 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -34,6 +34,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		       struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 {
 	struct wpan_dev *wpan_dev = &sdata->wpan_dev;
+	struct wpan_phy *wpan_phy = sdata->local->hw.phy;
 	__le16 span, sshort;
 	int rc;
 
@@ -42,6 +43,17 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 	span = wpan_dev->pan_id;
 	sshort = wpan_dev->short_addr;
 
+	/* Level 3 filtering: Only beacons are accepted during scans */
+	if (sdata->required_filtering == IEEE802154_FILTERING_3_SCAN &&
+	    sdata->required_filtering > wpan_phy->filtering) {
+		if (mac_cb(skb)->type != IEEE802154_FC_TYPE_BEACON) {
+			dev_dbg(&sdata->dev->dev,
+				"drop !beacon frame (0x%x) during scan\n",
+				mac_cb(skb)->type);
+			goto fail;
+		}
+	}
+
 	switch (mac_cb(skb)->dest.mode) {
 	case IEEE802154_ADDR_NONE:
 		if (hdr->source.mode != IEEE802154_ADDR_NONE)
-- 
2.34.1

[PATCH wpan/next v3 8/9] net: mac802154: Ensure proper general purpose frame filtering

[Miquel Raynal]

Most of the PHYs seem to cope with the standard filtering rules by
default. Some of them might not, like hwsim which is only software, and
in this case advertises its real filtering level with the new
"filtering" internal value.

The core then needs to check what is expected by looking at the PHY
requested filtering level and possibly apply additional filtering
rules.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/ieee802154_netdev.h |  8 ++++
 net/mac802154/rx.c              | 78 +++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+)

diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
index d0d188c3294b..1b82bbafe8c7 100644
--- a/include/net/ieee802154_netdev.h
+++ b/include/net/ieee802154_netdev.h
@@ -69,6 +69,14 @@ struct ieee802154_hdr_fc {
 #endif
 };
 
+enum ieee802154_frame_version {
+	IEEE802154_2003_STD,
+	IEEE802154_2006_STD,
+	IEEE802154_STD,
+	IEEE802154_RESERVED_STD,
+	IEEE802154_MULTIPURPOSE_STD = IEEE802154_2003_STD,
+};
+
 struct ieee802154_hdr {
 	struct ieee802154_hdr_fc fc;
 	u8 seq;
diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index c43289c0fdd7..bc46e4a7669d 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -52,6 +52,84 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 				mac_cb(skb)->type);
 			goto fail;
 		}
+	} else if (sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS &&
+		   sdata->required_filtering > wpan_phy->filtering) {
+		/* Level 4 filtering: Frame fields validity */
+
+		/* a) Drop reserved frame types */
+		switch (mac_cb(skb)->type) {
+		case IEEE802154_FC_TYPE_BEACON:
+		case IEEE802154_FC_TYPE_DATA:
+		case IEEE802154_FC_TYPE_ACK:
+		case IEEE802154_FC_TYPE_MAC_CMD:
+			break;
+		default:
+			dev_dbg(&sdata->dev->dev, "unrecognized frame type 0x%x\n",
+				mac_cb(skb)->type);
+			goto fail;
+		}
+
+		/* b) Drop reserved frame versions */
+		switch (hdr->fc.version) {
+		case IEEE802154_2003_STD:
+		case IEEE802154_2006_STD:
+		case IEEE802154_STD:
+			break;
+		default:
+			dev_dbg(&sdata->dev->dev,
+				"unrecognized frame version 0x%x\n",
+				hdr->fc.version);
+			goto fail;
+		}
+
+		/* c) PAN ID constraints */
+		if ((mac_cb(skb)->dest.mode == IEEE802154_ADDR_LONG ||
+		     mac_cb(skb)->dest.mode == IEEE802154_ADDR_SHORT) &&
+		    mac_cb(skb)->dest.pan_id != span &&
+		    mac_cb(skb)->dest.pan_id != cpu_to_le16(IEEE802154_PANID_BROADCAST)) {
+			dev_dbg(&sdata->dev->dev,
+				"unrecognized PAN ID %04x\n",
+				le16_to_cpu(mac_cb(skb)->dest.pan_id));
+			goto fail;
+		}
+
+		/* d1) Short address constraints */
+		if (mac_cb(skb)->dest.mode == IEEE802154_ADDR_SHORT &&
+		    mac_cb(skb)->dest.short_addr != sshort &&
+		    mac_cb(skb)->dest.short_addr != cpu_to_le16(IEEE802154_ADDR_BROADCAST)) {
+			dev_dbg(&sdata->dev->dev,
+				"unrecognized short address %04x\n",
+				le16_to_cpu(mac_cb(skb)->dest.short_addr));
+			goto fail;
+		}
+
+		/* d2) Extended address constraints */
+		if (mac_cb(skb)->dest.mode == IEEE802154_ADDR_LONG &&
+		    mac_cb(skb)->dest.extended_addr != wpan_dev->extended_addr) {
+			dev_dbg(&sdata->dev->dev,
+				"unrecognized long address 0x%016llx\n",
+				mac_cb(skb)->dest.extended_addr);
+			goto fail;
+		}
+
+		/* d4) Specific PAN coordinator case (no parent) */
+		if ((mac_cb(skb)->type == IEEE802154_FC_TYPE_DATA ||
+		     mac_cb(skb)->type == IEEE802154_FC_TYPE_MAC_CMD) &&
+		    mac_cb(skb)->dest.mode == IEEE802154_ADDR_NONE) {
+			dev_dbg(&sdata->dev->dev,
+				"relaying is not supported\n");
+			goto fail;
+		}
+	}
+
+	/* e) Beacon frames follow specific PAN ID rules */
+	if (mac_cb(skb)->type == IEEE802154_FC_TYPE_BEACON &&
+	    span != cpu_to_le16(IEEE802154_PANID_BROADCAST) &&
+	    mac_cb(skb)->dest.pan_id != span) {
+		dev_dbg(&sdata->dev->dev,
+			"invalid beacon PAN ID %04x\n",
+			le16_to_cpu(mac_cb(skb)->dest.pan_id));
+		goto fail;
 	}
 
 	switch (mac_cb(skb)->dest.mode) {
-- 
2.34.1

[PATCH wpan/next v3 9/9] ieee802154: atusb: add support for trac feature

[Miquel Raynal]

From: Alexander Aring <aahringo@redhat.com>

This patch adds support for reading the trac register if atusb firmware
reports tx done. There is currently a feature to compare a sequence
number, if the payload is 1 it tells the driver only the sequence number
is available if it's two there is additional the trac status register as
payload.

Currently the atusb_in_good() function determines if it's a tx done or
rx done if according the payload length. This patch is doing the same
and assumes this behaviour.

Signed-off-by: Alexander Aring <aahringo@redhat.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/atusb.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c
index 2c338783893d..95a4a3cdc8a4 100644
--- a/drivers/net/ieee802154/atusb.c
+++ b/drivers/net/ieee802154/atusb.c
@@ -191,7 +191,7 @@ static void atusb_work_urbs(struct work_struct *work)
 
 /* ----- Asynchronous USB -------------------------------------------------- */
 
-static void atusb_tx_done(struct atusb *atusb, u8 seq)
+static void atusb_tx_done(struct atusb *atusb, u8 seq, int reason)
 {
 	struct usb_device *usb_dev = atusb->usb_dev;
 	u8 expect = atusb->tx_ack_seq;
@@ -199,7 +199,10 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq)
 	dev_dbg(&usb_dev->dev, "%s (0x%02x/0x%02x)\n", __func__, seq, expect);
 	if (seq == expect) {
 		/* TODO check for ifs handling in firmware */
-		ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false);
+		if (reason == IEEE802154_SUCCESS)
+			ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false);
+		else
+			ieee802154_xmit_error(atusb->hw, atusb->tx_skb, reason);
 	} else {
 		/* TODO I experience this case when atusb has a tx complete
 		 * irq before probing, we should fix the firmware it's an
@@ -215,7 +218,8 @@ static void atusb_in_good(struct urb *urb)
 	struct usb_device *usb_dev = urb->dev;
 	struct sk_buff *skb = urb->context;
 	struct atusb *atusb = SKB_ATUSB(skb);
-	u8 len, lqi;
+	int result = IEEE802154_SUCCESS;
+	u8 len, lqi, trac;
 
 	if (!urb->actual_length) {
 		dev_dbg(&usb_dev->dev, "atusb_in: zero-sized URB ?\n");
@@ -224,8 +228,27 @@ static void atusb_in_good(struct urb *urb)
 
 	len = *skb->data;
 
-	if (urb->actual_length == 1) {
-		atusb_tx_done(atusb, len);
+	switch (urb->actual_length) {
+	case 2:
+		trac = TRAC_MASK(*(skb->data + 1));
+		switch (trac) {
+		case TRAC_SUCCESS:
+		case TRAC_SUCCESS_DATA_PENDING:
+			/* already IEEE802154_SUCCESS */
+			break;
+		case TRAC_CHANNEL_ACCESS_FAILURE:
+			result = IEEE802154_CHANNEL_ACCESS_FAILURE;
+			break;
+		case TRAC_NO_ACK:
+			result = IEEE802154_NO_ACK;
+			break;
+		default:
+			result = IEEE802154_SYSTEM_ERROR;
+		}
+
+		fallthrough;
+	case 1:
+		atusb_tx_done(atusb, len, result);
 		return;
 	}
 
-- 
2.34.1

Re: [PATCH wpan/next v3 0/9] net: ieee802154: Support scanning/beaconing

[Alexander Aring]

Hi,

On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hello,
>
> A third version of this series, dropping the scan patches for now
> because before we need to settle on the filtering topic and the
> coordinator interface topic. Here is just the filtering part, I've
> integrated Alexander's patches, as well as the atusb fix. Once this is
> merge there are a few coordinator-related patches, and finally the
> scan.

I think we have a communication problem here and we should talk about
what the problems are and agree on a way to solve them.

The problems are:

1. We never supported switching from an operating phy (interfaces are
up) into another filtering mode.

2. Scan requires to be in "promiscuous mode" (according to the
802.15.4 spec promiscuous mode). We don't support promiscuous mode
(according to the 802.15.4 spec promiscuous mode). We "can" however
use the currently supported mode which does not filter anything
(IEEE802154_FILTERING_NONE) when we do additional filtering in
mac802154. _But_ this is only required when the phy is scanning, it
will also deliver anything to the upper layers.

This patch-series tries to do the second thing, okay that's fine. But
I thought this should only be done while the phy is in "scanning
mode"? The other receive path while not in promiscuous mode
(phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never
require any additional filtering. I somehow miss this point here.

For 1), the driver should change the filtering mode" when we start to
"listen", this is done by the start() driver callback. They should get
all receive parameters and set up receiving to whatever mac802154,
currently there is a bit of chaos there. To move it into drv_start()
is just a workaround to begin this step that we move it at some point
to the driver. I mention 1) here because that should be part of the
picture how everything works together when the phy is switched to a
different filter level while it's operating (I mean there are running
interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS)
which then activates the different receive path for the use case of
scanning (something like (phy->state & WPANPHY_SCANING) == true)?

I am sorry, but I somehow miss the picture of how those things work
together. It is not clear for me and I miss those parts to get a whole
picture of this. For me it's not clear that those patches are going in
this direction.

- Alex

Re: [PATCH wpan/next v3 0/9] net: ieee802154: Support scanning/beaconing

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400:

> Hi,
> 
> On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hello,
> >
> > A third version of this series, dropping the scan patches for now
> > because before we need to settle on the filtering topic and the
> > coordinator interface topic. Here is just the filtering part, I've
> > integrated Alexander's patches, as well as the atusb fix. Once this is
> > merge there are a few coordinator-related patches, and finally the
> > scan.  
> 
> I think we have a communication problem here and we should talk about
> what the problems are and agree on a way to solve them.
> 
> The problems are:
> 
> 1. We never supported switching from an operating phy (interfaces are
> up) into another filtering mode.

In the trigger scan path there is a:

	mlme_op_pre() // stop Tx
	drv_stop() // stop Rx
	synchronize_net()
	drv_start(params) // restart Rx with another hw filtering level

> 2. Scan requires to be in "promiscuous mode" (according to the
> 802.15.4 spec promiscuous mode). We don't support promiscuous mode
> (according to the 802.15.4 spec promiscuous mode). We "can" however
> use the currently supported mode which does not filter anything
> (IEEE802154_FILTERING_NONE) when we do additional filtering in
> mac802154. _But_ this is only required when the phy is scanning, it
> will also deliver anything to the upper layers.
> 
> This patch-series tries to do the second thing, okay that's fine. But
> I thought this should only be done while the phy is in "scanning
> mode"?

I don't understand what's wrong then. We ask for the "scan mode"
filtering level when starting the scan [1] and we ask for the normal
filtering level when it's done/aborted [2] [3].

[1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326
[2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55

> The other receive path while not in promiscuous mode
> (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never
> require any additional filtering. I somehow miss this point here.

Maybe the drv_start() function should receive an sdata pointer. This way
instead of changing the PHY filtering level to what has just be asked
blindly, the code should look at the filtering level of all the
interfaces up on the PHY and apply the lowest filtering level by
hardware, knowing that on a per interface basis, the software will
compensate.

It should work just fine because local->phy->filtering shows the actual
filtering level of the PHY while sdata->requested_filtering shows the
level of filtering that was expected on each interface. If you don't
like the idea of having a mutable sdata->requested_filtering entry, I
can have an sdata->base_filtering which should only be set by
ieee802154_setup_sdata() and an sdata->expected_filtering which would
reflect what the mac expects on this interface at the present moment.

> For 1), the driver should change the filtering mode" when we start to
> "listen", this is done by the start() driver callback. They should get
> all receive parameters and set up receiving to whatever mac802154,
> currently there is a bit of chaos there. To move it into drv_start()
> is just a workaround to begin this step that we move it at some point
> to the driver. I mention 1) here because that should be part of the
> picture how everything works together when the phy is switched to a
> different filter level while it's operating (I mean there are running
> interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS)
> which then activates the different receive path for the use case of
> scanning (something like (phy->state & WPANPHY_SCANING) == true)?

Scanning is a dedicated filtering level per-se because it must discard
!beacon frames, that's why I request this level of filtering (which
maybe I should do on a per-interface basis instead of using the *local
poiner).

> I am sorry, but I somehow miss the picture of how those things work
> together. It is not clear for me and I miss those parts to get a whole
> picture of this. For me it's not clear that those patches are going in
> this direction.

Please tell me if it's more clear and if you agree with this vision. I
don't have time to draft something this week.

Thanks,
Miquèl

Re: [PATCH wpan/next v3 0/9] net: ieee802154: Support scanning/beaconing

[Alexander Aring]

Hi,

On Thu, Sep 8, 2022 at 3:37 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400:
>
> > Hi,
> >
> > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hello,
> > >
> > > A third version of this series, dropping the scan patches for now
> > > because before we need to settle on the filtering topic and the
> > > coordinator interface topic. Here is just the filtering part, I've
> > > integrated Alexander's patches, as well as the atusb fix. Once this is
> > > merge there are a few coordinator-related patches, and finally the
> > > scan.
> >
> > I think we have a communication problem here and we should talk about
> > what the problems are and agree on a way to solve them.
> >
> > The problems are:
> >
> > 1. We never supported switching from an operating phy (interfaces are
> > up) into another filtering mode.
>
> In the trigger scan path there is a:
>
>         mlme_op_pre() // stop Tx
>         drv_stop() // stop Rx
>         synchronize_net()
>         drv_start(params) // restart Rx with another hw filtering level
>

Okay, that's looking good.

> > 2. Scan requires to be in "promiscuous mode" (according to the
> > 802.15.4 spec promiscuous mode). We don't support promiscuous mode
> > (according to the 802.15.4 spec promiscuous mode). We "can" however
> > use the currently supported mode which does not filter anything
> > (IEEE802154_FILTERING_NONE) when we do additional filtering in
> > mac802154. _But_ this is only required when the phy is scanning, it
> > will also deliver anything to the upper layers.
> >
> > This patch-series tries to do the second thing, okay that's fine. But
> > I thought this should only be done while the phy is in "scanning
> > mode"?
>
> I don't understand what's wrong then. We ask for the "scan mode"
> filtering level when starting the scan [1] and we ask for the normal
> filtering level when it's done/aborted [2] [3].
>

There is no problem with that. There is for me a problem with the
receive path when certain filtering levels are active.

> [1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326
> [2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55
>
> > The other receive path while not in promiscuous mode
> > (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never
> > require any additional filtering. I somehow miss this point here.
>
> Maybe the drv_start() function should receive an sdata pointer. This way
> instead of changing the PHY filtering level to what has just be asked
> blindly, the code should look at the filtering level of all the
> interfaces up on the PHY and apply the lowest filtering level by
> hardware, knowing that on a per interface basis, the software will
> compensate.
>
> It should work just fine because local->phy->filtering shows the actual
> filtering level of the PHY while sdata->requested_filtering shows the
> level of filtering that was expected on each interface. If you don't
> like the idea of having a mutable sdata->requested_filtering entry, I
> can have an sdata->base_filtering which should only be set by
> ieee802154_setup_sdata() and an sdata->expected_filtering which would
> reflect what the mac expects on this interface at the present moment.
>

From my view is that if we disable address filters (all filtering
modes except IEEE802154_FILTERING_4_FRAME_FIELDS) we never can call
netif_receive_skb(). This patch series tries to "compensate" the
missing filtering on phy which is fine only to handle things related
for the scan operation but nothing else.

The reason why we can't call netif_receive_skb() is because we don't
have ackknowledge handling, whereas for scanning we ignore ack frames
and that's why we don't need it.

> > For 1), the driver should change the filtering mode" when we start to
> > "listen", this is done by the start() driver callback. They should get
> > all receive parameters and set up receiving to whatever mac802154,
> > currently there is a bit of chaos there. To move it into drv_start()
> > is just a workaround to begin this step that we move it at some point
> > to the driver. I mention 1) here because that should be part of the
> > picture how everything works together when the phy is switched to a
> > different filter level while it's operating (I mean there are running
> > interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS)
> > which then activates the different receive path for the use case of
> > scanning (something like (phy->state & WPANPHY_SCANING) == true)?
>
> Scanning is a dedicated filtering level per-se because it must discard
> !beacon frames, that's why I request this level of filtering (which
> maybe I should do on a per-interface basis instead of using the *local
> poiner).
>

We only can do a per filter level per interface if the hardware has
support for such a thing. Currently there is one address filter and if
it's disabled we lose ackknowledge handling (as general rule), we
can't compensate by doing any additional filtering by software in this
mode.

> > I am sorry, but I somehow miss the picture of how those things work
> > together. It is not clear for me and I miss those parts to get a whole
> > picture of this. For me it's not clear that those patches are going in
> > this direction.
>
> Please tell me if it's more clear and if you agree with this vision. I
> don't have time to draft something this week.
>

That's fine. We should agree on things like compensate lower filter
levels by doing additional softmac filtering to reach
IEEE802154_FILTERING_4_FRAME_FIELDS filtering from others because we
will lose AACK handling. It is only fine to do that in mac802154
receive path but don't deliver it to the upper layer.

- Alex

Re: [PATCH wpan/next v3 6/9] net: mac802154: Add promiscuous software filtering

[Alexander Aring]

Hi,

On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Currently, the promiscuous mode was not as open as it should. It was not
> a big deal because until now promiscuous modes were only used on monitor
> interfaces, which would never go this far in the filtering. But as we
> might now use this promiscuous mode with NODEs or COORDs, it becomes
> necessary to really forward the packets to the upper layers without

no, they should never deliver to upper layers in filtering modes where
address filtering is disabled.

> additional filtering when relevant. Let's add the necessary logic to
> handle this situation.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  net/mac802154/rx.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index bd1a92fceef7..8a8c5a4a2f28 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -196,10 +196,31 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local,
>         int ret;
>         struct ieee802154_sub_if_data *sdata;
>         struct ieee802154_hdr hdr;
> +       struct sk_buff *skb2;
>
> +       /* Level 2 filtering: Avoid further processing in IEEE 802.15.4 promiscuous modes */
> +       list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> +               if (!ieee802154_sdata_running(sdata))
> +                       continue;
> +
> +               if (sdata->required_filtering < IEEE802154_FILTERING_1_FCS ||
> +                   sdata->required_filtering > IEEE802154_FILTERING_2_PROMISCUOUS)
> +                       continue;
> +

I am confused about using "sdata->required_filtering" here.

> +               skb2 = skb_clone(skb, GFP_ATOMIC);
> +               if (skb2) {
> +                       skb2->dev = sdata->dev;
> +                       ieee802154_deliver_skb(skb2);
> +
> +                       sdata->dev->stats.rx_packets++;
> +                       sdata->dev->stats.rx_bytes += skb->len;
> +               }
> +       }
> +

I am confused about this change here.

- Alex

Re: [PATCH wpan/next v3 5/9] net: mac802154: Drop IEEE802154_HW_RX_DROP_BAD_CKSUM

[Alexander Aring]

Hi,

On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to
> reflect the fact that it would not validate the checksum (FCS). In other
> words, the filtering level of hwsim is always "NONE" while the core
> expects it to be higher.
>
> Now that we have access to real filtering levels, we can actually use
> them and always enforce the "NONE" level in hwsim. This case is already
> correctly handled in the receive so we can drop the flag.
>

I would say the whole hwsim driver currently only works because we
don't transmit wrong frames on a virtual hardware... However this can
be improved, yes. In my opinion the hwsim driver should pretend to
work like other transceivers sending frames to mac802154. That means
the filtering level should be implemented in hwsim not in mac802154 as
on real hardware the hardware would do filtering.

I think you should assume for now the previous behaviour that hwsim
does not send bad frames out. Of course there is a bug but it was
already there before, but the fix would be to change hwsim driver.

- Alex

Re: [PATCH wpan/next v3 8/9] net: mac802154: Ensure proper general purpose frame filtering

[Alexander Aring]

Hi,

On Mon, Sep 5, 2022 at 4:35 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Most of the PHYs seem to cope with the standard filtering rules by
> default. Some of them might not, like hwsim which is only software, and

yes, as I said before hwsim should pretend to be like all other
hardware we have.

> in this case advertises its real filtering level with the new
> "filtering" internal value.
>
> The core then needs to check what is expected by looking at the PHY
> requested filtering level and possibly apply additional filtering
> rules.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/net/ieee802154_netdev.h |  8 ++++
>  net/mac802154/rx.c              | 78 +++++++++++++++++++++++++++++++++
>  2 files changed, 86 insertions(+)
>
> diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> index d0d188c3294b..1b82bbafe8c7 100644
> --- a/include/net/ieee802154_netdev.h
> +++ b/include/net/ieee802154_netdev.h
> @@ -69,6 +69,14 @@ struct ieee802154_hdr_fc {
>  #endif
>  };
>
> +enum ieee802154_frame_version {
> +       IEEE802154_2003_STD,
> +       IEEE802154_2006_STD,
> +       IEEE802154_STD,
> +       IEEE802154_RESERVED_STD,
> +       IEEE802154_MULTIPURPOSE_STD = IEEE802154_2003_STD,
> +};
> +
>  struct ieee802154_hdr {
>         struct ieee802154_hdr_fc fc;
>         u8 seq;
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index c43289c0fdd7..bc46e4a7669d 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -52,6 +52,84 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>                                 mac_cb(skb)->type);
>                         goto fail;
>                 }
> +       } else if (sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS &&

We switch here from determine that receive path, means way we are
going from interface type to the required filtering value. Sure there
is currently a 1:1 mapping for them now but I don't know why we are
doing that and this is in my opinion wrong. The receive path should
depend on interface type as it was before and for scanning there is
some early check like:

if (wpan_phy_is_in_scan_mode_state(local)) {
     do_receive_scanning(...)
     /* don't do any other delivery because they provide it to upper layer */
     return;
}

Maybe you should do monitors receive that frame before as well, but
every other interface type should currently not receive it.

- Alex

Re: [PATCH wpan/next v3 8/9] net: mac802154: Ensure proper general purpose frame filtering

[Alexander Aring]

Hi,

On Thu, Sep 8, 2022 at 9:00 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Mon, Sep 5, 2022 at 4:35 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Most of the PHYs seem to cope with the standard filtering rules by
> > default. Some of them might not, like hwsim which is only software, and
>
> yes, as I said before hwsim should pretend to be like all other
> hardware we have.
>
> > in this case advertises its real filtering level with the new
> > "filtering" internal value.
> >
> > The core then needs to check what is expected by looking at the PHY
> > requested filtering level and possibly apply additional filtering
> > rules.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/net/ieee802154_netdev.h |  8 ++++
> >  net/mac802154/rx.c              | 78 +++++++++++++++++++++++++++++++++
> >  2 files changed, 86 insertions(+)
> >
> > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> > index d0d188c3294b..1b82bbafe8c7 100644
> > --- a/include/net/ieee802154_netdev.h
> > +++ b/include/net/ieee802154_netdev.h
> > @@ -69,6 +69,14 @@ struct ieee802154_hdr_fc {
> >  #endif
> >  };
> >
> > +enum ieee802154_frame_version {
> > +       IEEE802154_2003_STD,
> > +       IEEE802154_2006_STD,
> > +       IEEE802154_STD,
> > +       IEEE802154_RESERVED_STD,
> > +       IEEE802154_MULTIPURPOSE_STD = IEEE802154_2003_STD,
> > +};
> > +
> >  struct ieee802154_hdr {
> >         struct ieee802154_hdr_fc fc;
> >         u8 seq;
> > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > index c43289c0fdd7..bc46e4a7669d 100644
> > --- a/net/mac802154/rx.c
> > +++ b/net/mac802154/rx.c
> > @@ -52,6 +52,84 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
> >                                 mac_cb(skb)->type);
> >                         goto fail;
> >                 }
> > +       } else if (sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS &&
>
> We switch here from determine that receive path, means way we are

- way

> going from interface type to the required filtering value. Sure there
> is currently a 1:1 mapping for them now but I don't know why we are
> doing that and this is in my opinion wrong. The receive path should
> depend on interface type as it was before and for scanning there is
> some early check like:
>
> if (wpan_phy_is_in_scan_mode_state(local)) {
>      do_receive_scanning(...)
>      /* don't do any other delivery because they provide it to upper layer */

In the assumption we know if the condition above is true we have
address filtering disabled.

- Alex

Re: [PATCH wpan/next v3 0/9] net: ieee802154: Support scanning/beaconing

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Thu, 8 Sep 2022 20:41:14 -0400:

> Hi,
> 
> On Thu, Sep 8, 2022 at 3:37 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400:
> >  
> > > Hi,
> > >
> > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Hello,
> > > >
> > > > A third version of this series, dropping the scan patches for now
> > > > because before we need to settle on the filtering topic and the
> > > > coordinator interface topic. Here is just the filtering part, I've
> > > > integrated Alexander's patches, as well as the atusb fix. Once this is
> > > > merge there are a few coordinator-related patches, and finally the
> > > > scan.  
> > >
> > > I think we have a communication problem here and we should talk about
> > > what the problems are and agree on a way to solve them.
> > >
> > > The problems are:
> > >
> > > 1. We never supported switching from an operating phy (interfaces are
> > > up) into another filtering mode.  
> >
> > In the trigger scan path there is a:
> >
> >         mlme_op_pre() // stop Tx
> >         drv_stop() // stop Rx
> >         synchronize_net()
> >         drv_start(params) // restart Rx with another hw filtering level
> >  
> 
> Okay, that's looking good.
> 
> > > 2. Scan requires to be in "promiscuous mode" (according to the
> > > 802.15.4 spec promiscuous mode). We don't support promiscuous mode
> > > (according to the 802.15.4 spec promiscuous mode). We "can" however
> > > use the currently supported mode which does not filter anything
> > > (IEEE802154_FILTERING_NONE) when we do additional filtering in
> > > mac802154. _But_ this is only required when the phy is scanning, it
> > > will also deliver anything to the upper layers.
> > >
> > > This patch-series tries to do the second thing, okay that's fine. But
> > > I thought this should only be done while the phy is in "scanning
> > > mode"?  
> >
> > I don't understand what's wrong then. We ask for the "scan mode"
> > filtering level when starting the scan [1] and we ask for the normal
> > filtering level when it's done/aborted [2] [3].
> >  
> 
> There is no problem with that. There is for me a problem with the
> receive path when certain filtering levels are active.
> 
> > [1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326
> > [2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55
> >  
> > > The other receive path while not in promiscuous mode
> > > (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never
> > > require any additional filtering. I somehow miss this point here.  
> >
> > Maybe the drv_start() function should receive an sdata pointer. This way
> > instead of changing the PHY filtering level to what has just be asked
> > blindly, the code should look at the filtering level of all the
> > interfaces up on the PHY and apply the lowest filtering level by
> > hardware, knowing that on a per interface basis, the software will
> > compensate.
> >
> > It should work just fine because local->phy->filtering shows the actual
> > filtering level of the PHY while sdata->requested_filtering shows the
> > level of filtering that was expected on each interface. If you don't
> > like the idea of having a mutable sdata->requested_filtering entry, I
> > can have an sdata->base_filtering which should only be set by
> > ieee802154_setup_sdata() and an sdata->expected_filtering which would
> > reflect what the mac expects on this interface at the present moment.
> >  
> 
> From my view is that if we disable address filters (all filtering
> modes except IEEE802154_FILTERING_4_FRAME_FIELDS) we never can call
> netif_receive_skb(). This patch series tries to "compensate" the
> missing filtering on phy which is fine only to handle things related
> for the scan operation but nothing else.
> 
> The reason why we can't call netif_receive_skb() is because we don't
> have ackknowledge handling, whereas for scanning we ignore ack frames
> and that's why we don't need it.

I've digested all of that right before the conference so I think I now
understand all your fears regarding the possible absence of ACK in
certain situations. I even share them now.

> > > For 1), the driver should change the filtering mode" when we start to
> > > "listen", this is done by the start() driver callback. They should get
> > > all receive parameters and set up receiving to whatever mac802154,
> > > currently there is a bit of chaos there. To move it into drv_start()
> > > is just a workaround to begin this step that we move it at some point
> > > to the driver. I mention 1) here because that should be part of the
> > > picture how everything works together when the phy is switched to a
> > > different filter level while it's operating (I mean there are running
> > > interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS)
> > > which then activates the different receive path for the use case of
> > > scanning (something like (phy->state & WPANPHY_SCANING) == true)?  
> >
> > Scanning is a dedicated filtering level per-se because it must discard
> > !beacon frames, that's why I request this level of filtering (which
> > maybe I should do on a per-interface basis instead of using the *local
> > poiner).
> >  
> 
> We only can do a per filter level per interface if the hardware has
> support for such a thing. Currently there is one address filter and if
> it's disabled we lose ackknowledge handling (as general rule), we
> can't compensate by doing any additional filtering by software in this
> mode.

Yes.

> 
> > > I am sorry, but I somehow miss the picture of how those things work
> > > together. It is not clear for me and I miss those parts to get a whole
> > > picture of this. For me it's not clear that those patches are going in
> > > this direction.  
> >
> > Please tell me if it's more clear and if you agree with this vision. I
> > don't have time to draft something this week.
> >  
> 
> That's fine. We should agree on things like compensate lower filter
> levels by doing additional softmac filtering to reach
> IEEE802154_FILTERING_4_FRAME_FIELDS filtering from others because we
> will lose AACK handling. It is only fine to do that in mac802154
> receive path but don't deliver it to the upper layer.

So actually, if I try to summarize the situation.

I've tried to make several different subif working on a single PHY.
Unfortunately, today there is only one address filter per PHY, so
disabling the address filter on one interface would also disable it on
the other, leading to the ACKs not being handled anymore, which we
cannot afford.

So overall I guess we should settle on how we want to handle the
situation. I propose, to move forward, that we continue to assume that
it is *not* possible to have several different interfaces running at the
same time on a single PHY. This involves dropping all the "software
address filtering" which I proposed, but I guess that's fine.

Thanks,
Miquèl

Re: [PATCH wpan/next v3 5/9] net: mac802154: Drop IEEE802154_HW_RX_DROP_BAD_CKSUM

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Thu, 8 Sep 2022 20:49:36 -0400:

> Hi,
> 
> On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to
> > reflect the fact that it would not validate the checksum (FCS). In other
> > words, the filtering level of hwsim is always "NONE" while the core
> > expects it to be higher.
> >
> > Now that we have access to real filtering levels, we can actually use
> > them and always enforce the "NONE" level in hwsim. This case is already
> > correctly handled in the receive so we can drop the flag.
> >  
> 
> I would say the whole hwsim driver currently only works because we
> don't transmit wrong frames on a virtual hardware... However this can
> be improved, yes. In my opinion the hwsim driver should pretend to
> work like other transceivers sending frames to mac802154. That means
> the filtering level should be implemented in hwsim not in mac802154 as
> on real hardware the hardware would do filtering.
> 
> I think you should assume for now the previous behaviour that hwsim
> does not send bad frames out. Of course there is a bug but it was
> already there before, but the fix would be to change hwsim driver.

Well, somehow I already implemented all the filtering by software in
one of the other patches. I now agree that it was not relevant (because
of the AACK issue you raised), but instead of fully dropping this code
I might just move it to hwsim because there it would perfectly make
sense?

Thanks,
Miquèl

Re: [PATCH wpan/next v3 8/9] net: mac802154: Ensure proper general purpose frame filtering

[Miquel Raynal]

Hi Alexander,

aahringo@redhat.com wrote on Thu, 8 Sep 2022 21:00:37 -0400:

> Hi,
> 
> On Mon, Sep 5, 2022 at 4:35 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Most of the PHYs seem to cope with the standard filtering rules by
> > default. Some of them might not, like hwsim which is only software, and  
> 
> yes, as I said before hwsim should pretend to be like all other
> hardware we have.
> 
> > in this case advertises its real filtering level with the new
> > "filtering" internal value.
> >
> > The core then needs to check what is expected by looking at the PHY
> > requested filtering level and possibly apply additional filtering
> > rules.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/net/ieee802154_netdev.h |  8 ++++
> >  net/mac802154/rx.c              | 78 +++++++++++++++++++++++++++++++++
> >  2 files changed, 86 insertions(+)
> >
> > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> > index d0d188c3294b..1b82bbafe8c7 100644
> > --- a/include/net/ieee802154_netdev.h
> > +++ b/include/net/ieee802154_netdev.h
> > @@ -69,6 +69,14 @@ struct ieee802154_hdr_fc {
> >  #endif
> >  };
> >
> > +enum ieee802154_frame_version {
> > +       IEEE802154_2003_STD,
> > +       IEEE802154_2006_STD,
> > +       IEEE802154_STD,
> > +       IEEE802154_RESERVED_STD,
> > +       IEEE802154_MULTIPURPOSE_STD = IEEE802154_2003_STD,
> > +};
> > +
> >  struct ieee802154_hdr {
> >         struct ieee802154_hdr_fc fc;
> >         u8 seq;
> > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > index c43289c0fdd7..bc46e4a7669d 100644
> > --- a/net/mac802154/rx.c
> > +++ b/net/mac802154/rx.c
> > @@ -52,6 +52,84 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
> >                                 mac_cb(skb)->type);
> >                         goto fail;
> >                 }
> > +       } else if (sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS &&  
> 
> We switch here from determine that receive path, means way we are
> going from interface type to the required filtering value. Sure there
> is currently a 1:1 mapping for them now but I don't know why we are
> doing that and this is in my opinion wrong. The receive path should
> depend on interface type as it was before and for scanning there is
> some early check like:

Maybe on this one I am not fully convinced yet.

In your opinion (I try to rephrase so that we align on what you told
me) the total lack of filtering is only something that is reserved to
monitor interfaces, so you make an implicit link between interface type
and filtering level.

I would argue that this is true today, but as the "no filtering at all"
level is defined in the spec, I assumed it was a possible level that
one would want to achieve some day (not sure for what purpose yet). So
I assumed it would be more relevant to only work with the
expected filtering level in the receive path rather than on the
interface type, it makes more sense IMHO. In practice I agree it should
be the same filtering-wise, but from a conceptual point of view I find
the current logic partially satisfying.

Would you agree with me only using "expected filtering levels" rather
than:
- sometimes the interface type
- sometimes the mac state (scan)
- otherwise, by default, the highest filtering level
?

I think it would clarify the receive path.

I will of course get rid of most of all the other "nasty"
software filtering additions you nacked in the other threads.

> if (wpan_phy_is_in_scan_mode_state(local)) {
>      do_receive_scanning(...)
>      /* don't do any other delivery because they provide it to upper layer */
>      return;
> }
> 
> Maybe you should do monitors receive that frame before as well, but
> every other interface type should currently not receive it.
> 
> - Alex
> 


Thanks,
Miquèl

Re: [PATCH wpan/next v3 5/9] net: mac802154: Drop IEEE802154_HW_RX_DROP_BAD_CKSUM

[Alexander Aring]

Hi,

On Wed, Sep 21, 2022 at 11:49 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Thu, 8 Sep 2022 20:49:36 -0400:
>
> > Hi,
> >
> > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to
> > > reflect the fact that it would not validate the checksum (FCS). In other
> > > words, the filtering level of hwsim is always "NONE" while the core
> > > expects it to be higher.
> > >
> > > Now that we have access to real filtering levels, we can actually use
> > > them and always enforce the "NONE" level in hwsim. This case is already
> > > correctly handled in the receive so we can drop the flag.
> > >
> >
> > I would say the whole hwsim driver currently only works because we
> > don't transmit wrong frames on a virtual hardware... However this can
> > be improved, yes. In my opinion the hwsim driver should pretend to
> > work like other transceivers sending frames to mac802154. That means
> > the filtering level should be implemented in hwsim not in mac802154 as
> > on real hardware the hardware would do filtering.
> >
> > I think you should assume for now the previous behaviour that hwsim
> > does not send bad frames out. Of course there is a bug but it was
> > already there before, but the fix would be to change hwsim driver.
>
> Well, somehow I already implemented all the filtering by software in
> one of the other patches. I now agree that it was not relevant (because
> of the AACK issue you raised), but instead of fully dropping this code
> I might just move it to hwsim because there it would perfectly make
> sense?
>

Yes, I agree. You should make the "in-driver receive path" acting like
other hardware (In sense what we currently agree on what filtering
they do) if promiscuous mode is turned off/on.

It makes sense and should be done there. The
IEEE802154_HW_RX_DROP_BAD_CKSUM flag should still be dropped.

- Alex

Re: [PATCH wpan/next v3 0/9] net: ieee802154: Support scanning/beaconing

[Alexander Aring]

Hi,

On Wed, Sep 21, 2022 at 11:46 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Thu, 8 Sep 2022 20:41:14 -0400:
>
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 3:37 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > aahringo@redhat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > A third version of this series, dropping the scan patches for now
> > > > > because before we need to settle on the filtering topic and the
> > > > > coordinator interface topic. Here is just the filtering part, I've
> > > > > integrated Alexander's patches, as well as the atusb fix. Once this is
> > > > > merge there are a few coordinator-related patches, and finally the
> > > > > scan.
> > > >
> > > > I think we have a communication problem here and we should talk about
> > > > what the problems are and agree on a way to solve them.
> > > >
> > > > The problems are:
> > > >
> > > > 1. We never supported switching from an operating phy (interfaces are
> > > > up) into another filtering mode.
> > >
> > > In the trigger scan path there is a:
> > >
> > >         mlme_op_pre() // stop Tx
> > >         drv_stop() // stop Rx
> > >         synchronize_net()
> > >         drv_start(params) // restart Rx with another hw filtering level
> > >
> >
> > Okay, that's looking good.
> >
> > > > 2. Scan requires to be in "promiscuous mode" (according to the
> > > > 802.15.4 spec promiscuous mode). We don't support promiscuous mode
> > > > (according to the 802.15.4 spec promiscuous mode). We "can" however
> > > > use the currently supported mode which does not filter anything
> > > > (IEEE802154_FILTERING_NONE) when we do additional filtering in
> > > > mac802154. _But_ this is only required when the phy is scanning, it
> > > > will also deliver anything to the upper layers.
> > > >
> > > > This patch-series tries to do the second thing, okay that's fine. But
> > > > I thought this should only be done while the phy is in "scanning
> > > > mode"?
> > >
> > > I don't understand what's wrong then. We ask for the "scan mode"
> > > filtering level when starting the scan [1] and we ask for the normal
> > > filtering level when it's done/aborted [2] [3].
> > >
> >
> > There is no problem with that. There is for me a problem with the
> > receive path when certain filtering levels are active.
> >
> > > [1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326
> > > [2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55
> > >
> > > > The other receive path while not in promiscuous mode
> > > > (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never
> > > > require any additional filtering. I somehow miss this point here.
> > >
> > > Maybe the drv_start() function should receive an sdata pointer. This way
> > > instead of changing the PHY filtering level to what has just be asked
> > > blindly, the code should look at the filtering level of all the
> > > interfaces up on the PHY and apply the lowest filtering level by
> > > hardware, knowing that on a per interface basis, the software will
> > > compensate.
> > >
> > > It should work just fine because local->phy->filtering shows the actual
> > > filtering level of the PHY while sdata->requested_filtering shows the
> > > level of filtering that was expected on each interface. If you don't
> > > like the idea of having a mutable sdata->requested_filtering entry, I
> > > can have an sdata->base_filtering which should only be set by
> > > ieee802154_setup_sdata() and an sdata->expected_filtering which would
> > > reflect what the mac expects on this interface at the present moment.
> > >
> >
> > From my view is that if we disable address filters (all filtering
> > modes except IEEE802154_FILTERING_4_FRAME_FIELDS) we never can call
> > netif_receive_skb(). This patch series tries to "compensate" the
> > missing filtering on phy which is fine only to handle things related
> > for the scan operation but nothing else.
> >
> > The reason why we can't call netif_receive_skb() is because we don't
> > have ackknowledge handling, whereas for scanning we ignore ack frames
> > and that's why we don't need it.
>
> I've digested all of that right before the conference so I think I now
> understand all your fears regarding the possible absence of ACK in
> certain situations. I even share them now.
>
> > > > For 1), the driver should change the filtering mode" when we start to
> > > > "listen", this is done by the start() driver callback. They should get
> > > > all receive parameters and set up receiving to whatever mac802154,
> > > > currently there is a bit of chaos there. To move it into drv_start()
> > > > is just a workaround to begin this step that we move it at some point
> > > > to the driver. I mention 1) here because that should be part of the
> > > > picture how everything works together when the phy is switched to a
> > > > different filter level while it's operating (I mean there are running
> > > > interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS)
> > > > which then activates the different receive path for the use case of
> > > > scanning (something like (phy->state & WPANPHY_SCANING) == true)?
> > >
> > > Scanning is a dedicated filtering level per-se because it must discard
> > > !beacon frames, that's why I request this level of filtering (which
> > > maybe I should do on a per-interface basis instead of using the *local
> > > poiner).
> > >
> >
> > We only can do a per filter level per interface if the hardware has
> > support for such a thing. Currently there is one address filter and if
> > it's disabled we lose ackknowledge handling (as general rule), we
> > can't compensate by doing any additional filtering by software in this
> > mode.
>
> Yes.
>
> >
> > > > I am sorry, but I somehow miss the picture of how those things work
> > > > together. It is not clear for me and I miss those parts to get a whole
> > > > picture of this. For me it's not clear that those patches are going in
> > > > this direction.
> > >
> > > Please tell me if it's more clear and if you agree with this vision. I
> > > don't have time to draft something this week.
> > >
> >
> > That's fine. We should agree on things like compensate lower filter
> > levels by doing additional softmac filtering to reach
> > IEEE802154_FILTERING_4_FRAME_FIELDS filtering from others because we
> > will lose AACK handling. It is only fine to do that in mac802154
> > receive path but don't deliver it to the upper layer.
>
> So actually, if I try to summarize the situation.
>
> I've tried to make several different subif working on a single PHY.
> Unfortunately, today there is only one address filter per PHY, so

correct. But you are assuming hardware which is currently around. As I
said atusb is one candidate to make a more clever hardware because it
has a co-processor which eventually could handle ack handling if done
right.

> disabling the address filter on one interface would also disable it on
> the other, leading to the ACKs not being handled anymore, which we
> cannot afford.
>

For hardware which has such limitation of one address filter yes.

> So overall I guess we should settle on how we want to handle the
> situation. I propose, to move forward, that we continue to assume that
> it is *not* possible to have several different interfaces running at the
> same time on a single PHY. This involves dropping all the "software
> address filtering" which I proposed, but I guess that's fine.

I agree here, but don't remove code which could add such handling and
allow multiple monitors at the same time.

- Alex

Re: [PATCH wpan/next v3 8/9] net: mac802154: Ensure proper general purpose frame filtering

[Alexander Aring]

Hi,

On Wed, Sep 21, 2022 at 11:59 AM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Thu, 8 Sep 2022 21:00:37 -0400:
>
> > Hi,
> >
> > On Mon, Sep 5, 2022 at 4:35 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Most of the PHYs seem to cope with the standard filtering rules by
> > > default. Some of them might not, like hwsim which is only software, and
> >
> > yes, as I said before hwsim should pretend to be like all other
> > hardware we have.
> >
> > > in this case advertises its real filtering level with the new
> > > "filtering" internal value.
> > >
> > > The core then needs to check what is expected by looking at the PHY
> > > requested filtering level and possibly apply additional filtering
> > > rules.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  include/net/ieee802154_netdev.h |  8 ++++
> > >  net/mac802154/rx.c              | 78 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 86 insertions(+)
> > >
> > > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> > > index d0d188c3294b..1b82bbafe8c7 100644
> > > --- a/include/net/ieee802154_netdev.h
> > > +++ b/include/net/ieee802154_netdev.h
> > > @@ -69,6 +69,14 @@ struct ieee802154_hdr_fc {
> > >  #endif
> > >  };
> > >
> > > +enum ieee802154_frame_version {
> > > +       IEEE802154_2003_STD,
> > > +       IEEE802154_2006_STD,
> > > +       IEEE802154_STD,
> > > +       IEEE802154_RESERVED_STD,
> > > +       IEEE802154_MULTIPURPOSE_STD = IEEE802154_2003_STD,
> > > +};
> > > +
> > >  struct ieee802154_hdr {
> > >         struct ieee802154_hdr_fc fc;
> > >         u8 seq;
> > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > > index c43289c0fdd7..bc46e4a7669d 100644
> > > --- a/net/mac802154/rx.c
> > > +++ b/net/mac802154/rx.c
> > > @@ -52,6 +52,84 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
> > >                                 mac_cb(skb)->type);
> > >                         goto fail;
> > >                 }
> > > +       } else if (sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS &&
> >
> > We switch here from determine that receive path, means way we are
> > going from interface type to the required filtering value. Sure there
> > is currently a 1:1 mapping for them now but I don't know why we are
> > doing that and this is in my opinion wrong. The receive path should
> > depend on interface type as it was before and for scanning there is
> > some early check like:
>
> Maybe on this one I am not fully convinced yet.
>
> In your opinion (I try to rephrase so that we align on what you told
> me) the total lack of filtering is only something that is reserved to
> monitor interfaces, so you make an implicit link between interface type
> and filtering level.

it always depends on the use case, but in the sense of filtering-level
in "normal" operating mode and calling netif_skb_deliver_foo(), yes.

The use case for e.g. scan is different and mac802154 takes control of it.

>
> I would argue that this is true today, but as the "no filtering at all"
> level is defined in the spec, I assumed it was a possible level that
> one would want to achieve some day (not sure for what purpose yet). So
> I assumed it would be more relevant to only work with the
> expected filtering level in the receive path rather than on the
> interface type, it makes more sense IMHO. In practice I agree it should
> be the same filtering-wise, but from a conceptual point of view I find
> the current logic partially satisfying.
>

I don't quite follow here. I would say we currently only support to
tell the hardware the whole filtering level (with AACK support) or the
non-filtering level. With both we should somehow able to support
interface types which requires

> Would you agree with me only using "expected filtering levels" rather
> than:
> - sometimes the interface type
> - sometimes the mac state (scan)
> - otherwise, by default, the highest filtering level
> ?

I think so, yes? I don't know what "otherwise, by default, the highest
filtering level" means, it is the interface type which declares what
it's actually needs at netif_skb_deliver_foo(), e.g. monitors will
call netif_skb_deliver_foo() even without AACK support... because
that's how they working. They also don't have an address in the
network. It is a kind of experimenting, debugging, or making chaos in
your network interface type.

For other node, or coordinator interfaces? They need at least AACK
support (and they having a valid address filtering going on) they need
it...

however on scan I would say then they are in a kind of "hidden"
non-operating interface and the phy will do some operation on phy
level and do something and restore after it's done. Scan should not
have anything todo with interfaces, BUT there might be the possibility
to specify the interface on iwpan layer to make a scan, _however_ it
will be only a shortcut to specify the phy which the interface is
using...

- Alex

Re: [PATCH wpan/next v3 8/9] net: mac802154: Ensure proper general purpose frame filtering

[Alexander Aring]

Hi,

On Sun, Sep 25, 2022 at 6:27 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Wed, Sep 21, 2022 at 11:59 AM Miquel Raynal
> <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > aahringo@redhat.com wrote on Thu, 8 Sep 2022 21:00:37 -0400:
> >
> > > Hi,
> > >
> > > On Mon, Sep 5, 2022 at 4:35 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >
> > > > Most of the PHYs seem to cope with the standard filtering rules by
> > > > default. Some of them might not, like hwsim which is only software, and
> > >
> > > yes, as I said before hwsim should pretend to be like all other
> > > hardware we have.
> > >
> > > > in this case advertises its real filtering level with the new
> > > > "filtering" internal value.
> > > >
> > > > The core then needs to check what is expected by looking at the PHY
> > > > requested filtering level and possibly apply additional filtering
> > > > rules.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  include/net/ieee802154_netdev.h |  8 ++++
> > > >  net/mac802154/rx.c              | 78 +++++++++++++++++++++++++++++++++
> > > >  2 files changed, 86 insertions(+)
> > > >
> > > > diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h
> > > > index d0d188c3294b..1b82bbafe8c7 100644
> > > > --- a/include/net/ieee802154_netdev.h
> > > > +++ b/include/net/ieee802154_netdev.h
> > > > @@ -69,6 +69,14 @@ struct ieee802154_hdr_fc {
> > > >  #endif
> > > >  };
> > > >
> > > > +enum ieee802154_frame_version {
> > > > +       IEEE802154_2003_STD,
> > > > +       IEEE802154_2006_STD,
> > > > +       IEEE802154_STD,
> > > > +       IEEE802154_RESERVED_STD,
> > > > +       IEEE802154_MULTIPURPOSE_STD = IEEE802154_2003_STD,
> > > > +};
> > > > +
> > > >  struct ieee802154_hdr {
> > > >         struct ieee802154_hdr_fc fc;
> > > >         u8 seq;
> > > > diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> > > > index c43289c0fdd7..bc46e4a7669d 100644
> > > > --- a/net/mac802154/rx.c
> > > > +++ b/net/mac802154/rx.c
> > > > @@ -52,6 +52,84 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
> > > >                                 mac_cb(skb)->type);
> > > >                         goto fail;
> > > >                 }
> > > > +       } else if (sdata->required_filtering == IEEE802154_FILTERING_4_FRAME_FIELDS &&
> > >
> > > We switch here from determine that receive path, means way we are
> > > going from interface type to the required filtering value. Sure there
> > > is currently a 1:1 mapping for them now but I don't know why we are
> > > doing that and this is in my opinion wrong. The receive path should
> > > depend on interface type as it was before and for scanning there is
> > > some early check like:
> >
> > Maybe on this one I am not fully convinced yet.
> >
> > In your opinion (I try to rephrase so that we align on what you told
> > me) the total lack of filtering is only something that is reserved to
> > monitor interfaces, so you make an implicit link between interface type
> > and filtering level.
>
> it always depends on the use case, but in the sense of filtering-level
> in "normal" operating mode and calling netif_skb_deliver_foo(), yes.
>
> The use case for e.g. scan is different and mac802154 takes control of it.
>
> >
> > I would argue that this is true today, but as the "no filtering at all"
> > level is defined in the spec, I assumed it was a possible level that
> > one would want to achieve some day (not sure for what purpose yet). So
> > I assumed it would be more relevant to only work with the
> > expected filtering level in the receive path rather than on the
> > interface type, it makes more sense IMHO. In practice I agree it should
> > be the same filtering-wise, but from a conceptual point of view I find
> > the current logic partially satisfying.
> >
>
> I don't quite follow here. I would say we currently only support to
> tell the hardware the whole filtering level (with AACK support) or the
> non-filtering level. With both we should somehow able to support
> interface types which requires
>
> > Would you agree with me only using "expected filtering levels" rather
> > than:
> > - sometimes the interface type
> > - sometimes the mac state (scan)
> > - otherwise, by default, the highest filtering level
> > ?
>
> I think so, yes? I don't know what "otherwise, by default, the highest
> filtering level" means, it is the interface type which declares what
> it's actually needs at netif_skb_deliver_foo(), e.g. monitors will
> call netif_skb_deliver_foo() even without AACK support... because
> that's how they working. They also don't have an address in the

they don't have an address -> the hardware filter is set to invalid
destination address setting and this should always be set when
switching to a mode which disables address filter. In case of your
scan command it should be then switched back.

- Alex

Re: [PATCH wpan/next v3 9/9] ieee802154: atusb: add support for trac feature

[Stefan Schmidt]

Hello Miquel, Alexander.

On 05.09.22 22:34, Miquel Raynal wrote:
> From: Alexander Aring <aahringo@redhat.com>
> 
> This patch adds support for reading the trac register if atusb firmware
> reports tx done. There is currently a feature to compare a sequence
> number, if the payload is 1 it tells the driver only the sequence number
> is available if it's two there is additional the trac status register as
> payload.
> 
> Currently the atusb_in_good() function determines if it's a tx done or
> rx done if according the payload length. This patch is doing the same
> and assumes this behaviour.
> 
> Signed-off-by: Alexander Aring <aahringo@redhat.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   drivers/net/ieee802154/atusb.c | 33 ++++++++++++++++++++++++++++-----
>   1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c
> index 2c338783893d..95a4a3cdc8a4 100644
> --- a/drivers/net/ieee802154/atusb.c
> +++ b/drivers/net/ieee802154/atusb.c
> @@ -191,7 +191,7 @@ static void atusb_work_urbs(struct work_struct *work)
>   
>   /* ----- Asynchronous USB -------------------------------------------------- */
>   
> -static void atusb_tx_done(struct atusb *atusb, u8 seq)
> +static void atusb_tx_done(struct atusb *atusb, u8 seq, int reason)
>   {
>   	struct usb_device *usb_dev = atusb->usb_dev;
>   	u8 expect = atusb->tx_ack_seq;
> @@ -199,7 +199,10 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq)
>   	dev_dbg(&usb_dev->dev, "%s (0x%02x/0x%02x)\n", __func__, seq, expect);
>   	if (seq == expect) {
>   		/* TODO check for ifs handling in firmware */
> -		ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false);
> +		if (reason == IEEE802154_SUCCESS)
> +			ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false);
> +		else
> +			ieee802154_xmit_error(atusb->hw, atusb->tx_skb, reason);
>   	} else {
>   		/* TODO I experience this case when atusb has a tx complete
>   		 * irq before probing, we should fix the firmware it's an
> @@ -215,7 +218,8 @@ static void atusb_in_good(struct urb *urb)
>   	struct usb_device *usb_dev = urb->dev;
>   	struct sk_buff *skb = urb->context;
>   	struct atusb *atusb = SKB_ATUSB(skb);
> -	u8 len, lqi;
> +	int result = IEEE802154_SUCCESS;
> +	u8 len, lqi, trac;
>   
>   	if (!urb->actual_length) {
>   		dev_dbg(&usb_dev->dev, "atusb_in: zero-sized URB ?\n");
> @@ -224,8 +228,27 @@ static void atusb_in_good(struct urb *urb)
>   
>   	len = *skb->data;
>   
> -	if (urb->actual_length == 1) {
> -		atusb_tx_done(atusb, len);
> +	switch (urb->actual_length) {
> +	case 2:
> +		trac = TRAC_MASK(*(skb->data + 1));
> +		switch (trac) {
> +		case TRAC_SUCCESS:
> +		case TRAC_SUCCESS_DATA_PENDING:
> +			/* already IEEE802154_SUCCESS */
> +			break;
> +		case TRAC_CHANNEL_ACCESS_FAILURE:
> +			result = IEEE802154_CHANNEL_ACCESS_FAILURE;
> +			break;
> +		case TRAC_NO_ACK:
> +			result = IEEE802154_NO_ACK;
> +			break;
> +		default:
> +			result = IEEE802154_SYSTEM_ERROR;
> +		}
> +
> +		fallthrough;
> +	case 1:
> +		atusb_tx_done(atusb, len, result);
>   		return;
>   	}
>   

There have been various RFC patches from either of you two on this. From 
what I can see this is the last one.

I am glad that this is done in a backwards compatible way, so it can 
keep working with the older v.03 firmware. See my comments on the 
firmware part of this in the other thread.

This patch has been applied to the wpan-next tree and will be
part of the next pull request to net-next. Thanks!

regards
Stefan Schmidt