[PATCHv2] wifi: ath12k: Avoid napi_sync before napi_enable

[PATCHv2] wifi: ath12k: Avoid napi_sync before napi_enable

[Tamizh Chelvam Raja]

From: Avula Sri Charan <quic_asrichar@quicinc.com>

In case of MHI error a reset work will be queued
which will try napi_disable after napi_synchronize.

As the napi will be only enabled after qmi_firmware_ready
event trying napi_synchronize before napi_enable will
result in indefinite sleep in case of a firmware crash
in QMI init sequence.

To avoid this introduce napi_enabled flag to check
if napi is enabled or not before calling napi_synchronize.

Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1

Signed-off-by: Avula Sri Charan <quic_asrichar@quicinc.com>
Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com>
---
v2:
  * Removed prerequisite-patch-id tag which added wrongly

 drivers/net/wireless/ath/ath12k/core.h |  1 +
 drivers/net/wireless/ath/ath12k/pci.c  | 13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 86a1eeec64a6..b5adbb21315a 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -166,6 +166,7 @@ struct ath12k_ext_irq_grp {
 	u32 num_irq;
 	u32 grp_id;
 	u64 timestamp;
+	bool napi_enabled;
 	struct napi_struct napi;
 	struct net_device *napi_ndev;
 };
diff --git a/drivers/net/wireless/ath/ath12k/pci.c b/drivers/net/wireless/ath/ath12k/pci.c
index 4a50d7118b3e..24c5ef96baf6 100644
--- a/drivers/net/wireless/ath/ath12k/pci.c
+++ b/drivers/net/wireless/ath/ath12k/pci.c
@@ -483,8 +483,11 @@ static void __ath12k_pci_ext_irq_disable(struct ath12k_base *ab)
 
 		ath12k_pci_ext_grp_disable(irq_grp);
 
-		napi_synchronize(&irq_grp->napi);
-		napi_disable(&irq_grp->napi);
+		if (irq_grp->napi_enabled) {
+			napi_synchronize(&irq_grp->napi);
+			napi_disable(&irq_grp->napi);
+			irq_grp->napi_enabled = false;
+		}
 	}
 }
 
@@ -1114,7 +1117,11 @@ void ath12k_pci_ext_irq_enable(struct ath12k_base *ab)
 	for (i = 0; i < ATH12K_EXT_IRQ_GRP_NUM_MAX; i++) {
 		struct ath12k_ext_irq_grp *irq_grp = &ab->ext_irq_grp[i];
 
-		napi_enable(&irq_grp->napi);
+		if (!irq_grp->napi_enabled) {
+			napi_enable(&irq_grp->napi);
+			irq_grp->napi_enabled = true;
+		}
+
 		ath12k_pci_ext_grp_enable(irq_grp);
 	}
 

base-commit: 376673aa393c1c232299be3e910d7f2e6d974b2f
-- 
2.34.1

Re: [PATCHv2] wifi: ath12k: Avoid napi_sync before napi_enable

[Aditya Kumar Singh]

On 1/24/25 14:30, Tamizh Chelvam Raja wrote:
> From: Avula Sri Charan <quic_asrichar@quicinc.com>
> 
> In case of MHI error a reset work will be queued
> which will try napi_disable after napi_synchronize.

nit: always better to refer function name with () at end. I guess Jeff 
can fix in pending?

> 
> As the napi will be only enabled after qmi_firmware_ready
> event trying napi_synchronize before napi_enable will
> result in indefinite sleep in case of a firmware crash
> in QMI init sequence.
> 
> To avoid this introduce napi_enabled flag to check
> if napi is enabled or not before calling napi_synchronize.
> 
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Avula Sri Charan <quic_asrichar@quicinc.com>
> Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com>
> ---

Reviewed-by: Aditya Kumar Singh <aditya.kumar.singh@oss.qualcomm.com>

-- 
Aditya

Re: [PATCHv2] wifi: ath12k: Avoid napi_sync before napi_enable

[Jeff Johnson]

On 1/24/2025 8:55 PM, Aditya Kumar Singh wrote:
> On 1/24/25 14:30, Tamizh Chelvam Raja wrote:
>> From: Avula Sri Charan <quic_asrichar@quicinc.com>
>>
>> In case of MHI error a reset work will be queued
>> which will try napi_disable after napi_synchronize.
> 
> nit: always better to refer function name with () at end. I guess Jeff 
> can fix in pending?

I fixed this, as well as did some other grammar & formatting fixes.

Please check:
https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/?h=pending&id=a644c8784220b7d5dd0a10a45c5e6779605778a5

> 
>>
>> As the napi will be only enabled after qmi_firmware_ready
>> event trying napi_synchronize before napi_enable will
>> result in indefinite sleep in case of a firmware crash
>> in QMI init sequence.
>>
>> To avoid this introduce napi_enabled flag to check
>> if napi is enabled or not before calling napi_synchronize.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Avula Sri Charan <quic_asrichar@quicinc.com>
>> Signed-off-by: Tamizh Chelvam Raja <quic_tamizhr@quicinc.com>
>> ---
> 
> Reviewed-by: Aditya Kumar Singh <aditya.kumar.singh@oss.qualcomm.com>
> 

Re: [PATCHv2] wifi: ath12k: Avoid napi_sync before napi_enable

[Aditya Kumar Singh]

On 1/30/25 20:35, Jeff Johnson wrote:
> On 1/24/2025 8:55 PM, Aditya Kumar Singh wrote:
>> On 1/24/25 14:30, Tamizh Chelvam Raja wrote:
>>> From: Avula Sri Charan<quic_asrichar@quicinc.com>
>>>
>>> In case of MHI error a reset work will be queued
>>> which will try napi_disable after napi_synchronize.
>> nit: always better to refer function name with () at end. I guess Jeff
>> can fix in pending?
> I fixed this, as well as did some other grammar & formatting fixes.
> 
> Please check:
> https://git.kernel.org/pub/scm/linux/kernel/git/ath/ath.git/commit/? 
> h=pending&id=a644c8784220b7d5dd0a10a45c5e6779605778a5

Looks good, thanks.

-- 
Aditya

Re: [PATCHv2] wifi: ath12k: Avoid napi_sync before napi_enable

[Jeff Johnson]

On Fri, 24 Jan 2025 14:30:58 +0530, Tamizh Chelvam Raja wrote:
> In case of MHI error a reset work will be queued
> which will try napi_disable after napi_synchronize.
> 
> As the napi will be only enabled after qmi_firmware_ready
> event trying napi_synchronize before napi_enable will
> result in indefinite sleep in case of a firmware crash
> in QMI init sequence.
> 
> [...]

Applied, thanks!

[1/1] wifi: ath12k: Avoid napi_sync before napi_enable
      commit: 268c73d470a5790a492a2fc2ded084b909d144f3

Best regards,
-- 
Jeff Johnson <jeff.johnson@oss.qualcomm.com>