Skip to contentSkip to conversation

ath11k: Fix error code in ath11k_core_suspend()

PATCH In review Dan Carpenter 2 participants 7 messages Dec 16, 2020 – Dec 17, 2020

Subsystems (2)
Kernel janitors Qualcomm Atheros ATH11K wireless driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/2 ath11k: Fix error code in ath11k_core_suspend()

The "if (!ret)" condition is inverted and it should be "if (ret)". It means that we return success when we had intended to return an error code.

Fixes d1b0c33850d2 ("ath11k: implement suspend for QCA6390 PCI devices")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/ath/ath11k/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/ath/ath11k/core.c +1 -1
index b97c38b9a270..350b7913622c 100644
@@ -185,7 +185,7 @@ int ath11k_core_suspend(struct ath11k_base *ab)
 	ath11k_hif_ce_irq_disable(ab);
 
 	ret = ath11k_hif_suspend(ab);
-	if (!ret) {
+	if (ret) {
 		ath11k_warn(ab, "failed to suspend hif: %d\n", ret);
 		return ret;
 	}
Signature · 1 line
2.29.2
author
#1 lore raw

PATCH 2/2 ath11k: Fix ath11k_pci_fix_l1ss()

All these conditions are reversed so presumably most of the function is dead code.

Fixes 0699940755e9 ("ath11k: pci: fix L1ss clock unstable problem")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/ath/ath11k/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/net/wireless/ath/ath11k/pci.c +4 -4
index 857647aa57c8..9f9a824a4c2d 100644
@@ -274,7 +274,7 @@ static int ath11k_pci_fix_l1ss(struct ath11k_base *ab)
 				      PCIE_QSERDES_COM_SYSCLK_EN_SEL_REG,
 				      PCIE_QSERDES_COM_SYSCLK_EN_SEL_VAL,
 				      PCIE_QSERDES_COM_SYSCLK_EN_SEL_MSK);
-	if (!ret) {
+	if (ret) {
 		ath11k_warn(ab, "failed to set sysclk: %d\n", ret);
 		return ret;
 	}

@@ -283,7 +283,7 @@ static int ath11k_pci_fix_l1ss(struct ath11k_base *ab)
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG1_REG,
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG1_VAL,
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG_MSK);
-	if (!ret) {
+	if (ret) {
 		ath11k_warn(ab, "failed to set dtct config1 error: %d\n", ret);
 		return ret;
 	}

@@ -292,7 +292,7 @@ static int ath11k_pci_fix_l1ss(struct ath11k_base *ab)
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG2_REG,
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG2_VAL,
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG_MSK);
-	if (!ret) {
+	if (ret) {
 		ath11k_warn(ab, "failed to set dtct config2: %d\n", ret);
 		return ret;
 	}

@@ -301,7 +301,7 @@ static int ath11k_pci_fix_l1ss(struct ath11k_base *ab)
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG4_REG,
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG4_VAL,
 				      PCIE_USB3_PCS_MISC_OSC_DTCT_CONFIG_MSK);
-	if (!ret) {
+	if (ret) {
 		ath11k_warn(ab, "failed to set dtct config4: %d\n", ret);
 		return ret;
 	}
Signature · 1 line
2.29.2
#2 lore raw

Re: [PATCH 2/2] ath11k: Fix ath11k_pci_fix_l1ss()

Quoted 5 lines · Dan Carpenter

All these conditions are reversed so presumably most of the function is dead code.

Fixes 0699940755e9 ("ath11k: pci: fix L1ss clock unstable problem")
Signed-off-by Dan Carpenter <[email protected]>

And this caused a spurious warning:

[   95.734922] ath11k_pci 0000:06:00.0: failed to set sysclk: 0

I'll update the commit log.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#3 lore raw
In reply to Dan Carpenter
Quoted 15 lines · Dan Carpenter

The "if (!ret)" condition is inverted and it should be "if (ret)". It means that we return success when we had intended to return an error code.

Fixes d1b0c33850d2 ("ath11k: implement suspend for QCA6390 PCI devices")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/ath/ath11k/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/ath/ath11k/core.c +0 -0
index b97c38b9a270..350b7913622c 100644

drivers/net/wireless/ath/ath11k/core.c int ath11k_core_suspend(struct ath11k_base *ab) · line 191

Quoted 8 lines
drivers/net/wireless/ath/ath11k/core.c +1 -1
@@ -185,7 +185,7 @@ int ath11k_core_suspend(struct ath11k_base *ab)
 	ath11k_hif_ce_irq_disable(ab);
 
 	ret = ath11k_hif_suspend(ab);
-	if (!ret) {
+	if (ret) {
 		ath11k_warn(ab, "failed to suspend hif: %d\n", ret);
 		return ret;
 	}

I suspect I created these bugs while cleaning up the patches. But I don't get how I missed them in testing, that's a mystery to me.

Anyway, I'll queue these two to v5.11.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#4 lore raw
Quoted 19 lines · Kalle Valo
Quoted 17 lines · Dan Carpenter

The "if (!ret)" condition is inverted and it should be "if (ret)". It means that we return success when we had intended to return an error code.

Fixes d1b0c33850d2 ("ath11k: implement suspend for QCA6390 PCI devices")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/net/wireless/ath/ath11k/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/wireless/ath/ath11k/core.c +0 -0
index b97c38b9a270..350b7913622c 100644
@@ -185,7 +185,7 @@ int ath11k_core_suspend(struct ath11k_base *ab)
 	ath11k_hif_ce_irq_disable(ab);
Quoted 9 lines

drivers/net/wireless/ath/ath11k/core.c int ath11k_core_suspend(struct ath11k_base *ab) · line 191

Quoted 6 lines
drivers/net/wireless/ath/ath11k/core.c +1 -1
 	ret = ath11k_hif_suspend(ab);
-	if (!ret) {
+	if (ret) {
 		ath11k_warn(ab, "failed to suspend hif: %d\n", ret);
 		return ret;
 	}

I suspect I created these bugs while cleaning up the patches. But I don't get how I missed them in testing, that's a mystery to me.

The warning was there:

[  297.186612] ath11k_pci 0000:06:00.0: failed to suspend hif: 0

But I had missed that because suspend was still working due to ath11k_core_suspend() returning 0. I'll update the commit log and mention about that spurious warning.

Thank you Dan, this was a very good catch! In the future I'll grep my logs more carefully, updated my scripts already.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#5 lore raw
In reply to Dan Carpenter
Quoted 9 lines · Dan Carpenter

The "if (!ret)" condition is inverted and it should be "if (ret)". It means that we return success when we had intended to return an error code. This also caused a spurious warning even when the suspend was successful:

[  297.186612] ath11k_pci 0000:06:00.0: failed to suspend hif: 0
Fixes d1b0c33850d2 ("ath11k: implement suspend for QCA6390 PCI devices")
Signed-off-by Dan Carpenter <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

2 patches applied to ath-current branch of ath.git, thanks.

9b09456258ea ath11k: Fix error code in ath11k_core_suspend() 30d085039314 ath11k: Fix ath11k_pci_fix_l1ss()

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/patch/X9nF17L2/EKOSbn/@mwanda/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#6 lore raw
In reply to Dan Carpenter
Quoted 9 lines · Dan Carpenter

The "if (!ret)" condition is inverted and it should be "if (ret)". It means that we return success when we had intended to return an error code. This also caused a spurious warning even when the suspend was successful:

[  297.186612] ath11k_pci 0000:06:00.0: failed to suspend hif: 0
Fixes d1b0c33850d2 ("ath11k: implement suspend for QCA6390 PCI devices")
Signed-off-by Dan Carpenter <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

2 patches applied to ath-current branch of ath.git, thanks.

9b09456258ea ath11k: Fix error code in ath11k_core_suspend() 30d085039314 ath11k: Fix ath11k_pci_fix_l1ss()

Signature · 9 lines
https://patchwork.kernel.org/project/linux-wireless/patch/X9nF17L2/EKOSbn/@mwanda/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


-- 
ath11k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath11k