| Message ID | 20220401120948.1312956-2-quic_bqiang@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e2e23a791745a194aa8e6fb4983c2e7b408ab6e1 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath11k: Add support for SAR | expand |
On 4/1/2022 5:09 AM, Baochen Qiang wrote: > From: Wen Gong <quic_wgong@quicinc.com> > > Currently the wmi service bis is report from firmware, it is divided "bis" is a typo? Also use of "Currently" here is confusing since it seems you are changing that behavior. But the firmware reporting isn't changing, only the host processing is changing. So perhaps: When the WMI service bits are reported from firmware they are divided into multiple segments, with 128 bits in each segment. > to 128 bits for each segment. The first segment is processed by > ath11k_wmi_service_bitmap_copy(), the second segment is processed by > ath11k_service_available_event() with WMI_TAG_SERVICE_AVAILABLE_EVENT, > When the service bit exceed 256 bits, then firmware report it by tag > WMI_TAG_ARRAY_UINT32 in WMI_SERVICE_AVAILABLE_EVENTID. > > ath11k does not process the third segment, this is to extend the wmi Here is where you can use "currently". And this is also a good place to say why the change is needed since that is also missing from the commit text. So consider: Currently ath11k does not process the third segment. Upcoming features need to know if firmware support is available for the features, so add processing of the third segment. Also IMO the implementation details below do not need to be in the commit text. I suspect Kalle can just apply these comments to his pending branch. > service bits from 256 to 384 for the third 128 bits. The 3 enum value > WMI_MAX_SERVICE(128)/WMI_MAX_EXT_SERVICE(256)/WMI_MAX_EXT2_SERVICE(384) > are convenient to process the service bits. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1 > > Signed-off-by: Wen Gong <quic_wgong@quicinc.com> > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/wmi.c | 81 ++++++++++++++++----------- > drivers/net/wireless/ath/ath11k/wmi.h | 9 ++- > 2 files changed, 56 insertions(+), 34 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c > index 3af24b18204e..a5876e0378fe 100644 > --- a/drivers/net/wireless/ath/ath11k/wmi.c > +++ b/drivers/net/wireless/ath/ath11k/wmi.c > @@ -7297,47 +7297,64 @@ static void ath11k_vdev_install_key_compl_event(struct ath11k_base *ab, > rcu_read_unlock(); > } > > -static void ath11k_service_available_event(struct ath11k_base *ab, struct sk_buff *skb) > +static int ath11k_wmi_tlv_services_parser(struct ath11k_base *ab, > + u16 tag, u16 len, > + const void *ptr, void *data) > { > - const void **tb; > const struct wmi_service_available_event *ev; > - int ret; > + u32 *wmi_ext2_service_bitmap; > int i, j; > > - tb = ath11k_wmi_tlv_parse_alloc(ab, skb->data, skb->len, GFP_ATOMIC); > - if (IS_ERR(tb)) { > - ret = PTR_ERR(tb); > - ath11k_warn(ab, "failed to parse tlv: %d\n", ret); > - return; > - } > + switch (tag) { > + case WMI_TAG_SERVICE_AVAILABLE_EVENT: > + ev = (struct wmi_service_available_event *)ptr; > + for (i = 0, j = WMI_MAX_SERVICE; > + i < WMI_SERVICE_SEGMENT_BM_SIZE32 && j < WMI_MAX_EXT_SERVICE; > + i++) { > + do { > + if (ev->wmi_service_segment_bitmap[i] & > + BIT(j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32)) > + set_bit(j, ab->wmi_ab.svc_map); > + } while (++j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32); > + } > > - ev = tb[WMI_TAG_SERVICE_AVAILABLE_EVENT]; > - if (!ev) { > - ath11k_warn(ab, "failed to fetch svc available ev"); > - kfree(tb); > - return; > - } > + ath11k_dbg(ab, ATH11K_DBG_WMI, > + "wmi_ext_service_bitmap 0:0x%04x, 1:0x%04x, 2:0x%04x, 3:0x%04x", > + ev->wmi_service_segment_bitmap[0], > + ev->wmi_service_segment_bitmap[1], > + ev->wmi_service_segment_bitmap[2], > + ev->wmi_service_segment_bitmap[3]); > + break; > + case WMI_TAG_ARRAY_UINT32: > + wmi_ext2_service_bitmap = (u32 *)ptr; > + for (i = 0, j = WMI_MAX_EXT_SERVICE; > + i < WMI_SERVICE_SEGMENT_BM_SIZE32 && j < WMI_MAX_EXT2_SERVICE; > + i++) { > + do { > + if (wmi_ext2_service_bitmap[i] & > + BIT(j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32)) > + set_bit(j, ab->wmi_ab.svc_map); > + } while (++j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32); > + } > > - /* TODO: Use wmi_service_segment_offset information to get the service > - * especially when more services are advertised in multiple sevice > - * available events. > - */ > - for (i = 0, j = WMI_MAX_SERVICE; > - i < WMI_SERVICE_SEGMENT_BM_SIZE32 && j < WMI_MAX_EXT_SERVICE; > - i++) { > - do { > - if (ev->wmi_service_segment_bitmap[i] & > - BIT(j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32)) > - set_bit(j, ab->wmi_ab.svc_map); > - } while (++j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32); > + ath11k_dbg(ab, ATH11K_DBG_WMI, > + "wmi_ext2_service__bitmap 0:0x%04x, 1:0x%04x, 2:0x%04x, 3:0x%04x", > + wmi_ext2_service_bitmap[0], wmi_ext2_service_bitmap[1], > + wmi_ext2_service_bitmap[2], wmi_ext2_service_bitmap[3]); > + break; > } > + return 0; > +} > > - ath11k_dbg(ab, ATH11K_DBG_WMI, > - "wmi_ext_service_bitmap 0:0x%x, 1:0x%x, 2:0x%x, 3:0x%x", > - ev->wmi_service_segment_bitmap[0], ev->wmi_service_segment_bitmap[1], > - ev->wmi_service_segment_bitmap[2], ev->wmi_service_segment_bitmap[3]); > +static void ath11k_service_available_event(struct ath11k_base *ab, struct sk_buff *skb) > +{ > + int ret; > > - kfree(tb); > + ret = ath11k_wmi_tlv_iter(ab, skb->data, skb->len, > + ath11k_wmi_tlv_services_parser, > + NULL); > + if (ret) > + ath11k_warn(ab, "failed to parse services available tlv %d\n", ret); > } > > static void ath11k_peer_assoc_conf_event(struct ath11k_base *ab, struct sk_buff *skb) > diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h > index 925b8003feae..da9b0a90fd70 100644 > --- a/drivers/net/wireless/ath/ath11k/wmi.h > +++ b/drivers/net/wireless/ath/ath11k/wmi.h > @@ -1992,6 +1992,7 @@ enum wmi_tlv_service { > WMI_TLV_SERVICE_ACK_TIMEOUT = 126, > WMI_TLV_SERVICE_PDEV_BSS_CHANNEL_INFO_64 = 127, > > + /* The first 128 bits */ > WMI_MAX_SERVICE = 128, > > WMI_TLV_SERVICE_CHAN_LOAD_INFO = 128, > @@ -2084,7 +2085,11 @@ enum wmi_tlv_service { > WMI_TLV_SERVICE_EXT2_MSG = 220, > WMI_TLV_SERVICE_SRG_SRP_SPATIAL_REUSE_SUPPORT = 249, > > - WMI_MAX_EXT_SERVICE > + /* The second 128 bits */ > + WMI_MAX_EXT_SERVICE = 256, > + > + /* The third 128 bits */ > + WMI_MAX_EXT2_SERVICE = 384 > }; > > enum { > @@ -5378,7 +5383,7 @@ struct ath11k_wmi_base { > > struct completion service_ready; > struct completion unified_ready; > - DECLARE_BITMAP(svc_map, WMI_MAX_EXT_SERVICE); > + DECLARE_BITMAP(svc_map, WMI_MAX_EXT2_SERVICE); > wait_queue_head_t tx_credits_wq; > const struct wmi_peer_flags_map *peer_flags; > u32 num_mem_chunks;
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > On 4/1/2022 5:09 AM, Baochen Qiang wrote: >> From: Wen Gong <quic_wgong@quicinc.com> >> >> Currently the wmi service bis is report from firmware, it is divided > > "bis" is a typo? > Also use of "Currently" here is confusing since it seems you are > changing that behavior. But the firmware reporting isn't changing, > only the host processing is changing. > > So perhaps: > When the WMI service bits are reported from firmware they are divided > into multiple segments, with 128 bits in each segment. > >> to 128 bits for each segment. The first segment is processed by >> ath11k_wmi_service_bitmap_copy(), the second segment is processed by >> ath11k_service_available_event() with WMI_TAG_SERVICE_AVAILABLE_EVENT, >> When the service bit exceed 256 bits, then firmware report it by tag >> WMI_TAG_ARRAY_UINT32 in WMI_SERVICE_AVAILABLE_EVENTID. >> >> ath11k does not process the third segment, this is to extend the wmi > > Here is where you can use "currently". And this is also a good place > to say why the change is needed since that is also missing from the > commit text. So consider: > Currently ath11k does not process the third segment. Upcoming features > need to know if firmware support is available for the features, so add > processing of the third segment. > > Also IMO the implementation details below do not need to be in the > commit text. > > I suspect Kalle can just apply these comments to his pending branch. Did that now, please check: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=d98a4c9bd0b32824ad9ee2d24ecabb345edfaafe
On 4/20/2022 9:35 AM, Kalle Valo wrote: >> I suspect Kalle can just apply these comments to his pending branch. > > Did that now, please check: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=d98a4c9bd0b32824ad9ee2d24ecabb345edfaafe > LGTM!
Baochen Qiang <quic_bqiang@quicinc.com> wrote: > When the WMI service bits are reported from firmware they are divided into > multiple segments, with 128 bits in each segment. The first segment is > processed by ath11k_wmi_service_bitmap_copy(), the second segment is processed > by ath11k_service_available_event() with WMI_TAG_SERVICE_AVAILABLE_EVENT. When > the service bit exceed 256 bits, then firmware reports it by tag > WMI_TAG_ARRAY_UINT32 in WMI_SERVICE_AVAILABLE_EVENTID. > > Currently ath11k does not process the third segment. Upcoming features > need to know if firmware support is available for the features, so add > processing of the third segment. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1 > > Signed-off-by: Wen Gong <quic_wgong@quicinc.com> > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> 2 patches applied to ath-next branch of ath.git, thanks. e2e23a791745 ath11k: add support for extended wmi service bit 652f69ed9c1b ath11k: Add support for SAR
diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c index 3af24b18204e..a5876e0378fe 100644 --- a/drivers/net/wireless/ath/ath11k/wmi.c +++ b/drivers/net/wireless/ath/ath11k/wmi.c @@ -7297,47 +7297,64 @@ static void ath11k_vdev_install_key_compl_event(struct ath11k_base *ab, rcu_read_unlock(); } -static void ath11k_service_available_event(struct ath11k_base *ab, struct sk_buff *skb) +static int ath11k_wmi_tlv_services_parser(struct ath11k_base *ab, + u16 tag, u16 len, + const void *ptr, void *data) { - const void **tb; const struct wmi_service_available_event *ev; - int ret; + u32 *wmi_ext2_service_bitmap; int i, j; - tb = ath11k_wmi_tlv_parse_alloc(ab, skb->data, skb->len, GFP_ATOMIC); - if (IS_ERR(tb)) { - ret = PTR_ERR(tb); - ath11k_warn(ab, "failed to parse tlv: %d\n", ret); - return; - } + switch (tag) { + case WMI_TAG_SERVICE_AVAILABLE_EVENT: + ev = (struct wmi_service_available_event *)ptr; + for (i = 0, j = WMI_MAX_SERVICE; + i < WMI_SERVICE_SEGMENT_BM_SIZE32 && j < WMI_MAX_EXT_SERVICE; + i++) { + do { + if (ev->wmi_service_segment_bitmap[i] & + BIT(j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32)) + set_bit(j, ab->wmi_ab.svc_map); + } while (++j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32); + } - ev = tb[WMI_TAG_SERVICE_AVAILABLE_EVENT]; - if (!ev) { - ath11k_warn(ab, "failed to fetch svc available ev"); - kfree(tb); - return; - } + ath11k_dbg(ab, ATH11K_DBG_WMI, + "wmi_ext_service_bitmap 0:0x%04x, 1:0x%04x, 2:0x%04x, 3:0x%04x", + ev->wmi_service_segment_bitmap[0], + ev->wmi_service_segment_bitmap[1], + ev->wmi_service_segment_bitmap[2], + ev->wmi_service_segment_bitmap[3]); + break; + case WMI_TAG_ARRAY_UINT32: + wmi_ext2_service_bitmap = (u32 *)ptr; + for (i = 0, j = WMI_MAX_EXT_SERVICE; + i < WMI_SERVICE_SEGMENT_BM_SIZE32 && j < WMI_MAX_EXT2_SERVICE; + i++) { + do { + if (wmi_ext2_service_bitmap[i] & + BIT(j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32)) + set_bit(j, ab->wmi_ab.svc_map); + } while (++j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32); + } - /* TODO: Use wmi_service_segment_offset information to get the service - * especially when more services are advertised in multiple sevice - * available events. - */ - for (i = 0, j = WMI_MAX_SERVICE; - i < WMI_SERVICE_SEGMENT_BM_SIZE32 && j < WMI_MAX_EXT_SERVICE; - i++) { - do { - if (ev->wmi_service_segment_bitmap[i] & - BIT(j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32)) - set_bit(j, ab->wmi_ab.svc_map); - } while (++j % WMI_AVAIL_SERVICE_BITS_IN_SIZE32); + ath11k_dbg(ab, ATH11K_DBG_WMI, + "wmi_ext2_service__bitmap 0:0x%04x, 1:0x%04x, 2:0x%04x, 3:0x%04x", + wmi_ext2_service_bitmap[0], wmi_ext2_service_bitmap[1], + wmi_ext2_service_bitmap[2], wmi_ext2_service_bitmap[3]); + break; } + return 0; +} - ath11k_dbg(ab, ATH11K_DBG_WMI, - "wmi_ext_service_bitmap 0:0x%x, 1:0x%x, 2:0x%x, 3:0x%x", - ev->wmi_service_segment_bitmap[0], ev->wmi_service_segment_bitmap[1], - ev->wmi_service_segment_bitmap[2], ev->wmi_service_segment_bitmap[3]); +static void ath11k_service_available_event(struct ath11k_base *ab, struct sk_buff *skb) +{ + int ret; - kfree(tb); + ret = ath11k_wmi_tlv_iter(ab, skb->data, skb->len, + ath11k_wmi_tlv_services_parser, + NULL); + if (ret) + ath11k_warn(ab, "failed to parse services available tlv %d\n", ret); } static void ath11k_peer_assoc_conf_event(struct ath11k_base *ab, struct sk_buff *skb) diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h index 925b8003feae..da9b0a90fd70 100644 --- a/drivers/net/wireless/ath/ath11k/wmi.h +++ b/drivers/net/wireless/ath/ath11k/wmi.h @@ -1992,6 +1992,7 @@ enum wmi_tlv_service { WMI_TLV_SERVICE_ACK_TIMEOUT = 126, WMI_TLV_SERVICE_PDEV_BSS_CHANNEL_INFO_64 = 127, + /* The first 128 bits */ WMI_MAX_SERVICE = 128, WMI_TLV_SERVICE_CHAN_LOAD_INFO = 128, @@ -2084,7 +2085,11 @@ enum wmi_tlv_service { WMI_TLV_SERVICE_EXT2_MSG = 220, WMI_TLV_SERVICE_SRG_SRP_SPATIAL_REUSE_SUPPORT = 249, - WMI_MAX_EXT_SERVICE + /* The second 128 bits */ + WMI_MAX_EXT_SERVICE = 256, + + /* The third 128 bits */ + WMI_MAX_EXT2_SERVICE = 384 }; enum { @@ -5378,7 +5383,7 @@ struct ath11k_wmi_base { struct completion service_ready; struct completion unified_ready; - DECLARE_BITMAP(svc_map, WMI_MAX_EXT_SERVICE); + DECLARE_BITMAP(svc_map, WMI_MAX_EXT2_SERVICE); wait_queue_head_t tx_credits_wq; const struct wmi_peer_flags_map *peer_flags; u32 num_mem_chunks;