| Message ID | 20220112173312.764660-24-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | IEEE 802.15.4 scan support | expand |
Hi, On Wed, 12 Jan 2022 at 12:34, Miquel Raynal <miquel.raynal@bootlin.com> wrote: ... > +static int mac802154_scan_send_beacon_req_locked(struct ieee802154_local *local) > +{ > + struct sk_buff *skb; > + int ret; > + > + lockdep_assert_held(&local->scan_lock); > + > + skb = alloc_skb(IEEE802154_BEACON_REQ_SKB_SZ, GFP_KERNEL); > + if (!skb) > + return -ENOBUFS; > + > + ret = ieee802154_beacon_req_push(skb, &local->beacon_req); > + if (ret) { > + kfree_skb(skb); > + return ret; > + } > + > + return drv_xmit_async(local, skb); I think you need to implement a sync transmit handling here. And what I mean is not using dryv_xmit_sync() (It is a long story and should not be used, it's just that the driver is allowed to call bus api functions which can sleep). We don't have such a function yet (but I think it can be implemented), you should wait until the transmission is done. If we don't wait we fill framebuffers on the hardware while the hardware is transmitting the framebuffer which is... bad. Please implement it so that we have a synchronous api above the asynchronous transmit api. - Alex
Hi Alexander, alex.aring@gmail.com wrote on Wed, 12 Jan 2022 18:16:11 -0500: > Hi, > > On Wed, 12 Jan 2022 at 12:34, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > ... > > +static int mac802154_scan_send_beacon_req_locked(struct ieee802154_local *local) > > +{ > > + struct sk_buff *skb; > > + int ret; > > + > > + lockdep_assert_held(&local->scan_lock); > > + > > + skb = alloc_skb(IEEE802154_BEACON_REQ_SKB_SZ, GFP_KERNEL); > > + if (!skb) > > + return -ENOBUFS; > > + > > + ret = ieee802154_beacon_req_push(skb, &local->beacon_req); > > + if (ret) { > > + kfree_skb(skb); > > + return ret; > > + } > > + > > + return drv_xmit_async(local, skb); > > I think you need to implement a sync transmit handling here. True. > And what > I mean is not using dryv_xmit_sync() (It is a long story and should > not be used, it's just that the driver is allowed to call bus api > functions which can sleep). Understood. > We don't have such a function yet (but I > think it can be implemented), you should wait until the transmission > is done. If we don't wait we fill framebuffers on the hardware while > the hardware is transmitting the framebuffer which is... bad. Do you already have something in mind? If I focus on the scan operation, it could be that we consider the queue empty, then we put this transfer, wait for completion and continue. But this only work for places where we know we have full control over what is transmitted (eg. during a scan) and not for all transfers. Would this fit your idea? Or do you want something more generic with some kind of an internal queue where we have the knowledge of what has been queued and a token to link with every xmit_done call that is made? I'm open to suggestions. Thanks, Miquèl
Hi, On Thu, 13 Jan 2022 at 12:14, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > alex.aring@gmail.com wrote on Wed, 12 Jan 2022 18:16:11 -0500: > > > Hi, > > > > On Wed, 12 Jan 2022 at 12:34, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > ... > > > +static int mac802154_scan_send_beacon_req_locked(struct ieee802154_local *local) > > > +{ > > > + struct sk_buff *skb; > > > + int ret; > > > + > > > + lockdep_assert_held(&local->scan_lock); > > > + > > > + skb = alloc_skb(IEEE802154_BEACON_REQ_SKB_SZ, GFP_KERNEL); > > > + if (!skb) > > > + return -ENOBUFS; > > > + > > > + ret = ieee802154_beacon_req_push(skb, &local->beacon_req); > > > + if (ret) { > > > + kfree_skb(skb); > > > + return ret; > > > + } > > > + > > > + return drv_xmit_async(local, skb); > > > > I think you need to implement a sync transmit handling here. > > True. > > > And what > > I mean is not using dryv_xmit_sync() (It is a long story and should > > not be used, it's just that the driver is allowed to call bus api > > functions which can sleep). > > Understood. > I think we should care about drivers which use drv_xmit_sync() or we disable scan operations for them... so the actual transmit function should prefer async but use sync if it's not implemented. I am not a fan of this inside the core, if some driver really want to workaround their bus system because it's simpler to use or whatever they should do that inside the driver and not let the core queue it for them in the right context. There are reasons why xmit_do is in softirq context. > > We don't have such a function yet (but I > > think it can be implemented), you should wait until the transmission > > is done. If we don't wait we fill framebuffers on the hardware while > > the hardware is transmitting the framebuffer which is... bad. > > Do you already have something in mind? > > If I focus on the scan operation, it could be that we consider the > queue empty, then we put this transfer, wait for completion and > continue. But this only work for places where we know we have full > control over what is transmitted (eg. during a scan) and not for all > transfers. Would this fit your idea? > > Or do you want something more generic with some kind of an > internal queue where we have the knowledge of what has been queued and > a token to link with every xmit_done call that is made? > > I'm open to suggestions. > That we currently allow only one skb at one time (because ?all? supported hardware doesn't have multiple tx framebuffers) this makes everything for now pretty simple. Don't let the queue run empty, the queue here is controlled by the qdsic (I suppose and I hope we are talking about the same queue) we already stop the queue which stops further skb to transmit to the hardware but there can be one ongoing which we need to wait for. I said in a previous mail a wait_for_completion()/complete() works here, but I think now that could be problematic because scan-op -> wait_for_completion() and complete() can run parallel in different contexts. I think we need to do that over a waitqueue and a wait_event(). Maybe you can track somehow with an atomic counter how many transmissions are currently ongoing (should be never higher than one currently). However the atomic counter will be future proofed when we support filling up more than one framebuffer. So the condition for wait_event() would be atomic_test(phy->ongoing_txs) - or something like that. Increment in transmit path and decrement in xmit_done path, if it hits zero wake_up() the queue so the condition will be checked again. That the queue is controlled by qdisc and we stop the queue for a long time will somehow act the qdisc badly and dropping skb's in the "hotpath" as I mentioned earlier it should be okay. Be sure we don't activate the queue again in the xmit complete function, if we do the WARN_ON(mac...queue_stopped()) should be triggered and this indicates we were not expecting to transmit something over this path. - Alex
Hi, On Thu, 13 Jan 2022 at 19:30, Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Thu, 13 Jan 2022 at 12:14, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > alex.aring@gmail.com wrote on Wed, 12 Jan 2022 18:16:11 -0500: > > > > > Hi, > > > > > > On Wed, 12 Jan 2022 at 12:34, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > ... > > > > +static int mac802154_scan_send_beacon_req_locked(struct ieee802154_local *local) > > > > +{ > > > > + struct sk_buff *skb; > > > > + int ret; > > > > + > > > > + lockdep_assert_held(&local->scan_lock); > > > > + > > > > + skb = alloc_skb(IEEE802154_BEACON_REQ_SKB_SZ, GFP_KERNEL); > > > > + if (!skb) > > > > + return -ENOBUFS; > > > > + > > > > + ret = ieee802154_beacon_req_push(skb, &local->beacon_req); > > > > + if (ret) { > > > > + kfree_skb(skb); > > > > + return ret; > > > > + } > > > > + > > > > + return drv_xmit_async(local, skb); > > > > > > I think you need to implement a sync transmit handling here. > > > > True. > > > > > And what > > > I mean is not using dryv_xmit_sync() (It is a long story and should > > > not be used, it's just that the driver is allowed to call bus api > > > functions which can sleep). > > > > Understood. > > > > I think we should care about drivers which use drv_xmit_sync() or we > disable scan operations for them... so the actual transmit function > should prefer async but use sync if it's not implemented. I am not a > fan of this inside the core, if some driver really want to workaround > their bus system because it's simpler to use or whatever they should > do that inside the driver and not let the core queue it for them in > the right context. There are reasons why xmit_do is in softirq > context. > > > > We don't have such a function yet (but I > > > think it can be implemented), you should wait until the transmission > > > is done. If we don't wait we fill framebuffers on the hardware while > > > the hardware is transmitting the framebuffer which is... bad. > > > > Do you already have something in mind? > > > > If I focus on the scan operation, it could be that we consider the > > queue empty, then we put this transfer, wait for completion and > > continue. But this only work for places where we know we have full > > control over what is transmitted (eg. during a scan) and not for all > > transfers. Would this fit your idea? > > > > Or do you want something more generic with some kind of an > > internal queue where we have the knowledge of what has been queued and > > a token to link with every xmit_done call that is made? > > > > I'm open to suggestions. > > > > That we currently allow only one skb at one time (because ?all? > supported hardware doesn't have multiple tx framebuffers) this makes > everything for now pretty simple. > > Don't let the queue run empty, the queue here is controlled by the > qdsic (I suppose and I hope we are talking about the same queue) we > already stop the queue which stops further skb to transmit to the > hardware but there can be one ongoing which we need to wait for. I > said in a previous mail a wait_for_completion()/complete() works here, > but I think now that could be problematic because scan-op -> > wait_for_completion() and complete() can run parallel in different > contexts. I think we need to do that over a waitqueue and a > wait_event(). Maybe you can track somehow with an atomic counter how > many transmissions are currently ongoing (should be never higher than > one currently). However the atomic counter will be future proofed when > we support filling up more than one framebuffer. So the condition for > wait_event() would be atomic_test(phy->ongoing_txs) - or something > like that. Increment in transmit path and decrement in xmit_done path, > if it hits zero wake_up() the queue so the condition will be checked > again. > I am sorry, the wait_event() will fix the issue after calling stop_queue() to be sure there are no ongoing transmissions. Now we need to have some idea about how to implement the synchronous transmit function. We have the function (drv_xmit_async, or something higher level to take care of sync as well) to transmit a skb and the complete handler is xmit_complete(). As I mentioned we allow only one skb for one time... Somehow we need to have a wait here and a per phy wait_for_completion()/complete() will in this case work. Even if there is hardware outside which has more transmit buffers, we probably would send one skb at one for slowpath (having full control of transmissions) anyway. - Alex
diff --git a/include/net/ieee802154_netdev.h b/include/net/ieee802154_netdev.h index f7716aeec93b..1bf1a4e508a2 100644 --- a/include/net/ieee802154_netdev.h +++ b/include/net/ieee802154_netdev.h @@ -58,6 +58,11 @@ struct ieee802154_beacon_hdr { #endif } __packed; +struct ieee802154_mac_cmd_pl { + u8 cmd_id; + /* TODO: content depending on the cmd_id */ +} __packed; + struct ieee802154_sechdr { #if defined(__LITTLE_ENDIAN_BITFIELD) u8 level:3, @@ -139,6 +144,11 @@ struct ieee802154_hdr { struct ieee802154_sechdr sec; }; +struct ieee802154_beacon_req_frame { + struct ieee802154_hdr mhr; + struct ieee802154_mac_cmd_pl mac_pl; +}; + struct ieee802154_beacon_frame { struct ieee802154_hdr mhr; struct ieee802154_beacon_hdr mac_pl; @@ -169,7 +179,9 @@ int ieee802154_hdr_peek_addrs(const struct sk_buff *skb, */ int ieee802154_hdr_peek(const struct sk_buff *skb, struct ieee802154_hdr *hdr); -/* pushes a beacon frame into an skb */ +/* pushes a beacon_req or a beacon frame into an skb */ +int ieee802154_beacon_req_push(struct sk_buff *skb, + struct ieee802154_beacon_req_frame *breq); int ieee802154_beacon_push(struct sk_buff *skb, struct ieee802154_beacon_frame *beacon); diff --git a/net/ieee802154/header_ops.c b/net/ieee802154/header_ops.c index bab710aa36f9..c31a9e429a14 100644 --- a/net/ieee802154/header_ops.c +++ b/net/ieee802154/header_ops.c @@ -121,6 +121,31 @@ ieee802154_hdr_push(struct sk_buff *skb, struct ieee802154_hdr *hdr) } EXPORT_SYMBOL_GPL(ieee802154_hdr_push); +int ieee802154_beacon_req_push(struct sk_buff *skb, + struct ieee802154_beacon_req_frame *breq) +{ + struct ieee802154_mac_cmd_pl *mac_pl = &breq->mac_pl; + struct ieee802154_hdr *mhr = &breq->mhr; + u16 crc; + int ret; + + skb_reserve(skb, sizeof(*mhr)); + ret = ieee802154_hdr_push(skb, mhr); + if (ret < 0) + return ret; + + skb_reset_mac_header(skb); + skb->mac_len = ret; + + skb_put_data(skb, mac_pl, sizeof(*mac_pl)); + + crc = crc_ccitt(0, skb->data, skb->len); + put_unaligned_le16(crc, skb_put(skb, 2)); + + return 0; +} +EXPORT_SYMBOL_GPL(ieee802154_beacon_req_push); + int ieee802154_beacon_push(struct sk_buff *skb, struct ieee802154_beacon_frame *beacon) { diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h index 4feb9181ff4f..19c840500bd8 100644 --- a/net/mac802154/ieee802154_i.h +++ b/net/mac802154/ieee802154_i.h @@ -56,6 +56,7 @@ struct ieee802154_local { struct cfg802154_scan_request __rcu *scan_req; struct ieee802154_sub_if_data __rcu *scan_sdata; struct delayed_work scan_work; + struct ieee802154_beacon_req_frame beacon_req; /* Beacons handling */ bool ongoing_beacons_request; diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c index 091cf6fdff41..a3ff65d5bb7a 100644 --- a/net/mac802154/scan.c +++ b/net/mac802154/scan.c @@ -19,10 +19,15 @@ #define IEEE802154_BEACON_MHR_SZ 13 #define IEEE802154_BEACON_PL_SZ 4 +#define IEEE802154_BEACON_REQ_MHR_SZ 7 +#define IEEE802154_BEACON_REQ_PL_SZ 1 #define IEEE802154_CRC_SZ 2 #define IEEE802154_BEACON_SKB_SZ (IEEE802154_BEACON_MHR_SZ + \ IEEE802154_BEACON_PL_SZ + \ IEEE802154_CRC_SZ) +#define IEEE802154_BEACON_REQ_SKB_SZ (IEEE802154_BEACON_REQ_MHR_SZ + \ + IEEE802154_BEACON_REQ_PL_SZ + \ + IEEE802154_CRC_SZ) static bool mac802154_check_promiscuous(struct ieee802154_local *local) { @@ -167,6 +172,41 @@ int mac802154_stop_beacons_locked(struct ieee802154_local *local) return 0; } +static int mac802154_scan_prepare_beacon_req(struct ieee802154_local *local) +{ + memset(&local->beacon_req, 0, sizeof(local->beacon_req)); + local->beacon_req.mhr.fc.type = IEEE802154_FC_TYPE_MAC_CMD; + local->beacon_req.mhr.fc.dest_addr_mode = IEEE802154_SHORT_ADDRESSING; + local->beacon_req.mhr.fc.version = IEEE802154_2003_STD; + local->beacon_req.mhr.fc.source_addr_mode = IEEE802154_NO_ADDRESSING; + local->beacon_req.mhr.dest.mode = IEEE802154_ADDR_SHORT; + local->beacon_req.mhr.dest.pan_id = cpu_to_le16(IEEE802154_PANID_BROADCAST); + local->beacon_req.mhr.dest.short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST); + local->beacon_req.mac_pl.cmd_id = IEEE802154_CMD_BEACON_REQ; + + return 0; +} + +static int mac802154_scan_send_beacon_req_locked(struct ieee802154_local *local) +{ + struct sk_buff *skb; + int ret; + + lockdep_assert_held(&local->scan_lock); + + skb = alloc_skb(IEEE802154_BEACON_REQ_SKB_SZ, GFP_KERNEL); + if (!skb) + return -ENOBUFS; + + ret = ieee802154_beacon_req_push(skb, &local->beacon_req); + if (ret) { + kfree_skb(skb); + return ret; + } + + return drv_xmit_async(local, skb); +} + static int mac802154_scan_send_beacon_locked(struct ieee802154_local *local, struct wpan_dev *wpan_dev) { @@ -236,6 +276,9 @@ void mac802154_scan_work(struct work_struct *work) ieee802154_set_symbol_duration(local->phy); } while (ret); + if (scan_req->type == NL802154_SCAN_ACTIVE) + mac802154_scan_send_beacon_req_locked(local); + queue_work: scan_duration = mac802154_scan_get_channel_time(scan_req->duration, local->phy->symbol_duration); @@ -262,8 +305,8 @@ int mac802154_trigger_scan_locked(struct ieee802154_sub_if_data *sdata, if (mac802154_scan_is_ongoing(local)) return -EBUSY; - /* TODO: support other scanning type */ - if (request->type != NL802154_SCAN_PASSIVE) + if (request->type != NL802154_SCAN_PASSIVE && + request->type != NL802154_SCAN_ACTIVE) return -EOPNOTSUPP; /* Store scanning parameters */ @@ -276,6 +319,9 @@ int mac802154_trigger_scan_locked(struct ieee802154_sub_if_data *sdata, else local->scan_addr = cpu_to_le64(get_unaligned_be64(sdata->dev->dev_addr)); + if (request->type == NL802154_SCAN_ACTIVE) + mac802154_scan_prepare_beacon_req(local); + local->scan_channel_idx = -1; atomic_set(&local->scanning, 1);