| Message ID | 20220207144804.708118-3-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ieee802154: Synchronous Tx API | expand |
Hi, On Mon, Feb 7, 2022 at 10:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > So far there is only a helper for successful transmission, which led > device drivers to implement their own handling in case of > error. Unfortunately, we really need all the drivers to give the hand > back to the core once they are done in order to be able to build a > proper synchronous API. So let's create a _xmit_error() helper. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > include/net/mac802154.h | 10 ++++++++++ > net/mac802154/util.c | 10 ++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h > index 2c3bbc6645ba..9fe8cfef1ba0 100644 > --- a/include/net/mac802154.h > +++ b/include/net/mac802154.h > @@ -498,4 +498,14 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw); > void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, > bool ifs_handling); > > +/** > + * ieee802154_xmit_error - frame transmission failed > + * > + * @hw: pointer as obtained from ieee802154_alloc_hw(). > + * @skb: buffer for transmission > + * @ifs_handling: indicate interframe space handling > + */ > +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, > + bool ifs_handling); > + > #endif /* NET_MAC802154_H */ > diff --git a/net/mac802154/util.c b/net/mac802154/util.c > index 6f82418e9dec..9016f634efba 100644 > --- a/net/mac802154/util.c > +++ b/net/mac802154/util.c > @@ -102,6 +102,16 @@ void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, > } > EXPORT_SYMBOL(ieee802154_xmit_complete); > > +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, > + bool ifs_handling) > +{ > + unsigned int skb_len = skb->len; > + > + dev_kfree_skb_any(skb); > + ieee802154_xmit_end(hw, ifs_handling, skb_len); > +} Remove ieee802154_xmit_end() function and just call to wake up the queue here, also drop the "ifs_handling" parameter here. - Alex
Hi, On Sun, Feb 20, 2022 at 6:31 PM Alexander Aring <aahringo@redhat.com> wrote: > > Hi, > > On Mon, Feb 7, 2022 at 10:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > So far there is only a helper for successful transmission, which led > > device drivers to implement their own handling in case of > > error. Unfortunately, we really need all the drivers to give the hand > > back to the core once they are done in order to be able to build a > > proper synchronous API. So let's create a _xmit_error() helper. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > include/net/mac802154.h | 10 ++++++++++ > > net/mac802154/util.c | 10 ++++++++++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h > > index 2c3bbc6645ba..9fe8cfef1ba0 100644 > > --- a/include/net/mac802154.h > > +++ b/include/net/mac802154.h > > @@ -498,4 +498,14 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw); > > void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, > > bool ifs_handling); > > > > +/** > > + * ieee802154_xmit_error - frame transmission failed > > + * > > + * @hw: pointer as obtained from ieee802154_alloc_hw(). > > + * @skb: buffer for transmission > > + * @ifs_handling: indicate interframe space handling > > + */ > > +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, > > + bool ifs_handling); > > + > > #endif /* NET_MAC802154_H */ > > diff --git a/net/mac802154/util.c b/net/mac802154/util.c > > index 6f82418e9dec..9016f634efba 100644 > > --- a/net/mac802154/util.c > > +++ b/net/mac802154/util.c > > @@ -102,6 +102,16 @@ void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, > > } > > EXPORT_SYMBOL(ieee802154_xmit_complete); > > > > +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, > > + bool ifs_handling) > > +{ > > + unsigned int skb_len = skb->len; > > + > > + dev_kfree_skb_any(skb); > > + ieee802154_xmit_end(hw, ifs_handling, skb_len); > > +} > > Remove ieee802154_xmit_end() function and just call to wake up the > queue here, also drop the "ifs_handling" parameter here. I am sorry, I think I should deliver an explanation here... I think the handling of success and error paths are just too different. In error there will also never ifs handling in the error path. Also please note there are not just errors as bus/transceiver errors, in future transceiver should also deliver [0] to the caller, in sync transmit it should return those errors to the caller... in async mode there exists different ways to deliver errors like (no ack) to user space by using socket error queue, here again is worth to look into wireless subsystem which have a similar feature. The errors in [0] are currently ignored but I think should be switched some time soon or with an additional patch by you to calling xmit_error with an int for $REASON. Those errors are happening on the transceiver because some functionality is offloaded. btw: so far I know some MLME-ops need to know if an ack is received or not. You can split the functionality somehow it makes sense, but with the above change I only see the wake up queue is the only thing that both (success/error) should have in common. - Alex [0] https://elixir.bootlin.com/linux/v5.16-rc7/source/drivers/net/ieee802154/at86rf230.c#L670
Hi Alexander, alex.aring@gmail.com wrote on Mon, 21 Feb 2022 15:22:40 -0500: > Hi, > > On Sun, Feb 20, 2022 at 6:31 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > Hi, > > > > On Mon, Feb 7, 2022 at 10:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > So far there is only a helper for successful transmission, which led > > > device drivers to implement their own handling in case of > > > error. Unfortunately, we really need all the drivers to give the hand > > > back to the core once they are done in order to be able to build a > > > proper synchronous API. So let's create a _xmit_error() helper. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > include/net/mac802154.h | 10 ++++++++++ > > > net/mac802154/util.c | 10 ++++++++++ > > > 2 files changed, 20 insertions(+) > > > > > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h > > > index 2c3bbc6645ba..9fe8cfef1ba0 100644 > > > --- a/include/net/mac802154.h > > > +++ b/include/net/mac802154.h > > > @@ -498,4 +498,14 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw); > > > void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, > > > bool ifs_handling); > > > > > > +/** > > > + * ieee802154_xmit_error - frame transmission failed > > > + * > > > + * @hw: pointer as obtained from ieee802154_alloc_hw(). > > > + * @skb: buffer for transmission > > > + * @ifs_handling: indicate interframe space handling > > > + */ > > > +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, > > > + bool ifs_handling); > > > + > > > #endif /* NET_MAC802154_H */ > > > diff --git a/net/mac802154/util.c b/net/mac802154/util.c > > > index 6f82418e9dec..9016f634efba 100644 > > > --- a/net/mac802154/util.c > > > +++ b/net/mac802154/util.c > > > @@ -102,6 +102,16 @@ void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, > > > } > > > EXPORT_SYMBOL(ieee802154_xmit_complete); > > > > > > +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, > > > + bool ifs_handling) > > > +{ > > > + unsigned int skb_len = skb->len; > > > + > > > + dev_kfree_skb_any(skb); > > > + ieee802154_xmit_end(hw, ifs_handling, skb_len); > > > +} > > > > Remove ieee802154_xmit_end() function and just call to wake up the > > queue here, also drop the "ifs_handling" parameter here. > > I am sorry, I think I should deliver an explanation here... I think > the handling of success and error paths are just too different. In > error there will also never ifs handling in the error path. Also > please note there are not just errors as bus/transceiver errors, in > future transceiver should also deliver [0] to the caller, in sync > transmit it should return those errors to the caller... in async mode > there exists different ways to deliver errors like (no ack) to user > space by using socket error queue, here again is worth to look into > wireless subsystem which have a similar feature. > > The errors in [0] are currently ignored but I think should be switched > some time soon or with an additional patch by you to calling > xmit_error with an int for $REASON. Those errors are happening on the > transceiver because some functionality is offloaded. btw: so far I > know some MLME-ops need to know if an ack is received or not. > > You can split the functionality somehow it makes sense, but with the > above change I only see the wake up queue is the only thing that both > (success/error) should have in common. Very clear, thanks for the additional details. Indeed I would find much better to be able to propagate the error reasons to upper layers. Even though at this time we don't propagate them all the way up to userspace, having them *at least* in the ieee core would be a nice feature. I'll see what I can do. > > - Alex > > [0] https://elixir.bootlin.com/linux/v5.16-rc7/source/drivers/net/ieee802154/at86rf230.c#L670 Thanks, Miquèl
Hi, On Tue, Feb 22, 2022 at 3:43 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Mon, 21 Feb 2022 15:22:40 -0500: > > > Hi, > > > > On Sun, Feb 20, 2022 at 6:31 PM Alexander Aring <aahringo@redhat.com> wrote: > > > > > > Hi, > > > > > > On Mon, Feb 7, 2022 at 10:09 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > So far there is only a helper for successful transmission, which led > > > > device drivers to implement their own handling in case of > > > > error. Unfortunately, we really need all the drivers to give the hand > > > > back to the core once they are done in order to be able to build a > > > > proper synchronous API. So let's create a _xmit_error() helper. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > include/net/mac802154.h | 10 ++++++++++ > > > > net/mac802154/util.c | 10 ++++++++++ > > > > 2 files changed, 20 insertions(+) > > > > > > > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h > > > > index 2c3bbc6645ba..9fe8cfef1ba0 100644 > > > > --- a/include/net/mac802154.h > > > > +++ b/include/net/mac802154.h > > > > @@ -498,4 +498,14 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw); > > > > void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, > > > > bool ifs_handling); > > > > > > > > +/** > > > > + * ieee802154_xmit_error - frame transmission failed > > > > + * > > > > + * @hw: pointer as obtained from ieee802154_alloc_hw(). > > > > + * @skb: buffer for transmission > > > > + * @ifs_handling: indicate interframe space handling > > > > + */ > > > > +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, > > > > + bool ifs_handling); > > > > + > > > > #endif /* NET_MAC802154_H */ > > > > diff --git a/net/mac802154/util.c b/net/mac802154/util.c > > > > index 6f82418e9dec..9016f634efba 100644 > > > > --- a/net/mac802154/util.c > > > > +++ b/net/mac802154/util.c > > > > @@ -102,6 +102,16 @@ void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, > > > > } > > > > EXPORT_SYMBOL(ieee802154_xmit_complete); > > > > > > > > +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, > > > > + bool ifs_handling) > > > > +{ > > > > + unsigned int skb_len = skb->len; > > > > + > > > > + dev_kfree_skb_any(skb); > > > > + ieee802154_xmit_end(hw, ifs_handling, skb_len); > > > > +} > > > > > > Remove ieee802154_xmit_end() function and just call to wake up the > > > queue here, also drop the "ifs_handling" parameter here. > > > > I am sorry, I think I should deliver an explanation here... I think > > the handling of success and error paths are just too different. In > > error there will also never ifs handling in the error path. Also > > please note there are not just errors as bus/transceiver errors, in > > future transceiver should also deliver [0] to the caller, in sync > > transmit it should return those errors to the caller... in async mode > > there exists different ways to deliver errors like (no ack) to user > > space by using socket error queue, here again is worth to look into > > wireless subsystem which have a similar feature. > > > > The errors in [0] are currently ignored but I think should be switched > > some time soon or with an additional patch by you to calling > > xmit_error with an int for $REASON. Those errors are happening on the > > transceiver because some functionality is offloaded. btw: so far I > > know some MLME-ops need to know if an ack is received or not. > > > > You can split the functionality somehow it makes sense, but with the > > above change I only see the wake up queue is the only thing that both > > (success/error) should have in common. > > Very clear, thanks for the additional details. Indeed I would find much > better to be able to propagate the error reasons to upper layers. Even > though at this time we don't propagate them all the way up to userspace, > having them *at least* in the ieee core would be a nice feature. I'll > see what I can do. > For now I think we can live with "success" or "error". It would be nice to have a reason and it is definitely necessary in future use. The case NO_ACK is a very specific 802.15.4 case which I think is important/sometimes even required to know and I would put it as an error, that means it was not received on the other end. Of course this error makes only sense if ack request bit was set and the transceiver should only inform about it if it was set. The other errors which might occur e.g. channel access failure... it might be that transceivers handle such cases "differently?" Even I don't even know if all transceivers have a functionality to request if there was an ack or not (in case of offloaded ARET handling). In my opinion they should at least have the possibility to report back if there was an ack or not, if they don't have such functionality we might always "think" there was an ack but the transceiver would act sometimes a little bit weird. At this point I would blame the hardware because it cannot report it. However, as I see in the at86rf230 driver there can be different success cases and different error cases... you don't need to support that. We currently handle only some bus/misc transceiver issues which should end in an error in the sync tx handling. Later we can look if we might add more error cases into it. It would be nice to see that if we can simply check in general if there was a success or error, if the caller wants to be more specific there can be an if/switch-case statement like the well-known handling with errnos. Don't tackle it now for userspace, I think it will require a lot of work... even it depends on which socket family you want to support it. I think it was just worth mentioning... - Alex
diff --git a/include/net/mac802154.h b/include/net/mac802154.h index 2c3bbc6645ba..9fe8cfef1ba0 100644 --- a/include/net/mac802154.h +++ b/include/net/mac802154.h @@ -498,4 +498,14 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw); void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, bool ifs_handling); +/** + * ieee802154_xmit_error - frame transmission failed + * + * @hw: pointer as obtained from ieee802154_alloc_hw(). + * @skb: buffer for transmission + * @ifs_handling: indicate interframe space handling + */ +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, + bool ifs_handling); + #endif /* NET_MAC802154_H */ diff --git a/net/mac802154/util.c b/net/mac802154/util.c index 6f82418e9dec..9016f634efba 100644 --- a/net/mac802154/util.c +++ b/net/mac802154/util.c @@ -102,6 +102,16 @@ void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, } EXPORT_SYMBOL(ieee802154_xmit_complete); +void ieee802154_xmit_error(struct ieee802154_hw *hw, struct sk_buff *skb, + bool ifs_handling) +{ + unsigned int skb_len = skb->len; + + dev_kfree_skb_any(skb); + ieee802154_xmit_end(hw, ifs_handling, skb_len); +} +EXPORT_SYMBOL(ieee802154_xmit_error); + void ieee802154_stop_device(struct ieee802154_local *local) { flush_workqueue(local->workqueue);
So far there is only a helper for successful transmission, which led device drivers to implement their own handling in case of error. Unfortunately, we really need all the drivers to give the hand back to the core once they are done in order to be able to build a proper synchronous API. So let's create a _xmit_error() helper. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- include/net/mac802154.h | 10 ++++++++++ net/mac802154/util.c | 10 ++++++++++ 2 files changed, 20 insertions(+)