Message-ID: <20220117100418.29fe6840@xps13>
Date:   Mon, 17 Jan 2022 10:04:18 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Alexander Aring <alex.aring@...il.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 Alexander,

alex.aring@...il.com wrote on Sun, 16 Jan 2022 17:48:03 -0500:

> 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.

For now I've handled it with a flag, I'll send the patch as it is so
that someone else can pick it up if you finally decide to not touch
this driver for now and change your mind later on.

I would however argue that having these constraint devices flagged in
some way is better than nothing. You personally know the issue, but
newcomers don't.

Thanks,
Miquèl

Powered by blists - more mailing lists