| Message ID | 20240611043342.2672998-1-quic_rgnanase@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath12k: Fix pdev id sent to firmware for single phy devices | expand |
On 6/10/2024 9:33 PM, Ramya Gnanasekar wrote: > From: Lingbo Kong <quic_lingbok@quicinc.com> > > Pdev id from mac phy capabilities will be sent as a part of > HTT/WMI command to firmware. This causes issue with single pdev > devices where firmware does not respond to the WMI/HTT request > sent from host. > > For single pdev devices firmware expects pdev id as 1 for 5 GHz/6 GHz > phy and 2 for 2 GHz band. Add wrapper ath12k_mac_get_target_pdev_id() > to help fetch right pdev for single pdev devices. > > 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: Lingbo Kong <quic_lingbok@quicinc.com> > Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes: > From: Lingbo Kong <quic_lingbok@quicinc.com> > > Pdev id from mac phy capabilities will be sent as a part of > HTT/WMI command to firmware. This causes issue with single pdev > devices where firmware does not respond to the WMI/HTT request > sent from host. But WCN7850 already works so I'm not really understanding the bug description. Can you elaborate, please? > @@ -81,5 +81,8 @@ int ath12k_mac_rfkill_config(struct ath12k *ar); > int ath12k_mac_wait_tx_complete(struct ath12k *ar); > void ath12k_mac_handle_beacon(struct ath12k *ar, struct sk_buff *skb); > void ath12k_mac_handle_beacon_miss(struct ath12k *ar, u32 vdev_id); > +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar); > +u8 ath12k_mac_get_target_pdev_id_from_vif(struct ath12k_vif *arvif); > +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k *ar); AFAICS ath12k_mac_get_target_pdev_id_from_vif() and ath12k_mac_get_vif_up() can be static, no need to add them to mac.h. So we are left with ath12k_mac_get_target_pdev_id() but I don't see it called anywhere. Isn't that dead code we are adding?
On 6/19/2024 7:43 PM, Kalle Valo wrote: > Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes: > >> From: Lingbo Kong <quic_lingbok@quicinc.com> >> >> Pdev id from mac phy capabilities will be sent as a part of >> HTT/WMI command to firmware. This causes issue with single pdev >> devices where firmware does not respond to the WMI/HTT request >> sent from host. > > But WCN7850 already works so I'm not really understanding the bug > description. Can you elaborate, please? pdev id from mac capabilities sent as a part of HTT stats request or WMI control path stats request was not expected by WCN7850 and hence fails to send the stats response. > >> @@ -81,5 +81,8 @@ int ath12k_mac_rfkill_config(struct ath12k *ar); >> int ath12k_mac_wait_tx_complete(struct ath12k *ar); >> void ath12k_mac_handle_beacon(struct ath12k *ar, struct sk_buff *skb); >> void ath12k_mac_handle_beacon_miss(struct ath12k *ar, u32 vdev_id); >> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar); >> +u8 ath12k_mac_get_target_pdev_id_from_vif(struct ath12k_vif *arvif); >> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k *ar); > > AFAICS ath12k_mac_get_target_pdev_id_from_vif() and > ath12k_mac_get_vif_up() can be static, no need to add them to mac.h. Ah! Sure Kalle. I will take care. > > So we are left with ath12k_mac_get_target_pdev_id() but I don't see it > called anywhere. Isn't that dead code we are adding? The wrapper is used by upcoming HTT stats and WMI control path stats support. Initially this change was added in the HTT stats infrastructure support series. Since WMI control path stats also depends on this patch, to eliminate the another patch series dependency, I pulled out this patch from HTT stats series. This wrapper will be used by below changes: HTT stats: https://lore.kernel.org/linux-wireless/20240614063404.2659089-1-quic_rgnanase@quicinc.com/T/ WMI control path stats: https://patchwork.kernel.org/project/linux-wireless/patch/20240529180920.1472003-1-quic_rgnanase@quicinc.com/ WMI control path stats has to be rebased on this current patch. Since it is deferred, I refrained from sending v2. >
Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes: > On 6/19/2024 7:43 PM, Kalle Valo wrote: >> Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes: >> >>> From: Lingbo Kong <quic_lingbok@quicinc.com> >>> >>> Pdev id from mac phy capabilities will be sent as a part of >>> HTT/WMI command to firmware. This causes issue with single pdev >>> devices where firmware does not respond to the WMI/HTT request >>> sent from host. >> >> But WCN7850 already works so I'm not really understanding the bug >> description. Can you elaborate, please? > > pdev id from mac capabilities sent as a part of HTT stats request or WMI > control path stats request was not expected by WCN7850 and hence fails > to send the stats response. Ok, now I understand. Please include that info to the commit message. >>> @@ -81,5 +81,8 @@ int ath12k_mac_rfkill_config(struct ath12k *ar); >>> int ath12k_mac_wait_tx_complete(struct ath12k *ar); >>> void ath12k_mac_handle_beacon(struct ath12k *ar, struct sk_buff *skb); >>> void ath12k_mac_handle_beacon_miss(struct ath12k *ar, u32 vdev_id); >>> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar); >>> +u8 ath12k_mac_get_target_pdev_id_from_vif(struct ath12k_vif *arvif); >>> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k *ar); >> >> AFAICS ath12k_mac_get_target_pdev_id_from_vif() and >> ath12k_mac_get_vif_up() can be static, no need to add them to mac.h. > > Ah! Sure Kalle. I will take care. Thanks. >> So we are left with ath12k_mac_get_target_pdev_id() but I don't see it >> called anywhere. Isn't that dead code we are adding? > > The wrapper is used by upcoming HTT stats and WMI control path stats > support. Initially this change was added in the HTT stats infrastructure > support series. Since WMI control path stats also depends on this patch, > to eliminate the another patch series dependency, I pulled out this > patch from HTT stats series. > This wrapper will be used by below changes: > HTT stats: > https://lore.kernel.org/linux-wireless/20240614063404.2659089-1-quic_rgnanase@quicinc.com/T/ > WMI control path stats: > https://patchwork.kernel.org/project/linux-wireless/patch/20240529180920.1472003-1-quic_rgnanase@quicinc.com/ > > WMI control path stats has to be rebased on this current patch. Since it > is deferred, I refrained from sending v2. Ok, you could for example mention this is in a comment after '---' line. Adding dead code is usually suspicious.
Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes: > From: Lingbo Kong <quic_lingbok@quicinc.com> > > Pdev id from mac phy capabilities will be sent as a part of > HTT/WMI command to firmware. This causes issue with single pdev > devices where firmware does not respond to the WMI/HTT request > sent from host. > > For single pdev devices firmware expects pdev id as 1 for 5 GHz/6 GHz > phy and 2 for 2 GHz band. Add wrapper ath12k_mac_get_target_pdev_id() > to help fetch right pdev for single pdev devices. > > 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: Lingbo Kong <quic_lingbok@quicinc.com> > Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com> [...] > +static bool ath12k_mac_band_match(enum nl80211_band band1, enum WMI_HOST_WLAN_BAND band2) > +{ > + return (((band1 == NL80211_BAND_2GHZ) && (band2 & WMI_HOST_WLAN_2G_CAP)) || > + (((band1 == NL80211_BAND_5GHZ) || (band1 == NL80211_BAND_6GHZ)) && > + (band2 & WMI_HOST_WLAN_5G_CAP))); > +} This is not really pleasent to read. What about something like this: switch (band1) { case NL80211_BAND_2GHZ: if (band2 & WMI_HOST_WLAN_2G_CAP) return true; break; case NL80211_BAND_5GHZ: case NL80211_BAND_6GHZ: if (band2 & WMI_HOST_WLAN_5G_CAP) return true; break; } return false; Or any other ideas? > +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar) > +{ > + struct ath12k_vif *arvif; > + struct ath12k_base *ab = ar->ab; > + > + if (!ab->hw_params->single_pdev_only) > + return ar->pdev->pdev_id; > + > + arvif = ath12k_mac_get_vif_up(ar); > + > + if (arvif) > + return ath12k_mac_get_target_pdev_id_from_vif(arvif); > + else > + return ar->ab->fw_pdev[0].pdev_id; > +} I find this easier to read: arvif = ath12k_mac_get_vif_up(ar); if (!arvif) return ar->ab->fw_pdev[0].pdev_id; return ath12k_mac_get_target_pdev_id_from_vif(arvif); But I still would prefer to have some a code comment explaining the idea behind here, especially why it's safe to use fw_pdev[0].pdev_id directly and what scenario that is.
On 6/19/2024 10:56 PM, Kalle Valo wrote: > Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes: > >> On 6/19/2024 7:43 PM, Kalle Valo wrote: >>> Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes: >>> >>>> From: Lingbo Kong <quic_lingbok@quicinc.com> >>>> >>>> Pdev id from mac phy capabilities will be sent as a part of >>>> HTT/WMI command to firmware. This causes issue with single pdev >>>> devices where firmware does not respond to the WMI/HTT request >>>> sent from host. >>> >>> But WCN7850 already works so I'm not really understanding the bug >>> description. Can you elaborate, please? >> >> pdev id from mac capabilities sent as a part of HTT stats request or WMI >> control path stats request was not expected by WCN7850 and hence fails >> to send the stats response. > > Ok, now I understand. Please include that info to the commit message. Sure I will add the information. > >>>> @@ -81,5 +81,8 @@ int ath12k_mac_rfkill_config(struct ath12k *ar); >>>> int ath12k_mac_wait_tx_complete(struct ath12k *ar); >>>> void ath12k_mac_handle_beacon(struct ath12k *ar, struct sk_buff *skb); >>>> void ath12k_mac_handle_beacon_miss(struct ath12k *ar, u32 vdev_id); >>>> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar); >>>> +u8 ath12k_mac_get_target_pdev_id_from_vif(struct ath12k_vif *arvif); >>>> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k *ar); >>> >>> AFAICS ath12k_mac_get_target_pdev_id_from_vif() and >>> ath12k_mac_get_vif_up() can be static, no need to add them to mac.h. >> >> Ah! Sure Kalle. I will take care. > > Thanks. > >>> So we are left with ath12k_mac_get_target_pdev_id() but I don't see it >>> called anywhere. Isn't that dead code we are adding? >> >> The wrapper is used by upcoming HTT stats and WMI control path stats >> support. Initially this change was added in the HTT stats infrastructure >> support series. Since WMI control path stats also depends on this patch, >> to eliminate the another patch series dependency, I pulled out this >> patch from HTT stats series. >> This wrapper will be used by below changes: >> HTT stats: >> https://lore.kernel.org/linux-wireless/20240614063404.2659089-1-quic_rgnanase@quicinc.com/T/ >> WMI control path stats: >> https://patchwork.kernel.org/project/linux-wireless/patch/20240529180920.1472003-1-quic_rgnanase@quicinc.com/ >> >> WMI control path stats has to be rebased on this current patch. Since it >> is deferred, I refrained from sending v2. > > Ok, you could for example mention this is in a comment after '---' line. > Adding dead code is usually suspicious. Yeah sure. I will brief the usage in commit message as you mentioned. >
On 6/19/2024 11:05 PM, Kalle Valo wrote: > Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes: > >> From: Lingbo Kong <quic_lingbok@quicinc.com> >> >> Pdev id from mac phy capabilities will be sent as a part of >> HTT/WMI command to firmware. This causes issue with single pdev >> devices where firmware does not respond to the WMI/HTT request >> sent from host. >> >> For single pdev devices firmware expects pdev id as 1 for 5 GHz/6 GHz >> phy and 2 for 2 GHz band. Add wrapper ath12k_mac_get_target_pdev_id() >> to help fetch right pdev for single pdev devices. >> >> 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: Lingbo Kong <quic_lingbok@quicinc.com> >> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com> > > [...] > >> +static bool ath12k_mac_band_match(enum nl80211_band band1, enum WMI_HOST_WLAN_BAND band2) >> +{ >> + return (((band1 == NL80211_BAND_2GHZ) && (band2 & WMI_HOST_WLAN_2G_CAP)) || >> + (((band1 == NL80211_BAND_5GHZ) || (band1 == NL80211_BAND_6GHZ)) && >> + (band2 & WMI_HOST_WLAN_5G_CAP))); >> +} > > This is not really pleasent to read. What about something like this: > > switch (band1) { > case NL80211_BAND_2GHZ: > if (band2 & WMI_HOST_WLAN_2G_CAP) > return true; > > break; > case NL80211_BAND_5GHZ: > case NL80211_BAND_6GHZ: > if (band2 & WMI_HOST_WLAN_5G_CAP) > return true; > > break; > } > > return false; > > Or any other ideas? Thanks for the suggestion Kalle. Switch looks aesthetic compared to conditional checks. I will refactor the code as above for better readability. > >> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar) >> +{ >> + struct ath12k_vif *arvif; >> + struct ath12k_base *ab = ar->ab; >> + >> + if (!ab->hw_params->single_pdev_only) >> + return ar->pdev->pdev_id; >> + >> + arvif = ath12k_mac_get_vif_up(ar); >> + >> + if (arvif) >> + return ath12k_mac_get_target_pdev_id_from_vif(arvif); >> + else >> + return ar->ab->fw_pdev[0].pdev_id; >> +} > > I find this easier to read: > > arvif = ath12k_mac_get_vif_up(ar); > if (!arvif) > return ar->ab->fw_pdev[0].pdev_id; > > return ath12k_mac_get_target_pdev_id_from_vif(arvif); > > But I still would prefer to have some a code comment explaining the idea > behind here, especially why it's safe to use fw_pdev[0].pdev_id directly > and what scenario that is. > Sure. I will get the necessary info added as comments. Thanks for the review Kalle.
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index 784964ae03ec..31a66ade7852 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -670,6 +670,65 @@ static struct ath12k *ath12k_get_ar_by_vif(struct ieee80211_hw *hw, return NULL; } +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k *ar) +{ + struct ath12k_vif *arvif; + + lockdep_assert_held(&ar->conf_mutex); + list_for_each_entry(arvif, &ar->arvifs, list) { + if (arvif->is_up) + return arvif; + } + + return NULL; +} + +static bool ath12k_mac_band_match(enum nl80211_band band1, enum WMI_HOST_WLAN_BAND band2) +{ + return (((band1 == NL80211_BAND_2GHZ) && (band2 & WMI_HOST_WLAN_2G_CAP)) || + (((band1 == NL80211_BAND_5GHZ) || (band1 == NL80211_BAND_6GHZ)) && + (band2 & WMI_HOST_WLAN_5G_CAP))); +} + +u8 ath12k_mac_get_target_pdev_id_from_vif(struct ath12k_vif *arvif) +{ + struct ath12k *ar = arvif->ar; + struct ath12k_base *ab = ar->ab; + struct ieee80211_vif *vif = arvif->vif; + struct cfg80211_chan_def def; + enum nl80211_band band; + u8 pdev_id = ab->fw_pdev[0].pdev_id; + int i; + + if (WARN_ON(ath12k_mac_vif_chan(vif, &def))) + return pdev_id; + + band = def.chan->band; + + for (i = 0; i < ab->fw_pdev_count; i++) { + if (ath12k_mac_band_match(band, ab->fw_pdev[i].supported_bands)) + return ab->fw_pdev[i].pdev_id; + } + + return pdev_id; +} + +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar) +{ + struct ath12k_vif *arvif; + struct ath12k_base *ab = ar->ab; + + if (!ab->hw_params->single_pdev_only) + return ar->pdev->pdev_id; + + arvif = ath12k_mac_get_vif_up(ar); + + if (arvif) + return ath12k_mac_get_target_pdev_id_from_vif(arvif); + else + return ar->ab->fw_pdev[0].pdev_id; +} + static void ath12k_pdev_caps_update(struct ath12k *ar) { struct ath12k_base *ab = ar->ab; diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h index 69fd282b9dd3..877005b56a7c 100644 --- a/drivers/net/wireless/ath/ath12k/mac.h +++ b/drivers/net/wireless/ath/ath12k/mac.h @@ -81,5 +81,8 @@ int ath12k_mac_rfkill_config(struct ath12k *ar); int ath12k_mac_wait_tx_complete(struct ath12k *ar); void ath12k_mac_handle_beacon(struct ath12k *ar, struct sk_buff *skb); void ath12k_mac_handle_beacon_miss(struct ath12k *ar, u32 vdev_id); +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar); +u8 ath12k_mac_get_target_pdev_id_from_vif(struct ath12k_vif *arvif); +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k *ar); #endif