Message-ID: <CAB_54W4MP0t1+PQG1hdyLrxhu7+VGUZFaFO85u=Qea095-P8hg@mail.gmail.com>
Date:   Sun, 16 Jan 2022 17:48:03 -0500
From:   Alexander Aring <alex.aring@...il.com>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     Stefan Schmidt <stefan@...enfreihafen.org>,
        linux-wpan - ML <linux-wpan@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        Michael Hennerich <michael.hennerich@...log.com>,
        Harry Morris <h.morris@...coda.com>,
        Varka Bhadram <varkabhadram@...il.com>,
        Xue Liu <liuxuenetmail@...il.com>, Alan Ott <alan@...nal11.us>,
        David Girault <david.girault@...vo.com>,
        Romuald Despres <romuald.despres@...vo.com>,
        Frederic Blain <frederic.blain@...vo.com>,
        Nicolas Schodet <nico@...fr.eu.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        "linux-wireless@...r.kernel.org Wireless" 
        <linux-wireless@...r.kernel.org>
Subject: Re: [wpan-next v2 27/27] net: ieee802154: ca8210: Refuse most of the
 scan operations

Hi,

On Fri, 14 Jan 2022 at 13:37, Miquel Raynal <miquel.raynal@...tlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@...il.com wrote on Thu, 13 Jan 2022 20:00:53 -0500:
>
> > Hi,
> >
> > On Thu, 13 Jan 2022 at 04:30, Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > alex.aring@...il.com wrote on Wed, 12 Jan 2022 17:48:59 -0500:
> > >
> > > > Hi,
> > > >
> > > > On Wed, 12 Jan 2022 at 12:34, Miquel Raynal <miquel.raynal@...tlin.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@...tlin.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

Powered by blists - more mailing lists