| Message ID | 20220120112115.448077-9-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ieee802154: A bunch of fixes | expand |
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. > + * 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. Where is the fix here? - Alex
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
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 + * 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. */ 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);
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(+)