From: Alexander Aring <alex.aring@gmail.com>
To: Miquel Raynal <miquel.raynal@bootlin.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 27/27] net: ieee802154: ca8210: Refuse most of the scan operations
Date: Sun, 16 Jan 2022 17:48:03 -0500 [thread overview]
Message-ID: <CAB_54W4MP0t1+PQG1hdyLrxhu7+VGUZFaFO85u=Qea095-P8hg@mail.gmail.com> (raw)
In-Reply-To: <20220114193740.600f5f33@xps13>
Hi,
On Fri, 14 Jan 2022 at 13:37, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@gmail.com wrote on Thu, 13 Jan 2022 20:00:53 -0500:
>
> > Hi,
> >
> > On Thu, 13 Jan 2022 at 04:30, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > alex.aring@gmail.com wrote on Wed, 12 Jan 2022 17:48:59 -0500:
> > >
> > > > Hi,
> > > >
> > > > On Wed, 12 Jan 2022 at 12:34, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >
> > > > > The Cascada 8210 hardware transceiver is kind of a hardMAC which
> > > > > interfaces with the softMAC and in practice does not support sending
> > > > > anything else than dataframes. This means we cannot send any BEACON_REQ
> > > > > during active scans nor any BEACON in general. Refuse these operations
> > > > > officially so that the user is aware of the limitation.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > ---
> > > > > drivers/net/ieee802154/ca8210.c | 25 ++++++++++++++++++++++++-
> > > > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> > > > > index d3a9e4fe05f4..49c274280e3c 100644
> > > > > --- a/drivers/net/ieee802154/ca8210.c
> > > > > +++ b/drivers/net/ieee802154/ca8210.c
> > > > > @@ -2385,6 +2385,25 @@ static int ca8210_set_promiscuous_mode(struct ieee802154_hw *hw, const bool on)
> > > > > return link_to_linux_err(status);
> > > > > }
> > > > >
> > > > > +static int ca8210_enter_scan_mode(struct ieee802154_hw *hw,
> > > > > + struct cfg802154_scan_request *request)
> > > > > +{
> > > > > + /* This xceiver can only send dataframes */
> > > > > + if (request->type != NL802154_SCAN_PASSIVE)
> > > > > + return -EOPNOTSUPP;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int ca8210_enter_beacons_mode(struct ieee802154_hw *hw,
> > > > > + struct cfg802154_beacons_request *request)
> > > > > +{
> > > > > + /* This xceiver can only send dataframes */
> > > > > + return -EOPNOTSUPP;
> > > > > +}
> > > > > +
> > > > > +static void ca8210_exit_scan_beacons_mode(struct ieee802154_hw *hw) { }
> > > > > +
> > > > > static const struct ieee802154_ops ca8210_phy_ops = {
> > > > > .start = ca8210_start,
> > > > > .stop = ca8210_stop,
> > > > > @@ -2397,7 +2416,11 @@ static const struct ieee802154_ops ca8210_phy_ops = {
> > > > > .set_cca_ed_level = ca8210_set_cca_ed_level,
> > > > > .set_csma_params = ca8210_set_csma_params,
> > > > > .set_frame_retries = ca8210_set_frame_retries,
> > > > > - .set_promiscuous_mode = ca8210_set_promiscuous_mode
> > > > > + .set_promiscuous_mode = ca8210_set_promiscuous_mode,
> > > > > + .enter_scan_mode = ca8210_enter_scan_mode,
> > > > > + .exit_scan_mode = ca8210_exit_scan_beacons_mode,
> > > > > + .enter_beacons_mode = ca8210_enter_beacons_mode,
> > > > > + .exit_beacons_mode = ca8210_exit_scan_beacons_mode,
> > > > > };
> > > >
> > > > so there is no flag that this driver can't support scanning currently
> > > > and it works now because the offload functionality will return
> > > > -ENOTSUPP? This is misleading because I would assume if it's not
> > > > supported we can do it by software which the driver can't do.
> > >
> > > I believe there is a misunderstanding.
> > >
> > > This is what I have understood from your previous comments in v1:
> > > "This driver does not support transmitting anything else than
> > > datagrams", which is what I assumed was a regular data packet. IOW,
> > > sending a MAC_CMD such as a beacon request or sending a beacon was not
> > > supported physically by the hardware. Hence, most of the scans
> > > operations cannot be performed and must be rejected (all but a passive
> > > scan, assuming that receiving beacons was okay).
> > >
> >
> > and I said that this driver is a HardMAC transceiver connected to the
> > SoftMAC layer which is already wrong to exist (very special handling
> > is required here).
> > dataframes here are "data" type frames and I suppose it's also not
> > able to deliver/receive other types than data to mac802154.
> >
> > It seems the author of this driver is happy to have data frames only
> > but we need to take care that additional mac802154 handling is simply
> > not possible to do here.
> >
> > > Please mind the update in that hook which currently is just an FYI from
> > > the mac to the drivers and not a "do it by yourself" injunction. So
> > > answering -EOPNOTSUPP to the mac here does not mean:
> > > "I cannot handle it by myself, the scan cannot happen"
> > > but
> > > "I cannot handle the forged frames, so let's just not try"
> > >
> >
> > The problem here is that a SoftMAC transceiver should always be
> > capable of doing it by software so the "but case" makes no sense in
> > this layer.
> > On a mac802154 layer and "offload" driver functions as they are and
> > they report me "-ENOTSUPP", I would assume that I can go back and do
> > it by software which again should always be possible to do in
> > mac802154.
> >
> > > > ... I see that the offload functions now are getting used and have a
> > > > reason to be upstream, but the use of it is wrong.
> > >
> > > As a personal matter of taste, I don't like flags when it comes to
> > > something complex like supporting a specific operation. Just in the
> > > scanning procedure there are 4 different actions and a driver might
> > > support only a subset of these, which is totally fine but hard to
> > > properly describe by well-named flags. Here the driver hooks say to
> > > the driver which are interested "here is what is going to happen" and
> > > then they can:
> > > - ignore the details by just not implementing the hooks, let the mac do
> > > its job, they will then transmit the relevant frames forged by the
> > > mac
> > > - eventually enter a specific mode internally for this operation, but
> > > basically do the same as above, ie. transmitting the frames forged
> > > by the mac
> > > - refuse the operation by returning an error code if something cannot
> > > be done
> > >
> > > I've experienced a number of situations in the MTD world and later with
> > > IIO drivers where flags have been remodeled and reused in different
> > > manners, until the flag description gets totally wrong and
> > > undescriptive regarding what it actually does. Hence my main idea of
> > > letting drivers refuse these operations instead of having the mac doing
> > > it for them.
> > >
> > > I can definitely use flags if you want, but in this case, what flags do
> > > you want to see?
> > >
> >
> > Do some phy quirks flags which indicate that the transceiver is not
> > capable of doing scan operation by software. Or just use a boolean in
> > phy capabilities (but don't export them to userspace, note that this
> > flag should be removed later) if this operation is not allowed.
>
> I've added a phy flag to distinguish this driver as early as possible.
I was thinking about this a lot and I think the driver is already
buggy in many ways e.g. what happens when we do AF_PACKET raw sockets
and do some different frame types than data. I have no idea, maybe we
should leave it as it is... and simply don't care? Maybe the driver
needs to drop a lot of frames if they are different... it runs already
different than other transceivers which we don't check at all.
- Alex
next prev parent reply other threads:[~2022-01-16 22:48 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
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 [this message]
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='CAB_54W4MP0t1+PQG1hdyLrxhu7+VGUZFaFO85u=Qea095-P8hg@mail.gmail.com' \
--to=alex.aring@gmail.com \
--cc=alan@signal11.us \
--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=miquel.raynal@bootlin.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).