Re: [PATCH wpan-next v2 02/14] net: mac802154: Create a transmit error helper

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

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