Re: [wpan-next v2 8/9] net: mac802154: Explain the use of ieee802154_wake/stop_queue()

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Alexander,

alex.aring@gmail.com wrote on Sun, 23 Jan 2022 15:56:22 -0500:

> Hi,
> 
> On Thu, 20 Jan 2022 at 06:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > It is not straightforward to the newcomer that a single skb can be sent
> > at a time and that the internal process is to stop the queue when
> > processing a frame before re-enabling it. Make this clear by documenting
> > the ieee802154_wake/stop_queue() helpers.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/net/mac802154.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> > index d524ffb9eb25..94b2e3008e77 100644
> > --- a/include/net/mac802154.h
> > +++ b/include/net/mac802154.h
> > @@ -464,6 +464,12 @@ void ieee802154_rx_irqsafe(struct ieee802154_hw *hw, struct sk_buff *skb,
> >   * ieee802154_wake_queue - wake ieee802154 queue
> >   * @hw: pointer as obtained from ieee802154_alloc_hw().
> >   *
> > + * Tranceivers have either one transmit framebuffer or one framebuffer for both
> > + * transmitting and receiving. Hence, the core only handles one frame at a time  
> 
> this is not a fundamental physical law, they might exist but not supported yet.

I think it's important to explain why we call these helpers
before/after transmissions. I've reworded the beginning of the sentence
to: "Tranceivers usually have..." to reflect that this is the current
state of the support, but is not marble solid either. I've also updated
the second sentence about the core "Hence, the core currently only
handles..."

> > + * for each phy, which means we had to stop the queue to avoid new skb to come
> > + * during the transmission. The queue then needs to be woken up after the
> > + * operation.
> > + *
> >   * Drivers should use this function instead of netif_wake_queue.  
> 
> It's a must.
>
> >   */
> >  void ieee802154_wake_queue(struct ieee802154_hw *hw);
> > @@ -472,6 +478,12 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw);
> >   * ieee802154_stop_queue - stop ieee802154 queue
> >   * @hw: pointer as obtained from ieee802154_alloc_hw().
> >   *
> > + * Tranceivers have either one transmit framebuffer or one framebuffer for both
> > + * transmitting and receiving. Hence, the core only handles one frame at a time
> > + * for each phy, which means we need to tell upper layers to stop giving us new
> > + * skbs while we are busy with the transmitted one. The queue must then be
> > + * stopped before transmitting.
> > + *
> >   * Drivers should use this function instead of netif_stop_queue.
> >   */
> >  void ieee802154_stop_queue(struct ieee802154_hw *hw);  
> 
> Same for stop, stop has something additional here... it is never used
> by any driver because we do that on mac802154 layer. Simply, they
> don't use this function.

That's true, and this is addressed in a later series where we actually
stop exporting these helpers because we want everything to be handled
by the _xmit_complete/error() helpers and keep full control of the
queue. The comments added above will remain because they are useful to
understand why these helpers are called. But the last sentence above
their use from drivers being preferred to the netif_ calls will be
dropped.

Thanks,
Miquèl