| Message ID | 20240206091954.4144454-4-quic_ramess@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: ath12k: Add single wiphy support | expand |
Rameshkumar Sundaram <quic_ramess@quicinc.com> writes: > From: Sriram R <quic_srirrama@quicinc.com> > > When mac80211 does drv start/stop, apply the state change > for all the radios within the wiphy in ath12k. > > 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: Rameshkumar Sundaram <quic_ramess@quicinc.com> I haven't reviewed the patchset fully yet, first quick impressions: > +static void ath12k_drain_tx(struct ath12k_hw *ah) > +{ > + int i; > + struct ath12k *ar; We usually try to have reverse xmas style, of course not always possible but here it is. > + ar = ah->radio; > + > + for (i = 0; i < ah->num_radio; i++) { > + ath12k_mac_drain_tx(ar); > + ar++; > + } > +} Please avoid pointer arithmetic (in this case 'ar++') as much as possible, it's just so easy to shoot yourself in the foot. In patch 1 you add ath12k_ah_to_ar(), why not use it? And I see this pattern quite a lot, should we have ath12k_for_each_radio() or something like that? Or did I see that macro in some patch already?
On 2/16/2024 8:35 AM, Kalle Valo wrote: > Rameshkumar Sundaram <quic_ramess@quicinc.com> writes: > >> From: Sriram R <quic_srirrama@quicinc.com> >> >> When mac80211 does drv start/stop, apply the state change >> for all the radios within the wiphy in ath12k. >> >> 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: Rameshkumar Sundaram <quic_ramess@quicinc.com> > > I haven't reviewed the patchset fully yet, first quick impressions: I didn't review it at all since it has a dependency upon a series that needs rework, and hence I figure this series will require rework.
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > On 2/16/2024 8:35 AM, Kalle Valo wrote: >> Rameshkumar Sundaram <quic_ramess@quicinc.com> writes: >> >>> From: Sriram R <quic_srirrama@quicinc.com> >>> >>> When mac80211 does drv start/stop, apply the state change >>> for all the radios within the wiphy in ath12k. >>> >>> 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: Rameshkumar Sundaram <quic_ramess@quicinc.com> >> >> I haven't reviewed the patchset fully yet, first quick impressions: > > I didn't review it at all since it has a dependency upon a series that > needs rework, and hence I figure this series will require rework. Makes sense, I'll then mark this as Changes Requested in patchwork.
> -----Original Message----- > From: Kalle Valo <kvalo@kernel.org> > Sent: Friday, February 16, 2024 10:06 PM > To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com> > Cc: ath12k@lists.infradead.org; linux-wireless@vger.kernel.org; Sriram > R <quic_srirrama@quicinc.com> > Subject: Re: [PATCH v2 03/12] wifi: ath12k: modify ath12k mac > start/stop ops for single wiphy > > Rameshkumar Sundaram <quic_ramess@quicinc.com> writes: > > > From: Sriram R <quic_srirrama@quicinc.com> > > > > When mac80211 does drv start/stop, apply the state change for all > the > > radios within the wiphy in ath12k. > > > > 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: Rameshkumar Sundaram <quic_ramess@quicinc.com> > > I haven't reviewed the patchset fully yet, first quick impressions: > > > +static void ath12k_drain_tx(struct ath12k_hw *ah) { > > + int i; > > + struct ath12k *ar; > > We usually try to have reverse xmas style, of course not always possible > but here it is. > Sure, Will fix it v3. > > + ar = ah->radio; > > + > > + for (i = 0; i < ah->num_radio; i++) { > > + ath12k_mac_drain_tx(ar); > > + ar++; > > + } > > +} > > Please avoid pointer arithmetic (in this case 'ar++') as much as possible, > it's just so easy to shoot yourself in the foot. In patch 1 you add > ath12k_ah_to_ar(), why not use it? Sure, will avoid this and take reference from ah->radio[] > > And I see this pattern quite a lot, should we have > ath12k_for_each_radio() or something like that? Or did I see that macro > in some patch already? Sure, makes sense, guess it was in v1 of patch [01/12] in this series, but was removed and used inline in subsequent patch sets. will introduced it back. > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submitti > ngpatches
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c index c58f753d4a10..ef952e8e4086 100644 --- a/drivers/net/wireless/ath/ath12k/mac.c +++ b/drivers/net/wireless/ath/ath12k/mac.c @@ -243,6 +243,7 @@ static const u32 ath12k_smps_map[] = { static int ath12k_start_vdev_delay(struct ath12k *ar, struct ath12k_vif *arvif); +static void ath12k_mac_stop(struct ath12k *ar); static const char *ath12k_mac_phymode_str(enum wmi_phy_mode mode) { @@ -5256,15 +5257,31 @@ static int ath12k_mac_start(struct ath12k *ar) return ret; } +static void ath12k_drain_tx(struct ath12k_hw *ah) +{ + int i; + struct ath12k *ar; + + ar = ah->radio; + + for (i = 0; i < ah->num_radio; i++) { + ath12k_mac_drain_tx(ar); + ar++; + } +} + static int ath12k_mac_op_start(struct ieee80211_hw *hw) { struct ath12k_hw *ah = ath12k_hw_to_ah(hw); - struct ath12k *ar = ath12k_ah_to_ar(ah, 0); - struct ath12k_base *ab = ar->ab; - int ret; + struct ath12k *ar; + struct ath12k_base *ab; + int ret = 0; + int i; - ath12k_mac_drain_tx(ar); + ath12k_drain_tx(ah); + ar = ah->radio; + ab = ar->ab; mutex_lock(&ah->conf_mutex); switch (ah->state) { @@ -5285,14 +5302,26 @@ static int ath12k_mac_op_start(struct ieee80211_hw *hw) goto err; } - ret = ath12k_mac_start(ar); - if (ret) { - ah->state = ATH12K_HW_STATE_OFF; - - ath12k_err(ab, "fail to start mac operations in pdev idx %d ret %d\n", - ar->pdev_idx, ret); + for (i = 0; i < ah->num_radio; i++) { + ret = ath12k_mac_start(ar); + if (ret) { + ah->state = ATH12K_HW_STATE_OFF; + ath12k_err(ab, "fail to start mac operations in pdev idx %d ret %d\n", + ar->pdev_idx, ret); + goto fail_start; + } + ar++; + ab = ar->ab; } + mutex_unlock(&ah->conf_mutex); + + return 0; +fail_start: + for (; i > 0; i--) { + ar--; + ath12k_mac_stop(ar); + } err: mutex_unlock(&ah->conf_mutex); @@ -5395,13 +5424,17 @@ static void ath12k_mac_op_stop(struct ieee80211_hw *hw) { struct ath12k_hw *ah = ath12k_hw_to_ah(hw); struct ath12k *ar = ath12k_ah_to_ar(ah, 0); + int i; - ath12k_mac_drain_tx(ar); + ath12k_drain_tx(ah); mutex_lock(&ah->conf_mutex); ah->state = ATH12K_HW_STATE_OFF; - ath12k_mac_stop(ar); + for (i = 0; i < ah->num_radio; i++) { + ath12k_mac_stop(ar); + ar++; + } mutex_unlock(&ah->conf_mutex); }