| Message ID | 20220406153441.1667375-10-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ieee802154: Better Tx error handling | expand |
Hi, On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ieee802154_xmit_error() is the right helper to call when a transmission > has failed. Let's use it instead of open-coding it. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/net/ieee802154/atusb.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > index f27a5f535808..d04db4d07a64 100644 > --- a/drivers/net/ieee802154/atusb.c > +++ b/drivers/net/ieee802154/atusb.c > @@ -271,9 +271,8 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > * unlikely case now that seq == expect is then true, but can > * happen and fail with a tx_skb = NULL; > */ > - ieee802154_wake_queue(atusb->hw); > - if (atusb->tx_skb) > - dev_kfree_skb_irq(atusb->tx_skb); > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, > + IEEE802154_SYSTEM_ERROR); That should then call the xmit_error for ANY other reason which is not 802.15.4 specific which is the bus_error() function? - Alex
Hi Alexander, alex.aring@gmail.com wrote on Wed, 6 Apr 2022 17:58:59 -0400: > Hi, > > On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > ieee802154_xmit_error() is the right helper to call when a transmission > > has failed. Let's use it instead of open-coding it. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/net/ieee802154/atusb.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > index f27a5f535808..d04db4d07a64 100644 > > --- a/drivers/net/ieee802154/atusb.c > > +++ b/drivers/net/ieee802154/atusb.c > > @@ -271,9 +271,8 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > > * unlikely case now that seq == expect is then true, but can > > * happen and fail with a tx_skb = NULL; > > */ > > - ieee802154_wake_queue(atusb->hw); > > - if (atusb->tx_skb) > > - dev_kfree_skb_irq(atusb->tx_skb); > > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, > > + IEEE802154_SYSTEM_ERROR); > > That should then call the xmit_error for ANY other reason which is not > 802.15.4 specific which is the bus_error() function? I'll drop the bus error function so we can stick to a regular _xmit_error() call. Besides, we do not have any trac information nor any easy access to what failed exactly, so it's probably best anyway. Thanks, Miquèl
Hi, On Thu, Apr 7, 2022 at 4:06 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Wed, 6 Apr 2022 17:58:59 -0400: > > > Hi, > > > > On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > ieee802154_xmit_error() is the right helper to call when a transmission > > > has failed. Let's use it instead of open-coding it. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/net/ieee802154/atusb.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > > index f27a5f535808..d04db4d07a64 100644 > > > --- a/drivers/net/ieee802154/atusb.c > > > +++ b/drivers/net/ieee802154/atusb.c > > > @@ -271,9 +271,8 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > > > * unlikely case now that seq == expect is then true, but can > > > * happen and fail with a tx_skb = NULL; > > > */ > > > - ieee802154_wake_queue(atusb->hw); > > > - if (atusb->tx_skb) > > > - dev_kfree_skb_irq(atusb->tx_skb); > > > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, > > > + IEEE802154_SYSTEM_ERROR); > > > > That should then call the xmit_error for ANY other reason which is not > > 802.15.4 specific which is the bus_error() function? > > I'll drop the bus error function so we can stick to a regular > _xmit_error() call. > Okay, this error is only hardware related. > Besides, we do not have any trac information nor any easy access to > what failed exactly, so it's probably best anyway. This is correct, Somebody needs to write support for it for atusb firmware. [0] However some transceivers can't yet or will never (because lack of functionality?) report such errors back... they will act a little bit weird. However, this return value is a BIG step moving into the right direction that other drivers can follow. I think for MLME ops we can definitely handle some transmit errors now and retry so that we don't wait for anything when we know the transceiver was never submitting. - Alex [0] http://projects.qi-hardware.com/index.php/p/ben-wpan/source/tree/master/atusb/fw
Hi, On Mon, Apr 25, 2022 at 8:35 AM Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Thu, Apr 7, 2022 at 4:06 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > alex.aring@gmail.com wrote on Wed, 6 Apr 2022 17:58:59 -0400: > > > > > Hi, > > > > > > On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > ieee802154_xmit_error() is the right helper to call when a transmission > > > > has failed. Let's use it instead of open-coding it. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/net/ieee802154/atusb.c | 5 ++--- > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > > > index f27a5f535808..d04db4d07a64 100644 > > > > --- a/drivers/net/ieee802154/atusb.c > > > > +++ b/drivers/net/ieee802154/atusb.c > > > > @@ -271,9 +271,8 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > > > > * unlikely case now that seq == expect is then true, but can > > > > * happen and fail with a tx_skb = NULL; > > > > */ > > > > - ieee802154_wake_queue(atusb->hw); > > > > - if (atusb->tx_skb) > > > > - dev_kfree_skb_irq(atusb->tx_skb); > > > > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, > > > > + IEEE802154_SYSTEM_ERROR); > > > > > > That should then call the xmit_error for ANY other reason which is not > > > 802.15.4 specific which is the bus_error() function? > > > > I'll drop the bus error function so we can stick to a regular > > _xmit_error() call. > > > > Okay, this error is only hardware related. > > > Besides, we do not have any trac information nor any easy access to > > what failed exactly, so it's probably best anyway. > > This is correct, Somebody needs to write support for it for atusb firmware. [0] > However some transceivers can't yet or will never (because lack of > functionality?) report such errors back... they will act a little bit > weird. > > However, this return value is a BIG step moving into the right > direction that other drivers can follow. > > I think for MLME ops we can definitely handle some transmit errors now > and retry so that we don't wait for anything when we know the > transceiver was never submitting. > s/submitting/transmitted/ I could more deeper into that topic: 1. In my opinion this result value was especially necessary for MLME-ops, for others which do not directly work with MAC... they provide an upper layer protocol if they want something reliable. 2. Later on we can do statistics like what was already going around in the linux-wpan community to have something like whatever dump to see all neighbors and see how many ack failures there, etc. Some people want to make some predictions about link quality here. The kernel should therefore only capture some stats and the $WHATEVER userspace capable netlink monitor daemon should make some heuristic by dumping those stats. 3. Sometimes even IP capable protocols (using 6LoWPAN) want to know if ack was received or not, as mentioned. But this required additional handling in the socket layers... I didn't look into that topic yet but I know wireless solved it because they have some similar problems. - Alex
Hi Alexander, alex.aring@gmail.com wrote on Mon, 25 Apr 2022 09:05:49 -0400: > Hi, > > On Mon, Apr 25, 2022 at 8:35 AM Alexander Aring <alex.aring@gmail.com> wrote: > > > > Hi, > > > > On Thu, Apr 7, 2022 at 4:06 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > alex.aring@gmail.com wrote on Wed, 6 Apr 2022 17:58:59 -0400: > > > > > > > Hi, > > > > > > > > On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > ieee802154_xmit_error() is the right helper to call when a transmission > > > > > has failed. Let's use it instead of open-coding it. > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > --- > > > > > drivers/net/ieee802154/atusb.c | 5 ++--- > > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > > > > index f27a5f535808..d04db4d07a64 100644 > > > > > --- a/drivers/net/ieee802154/atusb.c > > > > > +++ b/drivers/net/ieee802154/atusb.c > > > > > @@ -271,9 +271,8 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > > > > > * unlikely case now that seq == expect is then true, but can > > > > > * happen and fail with a tx_skb = NULL; > > > > > */ > > > > > - ieee802154_wake_queue(atusb->hw); > > > > > - if (atusb->tx_skb) > > > > > - dev_kfree_skb_irq(atusb->tx_skb); > > > > > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, > > > > > + IEEE802154_SYSTEM_ERROR); > > > > > > > > That should then call the xmit_error for ANY other reason which is not > > > > 802.15.4 specific which is the bus_error() function? > > > > > > I'll drop the bus error function so we can stick to a regular > > > _xmit_error() call. > > > > > > > Okay, this error is only hardware related. > > > > > Besides, we do not have any trac information nor any easy access to > > > what failed exactly, so it's probably best anyway. > > > > This is correct, Somebody needs to write support for it for atusb firmware. [0] > > However some transceivers can't yet or will never (because lack of > > functionality?) report such errors back... they will act a little bit > > weird. > > > > However, this return value is a BIG step moving into the right > > direction that other drivers can follow. > > > > I think for MLME ops we can definitely handle some transmit errors now > > and retry so that we don't wait for anything when we know the > > transceiver was never submitting. > > > > s/submitting/transmitted/ > > I could more deeper into that topic: > > 1. > > In my opinion this result value was especially necessary for MLME-ops, > for others which do not directly work with MAC... they provide an > upper layer protocol if they want something reliable. > > 2. > > Later on we can do statistics like what was already going around in > the linux-wpan community to have something like whatever dump to see > all neighbors and see how many ack failures there, etc. Some people > want to make some predictions about link quality here. The kernel > should therefore only capture some stats and the $WHATEVER userspace > capable netlink monitor daemon should make some heuristic by dumping > those stats. I like the idea of having a per-device dump of the stats. It would be really straightforward to implement with the current scan implementation that I am about to share. We already have a per PAN structure (with information like ID, channel, last time it was seen, strength, etc). We might improve this structure with counters for all the common mac errors. Maybe an option to the "pans dump" command (again, in the pipe) might be a good start to get all the stats, like "pans dump [stats]". I'll keep this in mind. > 3. > > Sometimes even IP capable protocols (using 6LoWPAN) want to know if > ack was received or not, as mentioned. But this required additional > handling in the socket layers... I didn't look into that topic yet but > I know wireless solved it because they have some similar problems. I did not look at the upper layers yet, but that would indeed be a nice use case of these MAC statuses. Thanks, Miquèl
Hi, On Mon, Apr 25, 2022 at 9:16 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Mon, 25 Apr 2022 09:05:49 -0400: > > > Hi, > > > > On Mon, Apr 25, 2022 at 8:35 AM Alexander Aring <alex.aring@gmail.com> wrote: > > > > > > Hi, > > > > > > On Thu, Apr 7, 2022 at 4:06 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > Hi Alexander, > > > > > > > > alex.aring@gmail.com wrote on Wed, 6 Apr 2022 17:58:59 -0400: > > > > > > > > > Hi, > > > > > > > > > > On Wed, Apr 6, 2022 at 11:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > > > ieee802154_xmit_error() is the right helper to call when a transmission > > > > > > has failed. Let's use it instead of open-coding it. > > > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > > --- > > > > > > drivers/net/ieee802154/atusb.c | 5 ++--- > > > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > > > > > index f27a5f535808..d04db4d07a64 100644 > > > > > > --- a/drivers/net/ieee802154/atusb.c > > > > > > +++ b/drivers/net/ieee802154/atusb.c > > > > > > @@ -271,9 +271,8 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > > > > > > * unlikely case now that seq == expect is then true, but can > > > > > > * happen and fail with a tx_skb = NULL; > > > > > > */ > > > > > > - ieee802154_wake_queue(atusb->hw); > > > > > > - if (atusb->tx_skb) > > > > > > - dev_kfree_skb_irq(atusb->tx_skb); > > > > > > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, > > > > > > + IEEE802154_SYSTEM_ERROR); > > > > > > > > > > That should then call the xmit_error for ANY other reason which is not > > > > > 802.15.4 specific which is the bus_error() function? > > > > > > > > I'll drop the bus error function so we can stick to a regular > > > > _xmit_error() call. > > > > > > > > > > Okay, this error is only hardware related. > > > > > > > Besides, we do not have any trac information nor any easy access to > > > > what failed exactly, so it's probably best anyway. > > > > > > This is correct, Somebody needs to write support for it for atusb firmware. [0] > > > However some transceivers can't yet or will never (because lack of > > > functionality?) report such errors back... they will act a little bit > > > weird. > > > > > > However, this return value is a BIG step moving into the right > > > direction that other drivers can follow. > > > > > > I think for MLME ops we can definitely handle some transmit errors now > > > and retry so that we don't wait for anything when we know the > > > transceiver was never submitting. > > > > > > > s/submitting/transmitted/ > > > > I could more deeper into that topic: > > > > 1. > > > > In my opinion this result value was especially necessary for MLME-ops, > > for others which do not directly work with MAC... they provide an > > upper layer protocol if they want something reliable. > > > > 2. > > > > Later on we can do statistics like what was already going around in > > the linux-wpan community to have something like whatever dump to see > > all neighbors and see how many ack failures there, etc. Some people > > want to make some predictions about link quality here. The kernel > > should therefore only capture some stats and the $WHATEVER userspace > > capable netlink monitor daemon should make some heuristic by dumping > > those stats. > > I like the idea of having a per-device dump of the stats. It would be > really straightforward to implement with the current scan > implementation that I am about to share. We already have a per PAN > structure (with information like ID, channel, last time it was seen, > strength, etc). We might improve this structure with counters for all > the common mac errors. Maybe an option to the "pans dump" command > (again, in the pipe) might be a good start to get all the stats, like > "pans dump [stats]". I'll keep this in mind. > sounds to me like a per pan filtering use case... which can be implemented in the kernel, but nowadays there might be more generic ways of filtering out dumps in the kernel (eBPF?). However we need to talk about this again if there are some patches. But yes there should be plenty of information about neighbors whichever frame was received (which also includes transmission case in that way if an ACK was received or not if ack request bit was set, channel access failures, etc?). There exists a difference between if we know some address information (MAC) or it's PHY related like channel access failure. Most people are interested in per node information (we have address information). > > 3. > > > > Sometimes even IP capable protocols (using 6LoWPAN) want to know if > > ack was received or not, as mentioned. But this required additional > > handling in the socket layers... I didn't look into that topic yet but > > I know wireless solved it because they have some similar problems. > > I did not look at the upper layers yet, but that would indeed be a nice > use case of these MAC statuses. > I am a little bit worried about that, because at some point we run in fragmentation of IPv6/6LoWPAN, and then one packet gets into several 802.15.4 frames... whereas such information is per frame. If all has the same result - no problems. There is a special handling needed which need to be documented well, e.g. I would say if one frame had no ack back I would say it should report back to user space that no ack came back... if possible provide detailed information but I think that isn't easier to get back to user space than just a bit if ack was back or not. Again this is something where a socket error queue needs to be involved. - Alex
diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c index f27a5f535808..d04db4d07a64 100644 --- a/drivers/net/ieee802154/atusb.c +++ b/drivers/net/ieee802154/atusb.c @@ -271,9 +271,8 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) * unlikely case now that seq == expect is then true, but can * happen and fail with a tx_skb = NULL; */ - ieee802154_wake_queue(atusb->hw); - if (atusb->tx_skb) - dev_kfree_skb_irq(atusb->tx_skb); + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, + IEEE802154_SYSTEM_ERROR); } }
ieee802154_xmit_error() is the right helper to call when a transmission has failed. Let's use it instead of open-coding it. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/net/ieee802154/atusb.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)