| Message ID | 20220905203412.1322947-1-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: ieee802154: Support scanning/beaconing | expand |
Hi, On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hello, > > A third version of this series, dropping the scan patches for now > because before we need to settle on the filtering topic and the > coordinator interface topic. Here is just the filtering part, I've > integrated Alexander's patches, as well as the atusb fix. Once this is > merge there are a few coordinator-related patches, and finally the > scan. I think we have a communication problem here and we should talk about what the problems are and agree on a way to solve them. The problems are: 1. We never supported switching from an operating phy (interfaces are up) into another filtering mode. 2. Scan requires to be in "promiscuous mode" (according to the 802.15.4 spec promiscuous mode). We don't support promiscuous mode (according to the 802.15.4 spec promiscuous mode). We "can" however use the currently supported mode which does not filter anything (IEEE802154_FILTERING_NONE) when we do additional filtering in mac802154. _But_ this is only required when the phy is scanning, it will also deliver anything to the upper layers. This patch-series tries to do the second thing, okay that's fine. But I thought this should only be done while the phy is in "scanning mode"? The other receive path while not in promiscuous mode (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never require any additional filtering. I somehow miss this point here. For 1), the driver should change the filtering mode" when we start to "listen", this is done by the start() driver callback. They should get all receive parameters and set up receiving to whatever mac802154, currently there is a bit of chaos there. To move it into drv_start() is just a workaround to begin this step that we move it at some point to the driver. I mention 1) here because that should be part of the picture how everything works together when the phy is switched to a different filter level while it's operating (I mean there are running interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS) which then activates the different receive path for the use case of scanning (something like (phy->state & WPANPHY_SCANING) == true)? I am sorry, but I somehow miss the picture of how those things work together. It is not clear for me and I miss those parts to get a whole picture of this. For me it's not clear that those patches are going in this direction. - Alex
Hi Alexander, aahringo@redhat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400: > Hi, > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hello, > > > > A third version of this series, dropping the scan patches for now > > because before we need to settle on the filtering topic and the > > coordinator interface topic. Here is just the filtering part, I've > > integrated Alexander's patches, as well as the atusb fix. Once this is > > merge there are a few coordinator-related patches, and finally the > > scan. > > I think we have a communication problem here and we should talk about > what the problems are and agree on a way to solve them. > > The problems are: > > 1. We never supported switching from an operating phy (interfaces are > up) into another filtering mode. In the trigger scan path there is a: mlme_op_pre() // stop Tx drv_stop() // stop Rx synchronize_net() drv_start(params) // restart Rx with another hw filtering level > 2. Scan requires to be in "promiscuous mode" (according to the > 802.15.4 spec promiscuous mode). We don't support promiscuous mode > (according to the 802.15.4 spec promiscuous mode). We "can" however > use the currently supported mode which does not filter anything > (IEEE802154_FILTERING_NONE) when we do additional filtering in > mac802154. _But_ this is only required when the phy is scanning, it > will also deliver anything to the upper layers. > > This patch-series tries to do the second thing, okay that's fine. But > I thought this should only be done while the phy is in "scanning > mode"? I don't understand what's wrong then. We ask for the "scan mode" filtering level when starting the scan [1] and we ask for the normal filtering level when it's done/aborted [2] [3]. [1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326 [2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55 > The other receive path while not in promiscuous mode > (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never > require any additional filtering. I somehow miss this point here. Maybe the drv_start() function should receive an sdata pointer. This way instead of changing the PHY filtering level to what has just be asked blindly, the code should look at the filtering level of all the interfaces up on the PHY and apply the lowest filtering level by hardware, knowing that on a per interface basis, the software will compensate. It should work just fine because local->phy->filtering shows the actual filtering level of the PHY while sdata->requested_filtering shows the level of filtering that was expected on each interface. If you don't like the idea of having a mutable sdata->requested_filtering entry, I can have an sdata->base_filtering which should only be set by ieee802154_setup_sdata() and an sdata->expected_filtering which would reflect what the mac expects on this interface at the present moment. > For 1), the driver should change the filtering mode" when we start to > "listen", this is done by the start() driver callback. They should get > all receive parameters and set up receiving to whatever mac802154, > currently there is a bit of chaos there. To move it into drv_start() > is just a workaround to begin this step that we move it at some point > to the driver. I mention 1) here because that should be part of the > picture how everything works together when the phy is switched to a > different filter level while it's operating (I mean there are running > interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS) > which then activates the different receive path for the use case of > scanning (something like (phy->state & WPANPHY_SCANING) == true)? Scanning is a dedicated filtering level per-se because it must discard !beacon frames, that's why I request this level of filtering (which maybe I should do on a per-interface basis instead of using the *local poiner). > I am sorry, but I somehow miss the picture of how those things work > together. It is not clear for me and I miss those parts to get a whole > picture of this. For me it's not clear that those patches are going in > this direction. Please tell me if it's more clear and if you agree with this vision. I don't have time to draft something this week. Thanks, Miquèl
Hi, On Thu, Sep 8, 2022 at 3:37 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400: > > > Hi, > > > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hello, > > > > > > A third version of this series, dropping the scan patches for now > > > because before we need to settle on the filtering topic and the > > > coordinator interface topic. Here is just the filtering part, I've > > > integrated Alexander's patches, as well as the atusb fix. Once this is > > > merge there are a few coordinator-related patches, and finally the > > > scan. > > > > I think we have a communication problem here and we should talk about > > what the problems are and agree on a way to solve them. > > > > The problems are: > > > > 1. We never supported switching from an operating phy (interfaces are > > up) into another filtering mode. > > In the trigger scan path there is a: > > mlme_op_pre() // stop Tx > drv_stop() // stop Rx > synchronize_net() > drv_start(params) // restart Rx with another hw filtering level > Okay, that's looking good. > > 2. Scan requires to be in "promiscuous mode" (according to the > > 802.15.4 spec promiscuous mode). We don't support promiscuous mode > > (according to the 802.15.4 spec promiscuous mode). We "can" however > > use the currently supported mode which does not filter anything > > (IEEE802154_FILTERING_NONE) when we do additional filtering in > > mac802154. _But_ this is only required when the phy is scanning, it > > will also deliver anything to the upper layers. > > > > This patch-series tries to do the second thing, okay that's fine. But > > I thought this should only be done while the phy is in "scanning > > mode"? > > I don't understand what's wrong then. We ask for the "scan mode" > filtering level when starting the scan [1] and we ask for the normal > filtering level when it's done/aborted [2] [3]. > There is no problem with that. There is for me a problem with the receive path when certain filtering levels are active. > [1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326 > [2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55 > > > The other receive path while not in promiscuous mode > > (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never > > require any additional filtering. I somehow miss this point here. > > Maybe the drv_start() function should receive an sdata pointer. This way > instead of changing the PHY filtering level to what has just be asked > blindly, the code should look at the filtering level of all the > interfaces up on the PHY and apply the lowest filtering level by > hardware, knowing that on a per interface basis, the software will > compensate. > > It should work just fine because local->phy->filtering shows the actual > filtering level of the PHY while sdata->requested_filtering shows the > level of filtering that was expected on each interface. If you don't > like the idea of having a mutable sdata->requested_filtering entry, I > can have an sdata->base_filtering which should only be set by > ieee802154_setup_sdata() and an sdata->expected_filtering which would > reflect what the mac expects on this interface at the present moment. > From my view is that if we disable address filters (all filtering modes except IEEE802154_FILTERING_4_FRAME_FIELDS) we never can call netif_receive_skb(). This patch series tries to "compensate" the missing filtering on phy which is fine only to handle things related for the scan operation but nothing else. The reason why we can't call netif_receive_skb() is because we don't have ackknowledge handling, whereas for scanning we ignore ack frames and that's why we don't need it. > > For 1), the driver should change the filtering mode" when we start to > > "listen", this is done by the start() driver callback. They should get > > all receive parameters and set up receiving to whatever mac802154, > > currently there is a bit of chaos there. To move it into drv_start() > > is just a workaround to begin this step that we move it at some point > > to the driver. I mention 1) here because that should be part of the > > picture how everything works together when the phy is switched to a > > different filter level while it's operating (I mean there are running > > interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS) > > which then activates the different receive path for the use case of > > scanning (something like (phy->state & WPANPHY_SCANING) == true)? > > Scanning is a dedicated filtering level per-se because it must discard > !beacon frames, that's why I request this level of filtering (which > maybe I should do on a per-interface basis instead of using the *local > poiner). > We only can do a per filter level per interface if the hardware has support for such a thing. Currently there is one address filter and if it's disabled we lose ackknowledge handling (as general rule), we can't compensate by doing any additional filtering by software in this mode. > > I am sorry, but I somehow miss the picture of how those things work > > together. It is not clear for me and I miss those parts to get a whole > > picture of this. For me it's not clear that those patches are going in > > this direction. > > Please tell me if it's more clear and if you agree with this vision. I > don't have time to draft something this week. > That's fine. We should agree on things like compensate lower filter levels by doing additional softmac filtering to reach IEEE802154_FILTERING_4_FRAME_FIELDS filtering from others because we will lose AACK handling. It is only fine to do that in mac802154 receive path but don't deliver it to the upper layer. - Alex
Hi Alexander, aahringo@redhat.com wrote on Thu, 8 Sep 2022 20:41:14 -0400: > Hi, > > On Thu, Sep 8, 2022 at 3:37 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400: > > > > > Hi, > > > > > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > Hello, > > > > > > > > A third version of this series, dropping the scan patches for now > > > > because before we need to settle on the filtering topic and the > > > > coordinator interface topic. Here is just the filtering part, I've > > > > integrated Alexander's patches, as well as the atusb fix. Once this is > > > > merge there are a few coordinator-related patches, and finally the > > > > scan. > > > > > > I think we have a communication problem here and we should talk about > > > what the problems are and agree on a way to solve them. > > > > > > The problems are: > > > > > > 1. We never supported switching from an operating phy (interfaces are > > > up) into another filtering mode. > > > > In the trigger scan path there is a: > > > > mlme_op_pre() // stop Tx > > drv_stop() // stop Rx > > synchronize_net() > > drv_start(params) // restart Rx with another hw filtering level > > > > Okay, that's looking good. > > > > 2. Scan requires to be in "promiscuous mode" (according to the > > > 802.15.4 spec promiscuous mode). We don't support promiscuous mode > > > (according to the 802.15.4 spec promiscuous mode). We "can" however > > > use the currently supported mode which does not filter anything > > > (IEEE802154_FILTERING_NONE) when we do additional filtering in > > > mac802154. _But_ this is only required when the phy is scanning, it > > > will also deliver anything to the upper layers. > > > > > > This patch-series tries to do the second thing, okay that's fine. But > > > I thought this should only be done while the phy is in "scanning > > > mode"? > > > > I don't understand what's wrong then. We ask for the "scan mode" > > filtering level when starting the scan [1] and we ask for the normal > > filtering level when it's done/aborted [2] [3]. > > > > There is no problem with that. There is for me a problem with the > receive path when certain filtering levels are active. > > > [1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326 > > [2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55 > > > > > The other receive path while not in promiscuous mode > > > (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never > > > require any additional filtering. I somehow miss this point here. > > > > Maybe the drv_start() function should receive an sdata pointer. This way > > instead of changing the PHY filtering level to what has just be asked > > blindly, the code should look at the filtering level of all the > > interfaces up on the PHY and apply the lowest filtering level by > > hardware, knowing that on a per interface basis, the software will > > compensate. > > > > It should work just fine because local->phy->filtering shows the actual > > filtering level of the PHY while sdata->requested_filtering shows the > > level of filtering that was expected on each interface. If you don't > > like the idea of having a mutable sdata->requested_filtering entry, I > > can have an sdata->base_filtering which should only be set by > > ieee802154_setup_sdata() and an sdata->expected_filtering which would > > reflect what the mac expects on this interface at the present moment. > > > > From my view is that if we disable address filters (all filtering > modes except IEEE802154_FILTERING_4_FRAME_FIELDS) we never can call > netif_receive_skb(). This patch series tries to "compensate" the > missing filtering on phy which is fine only to handle things related > for the scan operation but nothing else. > > The reason why we can't call netif_receive_skb() is because we don't > have ackknowledge handling, whereas for scanning we ignore ack frames > and that's why we don't need it. I've digested all of that right before the conference so I think I now understand all your fears regarding the possible absence of ACK in certain situations. I even share them now. > > > For 1), the driver should change the filtering mode" when we start to > > > "listen", this is done by the start() driver callback. They should get > > > all receive parameters and set up receiving to whatever mac802154, > > > currently there is a bit of chaos there. To move it into drv_start() > > > is just a workaround to begin this step that we move it at some point > > > to the driver. I mention 1) here because that should be part of the > > > picture how everything works together when the phy is switched to a > > > different filter level while it's operating (I mean there are running > > > interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS) > > > which then activates the different receive path for the use case of > > > scanning (something like (phy->state & WPANPHY_SCANING) == true)? > > > > Scanning is a dedicated filtering level per-se because it must discard > > !beacon frames, that's why I request this level of filtering (which > > maybe I should do on a per-interface basis instead of using the *local > > poiner). > > > > We only can do a per filter level per interface if the hardware has > support for such a thing. Currently there is one address filter and if > it's disabled we lose ackknowledge handling (as general rule), we > can't compensate by doing any additional filtering by software in this > mode. Yes. > > > > I am sorry, but I somehow miss the picture of how those things work > > > together. It is not clear for me and I miss those parts to get a whole > > > picture of this. For me it's not clear that those patches are going in > > > this direction. > > > > Please tell me if it's more clear and if you agree with this vision. I > > don't have time to draft something this week. > > > > That's fine. We should agree on things like compensate lower filter > levels by doing additional softmac filtering to reach > IEEE802154_FILTERING_4_FRAME_FIELDS filtering from others because we > will lose AACK handling. It is only fine to do that in mac802154 > receive path but don't deliver it to the upper layer. So actually, if I try to summarize the situation. I've tried to make several different subif working on a single PHY. Unfortunately, today there is only one address filter per PHY, so disabling the address filter on one interface would also disable it on the other, leading to the ACKs not being handled anymore, which we cannot afford. So overall I guess we should settle on how we want to handle the situation. I propose, to move forward, that we continue to assume that it is *not* possible to have several different interfaces running at the same time on a single PHY. This involves dropping all the "software address filtering" which I proposed, but I guess that's fine. Thanks, Miquèl
Hi, On Wed, Sep 21, 2022 at 11:46 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Thu, 8 Sep 2022 20:41:14 -0400: > > > Hi, > > > > On Thu, Sep 8, 2022 at 3:37 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Wed, 7 Sep 2022 21:40:13 -0400: > > > > > > > Hi, > > > > > > > > On Mon, Sep 5, 2022 at 4:34 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > Hello, > > > > > > > > > > A third version of this series, dropping the scan patches for now > > > > > because before we need to settle on the filtering topic and the > > > > > coordinator interface topic. Here is just the filtering part, I've > > > > > integrated Alexander's patches, as well as the atusb fix. Once this is > > > > > merge there are a few coordinator-related patches, and finally the > > > > > scan. > > > > > > > > I think we have a communication problem here and we should talk about > > > > what the problems are and agree on a way to solve them. > > > > > > > > The problems are: > > > > > > > > 1. We never supported switching from an operating phy (interfaces are > > > > up) into another filtering mode. > > > > > > In the trigger scan path there is a: > > > > > > mlme_op_pre() // stop Tx > > > drv_stop() // stop Rx > > > synchronize_net() > > > drv_start(params) // restart Rx with another hw filtering level > > > > > > > Okay, that's looking good. > > > > > > 2. Scan requires to be in "promiscuous mode" (according to the > > > > 802.15.4 spec promiscuous mode). We don't support promiscuous mode > > > > (according to the 802.15.4 spec promiscuous mode). We "can" however > > > > use the currently supported mode which does not filter anything > > > > (IEEE802154_FILTERING_NONE) when we do additional filtering in > > > > mac802154. _But_ this is only required when the phy is scanning, it > > > > will also deliver anything to the upper layers. > > > > > > > > This patch-series tries to do the second thing, okay that's fine. But > > > > I thought this should only be done while the phy is in "scanning > > > > mode"? > > > > > > I don't understand what's wrong then. We ask for the "scan mode" > > > filtering level when starting the scan [1] and we ask for the normal > > > filtering level when it's done/aborted [2] [3]. > > > > > > > There is no problem with that. There is for me a problem with the > > receive path when certain filtering levels are active. > > > > > [1] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L326 > > > [2] https://github.com/miquelraynal/linux/blob/wpan-next/scan/net/mac802154/scan.c#L55 > > > > > > > The other receive path while not in promiscuous mode > > > > (phy->filtering == IEEE802154_FILTERING_4_FRAME_FIELDS) should never > > > > require any additional filtering. I somehow miss this point here. > > > > > > Maybe the drv_start() function should receive an sdata pointer. This way > > > instead of changing the PHY filtering level to what has just be asked > > > blindly, the code should look at the filtering level of all the > > > interfaces up on the PHY and apply the lowest filtering level by > > > hardware, knowing that on a per interface basis, the software will > > > compensate. > > > > > > It should work just fine because local->phy->filtering shows the actual > > > filtering level of the PHY while sdata->requested_filtering shows the > > > level of filtering that was expected on each interface. If you don't > > > like the idea of having a mutable sdata->requested_filtering entry, I > > > can have an sdata->base_filtering which should only be set by > > > ieee802154_setup_sdata() and an sdata->expected_filtering which would > > > reflect what the mac expects on this interface at the present moment. > > > > > > > From my view is that if we disable address filters (all filtering > > modes except IEEE802154_FILTERING_4_FRAME_FIELDS) we never can call > > netif_receive_skb(). This patch series tries to "compensate" the > > missing filtering on phy which is fine only to handle things related > > for the scan operation but nothing else. > > > > The reason why we can't call netif_receive_skb() is because we don't > > have ackknowledge handling, whereas for scanning we ignore ack frames > > and that's why we don't need it. > > I've digested all of that right before the conference so I think I now > understand all your fears regarding the possible absence of ACK in > certain situations. I even share them now. > > > > > For 1), the driver should change the filtering mode" when we start to > > > > "listen", this is done by the start() driver callback. They should get > > > > all receive parameters and set up receiving to whatever mac802154, > > > > currently there is a bit of chaos there. To move it into drv_start() > > > > is just a workaround to begin this step that we move it at some point > > > > to the driver. I mention 1) here because that should be part of the > > > > picture how everything works together when the phy is switched to a > > > > different filter level while it's operating (I mean there are running > > > > interfaces on it which requires IEEE802154_FILTERING_4_FRAME_FIELDS) > > > > which then activates the different receive path for the use case of > > > > scanning (something like (phy->state & WPANPHY_SCANING) == true)? > > > > > > Scanning is a dedicated filtering level per-se because it must discard > > > !beacon frames, that's why I request this level of filtering (which > > > maybe I should do on a per-interface basis instead of using the *local > > > poiner). > > > > > > > We only can do a per filter level per interface if the hardware has > > support for such a thing. Currently there is one address filter and if > > it's disabled we lose ackknowledge handling (as general rule), we > > can't compensate by doing any additional filtering by software in this > > mode. > > Yes. > > > > > > > I am sorry, but I somehow miss the picture of how those things work > > > > together. It is not clear for me and I miss those parts to get a whole > > > > picture of this. For me it's not clear that those patches are going in > > > > this direction. > > > > > > Please tell me if it's more clear and if you agree with this vision. I > > > don't have time to draft something this week. > > > > > > > That's fine. We should agree on things like compensate lower filter > > levels by doing additional softmac filtering to reach > > IEEE802154_FILTERING_4_FRAME_FIELDS filtering from others because we > > will lose AACK handling. It is only fine to do that in mac802154 > > receive path but don't deliver it to the upper layer. > > So actually, if I try to summarize the situation. > > I've tried to make several different subif working on a single PHY. > Unfortunately, today there is only one address filter per PHY, so correct. But you are assuming hardware which is currently around. As I said atusb is one candidate to make a more clever hardware because it has a co-processor which eventually could handle ack handling if done right. > disabling the address filter on one interface would also disable it on > the other, leading to the ACKs not being handled anymore, which we > cannot afford. > For hardware which has such limitation of one address filter yes. > So overall I guess we should settle on how we want to handle the > situation. I propose, to move forward, that we continue to assume that > it is *not* possible to have several different interfaces running at the > same time on a single PHY. This involves dropping all the "software > address filtering" which I proposed, but I guess that's fine. I agree here, but don't remove code which could add such handling and allow multiple monitors at the same time. - Alex