[wpan-next v2 0/9] ieee802154: A bunch of fixes

[wpan-next v2 0/9] ieee802154: A bunch of fixes

[Miquel Raynal]

In preparation to a wider series, here are a number of small and random
fixes across the subsystem.

Changes in v2:
* Fixed the build error reported by a robot. It ended up being something
  which I fixed in a commit from a following series. I've now sorted
  this out and the patch now works on its own.

Miquel Raynal (9):
  net: ieee802154: hwsim: Ensure proper channel selection at probe time
  net: ieee802154: hwsim: Ensure frame checksum are valid
  net: ieee802154: mcr20a: Fix lifs/sifs periods
  net: ieee802154: at86rf230: Stop leaking skb's
  net: ieee802154: ca8210: Stop leaking skb's
  net: ieee802154: Use the IEEE802154_MAX_PAGE define when relevant
  net: ieee802154: Return meaningful error codes from the netlink
    helpers
  net: mac802154: Explain the use of ieee802154_wake/stop_queue()
  MAINTAINERS: Remove Harry Morris bouncing address

 MAINTAINERS                              |  3 +--
 drivers/net/ieee802154/at86rf230.c       |  1 +
 drivers/net/ieee802154/ca8210.c          |  1 +
 drivers/net/ieee802154/mac802154_hwsim.c | 12 ++----------
 drivers/net/ieee802154/mcr20a.c          |  4 ++--
 include/net/mac802154.h                  | 12 ++++++++++++
 net/ieee802154/nl-phy.c                  |  5 +++--
 net/ieee802154/nl802154.c                |  8 ++++----
 8 files changed, 26 insertions(+), 20 deletions(-)

-- 
2.27.0

[wpan-next v2 1/9] net: ieee802154: hwsim: Ensure proper channel selection at probe time

[Miquel Raynal]

Drivers are expected to set the PHY current_channel and current_page
according to their default state. The hwsim driver is advertising being
configured on channel 13 by default but that is not reflected in its own
internal pib structure. In order to ensure that this driver consider the
current channel as being 13 internally, we can call hwsim_hw_channel()
instead of creating an empty pib structure.

We assume here that kvfree_rcu(NULL) is a valid call.

Fixes: f25da51fdc38 ("ieee802154: hwsim: add replacement for fakelb")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/mac802154_hwsim.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
index 8caa61ec718f..795f8eb5387b 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -732,7 +732,6 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
 {
 	struct ieee802154_hw *hw;
 	struct hwsim_phy *phy;
-	struct hwsim_pib *pib;
 	int idx;
 	int err;
 
@@ -780,13 +779,8 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
 
 	/* hwsim phy channel 13 as default */
 	hw->phy->current_channel = 13;
-	pib = kzalloc(sizeof(*pib), GFP_KERNEL);
-	if (!pib) {
-		err = -ENOMEM;
-		goto err_pib;
-	}
+	hwsim_hw_channel(hw, hw->phy->current_page, hw->phy->current_channel);
 
-	rcu_assign_pointer(phy->pib, pib);
 	phy->idx = idx;
 	INIT_LIST_HEAD(&phy->edges);
 
@@ -815,8 +809,6 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
 err_subscribe:
 	ieee802154_unregister_hw(phy->hw);
 err_reg:
-	kfree(pib);
-err_pib:
 	ieee802154_free_hw(phy->hw);
 	return err;
 }
-- 
2.27.0

[wpan-next v2 2/9] net: ieee802154: hwsim: Ensure frame checksum are valid

[Miquel Raynal]

There is no point in accepting frames with a wrong or missing checksum,
at least not outside of a promiscuous setting. Set the right flag by
default in the hwsim driver to ensure checksums are not ignored.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/mac802154_hwsim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
index 795f8eb5387b..5324d0eda223 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -784,7 +784,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;
+	hw->flags = IEEE802154_HW_PROMISCUOUS | IEEE802154_HW_RX_DROP_BAD_CKSUM;
 	hw->parent = dev;
 
 	err = ieee802154_register_hw(hw);
-- 
2.27.0

[wpan-next v2 3/9] net: ieee802154: mcr20a: Fix lifs/sifs periods

[Miquel Raynal]

These periods are expressed in time units (microseconds) while 40 and 12
are the number of symbol durations these periods will last. We need to
multiply them both with phy->symbol_duration in order to get these
values in microseconds.

Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/mcr20a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
index 8dc04e2590b1..383231b85464 100644
--- a/drivers/net/ieee802154/mcr20a.c
+++ b/drivers/net/ieee802154/mcr20a.c
@@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp)
 	dev_dbg(printdev(lp), "%s\n", __func__);
 
 	phy->symbol_duration = 16;
-	phy->lifs_period = 40;
-	phy->sifs_period = 12;
+	phy->lifs_period = 40 * phy->symbol_duration;
+	phy->sifs_period = 12 * phy->symbol_duration;
 
 	hw->flags = IEEE802154_HW_TX_OMIT_CKSUM |
 			IEEE802154_HW_AFILT |
-- 
2.27.0

[wpan-next v2 4/9] net: ieee802154: at86rf230: Stop leaking skb's

[Miquel Raynal]

Upon error the ieee802154_xmit_complete() helper is not called. Only
ieee802154_wake_queue() is called manually. We then leak the skb
structure.

Free the skb structure upon error before returning.

There is no Fixes tag applying here, many changes have been made on this
area and the issue kind of always existed.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/at86rf230.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 7d67f41387f5..0746150f78cf 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context)
 		kfree(ctx);
 
 	ieee802154_wake_queue(lp->hw);
+	dev_kfree_skb_any(lp->tx_skb);
 }
 
 static void
-- 
2.27.0

[wpan-next v2 5/9] net: ieee802154: ca8210: Stop leaking skb's

[Miquel Raynal]

Upon error the ieee802154_xmit_complete() helper is not called. Only
ieee802154_wake_queue() is called manually. We then leak the skb
structure.

Free the skb structure upon error before returning.

Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/net/ieee802154/ca8210.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index ece6ff6049f6..8e69441f1fff 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -1772,6 +1772,7 @@ static int ca8210_async_xmit_complete(
 		);
 		if (status != MAC_TRANSACTION_OVERFLOW) {
 			ieee802154_wake_queue(priv->hw);
+			dev_kfree_skb_any(priv->tx_skb);
 			return 0;
 		}
 	}
-- 
2.27.0

[wpan-next v2 6/9] net: ieee802154: Use the IEEE802154_MAX_PAGE define when relevant

[Miquel Raynal]

This define already exist but is hardcoded in nl-phy.c. Use the
definition when relevant.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/ieee802154/nl-phy.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
index dd5a45f8a78a..02f6a53d0faa 100644
--- a/net/ieee802154/nl-phy.c
+++ b/net/ieee802154/nl-phy.c
@@ -30,7 +30,8 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
 {
 	void *hdr;
 	int i, pages = 0;
-	uint32_t *buf = kcalloc(32, sizeof(uint32_t), GFP_KERNEL);
+	uint32_t *buf = kcalloc(IEEE802154_MAX_PAGE + 1, sizeof(uint32_t),
+				GFP_KERNEL);
 
 	pr_debug("%s\n", __func__);
 
@@ -47,7 +48,7 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
 	    nla_put_u8(msg, IEEE802154_ATTR_PAGE, phy->current_page) ||
 	    nla_put_u8(msg, IEEE802154_ATTR_CHANNEL, phy->current_channel))
 		goto nla_put_failure;
-	for (i = 0; i < 32; i++) {
+	for (i = 0; i <= IEEE802154_MAX_PAGE; i++) {
 		if (phy->supported.channels[i])
 			buf[pages++] = phy->supported.channels[i] | (i << 27);
 	}
-- 
2.27.0

[wpan-next v2 7/9] net: ieee802154: Return meaningful error codes from the netlink helpers

[Miquel Raynal]

Returning -1 does not indicate anything useful.

Use a standard and meaningful error code instead.

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

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index 277124f206e0..e0b072aecf0f 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -1441,7 +1441,7 @@ static int nl802154_send_key(struct sk_buff *msg, u32 cmd, u32 portid,
 
 	hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
 	if (!hdr)
-		return -1;
+		return -ENOBUFS;
 
 	if (nla_put_u32(msg, NL802154_ATTR_IFINDEX, dev->ifindex))
 		goto nla_put_failure;
@@ -1634,7 +1634,7 @@ static int nl802154_send_device(struct sk_buff *msg, u32 cmd, u32 portid,
 
 	hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
 	if (!hdr)
-		return -1;
+		return -ENOBUFS;
 
 	if (nla_put_u32(msg, NL802154_ATTR_IFINDEX, dev->ifindex))
 		goto nla_put_failure;
@@ -1812,7 +1812,7 @@ static int nl802154_send_devkey(struct sk_buff *msg, u32 cmd, u32 portid,
 
 	hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
 	if (!hdr)
-		return -1;
+		return -ENOBUFS;
 
 	if (nla_put_u32(msg, NL802154_ATTR_IFINDEX, dev->ifindex))
 		goto nla_put_failure;
@@ -1988,7 +1988,7 @@ static int nl802154_send_seclevel(struct sk_buff *msg, u32 cmd, u32 portid,
 
 	hdr = nl802154hdr_put(msg, portid, seq, flags, cmd);
 	if (!hdr)
-		return -1;
+		return -ENOBUFS;
 
 	if (nla_put_u32(msg, NL802154_ATTR_IFINDEX, dev->ifindex))
 		goto nla_put_failure;
-- 
2.27.0

[wpan-next v2 8/9] net: mac802154: Explain the use of ieee802154_wake/stop_queue()

[Miquel Raynal]

It is not straightforward to the newcomer that a single skb can be sent
at a time and that the internal process is to stop the queue when
processing a frame before re-enabling it. Make this clear by documenting
the ieee802154_wake/stop_queue() helpers.

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

diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index d524ffb9eb25..94b2e3008e77 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -464,6 +464,12 @@ void ieee802154_rx_irqsafe(struct ieee802154_hw *hw, struct sk_buff *skb,
  * ieee802154_wake_queue - wake ieee802154 queue
  * @hw: pointer as obtained from ieee802154_alloc_hw().
  *
+ * Tranceivers have either one transmit framebuffer or one framebuffer for both
+ * transmitting and receiving. Hence, the core only handles one frame at a time
+ * for each phy, which means we had to stop the queue to avoid new skb to come
+ * during the transmission. The queue then needs to be woken up after the
+ * operation.
+ *
  * Drivers should use this function instead of netif_wake_queue.
  */
 void ieee802154_wake_queue(struct ieee802154_hw *hw);
@@ -472,6 +478,12 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw);
  * ieee802154_stop_queue - stop ieee802154 queue
  * @hw: pointer as obtained from ieee802154_alloc_hw().
  *
+ * Tranceivers have either one transmit framebuffer or one framebuffer for both
+ * transmitting and receiving. Hence, the core only handles one frame at a time
+ * for each phy, which means we need to tell upper layers to stop giving us new
+ * skbs while we are busy with the transmitted one. The queue must then be
+ * stopped before transmitting.
+ *
  * Drivers should use this function instead of netif_stop_queue.
  */
 void ieee802154_stop_queue(struct ieee802154_hw *hw);
-- 
2.27.0

[wpan-next v2 9/9] MAINTAINERS: Remove Harry Morris bouncing address

[Miquel Raynal]

Harry's e-mail address from Cascoda bounces, I have not found any
contributions from him since 2018 so let's drop the Maintainer entry
from the CA8210 driver and mark it Orphan.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 MAINTAINERS | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4d479b554361..ab2b32080b73 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4099,9 +4099,8 @@ N:	csky
 K:	csky
 
 CA8210 IEEE-802.15.4 RADIO DRIVER
-M:	Harry Morris <h.morris@cascoda.com>
 L:	linux-wpan@vger.kernel.org
-S:	Maintained
+S:	Orphan
 W:	https://github.com/Cascoda/ca8210-linux.git
 F:	Documentation/devicetree/bindings/net/ieee802154/ca8210.txt
 F:	drivers/net/ieee802154/ca8210.c
-- 
2.27.0

Re: [wpan-next v2 0/9] ieee802154: A bunch of fixes

[Alexander Aring]

Hi,

On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> In preparation to a wider series, here are a number of small and random
> fixes across the subsystem.
>
> Changes in v2:
> * Fixed the build error reported by a robot. It ended up being something
>   which I fixed in a commit from a following series. I've now sorted
>   this out and the patch now works on its own.
>

This patch series should be reviewed first and have all current
detected fixes, it also should be tagged "wpan" (no need to fix that
now). Then there is a following up series for a new feature which you
like to tackle, maybe the "more generic symbol duration handling"? It
should be based on this "fixes" patch series, Stefan will then get
things sorted out to queue them right for upstream.
Stefan, please correct me if I'm wrong.

Also, please give me the weekend to review this patch series.

Thanks.

- Alex

Re: [wpan-next v2 0/9] ieee802154: A bunch of fixes

[Alexander Aring]

Hi,

On Thu, 20 Jan 2022 at 17:52, Alexander Aring <alex.aring@gmail.com> wrote:
>
> Hi,
>
> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > In preparation to a wider series, here are a number of small and random
> > fixes across the subsystem.
> >
> > Changes in v2:
> > * Fixed the build error reported by a robot. It ended up being something
> >   which I fixed in a commit from a following series. I've now sorted
> >   this out and the patch now works on its own.
> >
>
> This patch series should be reviewed first and have all current
> detected fixes, it also should be tagged "wpan" (no need to fix that
> now). Then there is a following up series for a new feature which you
> like to tackle, maybe the "more generic symbol duration handling"? It
> should be based on this "fixes" patch series, Stefan will then get
> things sorted out to queue them right for upstream.
> Stefan, please correct me if I'm wrong.
>
> Also, please give me the weekend to review this patch series.

and please don't send other patch-series after this one isn't applied.
After it's applied, then send another patch series. Only one please,
then wait again for it to be applied... and so on.

- Alex

Re: [wpan-next v2 0/9] ieee802154: A bunch of fixes

[Miquel Raynal]

Hi Alexander,

alex.aring@gmail.com wrote on Thu, 20 Jan 2022 17:52:57 -0500:

> Hi,
> 
> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > In preparation to a wider series, here are a number of small and random
> > fixes across the subsystem.
> >
> > Changes in v2:
> > * Fixed the build error reported by a robot. It ended up being something
> >   which I fixed in a commit from a following series. I've now sorted
> >   this out and the patch now works on its own.
> >  
> 
> This patch series should be reviewed first and have all current
> detected fixes, it also should be tagged "wpan" (no need to fix that
> now). Then there is a following up series for a new feature which you
> like to tackle, maybe the "more generic symbol duration handling"? It
> should be based on this "fixes" patch series, Stefan will then get
> things sorted out to queue them right for upstream.
> Stefan, please correct me if I'm wrong.

Yup sorry that's not what I meant: the kernel robot detected that a
patch broke the build. This patch was part of the current series. The
issue was that I messed a copy paste error. But I didn't ran a
per-patch build test and another patch, which had nothing to do with
this fix, actually addressed the build issue. I very likely failed
something during my rebase operation.

So yes, this series should come first. Then we'll tackle the symbol
duration series, the Kconfig cleanup and after that we can start thick
topics :)

> Also, please give me the weekend to review this patch series.

Yes of course, you've been very (very) reactive so far, I try to be
also more reactive on my side but that's of course not a race!

Thanks,
Miquèl

Re: [wpan-next v2 0/9] ieee802154: A bunch of fixes

[Stefan Schmidt]

Hello.

On 21.01.22 09:27, Miquel Raynal wrote:
> Hi Alexander,
> 
> alex.aring@gmail.com wrote on Thu, 20 Jan 2022 17:52:57 -0500:
> 
>> Hi,
>>
>> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>>
>>> In preparation to a wider series, here are a number of small and random
>>> fixes across the subsystem.
>>>
>>> Changes in v2:
>>> * Fixed the build error reported by a robot. It ended up being something
>>>    which I fixed in a commit from a following series. I've now sorted
>>>    this out and the patch now works on its own.
>>>   
>>
>> This patch series should be reviewed first and have all current
>> detected fixes, it also should be tagged "wpan" (no need to fix that
>> now). Then there is a following up series for a new feature which you
>> like to tackle, maybe the "more generic symbol duration handling"? It
>> should be based on this "fixes" patch series, Stefan will then get
>> things sorted out to queue them right for upstream.
>> Stefan, please correct me if I'm wrong.

Alex, agreed. I will take this series first and see if the patches apply 
cleanly against my wpan tree. Once in they can be feed back into net, 
net-next and finally wpan-next again.

> Yup sorry that's not what I meant: the kernel robot detected that a
> patch broke the build. This patch was part of the current series. The
> issue was that I messed a copy paste error. But I didn't ran a
> per-patch build test and another patch, which had nothing to do with
> this fix, actually addressed the build issue. I very likely failed
> something during my rebase operation. >
> So yes, this series should come first. Then we'll tackle the symbol
> duration series, the Kconfig cleanup and after that we can start thick
> topics :)

That sounds like a great plan to me. I know splitting the huge amount of 
work you do up into digestible pieces is work not much liked, so I 
appreciate that you take this without much grumble. :-)

I also finally started to start my review backlog on your work. Catched 
up on the big v3 patchset now. Will look over the newest sets over the 
weekend so we should be ready to process the fixes series and maybe more 
next week.

>> Also, please give me the weekend to review this patch series.

Alex, whenever you are ready with them please add you ack and I will doe 
my review and testing in parallel.

> Yes of course, you've been very (very) reactive so far, I try to be
> also more reactive on my side but that's of course not a race!

And being so reactive is very much appreciated. We just need to throttle 
this a bit so we can keep up with reviewer resources. :-)

regards
Stefan Schmidt

Re: [wpan-next v2 6/9] net: ieee802154: Use the IEEE802154_MAX_PAGE define when relevant

[Stefan Schmidt]

Hello.

On 20.01.22 12:21, Miquel Raynal wrote:
> This define already exist but is hardcoded in nl-phy.c. Use the
> definition when relevant.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   net/ieee802154/nl-phy.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
> index dd5a45f8a78a..02f6a53d0faa 100644
> --- a/net/ieee802154/nl-phy.c
> +++ b/net/ieee802154/nl-phy.c
> @@ -30,7 +30,8 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
>   {
>   	void *hdr;
>   	int i, pages = 0;
> -	uint32_t *buf = kcalloc(32, sizeof(uint32_t), GFP_KERNEL);
> +	uint32_t *buf = kcalloc(IEEE802154_MAX_PAGE + 1, sizeof(uint32_t),

Please use u32 here. I know we have some uint*_t leftovers but for new 
code we should not use them anymore.

> +				GFP_KERNEL);
>   
>   	pr_debug("%s\n", __func__);
>   
> @@ -47,7 +48,7 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
>   	    nla_put_u8(msg, IEEE802154_ATTR_PAGE, phy->current_page) ||
>   	    nla_put_u8(msg, IEEE802154_ATTR_CHANNEL, phy->current_channel))
>   		goto nla_put_failure;
> -	for (i = 0; i < 32; i++) {
> +	for (i = 0; i <= IEEE802154_MAX_PAGE; i++) {
>   		if (phy->supported.channels[i])
>   			buf[pages++] = phy->supported.channels[i] | (i << 27);
>   	}
> 

regards
Stefan Schmidt

Re: [wpan-next v2 3/9] net: ieee802154: mcr20a: Fix lifs/sifs periods

[Stefan Schmidt]

Hello Xue.

You are still listed as maintainer for the mcr20a driver, thus this mail.

On 20.01.22 12:21, Miquel Raynal wrote:
> These periods are expressed in time units (microseconds) while 40 and 12
> are the number of symbol durations these periods will last. We need to
> multiply them both with phy->symbol_duration in order to get these
> values in microseconds.
> 
> Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   drivers/net/ieee802154/mcr20a.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> index 8dc04e2590b1..383231b85464 100644
> --- a/drivers/net/ieee802154/mcr20a.c
> +++ b/drivers/net/ieee802154/mcr20a.c
> @@ -976,8 +976,8 @@ static void mcr20a_hw_setup(struct mcr20a_local *lp)
>   	dev_dbg(printdev(lp), "%s\n", __func__);
>   
>   	phy->symbol_duration = 16;
> -	phy->lifs_period = 40;
> -	phy->sifs_period = 12;
> +	phy->lifs_period = 40 * phy->symbol_duration;
> +	phy->sifs_period = 12 * phy->symbol_duration;

To me this looks correct and I would go ahead and apply it. Xue, if you 
disagree please reply and explain why.

regards
Stefan Schmidt

Re: [wpan-next v2 0/9] ieee802154: A bunch of fixes

[Stefan Schmidt]

Hello Miquel

On 20.01.22 12:21, Miquel Raynal wrote:
> In preparation to a wider series, here are a number of small and random
> fixes across the subsystem.
> 
> Changes in v2:
> * Fixed the build error reported by a robot. It ended up being something
>    which I fixed in a commit from a following series. I've now sorted
>    this out and the patch now works on its own.
> 
> Miquel Raynal (9):
>    net: ieee802154: hwsim: Ensure proper channel selection at probe time
>    net: ieee802154: hwsim: Ensure frame checksum are valid
>    net: ieee802154: mcr20a: Fix lifs/sifs periods
>    net: ieee802154: at86rf230: Stop leaking skb's
>    net: ieee802154: ca8210: Stop leaking skb's
>    net: ieee802154: Use the IEEE802154_MAX_PAGE define when relevant
>    net: ieee802154: Return meaningful error codes from the netlink
>      helpers
>    net: mac802154: Explain the use of ieee802154_wake/stop_queue()
>    MAINTAINERS: Remove Harry Morris bouncing address
> 
>   MAINTAINERS                              |  3 +--
>   drivers/net/ieee802154/at86rf230.c       |  1 +
>   drivers/net/ieee802154/ca8210.c          |  1 +
>   drivers/net/ieee802154/mac802154_hwsim.c | 12 ++----------
>   drivers/net/ieee802154/mcr20a.c          |  4 ++--
>   include/net/mac802154.h                  | 12 ++++++++++++
>   net/ieee802154/nl-phy.c                  |  5 +++--
>   net/ieee802154/nl802154.c                |  8 ++++----
>   8 files changed, 26 insertions(+), 20 deletions(-)
> 

All patches apply without conflict to my wpan tree (to feed into net 
instead of net-next for these fixes) and compile testing showed no 
problems. I will do some light smoke testing on the weekend and also 
wait for Alex review. But beside one small review remark these should be 
ready to go in on the next iteration.

regards
Stefan Schmidt

Re: [wpan-next v2 1/9] net: ieee802154: hwsim: Ensure proper channel selection at probe time

[Alexander Aring]

Hi,

On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Drivers are expected to set the PHY current_channel and current_page
> according to their default state. The hwsim driver is advertising being
> configured on channel 13 by default but that is not reflected in its own
> internal pib structure. In order to ensure that this driver consider the
> current channel as being 13 internally, we can call hwsim_hw_channel()
> instead of creating an empty pib structure.
>
> We assume here that kvfree_rcu(NULL) is a valid call.
>
> Fixes: f25da51fdc38 ("ieee802154: hwsim: add replacement for fakelb")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/net/ieee802154/mac802154_hwsim.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> index 8caa61ec718f..795f8eb5387b 100644
> --- a/drivers/net/ieee802154/mac802154_hwsim.c
> +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> @@ -732,7 +732,6 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
>  {
>         struct ieee802154_hw *hw;
>         struct hwsim_phy *phy;
> -       struct hwsim_pib *pib;
>         int idx;
>         int err;
>
> @@ -780,13 +779,8 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
>
>         /* hwsim phy channel 13 as default */
>         hw->phy->current_channel = 13;
> -       pib = kzalloc(sizeof(*pib), GFP_KERNEL);
> -       if (!pib) {
> -               err = -ENOMEM;
> -               goto err_pib;
> -       }
> +       hwsim_hw_channel(hw, hw->phy->current_page, hw->phy->current_channel);

Probably you saw it already; this will end in a
"suspicious_RCU_usage", that's because of an additional lock check in
hwsim_hw_channel() which checks if rtnl is held. However, in this
situation it's not necessary to hold the rtnl lock because we know the
phy is not being registered yet.

Either we change it to rcu_derefence() but then we would reduce the
check if rtnl lock is being held or just simply initial the default
pib->channel here to 13 which makes that whole patch a one line fix.

- Alex

Re: [wpan-next v2 4/9] net: ieee802154: at86rf230: Stop leaking skb's

[Alexander Aring]

Hi,

On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Upon error the ieee802154_xmit_complete() helper is not called. Only
> ieee802154_wake_queue() is called manually. We then leak the skb
> structure.
>
> Free the skb structure upon error before returning.
>
> There is no Fixes tag applying here, many changes have been made on this
> area and the issue kind of always existed.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/net/ieee802154/at86rf230.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 7d67f41387f5..0746150f78cf 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context)
>                 kfree(ctx);
>
>         ieee802154_wake_queue(lp->hw);
> +       dev_kfree_skb_any(lp->tx_skb);

as I said in other mails there is more broken, we need a:

if (lp->is_tx) {
        ieee802154_wake_queue(lp->hw);
        dev_kfree_skb_any(lp->tx_skb);
        lp->is_tx = 0;
}

in at86rf230_async_error_recover().

Thanks.

- Alex

Re: [wpan-next v2 6/9] net: ieee802154: Use the IEEE802154_MAX_PAGE define when relevant

[Alexander Aring]

Hi,

On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> This define already exist but is hardcoded in nl-phy.c. Use the
> definition when relevant.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  net/ieee802154/nl-phy.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
> index dd5a45f8a78a..02f6a53d0faa 100644
> --- a/net/ieee802154/nl-phy.c
> +++ b/net/ieee802154/nl-phy.c
> @@ -30,7 +30,8 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
>  {
>         void *hdr;
>         int i, pages = 0;
> -       uint32_t *buf = kcalloc(32, sizeof(uint32_t), GFP_KERNEL);
> +       uint32_t *buf = kcalloc(IEEE802154_MAX_PAGE + 1, sizeof(uint32_t),
> +                               GFP_KERNEL);
>
>         pr_debug("%s\n", __func__);
>
> @@ -47,7 +48,7 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
>             nla_put_u8(msg, IEEE802154_ATTR_PAGE, phy->current_page) ||
>             nla_put_u8(msg, IEEE802154_ATTR_CHANNEL, phy->current_channel))
>                 goto nla_put_failure;
> -       for (i = 0; i < 32; i++) {
> +       for (i = 0; i <= IEEE802154_MAX_PAGE; i++) {
>                 if (phy->supported.channels[i])
>                         buf[pages++] = phy->supported.channels[i] | (i << 27);
>         }

Where is the fix here?

- Alex

Re: [wpan-next v2 8/9] net: mac802154: Explain the use of ieee802154_wake/stop_queue()

[Alexander Aring]

Hi,

On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> It is not straightforward to the newcomer that a single skb can be sent
> at a time and that the internal process is to stop the queue when
> processing a frame before re-enabling it. Make this clear by documenting
> the ieee802154_wake/stop_queue() helpers.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/net/mac802154.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> index d524ffb9eb25..94b2e3008e77 100644
> --- a/include/net/mac802154.h
> +++ b/include/net/mac802154.h
> @@ -464,6 +464,12 @@ void ieee802154_rx_irqsafe(struct ieee802154_hw *hw, struct sk_buff *skb,
>   * ieee802154_wake_queue - wake ieee802154 queue
>   * @hw: pointer as obtained from ieee802154_alloc_hw().
>   *
> + * Tranceivers have either one transmit framebuffer or one framebuffer for both
> + * transmitting and receiving. Hence, the core only handles one frame at a time

this is not a fundamental physical law, they might exist but not supported yet.

> + * for each phy, which means we had to stop the queue to avoid new skb to come
> + * during the transmission. The queue then needs to be woken up after the
> + * operation.
> + *
>   * Drivers should use this function instead of netif_wake_queue.

It's a must.

>   */
>  void ieee802154_wake_queue(struct ieee802154_hw *hw);
> @@ -472,6 +478,12 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw);
>   * ieee802154_stop_queue - stop ieee802154 queue
>   * @hw: pointer as obtained from ieee802154_alloc_hw().
>   *
> + * Tranceivers have either one transmit framebuffer or one framebuffer for both
> + * transmitting and receiving. Hence, the core only handles one frame at a time
> + * for each phy, which means we need to tell upper layers to stop giving us new
> + * skbs while we are busy with the transmitted one. The queue must then be
> + * stopped before transmitting.
> + *
>   * Drivers should use this function instead of netif_stop_queue.
>   */
>  void ieee802154_stop_queue(struct ieee802154_hw *hw);

Same for stop, stop has something additional here... it is never used
by any driver because we do that on mac802154 layer. Simply, they
don't use this function.

Where is the fix here?

- Alex

Re: [wpan-next v2 4/9] net: ieee802154: at86rf230: Stop leaking skb's

[Alexander Aring]

Hi,

On Sun, 23 Jan 2022 at 15:43, Alexander Aring <alex.aring@gmail.com> wrote:
>
> Hi,
>
> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Upon error the ieee802154_xmit_complete() helper is not called. Only
> > ieee802154_wake_queue() is called manually. We then leak the skb
> > structure.
> >
> > Free the skb structure upon error before returning.
> >
> > There is no Fixes tag applying here, many changes have been made on this
> > area and the issue kind of always existed.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/net/ieee802154/at86rf230.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> > index 7d67f41387f5..0746150f78cf 100644
> > --- a/drivers/net/ieee802154/at86rf230.c
> > +++ b/drivers/net/ieee802154/at86rf230.c
> > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context)
> >                 kfree(ctx);
> >
> >         ieee802154_wake_queue(lp->hw);
> > +       dev_kfree_skb_any(lp->tx_skb);
>
> as I said in other mails there is more broken, we need a:
>
> if (lp->is_tx) {
>         ieee802154_wake_queue(lp->hw);
>         dev_kfree_skb_any(lp->tx_skb);
>         lp->is_tx = 0;
> }
>

Also we should free the skb at first _then_ wake_queue().

- Alex

Re: [wpan-next v2 5/9] net: ieee802154: ca8210: Stop leaking skb's

[Alexander Aring]

Hi,

On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Upon error the ieee802154_xmit_complete() helper is not called. Only
> ieee802154_wake_queue() is called manually. We then leak the skb
> structure.
>
> Free the skb structure upon error before returning.
>
> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/net/ieee802154/ca8210.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index ece6ff6049f6..8e69441f1fff 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -1772,6 +1772,7 @@ static int ca8210_async_xmit_complete(
>                 );
>                 if (status != MAC_TRANSACTION_OVERFLOW) {
>                         ieee802154_wake_queue(priv->hw);
> +                       dev_kfree_skb_any(priv->tx_skb);
>                         return 0;

first free() then wake().

- Alex

Re: [wpan-next v2 4/9] net: ieee802154: at86rf230: Stop leaking skb's

[Alexander Aring]

Hi,

On Sun, 23 Jan 2022 at 15:43, Alexander Aring <alex.aring@gmail.com> wrote:
>
> Hi,
>
> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Upon error the ieee802154_xmit_complete() helper is not called. Only
> > ieee802154_wake_queue() is called manually. We then leak the skb
> > structure.
> >
> > Free the skb structure upon error before returning.
> >
> > There is no Fixes tag applying here, many changes have been made on this
> > area and the issue kind of always existed.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/net/ieee802154/at86rf230.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> > index 7d67f41387f5..0746150f78cf 100644
> > --- a/drivers/net/ieee802154/at86rf230.c
> > +++ b/drivers/net/ieee802154/at86rf230.c
> > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context)
> >                 kfree(ctx);
> >
> >         ieee802154_wake_queue(lp->hw);
> > +       dev_kfree_skb_any(lp->tx_skb);
>
> as I said in other mails there is more broken, we need a:
>
> if (lp->is_tx) {
>         ieee802154_wake_queue(lp->hw);
>         dev_kfree_skb_any(lp->tx_skb);
>         lp->is_tx = 0;
> }
>
> in at86rf230_async_error_recover().
>
s/at86rf230_async_error_recover/at86rf230_async_error_recover_complete/

move the is_tx = 0 out of at86rf230_async_error_recover().

- Alex

Re: [wpan-next v2 4/9] net: ieee802154: at86rf230: Stop leaking skb's

[Alexander Aring]

Hi,

On Sun, 23 Jan 2022 at 17:41, Alexander Aring <alex.aring@gmail.com> wrote:
>
> Hi,
>
> On Sun, 23 Jan 2022 at 15:43, Alexander Aring <alex.aring@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Upon error the ieee802154_xmit_complete() helper is not called. Only
> > > ieee802154_wake_queue() is called manually. We then leak the skb
> > > structure.
> > >
> > > Free the skb structure upon error before returning.
> > >
> > > There is no Fixes tag applying here, many changes have been made on this
> > > area and the issue kind of always existed.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/net/ieee802154/at86rf230.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> > > index 7d67f41387f5..0746150f78cf 100644
> > > --- a/drivers/net/ieee802154/at86rf230.c
> > > +++ b/drivers/net/ieee802154/at86rf230.c
> > > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context)
> > >                 kfree(ctx);
> > >
> > >         ieee802154_wake_queue(lp->hw);
> > > +       dev_kfree_skb_any(lp->tx_skb);
> >
> > as I said in other mails there is more broken, we need a:
> >
> > if (lp->is_tx) {
> >         ieee802154_wake_queue(lp->hw);
> >         dev_kfree_skb_any(lp->tx_skb);
> >         lp->is_tx = 0;
> > }
> >
> > in at86rf230_async_error_recover().
> >
> s/at86rf230_async_error_recover/at86rf230_async_error_recover_complete/
>
> move the is_tx = 0 out of at86rf230_async_error_recover().

Sorry, still seeing an issue here.

We cannot move is_tx = 0 out of at86rf230_async_error_recover()
because switching to RX_AACK_ON races with a new interrupt and is_tx
is not correct anymore. We need something new like "was_tx" to
remember that it was a tx case for the error handling in
at86rf230_async_error_recover_complete().

- Alex

Re: [wpan-next v2 6/9] net: ieee802154: Use the IEEE802154_MAX_PAGE define when relevant

[Stefan Schmidt]

Hello.

On 23.01.22 21:44, Alexander Aring wrote:
> Hi,
> 
> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> This define already exist but is hardcoded in nl-phy.c. Use the
>> definition when relevant.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>   net/ieee802154/nl-phy.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
>> index dd5a45f8a78a..02f6a53d0faa 100644
>> --- a/net/ieee802154/nl-phy.c
>> +++ b/net/ieee802154/nl-phy.c
>> @@ -30,7 +30,8 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
>>   {
>>          void *hdr;
>>          int i, pages = 0;
>> -       uint32_t *buf = kcalloc(32, sizeof(uint32_t), GFP_KERNEL);
>> +       uint32_t *buf = kcalloc(IEEE802154_MAX_PAGE + 1, sizeof(uint32_t),
>> +                               GFP_KERNEL);
>>
>>          pr_debug("%s\n", __func__);
>>
>> @@ -47,7 +48,7 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
>>              nla_put_u8(msg, IEEE802154_ATTR_PAGE, phy->current_page) ||
>>              nla_put_u8(msg, IEEE802154_ATTR_CHANNEL, phy->current_channel))
>>                  goto nla_put_failure;
>> -       for (i = 0; i < 32; i++) {
>> +       for (i = 0; i <= IEEE802154_MAX_PAGE; i++) {
>>                  if (phy->supported.channels[i])
>>                          buf[pages++] = phy->supported.channels[i] | (i << 27);
>>          }
> 
> Where is the fix here?

While its more cleanup than fix, its clear and easy and there is no 
problem for it to go into wpan instead of wpan-next.

regards
Stefan Schmidt

Re: [wpan-next v2 0/9] ieee802154: A bunch of fixes

[Miquel Raynal]

Hi Stefan,

stefan@datenfreihafen.org wrote on Fri, 21 Jan 2022 13:48:14 +0100:

> Hello.
> 
> On 21.01.22 09:27, Miquel Raynal wrote:
> > Hi Alexander,
> > 
> > alex.aring@gmail.com wrote on Thu, 20 Jan 2022 17:52:57 -0500:
> >   
> >> Hi,
> >>
> >> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> >>>
> >>> In preparation to a wider series, here are a number of small and random
> >>> fixes across the subsystem.
> >>>
> >>> Changes in v2:
> >>> * Fixed the build error reported by a robot. It ended up being something
> >>>    which I fixed in a commit from a following series. I've now sorted
> >>>    this out and the patch now works on its own.  
> >>>   >>  
> >> This patch series should be reviewed first and have all current
> >> detected fixes, it also should be tagged "wpan" (no need to fix that
> >> now). Then there is a following up series for a new feature which you
> >> like to tackle, maybe the "more generic symbol duration handling"? It
> >> should be based on this "fixes" patch series, Stefan will then get
> >> things sorted out to queue them right for upstream.
> >> Stefan, please correct me if I'm wrong.  
> 
> Alex, agreed. I will take this series first and see if the patches apply cleanly against my wpan tree. Once in they can be feed back into net, net-next and finally wpan-next again.
> 
> > Yup sorry that's not what I meant: the kernel robot detected that a
> > patch broke the build. This patch was part of the current series. The
> > issue was that I messed a copy paste error. But I didn't ran a
> > per-patch build test and another patch, which had nothing to do with
> > this fix, actually addressed the build issue. I very likely failed
> > something during my rebase operation. >
> > So yes, this series should come first. Then we'll tackle the symbol
> > duration series, the Kconfig cleanup and after that we can start thick
> > topics :)  
> 
> That sounds like a great plan to me. I know splitting the huge amount of work you do up into digestible pieces is work not much liked, so I appreciate that you take this without much grumble. :-)

Yeah no problem, I also know what it is like to be on the reviewer
side, and while I like to have the full contribution to get the big
picture, I also find it much easier to review smaller series, so let's
got for it now that the main boundaries for the scan support have been
shared.

> I also finally started to start my review backlog on your work. Catched up on the big v3 patchset now. Will look over the newest sets over the weekend so we should be ready to process the fixes series and maybe more next week.
> 
> >> Also, please give me the weekend to review this patch series.  
> 
> Alex, whenever you are ready with them please add you ack and I will doe my review and testing in parallel.
> 
> > Yes of course, you've been very (very) reactive so far, I try to be
> > also more reactive on my side but that's of course not a race!  
> 
> And being so reactive is very much appreciated. We just need to throttle this a bit so we can keep up with reviewer resources. :-)

Yup! I'll soon send a v3 addressing your comments, this time I'll even
split the first series so that you have a series with only fixes for
the wpan branch, and another series with very small cleanups for
wpan-next. I'll send both because they are not very big.

Thanks,
Miquèl

Re: [wpan-next v2 1/9] net: ieee802154: hwsim: Ensure proper channel selection at probe time

[Miquel Raynal]

Hi Alexander,

alex.aring@gmail.com wrote on Sun, 23 Jan 2022 15:34:14 -0500:

> Hi,
> 
> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Drivers are expected to set the PHY current_channel and current_page
> > according to their default state. The hwsim driver is advertising being
> > configured on channel 13 by default but that is not reflected in its own
> > internal pib structure. In order to ensure that this driver consider the
> > current channel as being 13 internally, we can call hwsim_hw_channel()
> > instead of creating an empty pib structure.
> >
> > We assume here that kvfree_rcu(NULL) is a valid call.
> >
> > Fixes: f25da51fdc38 ("ieee802154: hwsim: add replacement for fakelb")
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/net/ieee802154/mac802154_hwsim.c | 10 +---------
> >  1 file changed, 1 insertion(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> > index 8caa61ec718f..795f8eb5387b 100644
> > --- a/drivers/net/ieee802154/mac802154_hwsim.c
> > +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> > @@ -732,7 +732,6 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
> >  {
> >         struct ieee802154_hw *hw;
> >         struct hwsim_phy *phy;
> > -       struct hwsim_pib *pib;
> >         int idx;
> >         int err;
> >
> > @@ -780,13 +779,8 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev,
> >
> >         /* hwsim phy channel 13 as default */
> >         hw->phy->current_channel = 13;
> > -       pib = kzalloc(sizeof(*pib), GFP_KERNEL);
> > -       if (!pib) {
> > -               err = -ENOMEM;
> > -               goto err_pib;
> > -       }
> > +       hwsim_hw_channel(hw, hw->phy->current_page, hw->phy->current_channel);  
> 
> Probably you saw it already; this will end in a
> "suspicious_RCU_usage", that's because of an additional lock check in
> hwsim_hw_channel() which checks if rtnl is held. However, in this
> situation it's not necessary to hold the rtnl lock because we know the
> phy is not being registered yet.

yes, indeed!

> 
> Either we change it to rcu_derefence() but then we would reduce the
> check if rtnl lock is being held or just simply initial the default
> pib->channel here to 13 which makes that whole patch a one line fix.

In general I like to drop more lines than I add, hence my first choice
but just setting pib->channel to 13 also makes sense here and avoids
oversimplifying the rtnl check in hwsim_hw_channel(), so let's go for
it.

Thanks,
Miquèl

Re: [wpan-next v2 4/9] net: ieee802154: at86rf230: Stop leaking skb's

[Miquel Raynal]

Hi Alexander,

alex.aring@gmail.com wrote on Sun, 23 Jan 2022 18:14:12 -0500:

> Hi,
> 
> On Sun, 23 Jan 2022 at 17:41, Alexander Aring <alex.aring@gmail.com> wrote:
> >
> > Hi,
> >
> > On Sun, 23 Jan 2022 at 15:43, Alexander Aring <alex.aring@gmail.com> wrote:  
> > >
> > > Hi,
> > >
> > > On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> > > >
> > > > Upon error the ieee802154_xmit_complete() helper is not called. Only
> > > > ieee802154_wake_queue() is called manually. We then leak the skb
> > > > structure.
> > > >
> > > > Free the skb structure upon error before returning.
> > > >
> > > > There is no Fixes tag applying here, many changes have been made on this
> > > > area and the issue kind of always existed.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/net/ieee802154/at86rf230.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> > > > index 7d67f41387f5..0746150f78cf 100644
> > > > --- a/drivers/net/ieee802154/at86rf230.c
> > > > +++ b/drivers/net/ieee802154/at86rf230.c
> > > > @@ -344,6 +344,7 @@ at86rf230_async_error_recover_complete(void *context)
> > > >                 kfree(ctx);
> > > >
> > > >         ieee802154_wake_queue(lp->hw);
> > > > +       dev_kfree_skb_any(lp->tx_skb);  
> > >
> > > as I said in other mails there is more broken, we need a:
> > >
> > > if (lp->is_tx) {
> > >         ieee802154_wake_queue(lp->hw);
> > >         dev_kfree_skb_any(lp->tx_skb);
> > >         lp->is_tx = 0;
> > > }
> > >
> > > in at86rf230_async_error_recover().
> > >  
> > s/at86rf230_async_error_recover/at86rf230_async_error_recover_complete/
> >
> > move the is_tx = 0 out of at86rf230_async_error_recover().  
> 
> Sorry, still seeing an issue here.
> 
> We cannot move is_tx = 0 out of at86rf230_async_error_recover()
> because switching to RX_AACK_ON races with a new interrupt and is_tx
> is not correct anymore. We need something new like "was_tx" to
> remember that it was a tx case for the error handling in
> at86rf230_async_error_recover_complete().

It wasn't easy to catch...

I've added a was_tx boolean which is set at the same time is_tx is
reset. Then, in the complete handler, if was_tx was set we reset it and
run the kfree/wake calls. I believe this should sort it out.

Thanks,
Miquèl

Re: [wpan-next v2 6/9] net: ieee802154: Use the IEEE802154_MAX_PAGE define when relevant

[Miquel Raynal]

Hi Stefan,

stefan@datenfreihafen.org wrote on Mon, 24 Jan 2022 09:06:39 +0100:

> Hello.
> 
> On 23.01.22 21:44, Alexander Aring wrote:
> > Hi,
> > 
> > On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:  
> >>
> >> This define already exist but is hardcoded in nl-phy.c. Use the
> >> definition when relevant.
> >>
> >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >> ---
> >>   net/ieee802154/nl-phy.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
> >> index dd5a45f8a78a..02f6a53d0faa 100644
> >> --- a/net/ieee802154/nl-phy.c
> >> +++ b/net/ieee802154/nl-phy.c
> >> @@ -30,7 +30,8 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
> >>   {
> >>          void *hdr;
> >>          int i, pages = 0;
> >> -       uint32_t *buf = kcalloc(32, sizeof(uint32_t), GFP_KERNEL);
> >> +       uint32_t *buf = kcalloc(IEEE802154_MAX_PAGE + 1, sizeof(uint32_t),
> >> +                               GFP_KERNEL);
> >>
> >>          pr_debug("%s\n", __func__);
> >>
> >> @@ -47,7 +48,7 @@ static int ieee802154_nl_fill_phy(struct sk_buff *msg, u32 portid,
> >>              nla_put_u8(msg, IEEE802154_ATTR_PAGE, phy->current_page) ||
> >>              nla_put_u8(msg, IEEE802154_ATTR_CHANNEL, phy->current_channel))
> >>                  goto nla_put_failure;
> >> -       for (i = 0; i < 32; i++) {
> >> +       for (i = 0; i <= IEEE802154_MAX_PAGE; i++) {
> >>                  if (phy->supported.channels[i])
> >>                          buf[pages++] = phy->supported.channels[i] | (i << 27);
> >>          }  
> > 
> > Where is the fix here?  
> 
> While its more cleanup than fix, its clear and easy and there is no problem for it to go into wpan instead of wpan-next.

As answered earlier, I will split the series so that it's clearer what
should go to wpan and what should go to wpan-next, no problem with that.

Thanks,
Miquèl

Re: [wpan-next v2 8/9] net: mac802154: Explain the use of ieee802154_wake/stop_queue()

[Miquel Raynal]

Hi Alexander,

alex.aring@gmail.com wrote on Sun, 23 Jan 2022 15:56:22 -0500:

> Hi,
> 
> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > It is not straightforward to the newcomer that a single skb can be sent
> > at a time and that the internal process is to stop the queue when
> > processing a frame before re-enabling it. Make this clear by documenting
> > the ieee802154_wake/stop_queue() helpers.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/net/mac802154.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> > index d524ffb9eb25..94b2e3008e77 100644
> > --- a/include/net/mac802154.h
> > +++ b/include/net/mac802154.h
> > @@ -464,6 +464,12 @@ void ieee802154_rx_irqsafe(struct ieee802154_hw *hw, struct sk_buff *skb,
> >   * ieee802154_wake_queue - wake ieee802154 queue
> >   * @hw: pointer as obtained from ieee802154_alloc_hw().
> >   *
> > + * Tranceivers have either one transmit framebuffer or one framebuffer for both
> > + * transmitting and receiving. Hence, the core only handles one frame at a time  
> 
> this is not a fundamental physical law, they might exist but not supported yet.

I think it's important to explain why we call these helpers
before/after transmissions. I've reworded the beginning of the sentence
to: "Tranceivers usually have..." to reflect that this is the current
state of the support, but is not marble solid either. I've also updated
the second sentence about the core "Hence, the core currently only
handles..."

> > + * for each phy, which means we had to stop the queue to avoid new skb to come
> > + * during the transmission. The queue then needs to be woken up after the
> > + * operation.
> > + *
> >   * Drivers should use this function instead of netif_wake_queue.  
> 
> It's a must.
>
> >   */
> >  void ieee802154_wake_queue(struct ieee802154_hw *hw);
> > @@ -472,6 +478,12 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw);
> >   * ieee802154_stop_queue - stop ieee802154 queue
> >   * @hw: pointer as obtained from ieee802154_alloc_hw().
> >   *
> > + * Tranceivers have either one transmit framebuffer or one framebuffer for both
> > + * transmitting and receiving. Hence, the core only handles one frame at a time
> > + * for each phy, which means we need to tell upper layers to stop giving us new
> > + * skbs while we are busy with the transmitted one. The queue must then be
> > + * stopped before transmitting.
> > + *
> >   * Drivers should use this function instead of netif_stop_queue.
> >   */
> >  void ieee802154_stop_queue(struct ieee802154_hw *hw);  
> 
> Same for stop, stop has something additional here... it is never used
> by any driver because we do that on mac802154 layer. Simply, they
> don't use this function.

That's true, and this is addressed in a later series where we actually
stop exporting these helpers because we want everything to be handled
by the _xmit_complete/error() helpers and keep full control of the
queue. The comments added above will remain because they are useful to
understand why these helpers are called. But the last sentence above
their use from drivers being preferred to the netif_ calls will be
dropped.

Thanks,
Miquèl