Date:   Fri, 14 Jan 2022 19:37:40 +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 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 don't like this flag either, it is a workaround to still support the
> driver as it is and don't allow new fancy things because it cannot
> handle it. We should work on it to remove this flag, but this is a
> longer process. Either the driver is implementing those hooks "real"
> to somehow run a scan (but at it's own risk) or it needs to be
> implemented as a HardMAC driver. This driver is already difficult to
> maintain because it doesn't fit here...
> 
> - Alex


Thanks,
Miquèl

Powered by blists - more mailing lists