| Message ID | 20211222155743.256280-13-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | IEEE 802.15.4 passive scan support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
Hi, On Wed, 22 Dec 2021 at 10:58, Miquel Raynal <miquel.raynal@bootlin.com> wrote: ... > +{ > + bool promiscuous_on = mac802154_check_promiscuous(local); > + int ret; > + > + if ((state && promiscuous_on) || (!state && !promiscuous_on)) > + return 0; > + > + ret = drv_set_promiscuous_mode(local, state); > + if (ret) > + pr_err("Failed to %s promiscuous mode for SW scanning", > + state ? "set" : "reset"); > + The semantic of promiscuous mode on the driver layer is to turn off ack response, address filtering and crc checking. Some transceivers don't allow a more fine tuning on what to enable/disable. I think we should at least do the checksum checking per software then? Sure there is a possible tune up for more "powerful" transceivers then... - Alex
Hi, * Alexander Aring <alex.aring@gmail.com> [2021-12-29 09:30]: > Hi, > On Wed, 22 Dec 2021 at 10:58, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > +{ > > + bool promiscuous_on = mac802154_check_promiscuous(local); > > + int ret; > > + > > + if ((state && promiscuous_on) || (!state && !promiscuous_on)) > > + return 0; > > + > > + ret = drv_set_promiscuous_mode(local, state); > > + if (ret) > > + pr_err("Failed to %s promiscuous mode for SW scanning", > > + state ? "set" : "reset"); > The semantic of promiscuous mode on the driver layer is to turn off > ack response, address filtering and crc checking. Some transceivers > don't allow a more fine tuning on what to enable/disable. I think we > should at least do the checksum checking per software then? > Sure there is a possible tune up for more "powerful" transceivers then... In this case, the driver could change the (flags & IEEE802154_HW_RX_DROP_BAD_CKSUM) bit dynamically to signal it does not check the checksum anymore. Would it work? Nicolas.
Hi, On Wed, 29 Dec 2021 at 09:45, Nicolas Schodet <nico@ni.fr.eu.org> wrote: > > Hi, > > * Alexander Aring <alex.aring@gmail.com> [2021-12-29 09:30]: > > Hi, > > On Wed, 22 Dec 2021 at 10:58, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ... > > > +{ > > > + bool promiscuous_on = mac802154_check_promiscuous(local); > > > + int ret; > > > + > > > + if ((state && promiscuous_on) || (!state && !promiscuous_on)) > > > + return 0; > > > + > > > + ret = drv_set_promiscuous_mode(local, state); > > > + if (ret) > > > + pr_err("Failed to %s promiscuous mode for SW scanning", > > > + state ? "set" : "reset"); > > The semantic of promiscuous mode on the driver layer is to turn off > > ack response, address filtering and crc checking. Some transceivers > > don't allow a more fine tuning on what to enable/disable. I think we > > should at least do the checksum checking per software then? > > Sure there is a possible tune up for more "powerful" transceivers then... > > In this case, the driver could change the (flags & > IEEE802154_HW_RX_DROP_BAD_CKSUM) bit dynamically to signal it does not > check the checksum anymore. Would it work? I think that would work, although the intention of the hw->flags is to define what the hardware is supposed to support as not changing those values dynamically during runtime so mac will care about it. However we don't expose those flags to the userspace, so far I know. We can still introduce two separated flags if necessary in future. Why do we need promiscuous mode at all? Why is it necessary for a scan? What of "ack response, address filtering and crc checking" you want to disable and why? I see that beacons are sent out with "local->beacon.mhr.fc.dest_addr_mode = IEEE802154_NO_ADDRESSING;" shouldn't that be a broadcast destination? - Alex
Hi, On Thu, 30 Dec 2021 at 14:47, Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Wed, 29 Dec 2021 at 09:45, Nicolas Schodet <nico@ni.fr.eu.org> wrote: > > > > Hi, > > > > * Alexander Aring <alex.aring@gmail.com> [2021-12-29 09:30]: > > > Hi, > > > On Wed, 22 Dec 2021 at 10:58, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > ... > > > > +{ > > > > + bool promiscuous_on = mac802154_check_promiscuous(local); > > > > + int ret; > > > > + > > > > + if ((state && promiscuous_on) || (!state && !promiscuous_on)) > > > > + return 0; > > > > + > > > > + ret = drv_set_promiscuous_mode(local, state); > > > > + if (ret) > > > > + pr_err("Failed to %s promiscuous mode for SW scanning", > > > > + state ? "set" : "reset"); > > > The semantic of promiscuous mode on the driver layer is to turn off > > > ack response, address filtering and crc checking. Some transceivers > > > don't allow a more fine tuning on what to enable/disable. I think we > > > should at least do the checksum checking per software then? > > > Sure there is a possible tune up for more "powerful" transceivers then... > > > > In this case, the driver could change the (flags & > > IEEE802154_HW_RX_DROP_BAD_CKSUM) bit dynamically to signal it does not > > check the checksum anymore. Would it work? > > I think that would work, although the intention of the hw->flags is to > define what the hardware is supposed to support as not changing those > values dynamically during runtime so mac will care about it. However > we don't expose those flags to the userspace, so far I know. We can > still introduce two separated flags if necessary in future. > > Why do we need promiscuous mode at all? Why is it necessary for a > scan? What of "ack response, address filtering and crc checking" you > want to disable and why? > I see now why promiscuous mode is necessary here. The actual promiscuous mode setting for the driver is not the same as promiscuous mode in 802.15.4 spec. For until now it was there for running a sniffer device only. As the 802.15.4 spec defines some "filtering levels" I came up with a draft so we can define which filtering level should be done on the hardware. diff --git a/include/net/mac802154.h b/include/net/mac802154.h index 72978fb72a3a..3839ed3f8f0d 100644 --- a/include/net/mac802154.h +++ b/include/net/mac802154.h @@ -130,6 +130,48 @@ enum ieee802154_hw_flags { #define IEEE802154_HW_OMIT_CKSUM (IEEE802154_HW_TX_OMIT_CKSUM | \ IEEE802154_HW_RX_OMIT_CKSUM) +/** + * enum ieee802154_filter_mode - hardware filter mode that a driver will pass to + * pass to mac802154. + * + * @IEEE802154_FILTER_MODE_0: No MFR filtering at all. + * + * @IEEE802154_FILTER_MODE_1: IEEE802154_FILTER_MODE_1 with a bad FCS filter. + * + * @IEEE802154_FILTER_MODE_2: Same as IEEE802154_FILTER_MODE_1, known as + * 802.15.4 promiscuous mode, sets + * mib.PromiscuousMode. + * + * @IEEE802154_FILTER_MODE_3_SCAN: Same as IEEE802154_FILTER_MODE_2 without + * set mib.PromiscuousMode. + * + * @IEEE802154_FILTER_MODE_3_NO_SCAN: + * IEEE802154_FILTER_MODE_3_SCAN with MFR additional filter on: + * + * - No reserved value in frame type + * - No reserved value in frame version + * - Match mib.PanId or broadcast + * - Destination address field: + * - Match mib.ShortAddress or broadcast + * - Match mib.ExtendedAddress or GroupRxMode is true + * - ImplicitBroadcast is true and destination address field/destination + * panid is not included. + * - Device is coordinator only source address present in data + * frame/command frame and source panid matches mib.PanId + * - Device is coordinator only source address present in multipurpose + * frame and destination panid matches macPanId + * - Beacon frames source panid matches mib.PanId. If mib.PanId is + * broadcast it should always be accepted. + * + */ +enum ieee802154_filter_mode { + IEEE802154_FILTER_MODE_0, + IEEE802154_FILTER_MODE_1, + IEEE802154_FILTER_MODE_2, + IEEE802154_FILTER_MODE_3_SCAN, + IEEE802154_FILTER_MODE_3_NO_SCAN, +}; + /* struct ieee802154_ops - callbacks from mac802154 to the driver * * This structure contains various callbacks that the driver may @@ -249,7 +291,7 @@ struct ieee802154_ops { int (*set_frame_retries)(struct ieee802154_hw *hw, s8 retries); int (*set_promiscuous_mode)(struct ieee802154_hw *hw, - const bool on); + enum ieee802154_filter_mode mode); int (*enter_scan_mode)(struct ieee802154_hw *hw, struct cfg802154_scan_request *request); int (*exit_scan_mode)(struct ieee802154_hw *hw); --- In your case it will be IEEE802154_FILTER_MODE_3_SCAN mode, for a sniffer we probably want as default IEEE802154_FILTER_MODE_0, as "promiscuous mode" currently is. - Alex
Hi Alexander, > I see that beacons are sent out with > "local->beacon.mhr.fc.dest_addr_mode = IEEE802154_NO_ADDRESSING;" > shouldn't that be a broadcast destination? In the case of a beacon, 7.3.1.2 Beacon frame MHR field indicate: When the Frame Version field is 0b00–0b01: — The Destination Addressing mode field shall be set to indicated that the Destination Address and Destination PAN ID fields are not present. So I think the NO_ADDRESSING choice here is legit. The destination field however is useful in the Enhanced beacon frame case, but that's not yet supported. Thanks, Miquèl
Hi Alexander, alex.aring@gmail.com wrote on Fri, 31 Dec 2021 14:27:12 -0500: > Hi, > > On Thu, 30 Dec 2021 at 14:47, Alexander Aring <alex.aring@gmail.com> wrote: > > > > Hi, > > > > On Wed, 29 Dec 2021 at 09:45, Nicolas Schodet <nico@ni.fr.eu.org> wrote: > > > > > > Hi, > > > > > > * Alexander Aring <alex.aring@gmail.com> [2021-12-29 09:30]: > > > > Hi, > > > > On Wed, 22 Dec 2021 at 10:58, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > ... > > > > > +{ > > > > > + bool promiscuous_on = mac802154_check_promiscuous(local); > > > > > + int ret; > > > > > + > > > > > + if ((state && promiscuous_on) || (!state && !promiscuous_on)) > > > > > + return 0; > > > > > + > > > > > + ret = drv_set_promiscuous_mode(local, state); > > > > > + if (ret) > > > > > + pr_err("Failed to %s promiscuous mode for SW scanning", > > > > > + state ? "set" : "reset"); > > > > The semantic of promiscuous mode on the driver layer is to turn off > > > > ack response, address filtering and crc checking. Some transceivers > > > > don't allow a more fine tuning on what to enable/disable. I think we > > > > should at least do the checksum checking per software then? > > > > Sure there is a possible tune up for more "powerful" transceivers then... > > > > > > In this case, the driver could change the (flags & > > > IEEE802154_HW_RX_DROP_BAD_CKSUM) bit dynamically to signal it does not > > > check the checksum anymore. Would it work? > > > > I think that would work, although the intention of the hw->flags is to > > define what the hardware is supposed to support as not changing those > > values dynamically during runtime so mac will care about it. However > > we don't expose those flags to the userspace, so far I know. We can > > still introduce two separated flags if necessary in future. > > > > Why do we need promiscuous mode at all? Why is it necessary for a > > scan? What of "ack response, address filtering and crc checking" you > > want to disable and why? > > > > I see now why promiscuous mode is necessary here. The actual > promiscuous mode setting for the driver is not the same as promiscuous > mode in 802.15.4 spec. For until now it was there for running a > sniffer device only. > As the 802.15.4 spec defines some "filtering levels" I came up with a > draft so we can define which filtering level should be done on the > hardware. I like the idea but I'm not sure on what side you want to tackle the problem first. Is it the phy drivers which should advertise the mac about the promiscuous mode they support (which matches the description below but does not fit the purpose of an enum very well)? Or is it the MAC that requests a particular filtering mode? In this case what a phy driver should do if: - the requested mode is more constrained than its usual promiscuous capabilities? - the requested mode is less constrained than its usual promiscuous capabilities? > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h > index 72978fb72a3a..3839ed3f8f0d 100644 > --- a/include/net/mac802154.h > +++ b/include/net/mac802154.h > @@ -130,6 +130,48 @@ enum ieee802154_hw_flags { > #define IEEE802154_HW_OMIT_CKSUM (IEEE802154_HW_TX_OMIT_CKSUM | \ > IEEE802154_HW_RX_OMIT_CKSUM) > > +/** > + * enum ieee802154_filter_mode - hardware filter mode that a driver > will pass to > + * pass to mac802154. Isn't it the opposite: The filtering level the mac is requesting? Here it looks like we are describing driver capabilities (ie what drivers advertise supporting). > + * > + * @IEEE802154_FILTER_MODE_0: No MFR filtering at all. I suppose this would be for a sniffer accepting all frames, including the bad ones. > + * > + * @IEEE802154_FILTER_MODE_1: IEEE802154_FILTER_MODE_1 with a bad FCS filter. This means that the driver should only discard bad frames and propagate all the remaining frames, right? So this typically is a regular sniffer mode. > + * > + * @IEEE802154_FILTER_MODE_2: Same as IEEE802154_FILTER_MODE_1, known as > + * 802.15.4 promiscuous mode, sets > + * mib.PromiscuousMode. I believe what you call mib.PromiscuousMode is the mode that is referred in the spec, ie. being in the official promiscuous mode? So that is the mode that should be used "by default" when really asking for a 802154 promiscuous mode. Is there really a need for a different mode than mode_1 ? > + * > + * @IEEE802154_FILTER_MODE_3_SCAN: Same as IEEE802154_FILTER_MODE_2 without > + * set mib.PromiscuousMode. And here what is the difference between MODE_1 and MODE_3 ? I suppose here we should as well drop all non-beacon frames? > + * > + * @IEEE802154_FILTER_MODE_3_NO_SCAN: > + * IEEE802154_FILTER_MODE_3_SCAN with MFR additional filter on: > + * > + * - No reserved value in frame type > + * - No reserved value in frame version > + * - Match mib.PanId or broadcast > + * - Destination address field: > + * - Match mib.ShortAddress or broadcast > + * - Match mib.ExtendedAddress or GroupRxMode is true > + * - ImplicitBroadcast is true and destination address field/destination > + * panid is not included. > + * - Device is coordinator only source address present in data > + * frame/command frame and source panid matches mib.PanId > + * - Device is coordinator only source address present in multipurpose > + * frame and destination panid matches macPanId > + * - Beacon frames source panid matches mib.PanId. If mib.PanId is > + * broadcast it should always be accepted. This is a bit counter intuitive, but do we agree on the fact that the higher level of filtering should refer to promiscuous = false? > + * > + */ > +enum ieee802154_filter_mode { > + IEEE802154_FILTER_MODE_0, > + IEEE802154_FILTER_MODE_1, > + IEEE802154_FILTER_MODE_2, > + IEEE802154_FILTER_MODE_3_SCAN, > + IEEE802154_FILTER_MODE_3_NO_SCAN, > +}; > + > /* struct ieee802154_ops - callbacks from mac802154 to the driver > * > * This structure contains various callbacks that the driver may > @@ -249,7 +291,7 @@ struct ieee802154_ops { > int (*set_frame_retries)(struct ieee802154_hw *hw, > s8 retries); > int (*set_promiscuous_mode)(struct ieee802154_hw *hw, > - const bool on); > + enum > ieee802154_filter_mode mode); > int (*enter_scan_mode)(struct ieee802154_hw *hw, > struct > cfg802154_scan_request *request); > int (*exit_scan_mode)(struct ieee802154_hw *hw); > > --- > > In your case it will be IEEE802154_FILTER_MODE_3_SCAN mode, for a > sniffer we probably want as default IEEE802154_FILTER_MODE_0, as > "promiscuous mode" currently is. > > - Alex Thanks, Miquèl
Hi. On Tue, 4 Jan 2022 at 13:18, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > ... > > > > I see now why promiscuous mode is necessary here. The actual > > promiscuous mode setting for the driver is not the same as promiscuous > > mode in 802.15.4 spec. For until now it was there for running a > > sniffer device only. > > As the 802.15.4 spec defines some "filtering levels" I came up with a > > draft so we can define which filtering level should be done on the > > hardware. > > I like the idea but I'm not sure on what side you want to tackle the > problem first. Is it the phy drivers which should advertise the mac > about the promiscuous mode they support (which matches the description > below but does not fit the purpose of an enum very well)? Or is it the > MAC that requests a particular filtering mode? In this case what a phy > driver should do if: > - the requested mode is more constrained than its usual promiscuous > capabilities? Then, the driver needs to go one level lower and tell mac802154 to filter more out. > - the requested mode is less constrained than its usual promiscuous > capabilities? > Then mac802154 needs to filter more out. I am more worried at the point the transceiver will shut off automatic acknowledge handling which we probably can't do in software in cases where it's required. Some transceivers will shut that off if they turn off address filtering and if they don't have a detailed setting for that they will ack every frame what they see, which is... not so good. Future work would be to warn about mismatch of seeing frames, what the hardware would filter out vs what mac802154 sees. More further work could be to use a monitor interface and raw sockets and verify transceivers how they act to frames. > > > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h > > index 72978fb72a3a..3839ed3f8f0d 100644 > > --- a/include/net/mac802154.h > > +++ b/include/net/mac802154.h > > @@ -130,6 +130,48 @@ enum ieee802154_hw_flags { > > #define IEEE802154_HW_OMIT_CKSUM (IEEE802154_HW_TX_OMIT_CKSUM | \ > > IEEE802154_HW_RX_OMIT_CKSUM) > > > > +/** > > + * enum ieee802154_filter_mode - hardware filter mode that a driver > > will pass to > > + * pass to mac802154. > > Isn't it the opposite: The filtering level the mac is requesting? Here > it looks like we are describing driver capabilities (ie what drivers > advertise supporting). > I am sorry. I meant what the transceiver "should" deliver or "level less" to mac802154. I think the filtering when not much resources are required can also be done in a hardirq context. There exists a tasklet which is there to switch to a softirq context [0], currently we do all parsing there. > > + * > > + * @IEEE802154_FILTER_MODE_0: No MFR filtering at all. > > I suppose this would be for a sniffer accepting all frames, including > the bad ones. > yes. > > + * > > + * @IEEE802154_FILTER_MODE_1: IEEE802154_FILTER_MODE_1 with a bad FCS filter. > > This means that the driver should only discard bad frames and propagate > all the remaining frames, right? So this typically is a regular sniffer > mode. > I think this depends on what you want to filter out, so far I know in wireless this is configurable. Wireshark always expects the FCS in their payload for a linux 802.15.4 monitor interface and I think this is because of some historical reason to support the first 802.15.4 sniffers in wireshark. There is a difference between filter bad FCS and cutoff FCS. I need to look it up but I think wireless would cut off the checksum if FCS is filtered on hardware (may even some transceivers will not deliver FCS to you if you enable filtering). > > + * > > + * @IEEE802154_FILTER_MODE_2: Same as IEEE802154_FILTER_MODE_1, known as > > + * 802.15.4 promiscuous mode, sets > > + * mib.PromiscuousMode. > > I believe what you call mib.PromiscuousMode is the mode that is > referred in the spec, ie. being in the official promiscuous mode? So > that is the mode that should be used "by default" when really asking > for a 802154 promiscuous mode. > then we don't call it in driver level promiscuous mode, we call it "filtering level". And this is the filtering for cases when the standard says set "mib.PromiscuousMode". > Is there really a need for a different mode than mode_1 ? > I think so, I am not sure what they or will define if PromiscuousMode is set or not and might the transceiver need to get notice about it? It's not needed now, but we might keep it in mind then. > > + * > > + * @IEEE802154_FILTER_MODE_3_SCAN: Same as IEEE802154_FILTER_MODE_2 without > > + * set mib.PromiscuousMode. > > And here what is the difference between MODE_1 and MODE_3 ? > > I suppose here we should as well drop all non-beacon frames? Yes, additionally there could be a transceiver doing this filtering on hardware and tell that it's in scan and this is the difference. > > > + * > > + * @IEEE802154_FILTER_MODE_3_NO_SCAN: > > + * IEEE802154_FILTER_MODE_3_SCAN with MFR additional filter on: > > + * should be IEEE802154_FILTER_MODE_2. Maybe we can also get some better names for that but the standard describes it with numbers as well. > > + * - No reserved value in frame type > > + * - No reserved value in frame version > > + * - Match mib.PanId or broadcast > > + * - Destination address field: > > + * - Match mib.ShortAddress or broadcast > > + * - Match mib.ExtendedAddress or GroupRxMode is true > > + * - ImplicitBroadcast is true and destination address field/destination > > + * panid is not included. > > + * - Device is coordinator only source address present in data > > + * frame/command frame and source panid matches mib.PanId > > + * - Device is coordinator only source address present in multipurpose > > + * frame and destination panid matches macPanId > > + * - Beacon frames source panid matches mib.PanId. If mib.PanId is > > + * broadcast it should always be accepted. > > This is a bit counter intuitive, but do we agree on the fact that the > higher level of filtering should refer to promiscuous = false? > Yes, it's a lot of filter rules at this level. Yes, promiscuous is false in this case. That is what currently what wpan "node" interface should filter at mac802154 [1] (for cases device coordinator is false). I might mention a lot of future work here. I think we can live for now to make a difference between those levels and be sure that we drop everything else in the scan operation (inclusive check fcs in software). Moving stuff that we can do in hardware to hardware and the rest in software is a bigger task here... - Alex [0] https://elixir.bootlin.com/linux/v5.16-rc8/source/net/mac802154/rx.c#L294 [1] https://elixir.bootlin.com/linux/v5.16-rc8/source/net/mac802154/rx.c#L132
Hi, On Tue, 4 Jan 2022 at 12:43, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > > I see that beacons are sent out with > > "local->beacon.mhr.fc.dest_addr_mode = IEEE802154_NO_ADDRESSING;" > > shouldn't that be a broadcast destination? > > In the case of a beacon, 7.3.1.2 Beacon frame MHR field indicate: > > When the Frame Version field is 0b00–0b01: > — The Destination Addressing mode field shall be set to > indicated that the Destination Address and Destination > PAN ID fields are not present. > > So I think the NO_ADDRESSING choice here is legit. The destination > field however is useful in the Enhanced beacon frame case, but that's > not yet supported. ok, yes I agree. Thanks. - Alex
Hi Alexander, alex.aring@gmail.com wrote on Tue, 4 Jan 2022 20:16:30 -0500: > Hi. > > On Tue, 4 Jan 2022 at 13:18, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > ... > > > > > > I see now why promiscuous mode is necessary here. The actual > > > promiscuous mode setting for the driver is not the same as promiscuous > > > mode in 802.15.4 spec. For until now it was there for running a > > > sniffer device only. > > > As the 802.15.4 spec defines some "filtering levels" I came up with a > > > draft so we can define which filtering level should be done on the > > > hardware. > > > > I like the idea but I'm not sure on what side you want to tackle the > > problem first. Is it the phy drivers which should advertise the mac > > about the promiscuous mode they support (which matches the description > > below but does not fit the purpose of an enum very well)? Or is it the > > MAC that requests a particular filtering mode? In this case what a phy > > driver should do if: > > - the requested mode is more constrained than its usual promiscuous > > capabilities? > > Then, the driver needs to go one level lower and tell mac802154 to > filter more out. > > > - the requested mode is less constrained than its usual promiscuous > > capabilities? > > > > Then mac802154 needs to filter more out. > > I am more worried at the point the transceiver will shut off automatic > acknowledge handling which we probably can't do in software in cases > where it's required. Some transceivers will shut that off if they turn > off address filtering and if they don't have a detailed setting for > that they will ack every frame what they see, which is... not so good. > > Future work would be to warn about mismatch of seeing frames, what the > hardware would filter out vs what mac802154 sees. More further work > could be to use a monitor interface and raw sockets and verify > transceivers how they act to frames. > > > > > > > diff --git a/include/net/mac802154.h b/include/net/mac802154.h > > > index 72978fb72a3a..3839ed3f8f0d 100644 > > > --- a/include/net/mac802154.h > > > +++ b/include/net/mac802154.h > > > @@ -130,6 +130,48 @@ enum ieee802154_hw_flags { > > > #define IEEE802154_HW_OMIT_CKSUM (IEEE802154_HW_TX_OMIT_CKSUM | \ > > > IEEE802154_HW_RX_OMIT_CKSUM) > > > > > > +/** > > > + * enum ieee802154_filter_mode - hardware filter mode that a driver > > > will pass to > > > + * pass to mac802154. > > > > Isn't it the opposite: The filtering level the mac is requesting? Here > > it looks like we are describing driver capabilities (ie what drivers > > advertise supporting). > > > > I am sorry. I meant what the transceiver "should" deliver or "level > less" to mac802154. > > I think the filtering when not much resources are required can also be > done in a hardirq context. There exists a tasklet which is there to > switch to a softirq context [0], currently we do all parsing there. > > > > + * > > > + * @IEEE802154_FILTER_MODE_0: No MFR filtering at all. > > > > I suppose this would be for a sniffer accepting all frames, including > > the bad ones. > > > > yes. > > > > + * > > > + * @IEEE802154_FILTER_MODE_1: IEEE802154_FILTER_MODE_1 with a bad FCS filter. > > > > This means that the driver should only discard bad frames and propagate > > all the remaining frames, right? So this typically is a regular sniffer > > mode. > > > > I think this depends on what you want to filter out, so far I know in > wireless this is configurable. Wireshark always expects the FCS in > their payload for a linux 802.15.4 monitor interface and I think this > is because of some historical reason to support the first 802.15.4 > sniffers in wireshark. > There is a difference between filter bad FCS and cutoff FCS. I need to > look it up but I think wireless would cut off the checksum if FCS is > filtered on hardware (may even some transceivers will not deliver FCS > to you if you enable filtering). > > > > + * > > > + * @IEEE802154_FILTER_MODE_2: Same as IEEE802154_FILTER_MODE_1, known as > > > + * 802.15.4 promiscuous mode, sets > > > + * mib.PromiscuousMode. > > > > I believe what you call mib.PromiscuousMode is the mode that is > > referred in the spec, ie. being in the official promiscuous mode? So > > that is the mode that should be used "by default" when really asking > > for a 802154 promiscuous mode. > > > > then we don't call it in driver level promiscuous mode, we call it > "filtering level". And this is the filtering for cases when the > standard says set "mib.PromiscuousMode". > > > Is there really a need for a different mode than mode_1 ? > > > > I think so, I am not sure what they or will define if PromiscuousMode > is set or not and might the transceiver need to get notice about it? > It's not needed now, but we might keep it in mind then. > > > > + * > > > + * @IEEE802154_FILTER_MODE_3_SCAN: Same as IEEE802154_FILTER_MODE_2 without > > > + * set mib.PromiscuousMode. > > > > And here what is the difference between MODE_1 and MODE_3 ? > > > > I suppose here we should as well drop all non-beacon frames? > > Yes, additionally there could be a transceiver doing this filtering on > hardware and tell that it's in scan and this is the difference. > > > > > > + * > > > + * @IEEE802154_FILTER_MODE_3_NO_SCAN: > > > + * IEEE802154_FILTER_MODE_3_SCAN with MFR additional filter on: > > > + * > > should be IEEE802154_FILTER_MODE_2. Maybe we can also get some better > names for that but the standard describes it with numbers as well. > > > > + * - No reserved value in frame type > > > + * - No reserved value in frame version > > > + * - Match mib.PanId or broadcast > > > + * - Destination address field: > > > + * - Match mib.ShortAddress or broadcast > > > + * - Match mib.ExtendedAddress or GroupRxMode is true > > > + * - ImplicitBroadcast is true and destination address field/destination > > > + * panid is not included. > > > + * - Device is coordinator only source address present in data > > > + * frame/command frame and source panid matches mib.PanId > > > + * - Device is coordinator only source address present in multipurpose > > > + * frame and destination panid matches macPanId > > > + * - Beacon frames source panid matches mib.PanId. If mib.PanId is > > > + * broadcast it should always be accepted. > > > > This is a bit counter intuitive, but do we agree on the fact that the > > higher level of filtering should refer to promiscuous = false? > > > > Yes, it's a lot of filter rules at this level. > Yes, promiscuous is false in this case. That is what currently what > wpan "node" interface should filter at mac802154 [1] (for cases device > coordinator is false). > > I might mention a lot of future work here. I think we can live for now > to make a difference between those levels and be sure that we drop > everything else in the scan operation (inclusive check fcs in > software). Moving stuff that we can do in hardware to hardware and the > rest in software is a bigger task here... On the symbol duration side I feel I'm close to a working PoC. So there is 'only' this item left in my mind. Could you please clarify what you expect from me exactly in terms of support for the promiscuous filters we discussed so far? Also, just for the record, - should I keep copying the netdev list for v2? - should I monitor if net-next is open before sending or do you have your own set of rules? > [0] https://elixir.bootlin.com/linux/v5.16-rc8/source/net/mac802154/rx.c#L294 > [1] https://elixir.bootlin.com/linux/v5.16-rc8/source/net/mac802154/rx.c#L132 Thanks, Miquèl
Hi, On Wed, 5 Jan 2022 at 15:55, Miquel Raynal <miquel.raynal@bootlin.com> wrote: ... > > rest in software is a bigger task here... > > On the symbol duration side I feel I'm close to a working PoC. > oh, ok. > So there is 'only' this item left in my mind. Could you please clarify > what you expect from me exactly in terms of support for the promiscuous > filters we discussed so far? > I think for now it's okay to set the device into promiscuous mode and enable the flag which checks for bad FCS... we can still implement the filter modes later (and I think it should work on all supported transceivers (except that SoftMAC/HardMAC thing)). One point to promiscuous mode, currently we have a checking for if a phy is in promiscuous mode on ifup and it would forbid to ifup a node interface if the phy is in promiscuous mode (because of the missing automatic acknowledgement). I see there is a need to turn the phy into promiscuous mode during runtime... so we need somehow make sure the constraints are still valid here. Maybe we even forbid multiple devs on a phy if the transceiver/driver/firmware is poor and this is currently all transceivers (except hwsim? But that doesn't use any ack handling anyway). > Also, just for the record, > - should I keep copying the netdev list for v2? yes, why not. > - should I monitor if net-next is open before sending or do you have > your own set of rules? > I need to admit, Stefan is the "Thanks, applied." hero here and he should answer this question. - Alex
Hello. On 06.01.22 01:38, Alexander Aring wrote: > Hi, > > > On Wed, 5 Jan 2022 at 15:55, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... >> Also, just for the record, >> - should I keep copying the netdev list for v2? > > yes, why not. >> - should I monitor if net-next is open before sending or do you have >> your own set of rules? >> > > I need to admit, Stefan is the "Thanks, applied." hero here and he > should answer this question. No need to monitor if net-next is open for these patches (just don't add a net-next patch subject prefix as this would confuse Jakub and Dave. wpan-next would be more appropriate). I am following this patchset and the review from Alex. I have not done a full in depth review myself yet, its on my list. Basically keep working with Alex and use the wpan-next prefix and I will pick up the patches to my wpan-next tree and sent a pull to net-next when we are happy with it. Does that sound good to you? regards Stefan Schmidt
Hi Stefan, stefan@datenfreihafen.org wrote on Thu, 6 Jan 2022 09:44:50 +0100: > Hello. > > On 06.01.22 01:38, Alexander Aring wrote: > > Hi, > > > > > > On Wed, 5 Jan 2022 at 15:55, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ... > > >> Also, just for the record, > >> - should I keep copying the netdev list for v2? > > > > yes, why not. > > >> - should I monitor if net-next is open before sending or do you have > >> your own set of rules? > >> > > > > I need to admit, Stefan is the "Thanks, applied." hero here and he > > should answer this question. > > No need to monitor if net-next is open for these patches (just don't add a net-next patch subject prefix as this would confuse Jakub and Dave. wpan-next would be more appropriate). Sure! It might be worth updating [1] to tell people about this prefix? (only the userspace tools prefix is mentioned). [1] https://linux-wpan.org/contributing.html > I am following this patchset and the review from Alex. I have not done a full in depth review myself yet, its on my list. Yeah sure, no problem. > Basically keep working with Alex and use the wpan-next prefix and I will pick up the patches to my wpan-next tree and sent a pull to net-next when we are happy with it. Does that sound good to you? Yes of course, that's ideal. Thanks, Miquèl
Hi Alexander, alex.aring@gmail.com wrote on Wed, 5 Jan 2022 19:38:12 -0500: > Hi, > > > On Wed, 5 Jan 2022 at 15:55, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > > rest in software is a bigger task here... > > > > On the symbol duration side I feel I'm close to a working PoC. > > > > oh, ok. I think it's ready, I'll soon send two series: - the symbol duration update - v2 for this series, which will not apply without the symbol duration update. > > So there is 'only' this item left in my mind. Could you please clarify > > what you expect from me exactly in terms of support for the promiscuous > > filters we discussed so far? > > > > I think for now it's okay to set the device into promiscuous mode and > enable the flag which checks for bad FCS... we can still implement the > filter modes later (and I think it should work on all supported > transceivers (except that SoftMAC/HardMAC thing)). I considered the following options in order to do that: 1- Hack all ->set_promiscuous() driver implementations to set IEEE802154_HW_RX_DROP_BAD_CKSUM as long as it was not already set initially. 2- Set the above flag at scan level, ie. in scan.c:mac802154_set_promiscuous_mode(). But this would be a bit ugly and I'd need to add a persistent field somewhere in the wpan_dev structure to remember how the flags settings where before the scan code hacked it. 3- Add more code in hwsim to handle checksum manually instead of by default setting the above flag to request the core to do the job. This way no driver would actually set this flag. We can then consider it "volatile" and would not need to track its state. 4- We know that we are in a scan thanks to a mac802154 internal variable, we can just assume that all drivers are in promiscuous mode and that none of them actually checks the FCS. This is certainly the simplest yet effective solution. In the worst case, we are just doing the check twice, which I believe does not hurt as long as the checksum is not cut off. If the checksum is cut, then the core is buggy because it always remove the two last bytes. I picked 4 for now, but if you think this is unreliable, please tell me what do you prefer otherwise. > One point to promiscuous mode, currently we have a checking for if a > phy is in promiscuous mode on ifup and it would forbid to ifup a node > interface if the phy is in promiscuous mode (because of the missing > automatic acknowledgement). I see there is a need to turn the phy into > promiscuous mode during runtime... so we need somehow make sure the > constraints are still valid here. Yes, the code (rx.c) currently drops everything that is not a beacon during a scan. > Maybe we even forbid multiple devs > on a phy if the transceiver/driver/firmware is poor and this is > currently all transceivers (except hwsim? But that doesn't use any ack > handling anyway). Thanks, Miquèl
On Wed, 5 Jan 2022 19:38:12 -0500 Alexander Aring wrote: > > Also, just for the record, > > - should I keep copying the netdev list for v2? > > yes, why not. On the question of lists copied it may make sense to CC linux-wireless@ in case they have some precedent to share, and drop linux-kernel@.
Hi, On Thu, 6 Jan 2022 at 14:15, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Wed, 5 Jan 2022 19:38:12 -0500: > > > Hi, > > > > > > On Wed, 5 Jan 2022 at 15:55, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ... > > > > rest in software is a bigger task here... > > > > > > On the symbol duration side I feel I'm close to a working PoC. > > > > > > > oh, ok. > > I think it's ready, I'll soon send two series: > - the symbol duration update > - v2 for this series, which will not apply without the symbol duration > update. > ok. Thanks. > > > So there is 'only' this item left in my mind. Could you please clarify > > > what you expect from me exactly in terms of support for the promiscuous > > > filters we discussed so far? > > > > > > > I think for now it's okay to set the device into promiscuous mode and > > enable the flag which checks for bad FCS... we can still implement the > > filter modes later (and I think it should work on all supported > > transceivers (except that SoftMAC/HardMAC thing)). > > I considered the following options in order to do that: > 1- Hack all ->set_promiscuous() driver implementations to set > IEEE802154_HW_RX_DROP_BAD_CKSUM as long as it was not already set > initially. > 2- Set the above flag at scan level, ie. in > scan.c:mac802154_set_promiscuous_mode(). But this would be a bit > ugly and I'd need to add a persistent field somewhere in the > wpan_dev structure to remember how the flags settings where before > the scan code hacked it. I think there exists two layers of "promiscuous mode": there exists a phy level and a mac level. I am not sure at some points what's meant now. Whereas phy is regarding the filtering mode whatever will be delivered to mac802154, the wpan (mac) level is what 802.15.4 mac says it is. The mac promiscuous mode requires the phy promiscuous mode (so far I understand). > 3- Add more code in hwsim to handle checksum manually instead of > by default setting the above flag to request the core to do the > job. This way no driver would actually set this flag. We can then > consider it "volatile" and would not need to track its state. > 4- We know that we are in a scan thanks to a mac802154 internal > variable, we can just assume that all drivers are in promiscuous > mode and that none of them actually checks the FCS. This is > certainly the simplest yet effective solution. In the worst case, we > are just doing the check twice, which I believe does not hurt as > long as the checksum is not cut off. If the checksum is cut, then > the core is buggy because it always remove the two last bytes. > > I picked 4 for now, but if you think this is unreliable, please > tell me what do you prefer otherwise. > I think we have some flag to add a calculated checksum "IEEE802154_HW_RX_OMIT_CKSUM" which is currently not used by any driver. I think your case that the checksum is cut off does not exist in 4.? So far I understand we can still move the FCS check to the hardware by not breaking anything if the hardware supports it and the behavior should be the same. So do the 4.? > > One point to promiscuous mode, currently we have a checking for if a > > phy is in promiscuous mode on ifup and it would forbid to ifup a node > > interface if the phy is in promiscuous mode (because of the missing > > automatic acknowledgement). I see there is a need to turn the phy into > > promiscuous mode during runtime... so we need somehow make sure the > > constraints are still valid here. > > Yes, the code (rx.c) currently drops everything that is not a beacon > during a scan. > Okay, I will look at this code closely regarding whenever multiple wpan_devs are running. You should also check for possible stop of all possible wpan dev transmit queues, if it's not already done. I suppose a scan can take a long time and we should not send some data frames out. I am thinking about the long time scan operation... if we stop the queue for a long time I think we will drop a lot, however the scan can only be triggered by the right permissions and the user should be aware of the side effects. Proper reliable upper layer protocols will care or non reliable will not care about this. There still exists the driver "ca8210" which is the mentioned HardMAC transceiver in SoftMAC. There should somehow be a flag that it cannot do a scan and the operation should not be allowed as the xmit callback allows dataframes only. - Alex
Hi, On Thu, 6 Jan 2022 at 20:00, Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 5 Jan 2022 19:38:12 -0500 Alexander Aring wrote: > > > Also, just for the record, > > > - should I keep copying the netdev list for v2? > > > > yes, why not. > > On the question of lists copied it may make sense to CC linux-wireless@ > in case they have some precedent to share, and drop linux-kernel@. Yes, that makes sense. - Alex
Hi Alexander, alex.aring@gmail.com wrote on Thu, 6 Jan 2022 20:07:24 -0500: > Hi, > > On Thu, 6 Jan 2022 at 14:15, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > alex.aring@gmail.com wrote on Wed, 5 Jan 2022 19:38:12 -0500: > > > > > Hi, > > > > > > > > > On Wed, 5 Jan 2022 at 15:55, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > ... > > > > > rest in software is a bigger task here... > > > > > > > > On the symbol duration side I feel I'm close to a working PoC. > > > > > > > > > > oh, ok. > > > > I think it's ready, I'll soon send two series: > > - the symbol duration update > > - v2 for this series, which will not apply without the symbol duration > > update. > > > > ok. Thanks. > > > > > So there is 'only' this item left in my mind. Could you please clarify > > > > what you expect from me exactly in terms of support for the promiscuous > > > > filters we discussed so far? > > > > > > > > > > I think for now it's okay to set the device into promiscuous mode and > > > enable the flag which checks for bad FCS... we can still implement the > > > filter modes later (and I think it should work on all supported > > > transceivers (except that SoftMAC/HardMAC thing)). > > > > I considered the following options in order to do that: > > 1- Hack all ->set_promiscuous() driver implementations to set > > IEEE802154_HW_RX_DROP_BAD_CKSUM as long as it was not already set > > initially. > > 2- Set the above flag at scan level, ie. in > > scan.c:mac802154_set_promiscuous_mode(). But this would be a bit > > ugly and I'd need to add a persistent field somewhere in the > > wpan_dev structure to remember how the flags settings where before > > the scan code hacked it. > > I think there exists two layers of "promiscuous mode": there exists a > phy level and a mac level. I am not sure at some points what's meant > now. Whereas phy is regarding the filtering mode whatever will be > delivered to mac802154, the wpan (mac) level is what 802.15.4 mac says > it is. The mac promiscuous mode requires the phy promiscuous mode (so > far I understand). > > > 3- Add more code in hwsim to handle checksum manually instead of > > by default setting the above flag to request the core to do the > > job. This way no driver would actually set this flag. We can then > > consider it "volatile" and would not need to track its state. > > 4- We know that we are in a scan thanks to a mac802154 internal > > variable, we can just assume that all drivers are in promiscuous > > mode and that none of them actually checks the FCS. This is > > certainly the simplest yet effective solution. In the worst case, we > > are just doing the check twice, which I believe does not hurt as > > long as the checksum is not cut off. If the checksum is cut, then > > the core is buggy because it always remove the two last bytes. > > > > I picked 4 for now, but if you think this is unreliable, please > > tell me what do you prefer otherwise. > > > > I think we have some flag to add a calculated checksum > "IEEE802154_HW_RX_OMIT_CKSUM" which is currently not used by any > driver. I think your case that the checksum is cut off does not exist > in 4.? So far I understand we can still move the FCS check to the > hardware by not breaking anything if the hardware supports it and the > behavior should be the same. That is correct. > So do the 4.? Done, thanks! > > > One point to promiscuous mode, currently we have a checking for if a > > > phy is in promiscuous mode on ifup and it would forbid to ifup a node > > > interface if the phy is in promiscuous mode (because of the missing > > > automatic acknowledgement). I see there is a need to turn the phy into > > > promiscuous mode during runtime... so we need somehow make sure the > > > constraints are still valid here. > > > > Yes, the code (rx.c) currently drops everything that is not a beacon > > during a scan. > > > > Okay, I will look at this code closely regarding whenever multiple > wpan_devs are running. The "scanning" boolean is stored as a wpan_phy member (IIRC) so we should be good on this regard (now that I have a clearer picture of the dependencies). > You should also check for possible stop of all possible wpan dev > transmit queues, if it's not already done. I forgot about this path. Indeed I'll add a check in the transmit path as well, of course. > I suppose a scan can take a > long time and we should not send some data frames out. I am thinking > about the long time scan operation... if we stop the queue for a long > time I think we will drop a lot, however the scan can only be > triggered by the right permissions and the user should be aware of the > side effects. Proper reliable upper layer protocols will care or non > reliable will not care about this. > > There still exists the driver "ca8210" which is the mentioned HardMAC > transceiver in SoftMAC. There should somehow be a flag that it cannot > do a scan and the operation should not be allowed as the xmit callback > allows dataframes only. So it cannot do an active scan, but a passive scan would be allowed (there is no transmission, and the beacons are regular valid frames, I suppose they should not be filtered out by the hardware). So we actually need these hooks back :-) Because the right thing to do here is to use the "FYI here is the scan op that is starting" message from the mac to the drivers and this driver should return "nope, -ENOTSUPP". The mac would react in this case by canceling the operation and returning an error to the caller. Same when sending beacons if we consider beacons as !dataframes. Thanks, Miquèl
Hi, On Fri, 7 Jan 2022 at 06:02, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > > > > > > Okay, I will look at this code closely regarding whenever multiple > > wpan_devs are running. > > The "scanning" boolean is stored as a wpan_phy member (IIRC) so we > should be good on this regard (now that I have a clearer picture of the > dependencies). > ok. > > You should also check for possible stop of all possible wpan dev > > transmit queues, if it's not already done. > > I forgot about this path. Indeed I'll add a check in the transmit path > as well, of course. > What I mean is look into the functions "ieee802154_stop_queue()" and "ieee802154_wake_queue()". > > I suppose a scan can take a > > long time and we should not send some data frames out. I am thinking > > about the long time scan operation... if we stop the queue for a long > > time I think we will drop a lot, however the scan can only be > > triggered by the right permissions and the user should be aware of the > > side effects. Proper reliable upper layer protocols will care or non > > reliable will not care about this. > > > > There still exists the driver "ca8210" which is the mentioned HardMAC > > transceiver in SoftMAC. There should somehow be a flag that it cannot > > do a scan and the operation should not be allowed as the xmit callback > > allows dataframes only. > > So it cannot do an active scan, but a passive scan would be allowed > (there is no transmission, and the beacons are regular valid frames, > I suppose they should not be filtered out by the hardware). > > So we actually need these hooks back :-) Because the right thing to do > here is to use the "FYI here is the scan op that is starting" message > from the mac to the drivers and this driver should return "nope, > -ENOTSUPP". The mac would react in this case by canceling the > operation and returning an error to the caller. Same when sending > beacons if we consider beacons as !dataframes. > I believe that a HardMAC transceiver will handle a lot of the scan process on the transceiver side and is only capable of dumping what it's stored and start/stop scan? This is one reason why the scan/dump/etc cfg802154 interface should be close to the standard (MLME). At the end it should from userspace make no difference if at the end is a HardMAC or SoftMAC. Although there will always be a limitation and a SoftMAC is probably always "more" powerful than a HardMAC, but at some point there is a "minimum" of capable functionality... and SoftMAC transceivers might always have "some" things which they might offload to the hardware. - Alex
diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h index a9b09a9b8f70..60b09ff65d3d 100644 --- a/include/linux/ieee802154.h +++ b/include/linux/ieee802154.h @@ -46,6 +46,10 @@ /* Duration in superframe order */ #define IEEE802154_MAX_SCAN_DURATION 14 +/* Superframe duration in slots */ +#define IEEE802154_SUPERFRAME_PERIOD 16 +/* Various periods expressed in symbols */ +#define IEEE802154_SLOT_PERIOD 60 #define IEEE802154_LIFS_PERIOD 40 #define IEEE802154_SIFS_PERIOD 12 #define IEEE802154_MAX_SIFS_FRAME_SIZE 18 diff --git a/include/net/mac802154.h b/include/net/mac802154.h index d524ffb9eb25..19bfbf591ea1 100644 --- a/include/net/mac802154.h +++ b/include/net/mac802154.h @@ -486,4 +486,18 @@ void ieee802154_stop_queue(struct ieee802154_hw *hw); void ieee802154_xmit_complete(struct ieee802154_hw *hw, struct sk_buff *skb, bool ifs_handling); +/** + * ieee802154_queue_delayed_work - add work onto the mac802154 workqueue + * + * Drivers and mac802154 use this to queue delayed work onto the mac802154 + * workqueue. + * + * @hw: the hardware struct for the interface we are adding work for + * @dwork: delayable work to queue onto the mac802154 workqueue + * @delay: number of jiffies to wait before queueing + */ +void ieee802154_queue_delayed_work(struct ieee802154_hw *hw, + struct delayed_work *dwork, + unsigned long delay); + #endif /* NET_MAC802154_H */ diff --git a/net/mac802154/Makefile b/net/mac802154/Makefile index 4059295fdbf8..43d1347b37ee 100644 --- a/net/mac802154/Makefile +++ b/net/mac802154/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_MAC802154) += mac802154.o mac802154-objs := main.o rx.o tx.o mac_cmd.o mib.o \ - iface.o llsec.o util.o cfg.o trace.o + iface.o llsec.o util.o cfg.o scan.o trace.o CFLAGS_trace.o := -I$(src) diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c index fbeebe3bc31d..5c19d6f8e3eb 100644 --- a/net/mac802154/cfg.c +++ b/net/mac802154/cfg.c @@ -114,6 +114,10 @@ ieee802154_set_channel(struct wpan_phy *wpan_phy, u8 page, u8 channel) wpan_phy->current_channel == channel) return 0; + /* Refuse to change channels during a scanning operation */ + if (local->scanning) + return -EBUSY; + ret = drv_set_channel(local, page, channel); if (!ret) { wpan_phy->current_page = page; @@ -260,6 +264,39 @@ ieee802154_set_ackreq_default(struct wpan_phy *wpan_phy, return 0; } +static int mac802154_trigger_scan(struct wpan_phy *wpan_phy, + struct cfg802154_scan_request *req) +{ + struct ieee802154_local *local = wpan_phy_priv(wpan_phy); + struct ieee802154_sub_if_data *sdata; + int ret; + + sdata = IEEE802154_WPAN_DEV_TO_SUB_IF(req->wpan_dev); + + ASSERT_RTNL(); + + mutex_lock(&local->scan_lock); + ret = mac802154_trigger_scan_locked(sdata, req); + mutex_unlock(&local->scan_lock); + + return ret; +} + +static int mac802154_abort_scan(struct wpan_phy *wpan_phy, + struct wpan_dev *wpan_dev) +{ + struct ieee802154_local *local = wpan_phy_priv(wpan_phy); + int ret; + + ASSERT_RTNL(); + + mutex_lock(&local->scan_lock); + ret = mac802154_abort_scan_locked(local); + mutex_unlock(&local->scan_lock); + + return ret; +} + #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL static void ieee802154_get_llsec_table(struct wpan_phy *wpan_phy, @@ -467,6 +504,8 @@ const struct cfg802154_ops mac802154_config_ops = { .set_max_frame_retries = ieee802154_set_max_frame_retries, .set_lbt_mode = ieee802154_set_lbt_mode, .set_ackreq_default = ieee802154_set_ackreq_default, + .trigger_scan = mac802154_trigger_scan, + .abort_scan = mac802154_abort_scan, #ifdef CONFIG_IEEE802154_NL802154_EXPERIMENTAL .get_llsec_table = ieee802154_get_llsec_table, .lock_llsec_table = ieee802154_lock_llsec_table, diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h index 702560acc8ce..4945edf5c2ce 100644 --- a/net/mac802154/ieee802154_i.h +++ b/net/mac802154/ieee802154_i.h @@ -48,6 +48,15 @@ struct ieee802154_local { struct hrtimer ifs_timer; + /* Scanning */ + struct mutex scan_lock; + unsigned long scanning; + __le64 scan_addr; + int scan_channel_idx; + struct cfg802154_scan_request __rcu *scan_req; + struct ieee802154_sub_if_data __rcu *scan_sdata; + struct delayed_work scan_work; + bool started; bool suspended; @@ -166,6 +175,12 @@ void mac802154_unlock_table(struct net_device *dev); int mac802154_wpan_update_llsec(struct net_device *dev); +/* scanning handling */ +void mac802154_scan_work(struct work_struct *work); +int mac802154_trigger_scan_locked(struct ieee802154_sub_if_data *sdata, + struct cfg802154_scan_request *request); +int mac802154_abort_scan_locked(struct ieee802154_local *local); +int mac802154_scan_rx(struct ieee802154_local *local, struct sk_buff *skb); /* interface handling */ int ieee802154_iface_init(void); void ieee802154_iface_exit(void); diff --git a/net/mac802154/main.c b/net/mac802154/main.c index 520cedc594e1..568991734610 100644 --- a/net/mac802154/main.c +++ b/net/mac802154/main.c @@ -90,12 +90,14 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops) INIT_LIST_HEAD(&local->interfaces); mutex_init(&local->iflist_mtx); + mutex_init(&local->scan_lock); tasklet_setup(&local->tasklet, ieee802154_tasklet_handler); skb_queue_head_init(&local->skb_queue); INIT_WORK(&local->tx_work, ieee802154_xmit_worker); + INIT_DELAYED_WORK(&local->scan_work, mac802154_scan_work); /* init supported flags with 802.15.4 default ranges */ phy->supported.max_minbe = 8; diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c index b8ce84618a55..acbce67caedc 100644 --- a/net/mac802154/rx.c +++ b/net/mac802154/rx.c @@ -198,8 +198,13 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local, ret = ieee802154_parse_frame_start(skb, &hdr); if (ret) { pr_debug("got invalid frame\n"); - kfree_skb(skb); - return; + goto free_skb; + } + + if (unlikely(local->scanning)) { + if (mac_cb(skb)->type == IEEE802154_FC_TYPE_BEACON) + mac802154_scan_rx(local, skb); + goto free_skb; } list_for_each_entry_rcu(sdata, &local->interfaces, list) { @@ -214,6 +219,7 @@ __ieee802154_rx_handle_packet(struct ieee802154_local *local, break; } +free_skb: kfree_skb(skb); } diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c new file mode 100644 index 000000000000..c5b85eaec319 --- /dev/null +++ b/net/mac802154/scan.c @@ -0,0 +1,248 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * IEEE 802.15.4 scanning management + * + * Copyright (C) Qorvo, 2021 + * Authors: + * - David Girault <david.girault@qorvo.com> + * - Miquel Raynal <miquel.raynal@bootlin.com> + */ + +#include <linux/module.h> +#include <linux/random.h> +#include <linux/rtnetlink.h> +#include <net/mac802154.h> + +#include "ieee802154_i.h" +#include "driver-ops.h" +#include "../ieee802154/nl802154.h" + +static bool mac802154_check_promiscuous(struct ieee802154_local *local) +{ + struct ieee802154_sub_if_data *sdata; + bool promiscuous_on = false; + + /* Check if one subif is already in promiscuous mode. Since the list is + * protected by its own mutex, take it here to ensure no modification + * occurs during the check. + */ + mutex_lock(&local->iflist_mtx); + list_for_each_entry(sdata, &local->interfaces, list) { + if (ieee802154_sdata_running(sdata) && + sdata->wpan_dev.promiscuous_mode) { + /* At least one is in promiscuous mode */ + promiscuous_on = true; + break; + } + } + mutex_unlock(&local->iflist_mtx); + return promiscuous_on; +} + +static int mac802154_set_promiscuous_mode(struct ieee802154_local *local, + bool state) +{ + bool promiscuous_on = mac802154_check_promiscuous(local); + int ret; + + if ((state && promiscuous_on) || (!state && !promiscuous_on)) + return 0; + + ret = drv_set_promiscuous_mode(local, state); + if (ret) + pr_err("Failed to %s promiscuous mode for SW scanning", + state ? "set" : "reset"); + + return ret; +} + +static int mac802154_send_scan_done(struct ieee802154_local *local) +{ + struct cfg802154_registered_device *rdev; + struct cfg802154_scan_request *scan_req; + struct wpan_dev *wpan_dev; + + scan_req = rcu_dereference_protected(local->scan_req, + lockdep_is_held(&local->scan_lock)); + rdev = wpan_phy_to_rdev(scan_req->wpan_phy); + wpan_dev = scan_req->wpan_dev; + + return nl802154_send_scan_done(rdev, wpan_dev); +} + +static int mac802154_end_of_scan(struct ieee802154_local *local) +{ + drv_set_channel(local, local->phy->current_page, + local->phy->current_channel); + local->scanning = false; + mac802154_set_promiscuous_mode(local, false); + + return mac802154_send_scan_done(local); +} + +int mac802154_abort_scan_locked(struct ieee802154_local *local) +{ + lockdep_assert_held(&local->scan_lock); + + if (!local->scanning) + return -ESRCH; + + cancel_delayed_work(&local->scan_work); + + return mac802154_end_of_scan(local); +} + +static unsigned int mac802154_scan_get_channel_time(u8 duration_order, + u8 symbol_duration) +{ + u64 base_super_frame_duration = (u64)symbol_duration * + IEEE802154_SUPERFRAME_PERIOD * IEEE802154_SLOT_PERIOD; + + return usecs_to_jiffies(base_super_frame_duration * + (BIT(duration_order) + 1)); +} + +void mac802154_scan_work(struct work_struct *work) +{ + struct ieee802154_local *local = + container_of(work, struct ieee802154_local, scan_work.work); + struct cfg802154_scan_request *scan_req; + struct ieee802154_sub_if_data *sdata; + unsigned int scan_duration; + bool end_of_scan = false; + unsigned long chan; + int ret; + + mutex_lock(&local->scan_lock); + + if (!local->scanning) + goto unlock_mutex; + + sdata = rcu_dereference_protected(local->scan_sdata, + lockdep_is_held(&local->scan_lock)); + scan_req = rcu_dereference_protected(local->scan_req, + lockdep_is_held(&local->scan_lock)); + + if (local->suspended || !ieee802154_sdata_running(sdata)) + goto queue_work; + + do { + chan = find_next_bit((const unsigned long *)&scan_req->channels, + IEEE802154_MAX_CHANNEL + 1, + local->scan_channel_idx + 1); + + /* If there are no more channels left, complete the scan */ + if (chan > IEEE802154_MAX_CHANNEL) { + end_of_scan = true; + goto unlock_mutex; + } + + /* Channel switch cannot be made atomic so hide the chan number + * in order to prevent beacon processing during this timeframe. + */ + local->scan_channel_idx = -1; + /* Bypass the stack on purpose */ + ret = drv_set_channel(local, scan_req->page, chan); + local->scan_channel_idx = chan; + } while (ret); + +queue_work: + scan_duration = mac802154_scan_get_channel_time(scan_req->duration, + local->phy->symbol_duration); + pr_debug("Scan channel %lu of page %u for %ums\n", + chan, scan_req->page, jiffies_to_msecs(scan_duration)); + ieee802154_queue_delayed_work(&local->hw, &local->scan_work, + scan_duration); + +unlock_mutex: + if (end_of_scan) + mac802154_end_of_scan(local); + + mutex_unlock(&local->scan_lock); +} + +int mac802154_trigger_scan_locked(struct ieee802154_sub_if_data *sdata, + struct cfg802154_scan_request *request) +{ + struct ieee802154_local *local = sdata->local; + int ret; + + lockdep_assert_held(&local->scan_lock); + + if (local->scanning) + return -EBUSY; + + /* TODO: support other scanning type */ + if (request->type != NL802154_SCAN_PASSIVE) + return -EOPNOTSUPP; + + /* Store scanning parameters */ + rcu_assign_pointer(local->scan_req, request); + rcu_assign_pointer(local->scan_sdata, sdata); + + /* Configure scan_addr to use net_device addr or random */ + if (request->flags & NL802154_SCAN_FLAG_RANDOM_ADDR) + get_random_bytes(&local->scan_addr, sizeof(local->scan_addr)); + else + local->scan_addr = cpu_to_le64(get_unaligned_be64(sdata->dev->dev_addr)); + + local->scan_channel_idx = -1; + local->scanning = true; + + /* Software scanning requires to set promiscuous mode */ + ret = mac802154_set_promiscuous_mode(local, true); + if (ret) + return mac802154_end_of_scan(local); + + ieee802154_queue_delayed_work(&local->hw, &local->scan_work, 0); + + return 0; +} + +int mac802154_scan_rx(struct ieee802154_local *local, struct sk_buff *skb) +{ + struct ieee802154_beaconhdr *bh = (void *)skb->data; + struct ieee802154_addr *src = &mac_cb(skb)->source; + struct cfg802154_scan_request *scan_req; + struct ieee802154_pan_desc desc = {}; + int ret; + + /* Check the validity of the frame length */ + if (skb->len < sizeof(*bh)) + return -EINVAL; + + if (unlikely(src->mode == IEEE802154_ADDR_NONE)) + return -EINVAL; + + if (unlikely(!bh->pan_coordinator)) + return -ENODEV; + + scan_req = rcu_dereference(local->scan_req); + if (unlikely(!scan_req)) + return -EINVAL; + + if (unlikely(local->scan_channel_idx < 0)) { + pr_info("Dropping beacon received during channel change\n"); + return 0; + } + + pr_debug("Beacon received on channel %d of page %d\n", + local->scan_channel_idx, scan_req->page); + + /* Parse beacon and create PAN information */ + desc.coord = src; + desc.page = scan_req->page; + desc.channel = local->scan_channel_idx; + desc.link_quality = mac_cb(skb)->lqi; + desc.superframe_spec = get_unaligned_le16(skb->data); + desc.gts_permit = bh->gts_permit; + + /* Create or update the PAN entry in the management layer */ + ret = cfg802154_record_pan(local->phy, &desc); + if (ret) { + pr_err("Failed to save PAN descriptor\n"); + return ret; + } + + return 0; +} diff --git a/net/mac802154/util.c b/net/mac802154/util.c index f2078238718b..5ee65cb1dbcd 100644 --- a/net/mac802154/util.c +++ b/net/mac802154/util.c @@ -94,3 +94,29 @@ void ieee802154_stop_device(struct ieee802154_local *local) hrtimer_cancel(&local->ifs_timer); drv_stop(local); } + +/* Nothing should have been stuffed into the workqueue during + * the suspend->resume cycle. + */ +static bool ieee802154_can_queue_work(struct ieee802154_local *local) +{ + if (local->suspended) { + pr_warn("queueing ieee802154 work while suspended\n"); + return false; + } + + return true; +} + +void ieee802154_queue_delayed_work(struct ieee802154_hw *hw, + struct delayed_work *dwork, + unsigned long delay) +{ + struct ieee802154_local *local = hw_to_local(hw); + + if (!ieee802154_can_queue_work(local)) + return; + + queue_delayed_work(local->workqueue, dwork, delay); +} +EXPORT_SYMBOL(ieee802154_queue_delayed_work);