diff mbox series

[v2,03/12] wifi: ath12k: modify ath12k mac start/stop ops for single wiphy

Message ID 20240206091954.4144454-4-quic_ramess@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: Add single wiphy support | expand

Commit Message

Rameshkumar Sundaram Feb. 6, 2024, 9:19 a.m. UTC 
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>
---
 drivers/net/wireless/ath/ath12k/mac.c | 57 +++++++++++++++++++++------
 1 file changed, 45 insertions(+), 12 deletions(-)

Comments

Kalle Valo Feb. 16, 2024, 4:35 p.m. UTC | #1 
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?
Jeff Johnson Feb. 16, 2024, 4:45 p.m. UTC | #2 
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.
Kalle Valo Feb. 16, 2024, 5:02 p.m. UTC | #3 
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.
Rameshkumar Sundaram Feb. 20, 2024, 11:29 a.m. UTC | #4 
> -----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 mbox series

Patch

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);
 }