Re: [RFC wpan-next] atusb: add support for trac feature

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Alexander,

aahringo@redhat.com wrote on Sun,  4 Sep 2022 20:55:44 -0400:

> 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>
> ---
> 
> Just an RFC, need another weekend to test it.
> 
>  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));

I've been fighting all night thinking the issues were on the atusb side
(it was horribly difficult to get the atusb toolchain up and running,
I'll send a patch to update the instructions), in particular because of
the data[2] definition which needed to be declared static outside of
the functions (see the other mail) and, I guess, because of this
beginner error: I was using skb->data[1] but of course it can't work.

Anyway, this patch works, I've tested it by:
- associating a device (an Arduino Nano running Zephyr, btw)
- having the atusb disassociating the nano successfully (trac = 0)
- reassociating the nano
- powering off the nano
- trying to disassociate

The disassociation notification transmission fails with a TRAC status
NO_ACK. So:

Tested-by: Miquel Raynal <miquel.raynal@bootlin.com>

I still have the filtering thing to improve, I'm not sure I'll have
time to do that this week. I need some time to prepare the slides now
that the demo works :)

> +		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;
>  	}
>  


Thanks,
Miquèl