Hi,
On Sat, Oct 15, 2022 at 4:59 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> Hi Alexander,
>
> aahringo@xxxxxxxxxx wrote on Mon, 10 Oct 2022 21:21:17 -0400:
>
> > Hi,
> >
> > On Mon, Oct 10, 2022 at 9:13 PM Alexander Aring <aahringo@xxxxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Oct 10, 2022 at 9:04 PM Alexander Aring <aahringo@xxxxxxxxxx> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Fri, Oct 7, 2022 at 4:53 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> > > > >
> > > > > We have access to the address filters being theoretically applied, we
> > > > > also have access to the actual filtering level applied, so let's add a
> > > > > proper frame validation sequence in hwsim.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > > > > ---
> > > > > drivers/net/ieee802154/mac802154_hwsim.c | 111 ++++++++++++++++++++++-
> > > > > include/net/ieee802154_netdev.h | 8 ++
> > > > > 2 files changed, 117 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c
> > > > > index 458be66b5195..84ee948f35bc 100644
> > > > > --- a/drivers/net/ieee802154/mac802154_hwsim.c
> > > > > +++ b/drivers/net/ieee802154/mac802154_hwsim.c
> > > > > @@ -18,6 +18,7 @@
> > > > > #include <linux/netdevice.h>
> > > > > #include <linux/device.h>
> > > > > #include <linux/spinlock.h>
> > > > > +#include <net/ieee802154_netdev.h>
> > > > > #include <net/mac802154.h>
> > > > > #include <net/cfg802154.h>
> > > > > #include <net/genetlink.h>
> > > > > @@ -139,6 +140,113 @@ static int hwsim_hw_addr_filt(struct ieee802154_hw *hw,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static void hwsim_hw_receive(struct ieee802154_hw *hw, struct sk_buff *skb,
> > > > > + u8 lqi)
> > > > > +{
> > > > > + struct ieee802154_hdr hdr;
> > > > > + struct hwsim_phy *phy = hw->priv;
> > > > > + struct hwsim_pib *pib;
> > > > > +
> > > > > + rcu_read_lock();
> > > > > + pib = rcu_dereference(phy->pib);
> > > > > +
> > > > > + if (!pskb_may_pull(skb, 3)) {
> > > > > + dev_dbg(hw->parent, "invalid frame\n");
> > > > > + goto drop;
> > > > > + }
> > > > > +
> > > > > + memcpy(&hdr, skb->data, 3);
> > > > > +
> > > > > + /* Level 4 filtering: Frame fields validity */
> > > > > + if (hw->phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) {
> > >
> > > I see, there is this big if handling. But it accesses the
> > > hw->phy->filtering value. It should be part of the hwsim pib setting
> > > set by the driver callback. It is a question here of mac802154 layer
> > > setting vs driver layer setting. We should do what the mac802154 tells
> > > the driver to do, this way we do what the mac802154 layer is set to.
> > >
> > > However it's a minor thing and it's okay to do it so...
> >
> > * whereas we never let the driver know at any time of what different
> > filter levels exist _currently_ we have only the promiscuous mode
> > on/off switch which is do nothing or 4_FRAME_FIELDS.
> > It will work for now, changing anything in the mac802154 filtering
> > fields or something will end in probably breakage in this handling. In
> > my point of view as the current state is it should not do that, as
> > remember that hwsim will "simulate" hardware it should not be able to
> > access mac802154 fields (especially when doing receiving of frames) as
> > other hardware will only set register bits (as hwsim pib values is
> > there for)...
> >
> > Still I think it's fine for now.
>
> I see your point, indeed I could have added another PIB attribute
> instead of accessing the PHY state.
>
> I am fine doing it in a followup patch if this what you prefer. Shall I
> do it?
okay, note that you did it right with the address filter patches by
copying them from the drivers-ops.
- Alex
