| Message ID | 20241023133004.2253830-7-kvalo@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Johnson |
| Headers | show |
| Series | wifi: ath12k: MLO support part 2 | expand |
On 10/23/2024 6:30 AM, Kalle Valo wrote: > From: Sriram R <quic_srirrama@quicinc.com> > > Driver should indicate to firmware whether a peer is multi-link or not in peer > create command using multi-link flag. Add changes to support > WMI_TAG_MLO_PEER_CREATE_PARAMS in WMI_PEER_CREATE_CMDID. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3 > > Signed-off-by: Sriram R <quic_srirrama@quicinc.com> > Signed-off-by: Harshitha Prem <quic_hprem@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/mac.c | 5 +++-- > drivers/net/wireless/ath/ath12k/wmi.c | 27 +++++++++++++++++++++++---- > drivers/net/wireless/ath/ath12k/wmi.h | 6 ++++++ > 3 files changed, 32 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c > index 019a1a6c6777..b628bc2fd0f5 100644 > --- a/drivers/net/wireless/ath/ath12k/mac.c > +++ b/drivers/net/wireless/ath/ath12k/mac.c > @@ -4981,8 +4981,9 @@ static int ath12k_mac_station_add(struct ath12k *ar, > } > > peer_param.vdev_id = arvif->vdev_id; > - peer_param.peer_addr = sta->addr; > + peer_param.peer_addr = arsta->addr; > peer_param.peer_type = WMI_PEER_TYPE_DEFAULT; > + peer_param.ml_enabled = sta->mlo; > > ret = ath12k_peer_create(ar, arvif, sta, &peer_param); > if (ret) { > @@ -7016,7 +7017,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) > struct ath12k_vif *ahvif = arvif->ahvif; > struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif); > struct ath12k_wmi_vdev_create_arg vdev_arg = {0}; > - struct ath12k_wmi_peer_create_arg peer_param; > + struct ath12k_wmi_peer_create_arg peer_param = {0}; > struct ieee80211_bss_conf *link_conf; > u32 param_id, param_value; > u16 nss; > diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c > index e089b58bbea1..0583d832fac7 100644 > --- a/drivers/net/wireless/ath/ath12k/wmi.c > +++ b/drivers/net/wireless/ath/ath12k/wmi.c > @@ -1230,9 +1230,14 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, > struct ath12k_wmi_pdev *wmi = ar->wmi; > struct wmi_peer_create_cmd *cmd; > struct sk_buff *skb; > - int ret; > + int ret, len; > + struct wmi_peer_create_mlo_params *ml_param; > + void *ptr; > + struct wmi_tlv *tlv; > > - skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd)); > + len = sizeof(*cmd) + TLV_HDR_SIZE + sizeof(*ml_param); > + > + skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len); > if (!skb) > return -ENOMEM; > > @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, > cmd->peer_type = cpu_to_le32(arg->peer_type); > cmd->vdev_id = cpu_to_le32(arg->vdev_id); > > + ptr = skb->data + sizeof(*cmd); > + tlv = ptr; > + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, > + sizeof(*ml_param)); using the same TLV size both here and for the TLV that follows doesn't seem logical. is this missing + TLV_HDR_SIZE to account for its own TLV header? > + ptr += TLV_HDR_SIZE; > + ml_param = ptr; > + ml_param->tlv_header = > + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, > + sizeof(*ml_param)); > + if (arg->ml_enabled) > + ml_param->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED); > + > + ptr += sizeof(*ml_param); > + > ath12k_dbg(ar->ab, ATH12K_DBG_WMI, > - "WMI peer create vdev_id %d peer_addr %pM\n", > - arg->vdev_id, arg->peer_addr); > + "WMI peer create vdev_id %d peer_addr %pM ml_flags 0x%x\n", > + arg->vdev_id, arg->peer_addr, ml_param->flags); > > ret = ath12k_wmi_cmd_send(wmi, skb, WMI_PEER_CREATE_CMDID); > if (ret) { > diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h > index 33b9643644c6..07bd275608bf 100644 > --- a/drivers/net/wireless/ath/ath12k/wmi.h > +++ b/drivers/net/wireless/ath/ath12k/wmi.h > @@ -3026,6 +3026,12 @@ struct ath12k_wmi_peer_create_arg { > const u8 *peer_addr; > u32 peer_type; > u32 vdev_id; > + bool ml_enabled; > +}; > + > +struct wmi_peer_create_mlo_params { > + __le32 tlv_header; > + __le32 flags; > }; > > struct ath12k_wmi_pdev_set_regdomain_arg {
Kalle Valo <kvalo@kernel.org> wrote: > @@ -7016,7 +7017,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) > struct ath12k_vif *ahvif = arvif->ahvif; > struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif); > struct ath12k_wmi_vdev_create_arg vdev_arg = {0}; > - struct ath12k_wmi_peer_create_arg peer_param; > + struct ath12k_wmi_peer_create_arg peer_param = {0}; Using '= {}' as initializer would be better, no matter first field of struct will be changed in the future.
Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, >> cmd->peer_type = cpu_to_le32(arg->peer_type); >> cmd->vdev_id = cpu_to_le32(arg->vdev_id); >> >> + ptr = skb->data + sizeof(*cmd); >> + tlv = ptr; >> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, >> + sizeof(*ml_param)); > > using the same TLV size both here and for the TLV that follows doesn't seem > logical. is this missing + TLV_HDR_SIZE to account for its own TLV header? I have forgotten the details of WMI voodoo so I can't really comment right now :) >> + ptr += TLV_HDR_SIZE; >> + ml_param = ptr; >> + ml_param->tlv_header = >> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, >> + sizeof(*ml_param)); But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it reduces the header size: static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len) { return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE); }
On 10/29/2024 8:54 AM, Kalle Valo wrote: > Jeff Johnson <quic_jjohnson@quicinc.com> writes: > >>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, >>> cmd->peer_type = cpu_to_le32(arg->peer_type); >>> cmd->vdev_id = cpu_to_le32(arg->vdev_id); >>> >>> + ptr = skb->data + sizeof(*cmd); >>> + tlv = ptr; >>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, >>> + sizeof(*ml_param)); >> >> using the same TLV size both here and for the TLV that follows doesn't seem >> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header? > > I have forgotten the details of WMI voodoo so I can't really comment > right now :) > >>> + ptr += TLV_HDR_SIZE; >>> + ml_param = ptr; >>> + ml_param->tlv_header = >>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, >>> + sizeof(*ml_param)); > > But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it > reduces the header size: > > static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len) > { > return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE); > } > Yes, I missed that since that is evil to use the _cmd_ TLV function on something that isn't the command TLV. Please fix to use the standard function and subtract the thv header size from the length param
On 10/29/2024 9:01 AM, Jeff Johnson wrote: > On 10/29/2024 8:54 AM, Kalle Valo wrote: >> Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> >>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, >>>> cmd->peer_type = cpu_to_le32(arg->peer_type); >>>> cmd->vdev_id = cpu_to_le32(arg->vdev_id); >>>> >>>> + ptr = skb->data + sizeof(*cmd); >>>> + tlv = ptr; >>>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, >>>> + sizeof(*ml_param)); >>> >>> using the same TLV size both here and for the TLV that follows doesn't seem >>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header? >> >> I have forgotten the details of WMI voodoo so I can't really comment >> right now :) >> >>>> + ptr += TLV_HDR_SIZE; >>>> + ml_param = ptr; >>>> + ml_param->tlv_header = >>>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, >>>> + sizeof(*ml_param)); >> >> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it >> reduces the header size: >> >> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len) >> { >> return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE); >> } >> > > Yes, I missed that since that is evil to use the _cmd_ TLV function on > something that isn't the command TLV. > > Please fix to use the standard function and subtract the thv header size from the *tlv* header > the length param also note there is the existing inconsistency that some WMI params structs include the tlv header and some do not. IMO that lack of consistency will also impact maintainability. again something for the TODO list.
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > On 10/29/2024 8:54 AM, Kalle Valo wrote: >> Jeff Johnson <quic_jjohnson@quicinc.com> writes: >> >>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, >>>> cmd->peer_type = cpu_to_le32(arg->peer_type); >>>> cmd->vdev_id = cpu_to_le32(arg->vdev_id); >>>> >>>> + ptr = skb->data + sizeof(*cmd); >>>> + tlv = ptr; >>>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, >>>> + sizeof(*ml_param)); >>> >>> using the same TLV size both here and for the TLV that follows doesn't seem >>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header? So I assume you are referring to this: tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, sizeof(*ml_param)); ptr += TLV_HDR_SIZE; ml_param = ptr; ml_param->tlv_header = ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, sizeof(*ml_param)); I have never figured out how WMI_TAG_ARRAY_STRUCT is supposed to work but I see a similar pattern also in ath12k_wmi_wow_add_pattern(). Any ideas? >>>> + ptr += TLV_HDR_SIZE; >>>> + ml_param = ptr; >>>> + ml_param->tlv_header = >>>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, >>>> + sizeof(*ml_param)); >> >> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it >> reduces the header size: >> >> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len) >> { >> return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE); >> } >> > > Yes, I missed that since that is evil to use the _cmd_ TLV function on > something that isn't the command TLV. Ok, so you are saying that we should have a identical function but with name ath12k_wmi_tlv_param_hdr() or similar? That makes sense but I think that's separate cleanup as ath12k_wmi_tlv_cmd_hdr() is already used with several WMI params, like in ath12k_wmi_wow_add_pattern(). > Please fix to use the standard function and subtract the thv header size from > the length param I'm not a fan of manually subtracting lengths, as then it's easy to miss something. I would prefer to have functions for handling the length calculation, like ath12k_wmi_tlv_cmd_hdr() ath12k_wmi_tlv_param_hdr().
On 11/1/2024 7:06 AM, Kalle Valo wrote: > Jeff Johnson <quic_jjohnson@quicinc.com> writes: > >> On 10/29/2024 8:54 AM, Kalle Valo wrote: >>> Jeff Johnson <quic_jjohnson@quicinc.com> writes: >>> >>>>> @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, >>>>> cmd->peer_type = cpu_to_le32(arg->peer_type); >>>>> cmd->vdev_id = cpu_to_le32(arg->vdev_id); >>>>> >>>>> + ptr = skb->data + sizeof(*cmd); >>>>> + tlv = ptr; >>>>> + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, >>>>> + sizeof(*ml_param)); >>>> >>>> using the same TLV size both here and for the TLV that follows doesn't seem >>>> logical. is this missing + TLV_HDR_SIZE to account for its own TLV header? > > So I assume you are referring to this: > > tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, > sizeof(*ml_param)); > ptr += TLV_HDR_SIZE; > ml_param = ptr; > ml_param->tlv_header = > ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, > sizeof(*ml_param)); > > I have never figured out how WMI_TAG_ARRAY_STRUCT is supposed to work > but I see a similar pattern also in ath12k_wmi_wow_add_pattern(). Any > ideas? > >>>>> + ptr += TLV_HDR_SIZE; >>>>> + ml_param = ptr; >>>>> + ml_param->tlv_header = >>>>> + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, >>>>> + sizeof(*ml_param)); >>> >>> But did you notice that here is used ath12k_wmi_tlv_cmd_hdr() and it >>> reduces the header size: >>> >>> static __le32 ath12k_wmi_tlv_cmd_hdr(u32 cmd, u32 len) >>> { >>> return ath12k_wmi_tlv_hdr(cmd, len - TLV_HDR_SIZE); >>> } >>> >> >> Yes, I missed that since that is evil to use the _cmd_ TLV function on >> something that isn't the command TLV. > > Ok, so you are saying that we should have a identical function but with > name ath12k_wmi_tlv_param_hdr() or similar? That makes sense but I think > that's separate cleanup as ath12k_wmi_tlv_cmd_hdr() is already used with > several WMI params, like in ath12k_wmi_wow_add_pattern(). > >> Please fix to use the standard function and subtract the thv header size from >> the length param > > I'm not a fan of manually subtracting lengths, as then it's easy to miss > something. I would prefer to have functions for handling the length > calculation, like ath12k_wmi_tlv_cmd_hdr() ath12k_wmi_tlv_param_hdr(). > Whether it is manually subtracting lengths or using tlv_hdr vs tlv_param_hdr, both are easy to miss due to the lack of consistency in the params struct definitions. IMO the only way to avoid this is to consistently either always have the tlv header or to never have the tlv header in the params structs. This lack of consistency is the real underlying issue since whether or not you have the tlv header in the params struct is what dictates whether or not you need to account for the header when filling the TLV length, as well as if you have to separately account for it when you are determining the buffer allocation size and when you are populating the TLV header in the buffer. But the current code actually allocates and fills the data as firmware expects it for this command, so let's keep the current code for now, and we can discuss if it is appropriate to later update to achieve the consistency mentioned. /jeff
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index 019a1a6c6777..b628bc2fd0f5 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -4981,8 +4981,9 @@ static int ath12k_mac_station_add(struct ath12k *ar, } peer_param.vdev_id = arvif->vdev_id; - peer_param.peer_addr = sta->addr; + peer_param.peer_addr = arsta->addr; peer_param.peer_type = WMI_PEER_TYPE_DEFAULT; + peer_param.ml_enabled = sta->mlo; ret = ath12k_peer_create(ar, arvif, sta, &peer_param); if (ret) { @@ -7016,7 +7017,7 @@ int ath12k_mac_vdev_create(struct ath12k *ar, struct ath12k_link_vif *arvif) struct ath12k_vif *ahvif = arvif->ahvif; struct ieee80211_vif *vif = ath12k_ahvif_to_vif(ahvif); struct ath12k_wmi_vdev_create_arg vdev_arg = {0}; - struct ath12k_wmi_peer_create_arg peer_param; + struct ath12k_wmi_peer_create_arg peer_param = {0}; struct ieee80211_bss_conf *link_conf; u32 param_id, param_value; u16 nss; diff --git a/drivers/net/wireless/ath/ath12k/wmi.c b/drivers/net/wireless/ath/ath12k/wmi.c index e089b58bbea1..0583d832fac7 100644 --- a/drivers/net/wireless/ath/ath12k/wmi.c +++ b/drivers/net/wireless/ath/ath12k/wmi.c @@ -1230,9 +1230,14 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, struct ath12k_wmi_pdev *wmi = ar->wmi; struct wmi_peer_create_cmd *cmd; struct sk_buff *skb; - int ret; + int ret, len; + struct wmi_peer_create_mlo_params *ml_param; + void *ptr; + struct wmi_tlv *tlv; - skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, sizeof(*cmd)); + len = sizeof(*cmd) + TLV_HDR_SIZE + sizeof(*ml_param); + + skb = ath12k_wmi_alloc_skb(wmi->wmi_ab, len); if (!skb) return -ENOMEM; @@ -1244,9 +1249,23 @@ int ath12k_wmi_send_peer_create_cmd(struct ath12k *ar, cmd->peer_type = cpu_to_le32(arg->peer_type); cmd->vdev_id = cpu_to_le32(arg->vdev_id); + ptr = skb->data + sizeof(*cmd); + tlv = ptr; + tlv->header = ath12k_wmi_tlv_hdr(WMI_TAG_ARRAY_STRUCT, + sizeof(*ml_param)); + ptr += TLV_HDR_SIZE; + ml_param = ptr; + ml_param->tlv_header = + ath12k_wmi_tlv_cmd_hdr(WMI_TAG_MLO_PEER_CREATE_PARAMS, + sizeof(*ml_param)); + if (arg->ml_enabled) + ml_param->flags = cpu_to_le32(ATH12K_WMI_FLAG_MLO_ENABLED); + + ptr += sizeof(*ml_param); + ath12k_dbg(ar->ab, ATH12K_DBG_WMI, - "WMI peer create vdev_id %d peer_addr %pM\n", - arg->vdev_id, arg->peer_addr); + "WMI peer create vdev_id %d peer_addr %pM ml_flags 0x%x\n", + arg->vdev_id, arg->peer_addr, ml_param->flags); ret = ath12k_wmi_cmd_send(wmi, skb, WMI_PEER_CREATE_CMDID); if (ret) { diff --git a/drivers/net/wireless/ath/ath12k/wmi.h b/drivers/net/wireless/ath/ath12k/wmi.h index 33b9643644c6..07bd275608bf 100644 --- a/drivers/net/wireless/ath/ath12k/wmi.h +++ b/drivers/net/wireless/ath/ath12k/wmi.h @@ -3026,6 +3026,12 @@ struct ath12k_wmi_peer_create_arg { const u8 *peer_addr; u32 peer_type; u32 vdev_id; + bool ml_enabled; +}; + +struct wmi_peer_create_mlo_params { + __le32 tlv_header; + __le32 flags; }; struct ath12k_wmi_pdev_set_regdomain_arg {