| Message ID | 20220406153441.1667375-7-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: Better Tx error handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Hi, On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > In theory there are two distinct error path: > - The bus error when forwarding a packet to the transceiver fails. > - The transmitter error, after the transmission has been offloaded. > > Right now in this driver only the former situation is properly handled, > so rename the different helpers to reflect this situation before > improving the support of the other path. > I have no idea what I should think about this patch. On the driver layer there only exists "bus errors" okay, whatever error because spi_async() returns an error and we try to recover from it. Also async_error() will be called when there is a timeout because the transceiver took too long for some state change... In this case most often this async_error() is called if spi_async() returns an error but as I said it's not always the case (e.g. timeout)... it is some kind of hardware issue (indicated by 802.15.4 SYSTEM_ERROR for upper layer) and probably if it occurs we can't recover anyway from it (maybe rfkill support can do it, which does a whole transceiver reset routine, but this is always user triggered so far I know). However if you want that patch in that's it's fine for me, but for me this if somebody looks closely into the code it's obvious that in most cases it's called when spi_async() returns an error (which is not always the case see timeout). - Alex
Hi Alexander, alex.aring@gmail.com wrote on Wed, 6 Apr 2022 17:57:41 -0400: > Hi, > > On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > In theory there are two distinct error path: > > - The bus error when forwarding a packet to the transceiver fails. > > - The transmitter error, after the transmission has been offloaded. > > > > Right now in this driver only the former situation is properly handled, > > so rename the different helpers to reflect this situation before > > improving the support of the other path. > > > > I have no idea what I should think about this patch. > > On the driver layer there only exists "bus errors" okay, whatever > error because spi_async() returns an error and we try to recover from > it. Also async_error() will be called when there is a timeout because > the transceiver took too long for some state change... In this case > most often this async_error() is called if spi_async() returns an > error but as I said it's not always the case (e.g. timeout)... it is > some kind of hardware issue (indicated by 802.15.4 SYSTEM_ERROR for > upper layer) and probably if it occurs we can't recover anyway from it > (maybe rfkill support can do it, which does a whole transceiver reset > routine, but this is always user triggered so far I know). > > However if you want that patch in that's it's fine for me, but for me > this if somebody looks closely into the code it's obvious that in most > cases it's called when spi_async() returns an error (which is not > always the case see timeout). I thought it would clarify the situation but I overlooked the timeout situation. Actually I did wrote it before understanding what was wrong with the patch coming next (I assume my new approach is fine?), and the two changes are fully independent, so I'll drop this patch too. Thanks, Miquèl
Hi, On Thu, Apr 7, 2022 at 4:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Wed, 6 Apr 2022 17:57:41 -0400: > > > Hi, > > > > On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > In theory there are two distinct error path: > > > - The bus error when forwarding a packet to the transceiver fails. > > > - The transmitter error, after the transmission has been offloaded. > > > > > > Right now in this driver only the former situation is properly handled, > > > so rename the different helpers to reflect this situation before > > > improving the support of the other path. > > > > > > > I have no idea what I should think about this patch. > > > > On the driver layer there only exists "bus errors" okay, whatever > > error because spi_async() returns an error and we try to recover from > > it. Also async_error() will be called when there is a timeout because > > the transceiver took too long for some state change... In this case > > most often this async_error() is called if spi_async() returns an > > error but as I said it's not always the case (e.g. timeout)... it is > > some kind of hardware issue (indicated by 802.15.4 SYSTEM_ERROR for > > upper layer) and probably if it occurs we can't recover anyway from it > > (maybe rfkill support can do it, which does a whole transceiver reset > > routine, but this is always user triggered so far I know). > > > > However if you want that patch in that's it's fine for me, but for me > > this if somebody looks closely into the code it's obvious that in most > > cases it's called when spi_async() returns an error (which is not > > always the case see timeout). > > I thought it would clarify the situation but I overlooked the timeout > situation. Actually I did wrote it before understanding what was wrong > with the patch coming next (I assume my new approach is fine?), and > the two changes are fully independent, so I'll drop this patch too. > new patch is perfect, I like hw_error(). - Alex
diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c index 563031ce76f0..cafc786aab57 100644 --- a/drivers/net/ieee802154/at86rf230.c +++ b/drivers/net/ieee802154/at86rf230.c @@ -336,7 +336,7 @@ static const struct regmap_config at86rf230_regmap_spi_config = { }; static void -at86rf230_async_error_recover_complete(void *context) +at86rf230_async_bus_error_recover_complete(void *context) { struct at86rf230_state_change *ctx = context; struct at86rf230_local *lp = ctx->lp; @@ -352,7 +352,7 @@ at86rf230_async_error_recover_complete(void *context) } static void -at86rf230_async_error_recover(void *context) +at86rf230_async_bus_error_recover(void *context) { struct at86rf230_state_change *ctx = context; struct at86rf230_local *lp = ctx->lp; @@ -363,17 +363,17 @@ at86rf230_async_error_recover(void *context) } at86rf230_async_state_change(lp, ctx, STATE_RX_AACK_ON, - at86rf230_async_error_recover_complete); + at86rf230_async_bus_error_recover_complete); } static inline void -at86rf230_async_error(struct at86rf230_local *lp, - struct at86rf230_state_change *ctx, int rc) +at86rf230_async_bus_error(struct at86rf230_local *lp, + struct at86rf230_state_change *ctx, int rc) { dev_err(&lp->spi->dev, "spi_async error %d\n", rc); at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF, - at86rf230_async_error_recover); + at86rf230_async_bus_error_recover); } /* Generic function to get some register value in async mode */ @@ -390,7 +390,7 @@ at86rf230_async_read_reg(struct at86rf230_local *lp, u8 reg, ctx->msg.complete = complete; rc = spi_async(lp->spi, &ctx->msg); if (rc) - at86rf230_async_error(lp, ctx, rc); + at86rf230_async_bus_error(lp, ctx, rc); } static void @@ -405,7 +405,7 @@ at86rf230_async_write_reg(struct at86rf230_local *lp, u8 reg, u8 val, ctx->msg.complete = complete; rc = spi_async(lp->spi, &ctx->msg); if (rc) - at86rf230_async_error(lp, ctx, rc); + at86rf230_async_bus_error(lp, ctx, rc); } static void @@ -640,7 +640,7 @@ at86rf230_sync_state_change(struct at86rf230_local *lp, unsigned int state) rc = wait_for_completion_timeout(&lp->state_complete, msecs_to_jiffies(100)); if (!rc) { - at86rf230_async_error(lp, &lp->state, -ETIMEDOUT); + at86rf230_async_bus_error(lp, &lp->state, -ETIMEDOUT); return -ETIMEDOUT; } @@ -762,7 +762,7 @@ at86rf230_rx_trac_check(void *context) rc = spi_async(lp->spi, &ctx->msg); if (rc) { ctx->trx.len = 2; - at86rf230_async_error(lp, ctx, rc); + at86rf230_async_bus_error(lp, ctx, rc); } } @@ -839,7 +839,7 @@ static irqreturn_t at86rf230_isr(int irq, void *data) ctx->msg.complete = at86rf230_irq_status; rc = spi_async(lp->spi, &ctx->msg); if (rc) { - at86rf230_async_error(lp, ctx, rc); + at86rf230_async_bus_error(lp, ctx, rc); enable_irq(irq); return IRQ_NONE; } @@ -881,7 +881,7 @@ at86rf230_write_frame(void *context) rc = spi_async(lp->spi, &ctx->msg); if (rc) { ctx->trx.len = 2; - at86rf230_async_error(lp, ctx, rc); + at86rf230_async_bus_error(lp, ctx, rc); } }
In theory there are two distinct error path: - The bus error when forwarding a packet to the transceiver fails. - The transmitter error, after the transmission has been offloaded. Right now in this driver only the former situation is properly handled, so rename the different helpers to reflect this situation before improving the support of the other path. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/net/ieee802154/at86rf230.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)