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

[Alexander Aring <aahringo@redhat.com>]

Hi,

On Mon, Sep 5, 2022 at 10:19 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> aahringo@redhat.com wrote on Mon, 5 Sep 2022 07:16:42 -0400:
>
> > Hi,
> >
> > On Sun, Sep 4, 2022 at 11:22 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > 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
> >
> > Really? for me it was just apt install avr-gcc libc-avr (or what the
> > debian packages name is).
>
> I feel so bad. All the instructions I could find (from 2011) advised to
> download an old gcc, an old binutils and old avr-lib, to compile
> everything by hand... 2 out of 3 archives returned a 404 error, the
> builds were chaotic I had to disable -Werror and fix many issues
> manually, then I had to fight with binutils assembler failing, I tried
> 4 different versions before I got it right... I did write a commit to
> update the instructions but if you say that just downloading the stock
> pre-compiled binaries worked I am a bit disappointed because the
> instructions specifically told not to do so.
>

I am sorry that you had those problems.

> > Then the dfu-util and be sure you invoke
> > dfu-util (as root because you might need some udev rules otherwise)
> > when the atusb is booting up.
>
> Yes, dfu worked right away, very nice tool.
>
> > > 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.
> > >
> >
> > mhh, I am sorry. I am not sure if I understand what you mean? Does the
> > firmware patch have issues regarding data[2]?
>
> Actually I'm wrong.
> You did: *(skb->data + 1), which works.
> I did: skb->data[1], which does not.
> I thought that my mistake was related to data being a void *, but it's
> a char * so both should work. I don't know why it failed, maybe my
> attention level was too low and I missed something else.

I don't see a difference. No idea...

- Alex