| Message ID | 20220905203412.1322947-6-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ieee802154: Support scanning/beaconing | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Hi, On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to > reflect the fact that it would not validate the checksum (FCS). In other > words, the filtering level of hwsim is always "NONE" while the core > expects it to be higher. > > Now that we have access to real filtering levels, we can actually use > them and always enforce the "NONE" level in hwsim. This case is already > correctly handled in the receive so we can drop the flag. > I would say the whole hwsim driver currently only works because we don't transmit wrong frames on a virtual hardware... However this can be improved, yes. In my opinion the hwsim driver should pretend to work like other transceivers sending frames to mac802154. That means the filtering level should be implemented in hwsim not in mac802154 as on real hardware the hardware would do filtering. I think you should assume for now the previous behaviour that hwsim does not send bad frames out. Of course there is a bug but it was already there before, but the fix would be to change hwsim driver. - Alex
Hi Alexander, aahringo@redhat.com wrote on Thu, 8 Sep 2022 20:49:36 -0400: > Hi, > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to > > reflect the fact that it would not validate the checksum (FCS). In other > > words, the filtering level of hwsim is always "NONE" while the core > > expects it to be higher. > > > > Now that we have access to real filtering levels, we can actually use > > them and always enforce the "NONE" level in hwsim. This case is already > > correctly handled in the receive so we can drop the flag. > > > > I would say the whole hwsim driver currently only works because we > don't transmit wrong frames on a virtual hardware... However this can > be improved, yes. In my opinion the hwsim driver should pretend to > work like other transceivers sending frames to mac802154. That means > the filtering level should be implemented in hwsim not in mac802154 as > on real hardware the hardware would do filtering. > > I think you should assume for now the previous behaviour that hwsim > does not send bad frames out. Of course there is a bug but it was > already there before, but the fix would be to change hwsim driver. Well, somehow I already implemented all the filtering by software in one of the other patches. I now agree that it was not relevant (because of the AACK issue you raised), but instead of fully dropping this code I might just move it to hwsim because there it would perfectly make sense? Thanks, Miquèl
Hi, On Wed, Sep 21, 2022 at 11:49 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Thu, 8 Sep 2022 20:49:36 -0400: > > > Hi, > > > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to > > > reflect the fact that it would not validate the checksum (FCS). In other > > > words, the filtering level of hwsim is always "NONE" while the core > > > expects it to be higher. > > > > > > Now that we have access to real filtering levels, we can actually use > > > them and always enforce the "NONE" level in hwsim. This case is already > > > correctly handled in the receive so we can drop the flag. > > > > > > > I would say the whole hwsim driver currently only works because we > > don't transmit wrong frames on a virtual hardware... However this can > > be improved, yes. In my opinion the hwsim driver should pretend to > > work like other transceivers sending frames to mac802154. That means > > the filtering level should be implemented in hwsim not in mac802154 as > > on real hardware the hardware would do filtering. > > > > I think you should assume for now the previous behaviour that hwsim > > does not send bad frames out. Of course there is a bug but it was > > already there before, but the fix would be to change hwsim driver. > > Well, somehow I already implemented all the filtering by software in > one of the other patches. I now agree that it was not relevant (because > of the AACK issue you raised), but instead of fully dropping this code > I might just move it to hwsim because there it would perfectly make > sense? > Yes, I agree. You should make the "in-driver receive path" acting like other hardware (In sense what we currently agree on what filtering they do) if promiscuous mode is turned off/on. It makes sense and should be done there. The IEEE802154_HW_RX_DROP_BAD_CKSUM flag should still be dropped. - Alex
diff --git a/drivers/net/ieee802154/mac802154_hwsim.c b/drivers/net/ieee802154/mac802154_hwsim.c index 38c217bd7c82..d18a1391b61f 100644 --- a/drivers/net/ieee802154/mac802154_hwsim.c +++ b/drivers/net/ieee802154/mac802154_hwsim.c @@ -148,6 +148,7 @@ static int hwsim_hw_start(struct ieee802154_hw *hw) struct hwsim_phy *phy = hw->priv; phy->suspended = false; + hw->phy->filtering = IEEE802154_FILTERING_NONE; return 0; } @@ -791,7 +792,7 @@ static int hwsim_add_one(struct genl_info *info, struct device *dev, phy->idx = idx; INIT_LIST_HEAD(&phy->edges); - hw->flags = IEEE802154_HW_PROMISCUOUS | IEEE802154_HW_RX_DROP_BAD_CKSUM; + hw->flags = IEEE802154_HW_PROMISCUOUS; hw->parent = dev; err = ieee802154_register_hw(hw); diff --git a/include/net/mac802154.h b/include/net/mac802154.h index 357d25ef627a..4a3a9de9da73 100644 --- a/include/net/mac802154.h +++ b/include/net/mac802154.h @@ -111,9 +111,6 @@ struct ieee802154_hw { * promiscuous mode setting. * * @IEEE802154_HW_RX_OMIT_CKSUM: Indicates that receiver omits FCS. - * - * @IEEE802154_HW_RX_DROP_BAD_CKSUM: Indicates that receiver will not filter - * frames with bad checksum. */ enum ieee802154_hw_flags { IEEE802154_HW_TX_OMIT_CKSUM = BIT(0), @@ -123,7 +120,6 @@ enum ieee802154_hw_flags { IEEE802154_HW_AFILT = BIT(4), IEEE802154_HW_PROMISCUOUS = BIT(5), IEEE802154_HW_RX_OMIT_CKSUM = BIT(6), - IEEE802154_HW_RX_DROP_BAD_CKSUM = BIT(7), }; /* Indicates that receiver omits FCS and xmitter will add FCS on it's own. */ diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c index 26df79911f3e..bd1a92fceef7 100644 --- a/net/mac802154/rx.c +++ b/net/mac802154/rx.c @@ -270,15 +270,12 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb) ieee802154_monitors_rx(local, skb); - /* Check if transceiver doesn't validate the checksum. - * If not we validate the checksum here. - */ + /* Level 1 filtering: Check the FCS by software when relevant */ /* TODO do whatever you want here if necessary to filter by * check on IEEE802154_FILTERING_NONE. And upcomming receive * path in which state the phy is e.g. scanning. */ - if (local->hw.flags & IEEE802154_HW_RX_DROP_BAD_CKSUM || - local->phy->filtering == IEEE802154_FILTERING_NONE) { + if (local->hw.phy->filtering == IEEE802154_FILTERING_NONE) { crc = crc_ccitt(0, skb->data, skb->len); if (crc) { rcu_read_unlock();
This IEEE802154_HW_RX_DROP_BAD_CKSUM flag was only used by hwsim to reflect the fact that it would not validate the checksum (FCS). In other words, the filtering level of hwsim is always "NONE" while the core expects it to be higher. Now that we have access to real filtering levels, we can actually use them and always enforce the "NONE" level in hwsim. This case is already correctly handled in the receive so we can drop the flag. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/net/ieee802154/mac802154_hwsim.c | 3 ++- include/net/mac802154.h | 4 ---- net/mac802154/rx.c | 7 ++----- 3 files changed, 4 insertions(+), 10 deletions(-)