From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Alexander Aring <alex.aring@gmail.com>
Cc: Stefan Schmidt <stefan@datenfreihafen.org>,
linux-wpan - ML <linux-wpan@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
"open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>,
Michael Hennerich <michael.hennerich@analog.com>,
Harry Morris <h.morris@cascoda.com>,
Varka Bhadram <varkabhadram@gmail.com>,
Xue Liu <liuxuenetmail@gmail.com>, Alan Ott <alan@signal11.us>,
David Girault <david.girault@qorvo.com>,
Romuald Despres <romuald.despres@qorvo.com>,
Frederic Blain <frederic.blain@qorvo.com>,
Nicolas Schodet <nico@ni.fr.eu.org>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
"linux-wireless@vger.kernel.org Wireless"
<linux-wireless@vger.kernel.org>
Subject: Re: [wpan-next v2 18/27] net: mac802154: Handle scan requests
Date: Mon, 17 Jan 2022 10:00:07 +0100 [thread overview]
Message-ID: <20220117094647.3cc5b4de@xps13> (raw)
In-Reply-To: <CAB_54W5xJV-3fxOUyvdxBBfUZWYx7JU=BDVhTHFcjJ7SOEdeUw@mail.gmail.com>
Hi Alexander,
alex.aring@gmail.com wrote on Sun, 16 Jan 2022 17:44:18 -0500:
> Hi,
>
> On Fri, 14 Jan 2022 at 13:44, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > alex.aring@gmail.com wrote on Thu, 13 Jan 2022 19:01:56 -0500:
> >
> > > Hi,
> > >
> > > On Thu, 13 Jan 2022 at 12:07, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >
> > > > Hi Alexander,
> > > >
> > > > alex.aring@gmail.com wrote on Wed, 12 Jan 2022 17:44:02 -0500:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Wed, 12 Jan 2022 at 12:33, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > ...
> > > > > > + return 0;
> > > > > > +}
> > > > > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
> > > > > > index c829e4a75325..40656728c624 100644
> > > > > > --- a/net/mac802154/tx.c
> > > > > > +++ b/net/mac802154/tx.c
> > > > > > @@ -54,6 +54,9 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
> > > > > > struct net_device *dev = skb->dev;
> > > > > > int ret;
> > > > > >
> > > > > > + if (unlikely(mac802154_scan_is_ongoing(local)))
> > > > > > + return NETDEV_TX_BUSY;
> > > > > > +
> > > > >
> > > > > Please look into the functions "ieee802154_wake_queue()" and
> > > > > "ieee802154_stop_queue()" which prevent this function from being
> > > > > called. Call stop before starting scanning and wake after scanning is
> > > > > done or stopped.
> > > >
> > > > Mmmh all this is already done, isn't it?
> > > > - mac802154_trigger_scan_locked() stops the queue before setting the
> > > > promiscuous mode
> > > > - mac802154_end_of_scan() wakes the queue after resetting the
> > > > promiscuous mode to its original state
> > > >
> > > > Should I drop the check which stands for an extra precaution?
> > > >
> > >
> > > no, I think then it should be a WARN_ON() more without any return
> > > (hopefully it will survive). This case should never happen otherwise
> > > we have a bug that we wake the queue when we "took control about
> > > transmissions" only.
> > > Change the name, I think it will be in future not only scan related.
> > > Maybe "mac802154_queue_stopped()". Everything which is queued from
> > > socket/upperlayer(6lowpan) goes this way.
> >
> > Got it.
> >
> > I've changed the name of the helper, and used an atomic variable there
> > to follow the count.
> >
> > > > But overall I think I don't understand well this part. What is
> > > > a bit foggy to me is why the (async) tx implementation does:
> > > >
> > > > *Core* *Driver*
> > > >
> > > > stop_queue()
> > > > drv_async_xmit() -------
> > > > \------> do something
> > > > ------- calls ieee802154_xmit_complete()
> > > > wakeup_queue() <--------/
> > > >
> > > > So we actually disable the queue for transmitting. Why??
> > > >
> > >
> > > Because all transceivers have either _one_ transmit framebuffer or one
> > > framebuffer for transmit and receive one time. We need to report to
> > > stop giving us more skb's while we are busy with one to transmit.
> > > This all will/must be changed in future if there is hardware outside
> > > which is more powerful and the driver needs to control the flow here.
> > >
> > > That ieee802154_xmit_complete() calls wakeup_queue need to be
> > > forbidden when we are in "synchronous transmit mode"/the queue is
> > > stopped. The synchronous transmit mode is not for any hotpath, it's
> > > for MLME and I think we also need a per phy lock to avoid multiple
> > > synchronous transmissions at one time. Please note that I don't think
> > > here only about scan operation, also for other possible MLME-ops.
> > >
> >
> > First, thank you very much for all your guidance and reviews, I think I
> > have a much clearer understanding now.
> >
> > I've tried to follow your advices, creating:
> > - a way of tracking ongoing transmissions
> > - a synchronous API for MLME transfers
> >
>
> Please note that I think we cannot use netif_stop_queue() from context
> outside of netif xmit() callback. It's because the atomic counter
> itself is racy in xmit(), we need to be sure xmit() can't occur while
> stopping the queue.
In my current implementation I don't see this as a real problem because
for me, there is no real difference between:
- a transfer is started
- we call stop_queue()
* right here a transfer is ongoing *
and
- we call stop_queue()
- the counter is racy hence a last transfer is started
* right here a transfer is ongoing *
because stopping the queue and "flushing" it are two different things.
In the code I don't only rely on the queue being stopped but if I don't
want any more transfer to happen after that, so I also sync the queue
thanks to the new helpers introduced.
Please check v3 (which is coming very soon) and tell me what you think.
Maybe I missed something.
> I think maybe "netif_tx_disable()" is the right
> call to stop from another context, because it holds the tx_lock, which
> I believe is held while xmit().
> Where the wake queue call should be fine to call..., maybe we can
> remove some EXPORT_SYMBOL() then?
>
> I saw that some drivers call "ieee802154_wake_queue()" in error cases,
> may we introduce a new helper "?ieee802154_xmit_error?" for error
> cases so you can also catch error cases in your sync tx. See `grep -r
> "ieee802154_wake_queue" drivers/net/ieee802154`, if we have more
> information we might add more meaning into the error cases (e.g.
> proper errno).
Most of the time the calling functions are void functions. In fact they
all simply hardcode the xmit_done helper and even worse, sometimes they
simply leak the skb. I've handled that already by updating all these
callers to be sure the only way out is to call xmit_done, which helps a
lot tracking transfers.
Also, you are right, we can certainly drop a couple of EXPORT_SYMBOLS
:-)
> > I've decided to use the wait_queue + atomic combo which looks nice.
> > Everything seems to work, I just need a bit of time to clean and rework
> > a bit the series before sending a v3.
> >
>
> Okay, sounds good to implement both requirements.
>
> - Alex
Thanks,
Miquèl
next prev parent reply other threads:[~2022-01-17 9:00 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-12 17:32 [wpan-next v2 00/27] IEEE 802.15.4 scan support Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 01/27] net: mac802154: Split the set channel hook implementation Miquel Raynal
2022-01-12 22:30 ` Alexander Aring
2022-01-12 22:53 ` Alexander Aring
2022-01-13 11:12 ` Miquel Raynal
2022-01-13 9:32 ` Miquel Raynal
2022-01-13 23:27 ` Alexander Aring
2022-01-12 17:32 ` [wpan-next v2 02/27] net: mac802154: Ensure proper channel selection at probe time Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 03/27] net: ieee802154: Improve the way supported channels are declared Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 04/27] net: ieee802154: Give more details to the core about the channel configurations Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 05/27] net: mac802154: Convert the symbol duration into nanoseconds Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 06/27] net: mac802154: Set the symbol duration automatically Miquel Raynal
2022-01-12 22:25 ` Alexander Aring
2022-01-13 9:52 ` Miquel Raynal
2022-01-13 23:36 ` Alexander Aring
2022-01-14 10:18 ` Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 07/27] net: ieee802154: hwsim: Ensure frame checksum are valid Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 08/27] net: ieee802154: Drop symbol duration settings when the core does it already Miquel Raynal
2022-01-12 22:26 ` Alexander Aring
2022-01-13 11:16 ` Miquel Raynal
2022-01-13 23:34 ` Alexander Aring
2022-01-14 10:21 ` Miquel Raynal
2022-01-16 23:21 ` Alexander Aring
2022-01-17 9:12 ` Miquel Raynal
2022-01-17 23:38 ` Alexander Aring
2022-01-18 10:38 ` Miquel Raynal
2022-01-18 22:43 ` Alexander Aring
2022-01-19 22:26 ` Miquel Raynal
2022-01-19 23:26 ` Alexander Aring
2022-01-12 17:32 ` [wpan-next v2 09/27] net: ieee802154: Move IEEE 802.15.4 Kconfig main entry Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 10/27] net: mac802154: Include the softMAC stack inside the IEEE 802.15.4 menu Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 11/27] net: ieee802154: Move the address structure earlier Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 12/27] net: ieee802154: Add a kernel doc header to the ieee802154_addr structure Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 13/27] net: ieee802154: Return meaningful error codes from the netlink helpers Miquel Raynal
2022-01-12 17:32 ` [wpan-next v2 14/27] net: ieee802154: Add support for internal PAN management Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 15/27] net: ieee802154: Define a beacon frame header Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 16/27] net: ieee802154: Define frame types Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 17/27] net: ieee802154: Add support for scanning requests Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 18/27] net: mac802154: Handle scan requests Miquel Raynal
2022-01-12 22:44 ` Alexander Aring
2022-01-13 17:07 ` Miquel Raynal
2022-01-14 0:01 ` Alexander Aring
2022-01-14 18:44 ` Miquel Raynal
2022-01-16 22:44 ` Alexander Aring
2022-01-16 22:50 ` Alexander Aring
2022-01-17 9:00 ` Miquel Raynal [this message]
2022-01-12 17:33 ` [wpan-next v2 19/27] net: ieee802154: Full PAN management Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 20/27] net: ieee802154: Add support for beacon requests Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 21/27] net: mac802154: Handle beacons requests Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 22/27] net: ieee802154: Trace the registration of new PANs Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 23/27] net: mac802154: Add support for active scans Miquel Raynal
2022-01-12 23:16 ` Alexander Aring
2022-01-13 17:14 ` Miquel Raynal
2022-01-14 0:30 ` Alexander Aring
2022-01-14 0:39 ` Alexander Aring
2022-01-12 17:33 ` [wpan-next v2 24/27] net: mac802154: Add support for processing beacon requests Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 25/27] net: mac802154: Inform device drivers about scans Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 26/27] net: mac802154: Inform device drivers about beacon operations Miquel Raynal
2022-01-12 17:33 ` [wpan-next v2 27/27] net: ieee802154: ca8210: Refuse most of the scan operations Miquel Raynal
2022-01-12 22:48 ` Alexander Aring
2022-01-13 9:29 ` Miquel Raynal
2022-01-14 1:00 ` Alexander Aring
2022-01-14 18:37 ` Miquel Raynal
2022-01-16 22:48 ` Alexander Aring
2022-01-17 9:04 ` Miquel Raynal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220117094647.3cc5b4de@xps13 \
--to=miquel.raynal@bootlin.com \
--cc=alan@signal11.us \
--cc=alex.aring@gmail.com \
--cc=davem@davemloft.net \
--cc=david.girault@qorvo.com \
--cc=frederic.blain@qorvo.com \
--cc=h.morris@cascoda.com \
--cc=kuba@kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linux-wpan@vger.kernel.org \
--cc=liuxuenetmail@gmail.com \
--cc=michael.hennerich@analog.com \
--cc=netdev@vger.kernel.org \
--cc=nico@ni.fr.eu.org \
--cc=romuald.despres@qorvo.com \
--cc=stefan@datenfreihafen.org \
--cc=thomas.petazzoni@bootlin.com \
--cc=varkabhadram@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).