Re: [PATCH wpan-next v4 07/11] net: ieee802154: at86rf230: Provide meaningful error codes when possible

[Alexander Aring <alex.aring@gmail.com>]

Hi,

On Fri, Mar 18, 2022 at 2:56 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Either the spi operation failed, or the offloaded transmit operation
> failed and returned a TRAC value. Use this value when available or use
> the default "SYSTEM_ERROR" otherwise, in order to propagate one step
> above the error.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/net/ieee802154/at86rf230.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index d3cf6d23b57e..34d199f597c9 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -358,7 +358,23 @@ static inline void
>  at86rf230_async_error(struct at86rf230_local *lp,
>                       struct at86rf230_state_change *ctx, int rc)
>  {
> -       dev_err(&lp->spi->dev, "spi_async error %d\n", rc);
> +       int reason;
> +
> +       switch (rc) {

I think there was a miscommunication last time, this rc variable is
not a trac register value, it is a linux errno. Also the error here
has nothing to do with a trac error. A trac error is the result of the
offloaded transmit functionality on the transceiver, here we dealing
about bus communication errors produced by the spi subsystem. What we
need is to report it to the softmac layer as "IEEE802154_SYSTEM_ERROR"
(as we decided that this is a user specific error and can be returned
by the transceiver for non 802.15.4 "error" return code.

> +       case TRAC_CHANNEL_ACCESS_FAILURE:
> +               reason = IEEE802154_CHANNEL_ACCESS_FAILURE;
> +               break;
> +       case TRAC_NO_ACK:
> +               reason = IEEE802154_NO_ACK;
> +               break;
> +       default:
> +               reason = IEEE802154_SYSTEM_ERROR;
> +       }
> +
> +       if (rc < 0)
> +               dev_err(&lp->spi->dev, "spi_async error %d\n", rc);
> +       else
> +               dev_err(&lp->spi->dev, "xceiver error %d\n", reason);
>
>         at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF,
>                                      at86rf230_async_error_recover);
> @@ -666,10 +682,15 @@ at86rf230_tx_trac_check(void *context)
>         case TRAC_SUCCESS:
>         case TRAC_SUCCESS_DATA_PENDING:
>                 at86rf230_async_state_change(lp, ctx, STATE_TX_ON, at86rf230_tx_on);
> +               return;
> +       case TRAC_CHANNEL_ACCESS_FAILURE:
> +       case TRAC_NO_ACK:
>                 break;
>         default:
> -               at86rf230_async_error(lp, ctx, -EIO);
> +               trac = TRAC_INVALID;
>         }
> +
> +       at86rf230_async_error(lp, ctx, trac);

That makes no sense, at86rf230_async_error() is not a trac error
handling, it is a bus error handling. As noted above. With this change
you mix bus errors and trac errors (which are not bus errors). If
there are no bus errors then trac should be evaluated and should
either deliver some 802.15.4 $SUCCESS_CODE or $ERROR_CODE to the
softmac stack, which is xmit_complete() or xmit_error().

- Alex