[PATCH] [v2] wifi: ath10k: cleanup CE initialization

[PATCH] [v2] wifi: ath10k: cleanup CE initialization

[Dmitry Antipov]

Commit 25d0dbcbd5c7 ("ath10k: split ce initialization and allocation")
changes 'ath10k_ce_init_src_ring()' and 'ath10k_ce_init_dest_ring()'
so these functions can't return -ENOMEM but always returns 0. This way
both of the above may be converted to 'void' and related code may be
simplified as well.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: change 'ath10k_ce_alloc_rri()' to return -ENOMEM in case
of 'dma_alloc_coherent()' failure and fix error handling in
'ath10k_snoc_hif_power_up()'
---
 drivers/net/wireless/ath/ath10k/ahb.c  |  6 +---
 drivers/net/wireless/ath/ath10k/ce.c   | 50 +++++++++-----------------
 drivers/net/wireless/ath/ath10k/ce.h   |  6 ++--
 drivers/net/wireless/ath/ath10k/pci.c  | 29 ++++-----------
 drivers/net/wireless/ath/ath10k/pci.h  |  2 +-
 drivers/net/wireless/ath/ath10k/snoc.c | 30 +++++-----------
 6 files changed, 36 insertions(+), 87 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
index 76efea2f1138..c3a2eeb5542d 100644
--- a/drivers/net/wireless/ath/ath10k/ahb.c
+++ b/drivers/net/wireless/ath/ath10k/ahb.c
@@ -655,11 +655,7 @@ static int ath10k_ahb_hif_power_up(struct ath10k *ar,
 		goto out;
 	}
 
-	ret = ath10k_pci_init_pipes(ar);
-	if (ret) {
-		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
-		goto out;
-	}
+	ath10k_pci_init_pipes(ar);
 
 	ret = ath10k_pci_init_config(ar);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index c27b8204718a..d7275dcc1f99 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -1352,9 +1352,9 @@ void ath10k_ce_enable_interrupts(struct ath10k *ar)
 }
 EXPORT_SYMBOL(ath10k_ce_enable_interrupts);
 
-static int ath10k_ce_init_src_ring(struct ath10k *ar,
-				   unsigned int ce_id,
-				   const struct ce_attr *attr)
+static void ath10k_ce_init_src_ring(struct ath10k *ar,
+				    unsigned int ce_id,
+				    const struct ce_attr *attr)
 {
 	struct ath10k_ce *ce = ath10k_ce_priv(ar);
 	struct ath10k_ce_pipe *ce_state = &ce->ce_states[ce_id];
@@ -1389,13 +1389,11 @@ static int ath10k_ce_init_src_ring(struct ath10k *ar,
 	ath10k_dbg(ar, ATH10K_DBG_BOOT,
 		   "boot init ce src ring id %d entries %d base_addr %pK\n",
 		   ce_id, nentries, src_ring->base_addr_owner_space);
-
-	return 0;
 }
 
-static int ath10k_ce_init_dest_ring(struct ath10k *ar,
-				    unsigned int ce_id,
-				    const struct ce_attr *attr)
+static void ath10k_ce_init_dest_ring(struct ath10k *ar,
+				     unsigned int ce_id,
+				     const struct ce_attr *attr)
 {
 	struct ath10k_ce *ce = ath10k_ce_priv(ar);
 	struct ath10k_ce_pipe *ce_state = &ce->ce_states[ce_id];
@@ -1427,8 +1425,6 @@ static int ath10k_ce_init_dest_ring(struct ath10k *ar,
 	ath10k_dbg(ar, ATH10K_DBG_BOOT,
 		   "boot ce dest ring id %d entries %d base_addr %pK\n",
 		   ce_id, nentries, dest_ring->base_addr_owner_space);
-
-	return 0;
 }
 
 static int ath10k_ce_alloc_shadow_base(struct ath10k *ar,
@@ -1659,30 +1655,14 @@ ath10k_ce_alloc_dest_ring_64(struct ath10k *ar, unsigned int ce_id,
  * initialization. It may be that only one side or the other is
  * initialized by software/firmware.
  */
-int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
-			const struct ce_attr *attr)
+void ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
+			 const struct ce_attr *attr)
 {
-	int ret;
-
-	if (attr->src_nentries) {
-		ret = ath10k_ce_init_src_ring(ar, ce_id, attr);
-		if (ret) {
-			ath10k_err(ar, "Failed to initialize CE src ring for ID: %d (%d)\n",
-				   ce_id, ret);
-			return ret;
-		}
-	}
-
-	if (attr->dest_nentries) {
-		ret = ath10k_ce_init_dest_ring(ar, ce_id, attr);
-		if (ret) {
-			ath10k_err(ar, "Failed to initialize CE dest ring for ID: %d (%d)\n",
-				   ce_id, ret);
-			return ret;
-		}
-	}
+	if (attr->src_nentries)
+		ath10k_ce_init_src_ring(ar, ce_id, attr);
 
-	return 0;
+	if (attr->dest_nentries)
+		ath10k_ce_init_dest_ring(ar, ce_id, attr);
 }
 EXPORT_SYMBOL(ath10k_ce_init_pipe);
 
@@ -1926,7 +1906,7 @@ int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
 }
 EXPORT_SYMBOL(ath10k_ce_alloc_pipe);
 
-void ath10k_ce_alloc_rri(struct ath10k *ar)
+int ath10k_ce_alloc_rri(struct ath10k *ar)
 {
 	int i;
 	u32 value;
@@ -1939,7 +1919,7 @@ void ath10k_ce_alloc_rri(struct ath10k *ar)
 					   &ce->paddr_rri, GFP_KERNEL);
 
 	if (!ce->vaddr_rri)
-		return;
+		return -ENOMEM;
 
 	ath10k_ce_write32(ar, ar->hw_ce_regs->ce_rri_low,
 			  lower_32_bits(ce->paddr_rri));
@@ -1954,6 +1934,8 @@ void ath10k_ce_alloc_rri(struct ath10k *ar)
 		value |= ar->hw_ce_regs->upd->mask;
 		ath10k_ce_write32(ar, ce_base_addr + ctrl1_regs, value);
 	}
+
+	return 0;
 }
 EXPORT_SYMBOL(ath10k_ce_alloc_rri);
 
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index 666ce384a1d8..c90c00316356 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -220,8 +220,8 @@ int ath10k_ce_completed_send_next_nolock(struct ath10k_ce_pipe *ce_state,
 
 /*==================CE Engine Initialization=======================*/
 
-int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
-			const struct ce_attr *attr);
+void ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
+			 const struct ce_attr *attr);
 void ath10k_ce_deinit_pipe(struct ath10k *ar, unsigned int ce_id);
 int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
 			 const struct ce_attr *attr);
@@ -262,7 +262,7 @@ void ath10k_ce_enable_interrupts(struct ath10k *ar);
 void ath10k_ce_dump_registers(struct ath10k *ar,
 			      struct ath10k_fw_crash_data *crash_data);
 
-void ath10k_ce_alloc_rri(struct ath10k *ar);
+int ath10k_ce_alloc_rri(struct ath10k *ar);
 void ath10k_ce_free_rri(struct ath10k *ar);
 
 /* ce_attr.flags values */
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a7f44f6335fb..f6988075cd83 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2524,21 +2524,13 @@ void ath10k_pci_free_pipes(struct ath10k *ar)
 		ath10k_ce_free_pipe(ar, i);
 }
 
-int ath10k_pci_init_pipes(struct ath10k *ar)
+void ath10k_pci_init_pipes(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	int i, ret;
-
-	for (i = 0; i < CE_COUNT; i++) {
-		ret = ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
-		if (ret) {
-			ath10k_err(ar, "failed to initialize copy engine pipe %d: %d\n",
-				   i, ret);
-			return ret;
-		}
-	}
+	int i;
 
-	return 0;
+	for (i = 0; i < CE_COUNT; i++)
+		ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
 }
 
 static bool ath10k_pci_has_fw_crashed(struct ath10k *ar)
@@ -2703,12 +2695,7 @@ static int ath10k_pci_qca988x_chip_reset(struct ath10k *ar)
 		 * sufficient to verify if device is capable of booting
 		 * firmware blob.
 		 */
-		ret = ath10k_pci_init_pipes(ar);
-		if (ret) {
-			ath10k_warn(ar, "failed to init copy engine: %d\n",
-				    ret);
-			continue;
-		}
+		ath10k_pci_init_pipes(ar);
 
 		ret = ath10k_pci_diag_read32(ar, QCA988X_HOST_INTEREST_ADDRESS,
 					     &val);
@@ -2846,11 +2833,7 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
 		goto err_sleep;
 	}
 
-	ret = ath10k_pci_init_pipes(ar);
-	if (ret) {
-		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
-		goto err_sleep;
-	}
+	ath10k_pci_init_pipes(ar);
 
 	ret = ath10k_pci_init_config(ar);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 480cd97ab739..3b3ded87aef9 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -238,7 +238,7 @@ void ath10k_pci_free_pipes(struct ath10k *ar);
 void ath10k_pci_rx_replenish_retry(struct timer_list *t);
 void ath10k_pci_ce_deinit(struct ath10k *ar);
 void ath10k_pci_init_napi(struct ath10k *ar);
-int ath10k_pci_init_pipes(struct ath10k *ar);
+void ath10k_pci_init_pipes(struct ath10k *ar);
 int ath10k_pci_init_config(struct ath10k *ar);
 void ath10k_pci_rx_post(struct ath10k *ar);
 void ath10k_pci_flush(struct ath10k *ar);
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 26214c00cd0d..f41948d9eebb 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -939,20 +939,12 @@ static int ath10k_snoc_hif_start(struct ath10k *ar)
 	return 0;
 }
 
-static int ath10k_snoc_init_pipes(struct ath10k *ar)
+static void ath10k_snoc_init_pipes(struct ath10k *ar)
 {
-	int i, ret;
-
-	for (i = 0; i < CE_COUNT; i++) {
-		ret = ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
-		if (ret) {
-			ath10k_err(ar, "failed to initialize copy engine pipe %d: %d\n",
-				   i, ret);
-			return ret;
-		}
-	}
+	int i;
 
-	return 0;
+	for (i = 0; i < CE_COUNT; i++)
+		ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
 }
 
 static int ath10k_snoc_wlan_enable(struct ath10k *ar,
@@ -1082,18 +1074,14 @@ static int ath10k_snoc_hif_power_up(struct ath10k *ar,
 		goto err_hw_power_off;
 	}
 
-	ath10k_ce_alloc_rri(ar);
-
-	ret = ath10k_snoc_init_pipes(ar);
-	if (ret) {
-		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
-		goto err_free_rri;
-	}
+	ret = ath10k_ce_alloc_rri(ar);
+	if (ret)
+		goto err_snoc_wlan_disable;
 
+	ath10k_snoc_init_pipes(ar);
 	return 0;
 
-err_free_rri:
-	ath10k_ce_free_rri(ar);
+err_snoc_wlan_disable:
 	ath10k_snoc_wlan_disable(ar);
 
 err_hw_power_off:
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/3] [v2] wifi: ath10k: simplify ath10k_peer_assoc_h_vht()

[Dmitry Antipov]

Commit 3db24065c2c8 ("ath10k: enable VHT160 and VHT80+80 modes")
introduces 'get_160mhz_nss_from_maxrate()' which never returns 0,
which means that 'ath10k_peer_assoc_h_vht()' may be simplified.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: adjust to match series
---
 drivers/net/wireless/ath/ath10k/mac.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 03e7bc5b6c0b..148d0fab4418 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2631,12 +2631,8 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
 		u32 max_rate;
 
 		max_rate = arg->peer_vht_rates.rx_max_rate;
-		rx_nss = get_160mhz_nss_from_maxrate(max_rate);
-
-		if (rx_nss == 0)
-			rx_nss = arg->peer_num_spatial_streams;
-		else
-			rx_nss = min(arg->peer_num_spatial_streams, rx_nss);
+		rx_nss = min(arg->peer_num_spatial_streams,
+			     get_160mhz_nss_from_maxrate(max_rate));
 
 		max_rate = hw->vht160_mcs_tx_highest;
 		rx_nss = min(rx_nss, get_160mhz_nss_from_maxrate(max_rate));
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/3] [v2] wifi: ath10k: simplify ath10k_peer_assoc_h_vht()

[Jeff Johnson]

On 8/23/2023 2:50 AM, Dmitry Antipov wrote:
> Commit 3db24065c2c8 ("ath10k: enable VHT160 and VHT80+80 modes")
> introduces 'get_160mhz_nss_from_maxrate()' which never returns 0,
> which means that 'ath10k_peer_assoc_h_vht()' may be simplified.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>



_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 3/3] [v2] wifi: ath10k: simplify ath10k_pci_pm_suspend()

[Dmitry Antipov]

Since 'ath10k_pci_suspend()' always returns 0, it may be converted to
'void' and 'ath10k_pci_pm_suspend()' may be simplified accordingly.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: adjust to match series
---
 drivers/net/wireless/ath/ath10k/pci.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index f6988075cd83..16037e77264b 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2871,7 +2871,7 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
 	return 0;
 }
 
-static int ath10k_pci_suspend(struct ath10k *ar)
+static void ath10k_pci_suspend(struct ath10k *ar)
 {
 	/* The grace timer can still be counting down and ar->ps_awake be true.
 	 * It is known that the device may be asleep after resuming regardless
@@ -2879,8 +2879,6 @@ static int ath10k_pci_suspend(struct ath10k *ar)
 	 * device is asleep before proceeding.
 	 */
 	ath10k_pci_sleep_sync(ar);
-
-	return 0;
 }
 
 static int ath10k_pci_hif_resume(struct ath10k *ar)
@@ -3734,13 +3732,9 @@ MODULE_DEVICE_TABLE(pci, ath10k_pci_id_table);
 static __maybe_unused int ath10k_pci_pm_suspend(struct device *dev)
 {
 	struct ath10k *ar = dev_get_drvdata(dev);
-	int ret;
 
-	ret = ath10k_pci_suspend(ar);
-	if (ret)
-		ath10k_warn(ar, "failed to suspend hif: %d\n", ret);
-
-	return ret;
+	ath10k_pci_suspend(ar);
+	return 0;
 }
 
 static __maybe_unused int ath10k_pci_pm_resume(struct device *dev)
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 3/3] [v2] wifi: ath10k: simplify ath10k_pci_pm_suspend()

[Jeff Johnson]

On 8/23/2023 2:50 AM, Dmitry Antipov wrote:
> Since 'ath10k_pci_suspend()' always returns 0, it may be converted to
> 'void' and 'ath10k_pci_pm_suspend()' may be simplified accordingly.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [lvc-project] [PATCH] [v2] wifi: ath10k: cleanup CE initialization

[Alexey Khoroshilov]

You could also use

git format-patch -v 2

to get Subject with version in it:
[PATCH v2 1/3]
[PATCH v2 2/3]
[PATCH v2 3/3]



> v2: adjust to match series

Do mean that nothing has been changed in the patch regarding the
previous version?

--
Alexey


On 23.08.2023 12:50, Dmitry Antipov wrote:
> Commit 25d0dbcbd5c7 ("ath10k: split ce initialization and allocation")
> changes 'ath10k_ce_init_src_ring()' and 'ath10k_ce_init_dest_ring()'
> so these functions can't return -ENOMEM but always returns 0. This way
> both of the above may be converted to 'void' and related code may be
> simplified as well.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v2: change 'ath10k_ce_alloc_rri()' to return -ENOMEM in case
> of 'dma_alloc_coherent()' failure and fix error handling in
> 'ath10k_snoc_hif_power_up()'
> ---
>  drivers/net/wireless/ath/ath10k/ahb.c  |  6 +---
>  drivers/net/wireless/ath/ath10k/ce.c   | 50 +++++++++-----------------
>  drivers/net/wireless/ath/ath10k/ce.h   |  6 ++--
>  drivers/net/wireless/ath/ath10k/pci.c  | 29 ++++-----------
>  drivers/net/wireless/ath/ath10k/pci.h  |  2 +-
>  drivers/net/wireless/ath/ath10k/snoc.c | 30 +++++-----------
>  6 files changed, 36 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
> index 76efea2f1138..c3a2eeb5542d 100644
> --- a/drivers/net/wireless/ath/ath10k/ahb.c
> +++ b/drivers/net/wireless/ath/ath10k/ahb.c
> @@ -655,11 +655,7 @@ static int ath10k_ahb_hif_power_up(struct ath10k *ar,
>  		goto out;
>  	}
>  
> -	ret = ath10k_pci_init_pipes(ar);
> -	if (ret) {
> -		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
> -		goto out;
> -	}
> +	ath10k_pci_init_pipes(ar);
>  
>  	ret = ath10k_pci_init_config(ar);
>  	if (ret) {
> diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
> index c27b8204718a..d7275dcc1f99 100644
> --- a/drivers/net/wireless/ath/ath10k/ce.c
> +++ b/drivers/net/wireless/ath/ath10k/ce.c
> @@ -1352,9 +1352,9 @@ void ath10k_ce_enable_interrupts(struct ath10k *ar)
>  }
>  EXPORT_SYMBOL(ath10k_ce_enable_interrupts);
>  
> -static int ath10k_ce_init_src_ring(struct ath10k *ar,
> -				   unsigned int ce_id,
> -				   const struct ce_attr *attr)
> +static void ath10k_ce_init_src_ring(struct ath10k *ar,
> +				    unsigned int ce_id,
> +				    const struct ce_attr *attr)
>  {
>  	struct ath10k_ce *ce = ath10k_ce_priv(ar);
>  	struct ath10k_ce_pipe *ce_state = &ce->ce_states[ce_id];
> @@ -1389,13 +1389,11 @@ static int ath10k_ce_init_src_ring(struct ath10k *ar,
>  	ath10k_dbg(ar, ATH10K_DBG_BOOT,
>  		   "boot init ce src ring id %d entries %d base_addr %pK\n",
>  		   ce_id, nentries, src_ring->base_addr_owner_space);
> -
> -	return 0;
>  }
>  
> -static int ath10k_ce_init_dest_ring(struct ath10k *ar,
> -				    unsigned int ce_id,
> -				    const struct ce_attr *attr)
> +static void ath10k_ce_init_dest_ring(struct ath10k *ar,
> +				     unsigned int ce_id,
> +				     const struct ce_attr *attr)
>  {
>  	struct ath10k_ce *ce = ath10k_ce_priv(ar);
>  	struct ath10k_ce_pipe *ce_state = &ce->ce_states[ce_id];
> @@ -1427,8 +1425,6 @@ static int ath10k_ce_init_dest_ring(struct ath10k *ar,
>  	ath10k_dbg(ar, ATH10K_DBG_BOOT,
>  		   "boot ce dest ring id %d entries %d base_addr %pK\n",
>  		   ce_id, nentries, dest_ring->base_addr_owner_space);
> -
> -	return 0;
>  }
>  
>  static int ath10k_ce_alloc_shadow_base(struct ath10k *ar,
> @@ -1659,30 +1655,14 @@ ath10k_ce_alloc_dest_ring_64(struct ath10k *ar, unsigned int ce_id,
>   * initialization. It may be that only one side or the other is
>   * initialized by software/firmware.
>   */
> -int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
> -			const struct ce_attr *attr)
> +void ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
> +			 const struct ce_attr *attr)
>  {
> -	int ret;
> -
> -	if (attr->src_nentries) {
> -		ret = ath10k_ce_init_src_ring(ar, ce_id, attr);
> -		if (ret) {
> -			ath10k_err(ar, "Failed to initialize CE src ring for ID: %d (%d)\n",
> -				   ce_id, ret);
> -			return ret;
> -		}
> -	}
> -
> -	if (attr->dest_nentries) {
> -		ret = ath10k_ce_init_dest_ring(ar, ce_id, attr);
> -		if (ret) {
> -			ath10k_err(ar, "Failed to initialize CE dest ring for ID: %d (%d)\n",
> -				   ce_id, ret);
> -			return ret;
> -		}
> -	}
> +	if (attr->src_nentries)
> +		ath10k_ce_init_src_ring(ar, ce_id, attr);
>  
> -	return 0;
> +	if (attr->dest_nentries)
> +		ath10k_ce_init_dest_ring(ar, ce_id, attr);
>  }
>  EXPORT_SYMBOL(ath10k_ce_init_pipe);
>  
> @@ -1926,7 +1906,7 @@ int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
>  }
>  EXPORT_SYMBOL(ath10k_ce_alloc_pipe);
>  
> -void ath10k_ce_alloc_rri(struct ath10k *ar)
> +int ath10k_ce_alloc_rri(struct ath10k *ar)
>  {
>  	int i;
>  	u32 value;
> @@ -1939,7 +1919,7 @@ void ath10k_ce_alloc_rri(struct ath10k *ar)
>  					   &ce->paddr_rri, GFP_KERNEL);
>  
>  	if (!ce->vaddr_rri)
> -		return;
> +		return -ENOMEM;
>  
>  	ath10k_ce_write32(ar, ar->hw_ce_regs->ce_rri_low,
>  			  lower_32_bits(ce->paddr_rri));
> @@ -1954,6 +1934,8 @@ void ath10k_ce_alloc_rri(struct ath10k *ar)
>  		value |= ar->hw_ce_regs->upd->mask;
>  		ath10k_ce_write32(ar, ce_base_addr + ctrl1_regs, value);
>  	}
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(ath10k_ce_alloc_rri);
>  
> diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
> index 666ce384a1d8..c90c00316356 100644
> --- a/drivers/net/wireless/ath/ath10k/ce.h
> +++ b/drivers/net/wireless/ath/ath10k/ce.h
> @@ -220,8 +220,8 @@ int ath10k_ce_completed_send_next_nolock(struct ath10k_ce_pipe *ce_state,
>  
>  /*==================CE Engine Initialization=======================*/
>  
> -int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
> -			const struct ce_attr *attr);
> +void ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
> +			 const struct ce_attr *attr);
>  void ath10k_ce_deinit_pipe(struct ath10k *ar, unsigned int ce_id);
>  int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
>  			 const struct ce_attr *attr);
> @@ -262,7 +262,7 @@ void ath10k_ce_enable_interrupts(struct ath10k *ar);
>  void ath10k_ce_dump_registers(struct ath10k *ar,
>  			      struct ath10k_fw_crash_data *crash_data);
>  
> -void ath10k_ce_alloc_rri(struct ath10k *ar);
> +int ath10k_ce_alloc_rri(struct ath10k *ar);
>  void ath10k_ce_free_rri(struct ath10k *ar);
>  
>  /* ce_attr.flags values */
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index a7f44f6335fb..f6988075cd83 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2524,21 +2524,13 @@ void ath10k_pci_free_pipes(struct ath10k *ar)
>  		ath10k_ce_free_pipe(ar, i);
>  }
>  
> -int ath10k_pci_init_pipes(struct ath10k *ar)
> +void ath10k_pci_init_pipes(struct ath10k *ar)
>  {
>  	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
> -	int i, ret;
> -
> -	for (i = 0; i < CE_COUNT; i++) {
> -		ret = ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
> -		if (ret) {
> -			ath10k_err(ar, "failed to initialize copy engine pipe %d: %d\n",
> -				   i, ret);
> -			return ret;
> -		}
> -	}
> +	int i;
>  
> -	return 0;
> +	for (i = 0; i < CE_COUNT; i++)
> +		ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
>  }
>  
>  static bool ath10k_pci_has_fw_crashed(struct ath10k *ar)
> @@ -2703,12 +2695,7 @@ static int ath10k_pci_qca988x_chip_reset(struct ath10k *ar)
>  		 * sufficient to verify if device is capable of booting
>  		 * firmware blob.
>  		 */
> -		ret = ath10k_pci_init_pipes(ar);
> -		if (ret) {
> -			ath10k_warn(ar, "failed to init copy engine: %d\n",
> -				    ret);
> -			continue;
> -		}
> +		ath10k_pci_init_pipes(ar);
>  
>  		ret = ath10k_pci_diag_read32(ar, QCA988X_HOST_INTEREST_ADDRESS,
>  					     &val);
> @@ -2846,11 +2833,7 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
>  		goto err_sleep;
>  	}
>  
> -	ret = ath10k_pci_init_pipes(ar);
> -	if (ret) {
> -		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
> -		goto err_sleep;
> -	}
> +	ath10k_pci_init_pipes(ar);
>  
>  	ret = ath10k_pci_init_config(ar);
>  	if (ret) {
> diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
> index 480cd97ab739..3b3ded87aef9 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.h
> +++ b/drivers/net/wireless/ath/ath10k/pci.h
> @@ -238,7 +238,7 @@ void ath10k_pci_free_pipes(struct ath10k *ar);
>  void ath10k_pci_rx_replenish_retry(struct timer_list *t);
>  void ath10k_pci_ce_deinit(struct ath10k *ar);
>  void ath10k_pci_init_napi(struct ath10k *ar);
> -int ath10k_pci_init_pipes(struct ath10k *ar);
> +void ath10k_pci_init_pipes(struct ath10k *ar);
>  int ath10k_pci_init_config(struct ath10k *ar);
>  void ath10k_pci_rx_post(struct ath10k *ar);
>  void ath10k_pci_flush(struct ath10k *ar);
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 26214c00cd0d..f41948d9eebb 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -939,20 +939,12 @@ static int ath10k_snoc_hif_start(struct ath10k *ar)
>  	return 0;
>  }
>  
> -static int ath10k_snoc_init_pipes(struct ath10k *ar)
> +static void ath10k_snoc_init_pipes(struct ath10k *ar)
>  {
> -	int i, ret;
> -
> -	for (i = 0; i < CE_COUNT; i++) {
> -		ret = ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
> -		if (ret) {
> -			ath10k_err(ar, "failed to initialize copy engine pipe %d: %d\n",
> -				   i, ret);
> -			return ret;
> -		}
> -	}
> +	int i;
>  
> -	return 0;
> +	for (i = 0; i < CE_COUNT; i++)
> +		ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
>  }
>  
>  static int ath10k_snoc_wlan_enable(struct ath10k *ar,
> @@ -1082,18 +1074,14 @@ static int ath10k_snoc_hif_power_up(struct ath10k *ar,
>  		goto err_hw_power_off;
>  	}
>  
> -	ath10k_ce_alloc_rri(ar);
> -
> -	ret = ath10k_snoc_init_pipes(ar);
> -	if (ret) {
> -		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
> -		goto err_free_rri;
> -	}
> +	ret = ath10k_ce_alloc_rri(ar);
> +	if (ret)
> +		goto err_snoc_wlan_disable;
>  
> +	ath10k_snoc_init_pipes(ar);
>  	return 0;
>  
> -err_free_rri:
> -	ath10k_ce_free_rri(ar);
> +err_snoc_wlan_disable:
>  	ath10k_snoc_wlan_disable(ar);
>  
>  err_hw_power_off:
> 


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [lvc-project] [PATCH] [v2] wifi: ath10k: cleanup CE initialization

[Antipov, Dmitriy]

On Wed, 2023-08-23 at 13:03 +0300, Alexey Khoroshilov wrote:

> > v2: adjust to match series
> 
> Do mean that nothing has been changed in the patch regarding the
> previous version?

Usually it is, including the cases where some patch of the series
is changed so the following ones are applied with offsets and thus
better to be regenerated.

And should say sorry for an inconsistent title of the first one.

Dmitry

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [lvc-project] [PATCH] [v2] wifi: ath10k: cleanup CE initialization

[Kalle Valo]

"Antipov, Dmitriy" <Dmitriy.Antipov@softline.com> writes:

> On Wed, 2023-08-23 at 13:03 +0300, Alexey Khoroshilov wrote:
>
>> > v2: adjust to match series
>> 
>> Do mean that nothing has been changed in the patch regarding the
>> previous version?
>
> Usually it is, including the cases where some patch of the series
> is changed so the following ones are applied with offsets and thus
> better to be regenerated.

Saying something like "no changes" or "rebased only" would be more
understandable. It is assumed that when sending a new version of the
patchset that offsets can change so saying "no changes" is good enough.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] [v2] wifi: ath10k: cleanup CE initialization

[Jeff Johnson]

On 8/23/2023 2:50 AM, Dmitry Antipov wrote:
> Commit 25d0dbcbd5c7 ("ath10k: split ce initialization and allocation")
> changes 'ath10k_ce_init_src_ring()' and 'ath10k_ce_init_dest_ring()'
> so these functions can't return -ENOMEM but always returns 0. This way
> both of the above may be converted to 'void' and related code may be
> simplified as well.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

IMO you are doing too many changes in a single patch which makes it 
difficult to quickly review. Please keep it simple. Suggest you change a 
single function signature per patch. Any related code simplification 
should be in a separate patch.

/jeff

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization

[Dmitry Antipov]

Commit 25d0dbcbd5c7 ("ath10k: split ce initialization and allocation")
changes 'ath10k_ce_init_src_ring()' and 'ath10k_ce_init_dest_ring()'
so these functions can't return -ENOMEM but always returns 0. This way
both of them may be converted to 'void', and 'ath10k_ce_init_pipe()'
may be simplified accordingly.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: split to smaller units per Jeff's suggestion
v2: change 'ath10k_ce_alloc_rri()' to return -ENOMEM in case
of 'dma_alloc_coherent()' failure and fix error handling in
'ath10k_snoc_hif_power_up()'
---
 drivers/net/wireless/ath/ath10k/ce.c | 38 ++++++++--------------------
 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index c27b8204718a..ace92c636733 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -1352,9 +1352,9 @@ void ath10k_ce_enable_interrupts(struct ath10k *ar)
 }
 EXPORT_SYMBOL(ath10k_ce_enable_interrupts);
 
-static int ath10k_ce_init_src_ring(struct ath10k *ar,
-				   unsigned int ce_id,
-				   const struct ce_attr *attr)
+static void ath10k_ce_init_src_ring(struct ath10k *ar,
+				    unsigned int ce_id,
+				    const struct ce_attr *attr)
 {
 	struct ath10k_ce *ce = ath10k_ce_priv(ar);
 	struct ath10k_ce_pipe *ce_state = &ce->ce_states[ce_id];
@@ -1389,13 +1389,11 @@ static int ath10k_ce_init_src_ring(struct ath10k *ar,
 	ath10k_dbg(ar, ATH10K_DBG_BOOT,
 		   "boot init ce src ring id %d entries %d base_addr %pK\n",
 		   ce_id, nentries, src_ring->base_addr_owner_space);
-
-	return 0;
 }
 
-static int ath10k_ce_init_dest_ring(struct ath10k *ar,
-				    unsigned int ce_id,
-				    const struct ce_attr *attr)
+static void ath10k_ce_init_dest_ring(struct ath10k *ar,
+				     unsigned int ce_id,
+				     const struct ce_attr *attr)
 {
 	struct ath10k_ce *ce = ath10k_ce_priv(ar);
 	struct ath10k_ce_pipe *ce_state = &ce->ce_states[ce_id];
@@ -1427,8 +1425,6 @@ static int ath10k_ce_init_dest_ring(struct ath10k *ar,
 	ath10k_dbg(ar, ATH10K_DBG_BOOT,
 		   "boot ce dest ring id %d entries %d base_addr %pK\n",
 		   ce_id, nentries, dest_ring->base_addr_owner_space);
-
-	return 0;
 }
 
 static int ath10k_ce_alloc_shadow_base(struct ath10k *ar,
@@ -1662,25 +1658,11 @@ ath10k_ce_alloc_dest_ring_64(struct ath10k *ar, unsigned int ce_id,
 int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
 			const struct ce_attr *attr)
 {
-	int ret;
-
-	if (attr->src_nentries) {
-		ret = ath10k_ce_init_src_ring(ar, ce_id, attr);
-		if (ret) {
-			ath10k_err(ar, "Failed to initialize CE src ring for ID: %d (%d)\n",
-				   ce_id, ret);
-			return ret;
-		}
-	}
+	if (attr->src_nentries)
+		ath10k_ce_init_src_ring(ar, ce_id, attr);
 
-	if (attr->dest_nentries) {
-		ret = ath10k_ce_init_dest_ring(ar, ce_id, attr);
-		if (ret) {
-			ath10k_err(ar, "Failed to initialize CE dest ring for ID: %d (%d)\n",
-				   ce_id, ret);
-			return ret;
-		}
-	}
+	if (attr->dest_nentries)
+		ath10k_ce_init_dest_ring(ar, ce_id, attr);
 
 	return 0;
 }
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/6] [v3] wifi: ath10k: simplify ath10k_ce_init_pipe()

[Dmitry Antipov]

Convert 'ath10k_ce_init_pipe()' to return 'void' and thus
simplify 'ath10k_pci_init_pipes()' and 'ath10k_snoc_init_pipes()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: split from the larger v2 patch
---
 drivers/net/wireless/ath/ath10k/ce.c   |  6 ++----
 drivers/net/wireless/ath/ath10k/ce.h   |  4 ++--
 drivers/net/wireless/ath/ath10k/pci.c  | 12 +++---------
 drivers/net/wireless/ath/ath10k/snoc.c | 12 +++---------
 4 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index ace92c636733..73aa3632b23c 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -1655,16 +1655,14 @@ ath10k_ce_alloc_dest_ring_64(struct ath10k *ar, unsigned int ce_id,
  * initialization. It may be that only one side or the other is
  * initialized by software/firmware.
  */
-int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
-			const struct ce_attr *attr)
+void ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
+			 const struct ce_attr *attr)
 {
 	if (attr->src_nentries)
 		ath10k_ce_init_src_ring(ar, ce_id, attr);
 
 	if (attr->dest_nentries)
 		ath10k_ce_init_dest_ring(ar, ce_id, attr);
-
-	return 0;
 }
 EXPORT_SYMBOL(ath10k_ce_init_pipe);
 
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index 666ce384a1d8..a0b408176f7f 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -220,8 +220,8 @@ int ath10k_ce_completed_send_next_nolock(struct ath10k_ce_pipe *ce_state,
 
 /*==================CE Engine Initialization=======================*/
 
-int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
-			const struct ce_attr *attr);
+void ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
+			 const struct ce_attr *attr);
 void ath10k_ce_deinit_pipe(struct ath10k *ar, unsigned int ce_id);
 int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
 			 const struct ce_attr *attr);
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index a7f44f6335fb..8e05326400bb 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2527,16 +2527,10 @@ void ath10k_pci_free_pipes(struct ath10k *ar)
 int ath10k_pci_init_pipes(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
-	int i, ret;
+	int i;
 
-	for (i = 0; i < CE_COUNT; i++) {
-		ret = ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
-		if (ret) {
-			ath10k_err(ar, "failed to initialize copy engine pipe %d: %d\n",
-				   i, ret);
-			return ret;
-		}
-	}
+	for (i = 0; i < CE_COUNT; i++)
+		ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
 
 	return 0;
 }
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 26214c00cd0d..4f835ad60080 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -941,16 +941,10 @@ static int ath10k_snoc_hif_start(struct ath10k *ar)
 
 static int ath10k_snoc_init_pipes(struct ath10k *ar)
 {
-	int i, ret;
+	int i;
 
-	for (i = 0; i < CE_COUNT; i++) {
-		ret = ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
-		if (ret) {
-			ath10k_err(ar, "failed to initialize copy engine pipe %d: %d\n",
-				   i, ret);
-			return ret;
-		}
-	}
+	for (i = 0; i < CE_COUNT; i++)
+		ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
 
 	return 0;
 }
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/6] [v3] wifi: ath10k: simplify ath10k_ce_init_pipe()

[Jeff Johnson]

On 8/23/2023 10:51 PM, Dmitry Antipov wrote:
> Convert 'ath10k_ce_init_pipe()' to return 'void' and thus
> simplify 'ath10k_pci_init_pipes()' and 'ath10k_snoc_init_pipes()'.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

> ---
> v3: split from the larger v2 patch
> ---
>   drivers/net/wireless/ath/ath10k/ce.c   |  6 ++----
>   drivers/net/wireless/ath/ath10k/ce.h   |  4 ++--
>   drivers/net/wireless/ath/ath10k/pci.c  | 12 +++---------
>   drivers/net/wireless/ath/ath10k/snoc.c | 12 +++---------
>   4 files changed, 10 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
> index ace92c636733..73aa3632b23c 100644
> --- a/drivers/net/wireless/ath/ath10k/ce.c
> +++ b/drivers/net/wireless/ath/ath10k/ce.c
> @@ -1655,16 +1655,14 @@ ath10k_ce_alloc_dest_ring_64(struct ath10k *ar, unsigned int ce_id,
>    * initialization. It may be that only one side or the other is
>    * initialized by software/firmware.
>    */
> -int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
> -			const struct ce_attr *attr)
> +void ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
> +			 const struct ce_attr *attr)
>   {
>   	if (attr->src_nentries)
>   		ath10k_ce_init_src_ring(ar, ce_id, attr);
>   
>   	if (attr->dest_nentries)
>   		ath10k_ce_init_dest_ring(ar, ce_id, attr);
> -
> -	return 0;
>   }
>   EXPORT_SYMBOL(ath10k_ce_init_pipe);
>   
> diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
> index 666ce384a1d8..a0b408176f7f 100644
> --- a/drivers/net/wireless/ath/ath10k/ce.h
> +++ b/drivers/net/wireless/ath/ath10k/ce.h
> @@ -220,8 +220,8 @@ int ath10k_ce_completed_send_next_nolock(struct ath10k_ce_pipe *ce_state,
>   
>   /*==================CE Engine Initialization=======================*/
>   
> -int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
> -			const struct ce_attr *attr);
> +void ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
> +			 const struct ce_attr *attr);
>   void ath10k_ce_deinit_pipe(struct ath10k *ar, unsigned int ce_id);
>   int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
>   			 const struct ce_attr *attr);
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index a7f44f6335fb..8e05326400bb 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2527,16 +2527,10 @@ void ath10k_pci_free_pipes(struct ath10k *ar)
>   int ath10k_pci_init_pipes(struct ath10k *ar)
>   {
>   	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
> -	int i, ret;
> +	int i;
>   
> -	for (i = 0; i < CE_COUNT; i++) {
> -		ret = ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
> -		if (ret) {
> -			ath10k_err(ar, "failed to initialize copy engine pipe %d: %d\n",
> -				   i, ret);
> -			return ret;
> -		}
> -	}
> +	for (i = 0; i < CE_COUNT; i++)
> +		ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
>   
>   	return 0;
>   }
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 26214c00cd0d..4f835ad60080 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -941,16 +941,10 @@ static int ath10k_snoc_hif_start(struct ath10k *ar)
>   
>   static int ath10k_snoc_init_pipes(struct ath10k *ar)
>   {
> -	int i, ret;
> +	int i;
>   
> -	for (i = 0; i < CE_COUNT; i++) {
> -		ret = ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
> -		if (ret) {
> -			ath10k_err(ar, "failed to initialize copy engine pipe %d: %d\n",
> -				   i, ret);
> -			return ret;
> -		}
> -	}
> +	for (i = 0; i < CE_COUNT; i++)
> +		ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
>   
>   	return 0;
>   }


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 3/6] [v3] wifi: ath10k: cleanup CE pipes initialization

[Dmitry Antipov]

Convert 'ath10k_pci_init_pipes()' and 'ath10k_snoc_init_pipes()'
to 'void' and thus simplify 'ath10k_ahb_hif_power_up()',
'ath10k_pci_qca988x_chip_reset()' and 'ath10k_snoc_hif_power_up()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: split from the larger v2 patch
---
 drivers/net/wireless/ath/ath10k/ahb.c  |  6 +-----
 drivers/net/wireless/ath/ath10k/pci.c  | 17 +++--------------
 drivers/net/wireless/ath/ath10k/pci.h  |  2 +-
 drivers/net/wireless/ath/ath10k/snoc.c | 16 ++--------------
 4 files changed, 7 insertions(+), 34 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
index 76efea2f1138..c3a2eeb5542d 100644
--- a/drivers/net/wireless/ath/ath10k/ahb.c
+++ b/drivers/net/wireless/ath/ath10k/ahb.c
@@ -655,11 +655,7 @@ static int ath10k_ahb_hif_power_up(struct ath10k *ar,
 		goto out;
 	}
 
-	ret = ath10k_pci_init_pipes(ar);
-	if (ret) {
-		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
-		goto out;
-	}
+	ath10k_pci_init_pipes(ar);
 
 	ret = ath10k_pci_init_config(ar);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 8e05326400bb..f6988075cd83 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2524,15 +2524,13 @@ void ath10k_pci_free_pipes(struct ath10k *ar)
 		ath10k_ce_free_pipe(ar, i);
 }
 
-int ath10k_pci_init_pipes(struct ath10k *ar)
+void ath10k_pci_init_pipes(struct ath10k *ar)
 {
 	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
 	int i;
 
 	for (i = 0; i < CE_COUNT; i++)
 		ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
-
-	return 0;
 }
 
 static bool ath10k_pci_has_fw_crashed(struct ath10k *ar)
@@ -2697,12 +2695,7 @@ static int ath10k_pci_qca988x_chip_reset(struct ath10k *ar)
 		 * sufficient to verify if device is capable of booting
 		 * firmware blob.
 		 */
-		ret = ath10k_pci_init_pipes(ar);
-		if (ret) {
-			ath10k_warn(ar, "failed to init copy engine: %d\n",
-				    ret);
-			continue;
-		}
+		ath10k_pci_init_pipes(ar);
 
 		ret = ath10k_pci_diag_read32(ar, QCA988X_HOST_INTEREST_ADDRESS,
 					     &val);
@@ -2840,11 +2833,7 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
 		goto err_sleep;
 	}
 
-	ret = ath10k_pci_init_pipes(ar);
-	if (ret) {
-		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
-		goto err_sleep;
-	}
+	ath10k_pci_init_pipes(ar);
 
 	ret = ath10k_pci_init_config(ar);
 	if (ret) {
diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
index 480cd97ab739..3b3ded87aef9 100644
--- a/drivers/net/wireless/ath/ath10k/pci.h
+++ b/drivers/net/wireless/ath/ath10k/pci.h
@@ -238,7 +238,7 @@ void ath10k_pci_free_pipes(struct ath10k *ar);
 void ath10k_pci_rx_replenish_retry(struct timer_list *t);
 void ath10k_pci_ce_deinit(struct ath10k *ar);
 void ath10k_pci_init_napi(struct ath10k *ar);
-int ath10k_pci_init_pipes(struct ath10k *ar);
+void ath10k_pci_init_pipes(struct ath10k *ar);
 int ath10k_pci_init_config(struct ath10k *ar);
 void ath10k_pci_rx_post(struct ath10k *ar);
 void ath10k_pci_flush(struct ath10k *ar);
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 4f835ad60080..815df15f58fb 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -939,14 +939,12 @@ static int ath10k_snoc_hif_start(struct ath10k *ar)
 	return 0;
 }
 
-static int ath10k_snoc_init_pipes(struct ath10k *ar)
+static void ath10k_snoc_init_pipes(struct ath10k *ar)
 {
 	int i;
 
 	for (i = 0; i < CE_COUNT; i++)
 		ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
-
-	return 0;
 }
 
 static int ath10k_snoc_wlan_enable(struct ath10k *ar,
@@ -1078,17 +1076,7 @@ static int ath10k_snoc_hif_power_up(struct ath10k *ar,
 
 	ath10k_ce_alloc_rri(ar);
 
-	ret = ath10k_snoc_init_pipes(ar);
-	if (ret) {
-		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
-		goto err_free_rri;
-	}
-
-	return 0;
-
-err_free_rri:
-	ath10k_ce_free_rri(ar);
-	ath10k_snoc_wlan_disable(ar);
+	ath10k_snoc_init_pipes(ar);
 
 err_hw_power_off:
 	ath10k_hw_power_off(ar);
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 3/6] [v3] wifi: ath10k: cleanup CE pipes initialization

[Jeff Johnson]

On 8/23/2023 10:51 PM, Dmitry Antipov wrote:
> Convert 'ath10k_pci_init_pipes()' and 'ath10k_snoc_init_pipes()'
> to 'void' and thus simplify 'ath10k_ahb_hif_power_up()',
> 'ath10k_pci_qca988x_chip_reset()' and 'ath10k_snoc_hif_power_up()'.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

> ---
> v3: split from the larger v2 patch
> ---
>   drivers/net/wireless/ath/ath10k/ahb.c  |  6 +-----
>   drivers/net/wireless/ath/ath10k/pci.c  | 17 +++--------------
>   drivers/net/wireless/ath/ath10k/pci.h  |  2 +-
>   drivers/net/wireless/ath/ath10k/snoc.c | 16 ++--------------
>   4 files changed, 7 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/ahb.c b/drivers/net/wireless/ath/ath10k/ahb.c
> index 76efea2f1138..c3a2eeb5542d 100644
> --- a/drivers/net/wireless/ath/ath10k/ahb.c
> +++ b/drivers/net/wireless/ath/ath10k/ahb.c
> @@ -655,11 +655,7 @@ static int ath10k_ahb_hif_power_up(struct ath10k *ar,
>   		goto out;
>   	}
>   
> -	ret = ath10k_pci_init_pipes(ar);
> -	if (ret) {
> -		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
> -		goto out;
> -	}
> +	ath10k_pci_init_pipes(ar);
>   
>   	ret = ath10k_pci_init_config(ar);
>   	if (ret) {
> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
> index 8e05326400bb..f6988075cd83 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2524,15 +2524,13 @@ void ath10k_pci_free_pipes(struct ath10k *ar)
>   		ath10k_ce_free_pipe(ar, i);
>   }
>   
> -int ath10k_pci_init_pipes(struct ath10k *ar)
> +void ath10k_pci_init_pipes(struct ath10k *ar)
>   {
>   	struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>   	int i;
>   
>   	for (i = 0; i < CE_COUNT; i++)
>   		ath10k_ce_init_pipe(ar, i, &ar_pci->attr[i]);
> -
> -	return 0;
>   }
>   
>   static bool ath10k_pci_has_fw_crashed(struct ath10k *ar)
> @@ -2697,12 +2695,7 @@ static int ath10k_pci_qca988x_chip_reset(struct ath10k *ar)
>   		 * sufficient to verify if device is capable of booting
>   		 * firmware blob.
>   		 */
> -		ret = ath10k_pci_init_pipes(ar);
> -		if (ret) {
> -			ath10k_warn(ar, "failed to init copy engine: %d\n",
> -				    ret);
> -			continue;
> -		}
> +		ath10k_pci_init_pipes(ar);
>   
>   		ret = ath10k_pci_diag_read32(ar, QCA988X_HOST_INTEREST_ADDRESS,
>   					     &val);
> @@ -2840,11 +2833,7 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar,
>   		goto err_sleep;
>   	}
>   
> -	ret = ath10k_pci_init_pipes(ar);
> -	if (ret) {
> -		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
> -		goto err_sleep;
> -	}
> +	ath10k_pci_init_pipes(ar);
>   
>   	ret = ath10k_pci_init_config(ar);
>   	if (ret) {
> diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
> index 480cd97ab739..3b3ded87aef9 100644
> --- a/drivers/net/wireless/ath/ath10k/pci.h
> +++ b/drivers/net/wireless/ath/ath10k/pci.h
> @@ -238,7 +238,7 @@ void ath10k_pci_free_pipes(struct ath10k *ar);
>   void ath10k_pci_rx_replenish_retry(struct timer_list *t);
>   void ath10k_pci_ce_deinit(struct ath10k *ar);
>   void ath10k_pci_init_napi(struct ath10k *ar);
> -int ath10k_pci_init_pipes(struct ath10k *ar);
> +void ath10k_pci_init_pipes(struct ath10k *ar);
>   int ath10k_pci_init_config(struct ath10k *ar);
>   void ath10k_pci_rx_post(struct ath10k *ar);
>   void ath10k_pci_flush(struct ath10k *ar);
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 4f835ad60080..815df15f58fb 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -939,14 +939,12 @@ static int ath10k_snoc_hif_start(struct ath10k *ar)
>   	return 0;
>   }
>   
> -static int ath10k_snoc_init_pipes(struct ath10k *ar)
> +static void ath10k_snoc_init_pipes(struct ath10k *ar)
>   {
>   	int i;
>   
>   	for (i = 0; i < CE_COUNT; i++)
>   		ath10k_ce_init_pipe(ar, i, &host_ce_config_wlan[i]);
> -
> -	return 0;
>   }
>   
>   static int ath10k_snoc_wlan_enable(struct ath10k *ar,
> @@ -1078,17 +1076,7 @@ static int ath10k_snoc_hif_power_up(struct ath10k *ar,
>   
>   	ath10k_ce_alloc_rri(ar);
>   
> -	ret = ath10k_snoc_init_pipes(ar);
> -	if (ret) {
> -		ath10k_err(ar, "failed to initialize CE: %d\n", ret);
> -		goto err_free_rri;
> -	}
> -
> -	return 0;
> -
> -err_free_rri:
> -	ath10k_ce_free_rri(ar);
> -	ath10k_snoc_wlan_disable(ar);
> +	ath10k_snoc_init_pipes(ar);
>   
>   err_hw_power_off:
>   	ath10k_hw_power_off(ar);


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 4/6] [v3] wifi: ath10k: do not ignore possible dma_alloc_coherent() error

[Dmitry Antipov]

Change 'ath10k_ce_alloc_rri()' to return -ENOMEM in case
of 'dma_alloc_coherent()' failure and fix error handling in
'ath10k_snoc_hif_power_up()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: split from the larger v2 patch
---
 drivers/net/wireless/ath/ath10k/ce.c   | 6 ++++--
 drivers/net/wireless/ath/ath10k/ce.h   | 2 +-
 drivers/net/wireless/ath/ath10k/snoc.c | 9 ++++++++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
index 73aa3632b23c..d7275dcc1f99 100644
--- a/drivers/net/wireless/ath/ath10k/ce.c
+++ b/drivers/net/wireless/ath/ath10k/ce.c
@@ -1906,7 +1906,7 @@ int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
 }
 EXPORT_SYMBOL(ath10k_ce_alloc_pipe);
 
-void ath10k_ce_alloc_rri(struct ath10k *ar)
+int ath10k_ce_alloc_rri(struct ath10k *ar)
 {
 	int i;
 	u32 value;
@@ -1919,7 +1919,7 @@ void ath10k_ce_alloc_rri(struct ath10k *ar)
 					   &ce->paddr_rri, GFP_KERNEL);
 
 	if (!ce->vaddr_rri)
-		return;
+		return -ENOMEM;
 
 	ath10k_ce_write32(ar, ar->hw_ce_regs->ce_rri_low,
 			  lower_32_bits(ce->paddr_rri));
@@ -1934,6 +1934,8 @@ void ath10k_ce_alloc_rri(struct ath10k *ar)
 		value |= ar->hw_ce_regs->upd->mask;
 		ath10k_ce_write32(ar, ce_base_addr + ctrl1_regs, value);
 	}
+
+	return 0;
 }
 EXPORT_SYMBOL(ath10k_ce_alloc_rri);
 
diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
index a0b408176f7f..c90c00316356 100644
--- a/drivers/net/wireless/ath/ath10k/ce.h
+++ b/drivers/net/wireless/ath/ath10k/ce.h
@@ -262,7 +262,7 @@ void ath10k_ce_enable_interrupts(struct ath10k *ar);
 void ath10k_ce_dump_registers(struct ath10k *ar,
 			      struct ath10k_fw_crash_data *crash_data);
 
-void ath10k_ce_alloc_rri(struct ath10k *ar);
+int ath10k_ce_alloc_rri(struct ath10k *ar);
 void ath10k_ce_free_rri(struct ath10k *ar);
 
 /* ce_attr.flags values */
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 815df15f58fb..b3acb6ad6f45 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -1074,10 +1074,17 @@ static int ath10k_snoc_hif_power_up(struct ath10k *ar,
 		goto err_hw_power_off;
 	}
 
-	ath10k_ce_alloc_rri(ar);
+	ret = ath10k_ce_alloc_rri(ar);
+	if (ret)
+		goto err_snoc_wlan_disable;
 
 	ath10k_snoc_init_pipes(ar);
 
+	return 0;
+
+err_snoc_wlan_disable:
+	ath10k_snoc_wlan_disable(ar);
+
 err_hw_power_off:
 	ath10k_hw_power_off(ar);
 
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/6] [v3] wifi: ath10k: do not ignore possible dma_alloc_coherent() error

[Jeff Johnson]

On 8/23/2023 10:51 PM, Dmitry Antipov wrote:
> Change 'ath10k_ce_alloc_rri()' to return -ENOMEM in case
> of 'dma_alloc_coherent()' failure and fix error handling in
> 'ath10k_snoc_hif_power_up()'.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

> ---
> v3: split from the larger v2 patch
> ---
>   drivers/net/wireless/ath/ath10k/ce.c   | 6 ++++--
>   drivers/net/wireless/ath/ath10k/ce.h   | 2 +-
>   drivers/net/wireless/ath/ath10k/snoc.c | 9 ++++++++-
>   3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
> index 73aa3632b23c..d7275dcc1f99 100644
> --- a/drivers/net/wireless/ath/ath10k/ce.c
> +++ b/drivers/net/wireless/ath/ath10k/ce.c
> @@ -1906,7 +1906,7 @@ int ath10k_ce_alloc_pipe(struct ath10k *ar, int ce_id,
>   }
>   EXPORT_SYMBOL(ath10k_ce_alloc_pipe);
>   
> -void ath10k_ce_alloc_rri(struct ath10k *ar)
> +int ath10k_ce_alloc_rri(struct ath10k *ar)
>   {
>   	int i;
>   	u32 value;
> @@ -1919,7 +1919,7 @@ void ath10k_ce_alloc_rri(struct ath10k *ar)
>   					   &ce->paddr_rri, GFP_KERNEL);
>   
>   	if (!ce->vaddr_rri)
> -		return;
> +		return -ENOMEM;
>   
>   	ath10k_ce_write32(ar, ar->hw_ce_regs->ce_rri_low,
>   			  lower_32_bits(ce->paddr_rri));
> @@ -1934,6 +1934,8 @@ void ath10k_ce_alloc_rri(struct ath10k *ar)
>   		value |= ar->hw_ce_regs->upd->mask;
>   		ath10k_ce_write32(ar, ce_base_addr + ctrl1_regs, value);
>   	}
> +
> +	return 0;
>   }
>   EXPORT_SYMBOL(ath10k_ce_alloc_rri);
>   
> diff --git a/drivers/net/wireless/ath/ath10k/ce.h b/drivers/net/wireless/ath/ath10k/ce.h
> index a0b408176f7f..c90c00316356 100644
> --- a/drivers/net/wireless/ath/ath10k/ce.h
> +++ b/drivers/net/wireless/ath/ath10k/ce.h
> @@ -262,7 +262,7 @@ void ath10k_ce_enable_interrupts(struct ath10k *ar);
>   void ath10k_ce_dump_registers(struct ath10k *ar,
>   			      struct ath10k_fw_crash_data *crash_data);
>   
> -void ath10k_ce_alloc_rri(struct ath10k *ar);
> +int ath10k_ce_alloc_rri(struct ath10k *ar);
>   void ath10k_ce_free_rri(struct ath10k *ar);
>   
>   /* ce_attr.flags values */
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 815df15f58fb..b3acb6ad6f45 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1074,10 +1074,17 @@ static int ath10k_snoc_hif_power_up(struct ath10k *ar,
>   		goto err_hw_power_off;
>   	}
>   
> -	ath10k_ce_alloc_rri(ar);
> +	ret = ath10k_ce_alloc_rri(ar);
> +	if (ret)
> +		goto err_snoc_wlan_disable;
>   
>   	ath10k_snoc_init_pipes(ar);
>   
> +	return 0;
> +
> +err_snoc_wlan_disable:
> +	ath10k_snoc_wlan_disable(ar);
> +
>   err_hw_power_off:
>   	ath10k_hw_power_off(ar);
>   


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 5/6] [v3] wifi: ath10k: simplify ath10k_peer_assoc_h_vht()

[Dmitry Antipov]

Commit 3db24065c2c8 ("ath10k: enable VHT160 and VHT80+80 modes")
introduces 'get_160mhz_nss_from_maxrate()' which never returns 0,
which means that 'ath10k_peer_assoc_h_vht()' may be simplified.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: add Acked-by: otherwise unchanged
v2: adjust to match series
---
 drivers/net/wireless/ath/ath10k/mac.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 03e7bc5b6c0b..148d0fab4418 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2631,12 +2631,8 @@ static void ath10k_peer_assoc_h_vht(struct ath10k *ar,
 		u32 max_rate;
 
 		max_rate = arg->peer_vht_rates.rx_max_rate;
-		rx_nss = get_160mhz_nss_from_maxrate(max_rate);
-
-		if (rx_nss == 0)
-			rx_nss = arg->peer_num_spatial_streams;
-		else
-			rx_nss = min(arg->peer_num_spatial_streams, rx_nss);
+		rx_nss = min(arg->peer_num_spatial_streams,
+			     get_160mhz_nss_from_maxrate(max_rate));
 
 		max_rate = hw->vht160_mcs_tx_highest;
 		rx_nss = min(rx_nss, get_160mhz_nss_from_maxrate(max_rate));
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 6/6] [v3] wifi: ath10k: simplify ath10k_pci_pm_suspend()

[Dmitry Antipov]

Since 'ath10k_pci_suspend()' always returns 0, it may be converted to
'void' and 'ath10k_pci_pm_suspend()' may be simplified accordingly.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: add Acked-by: otherwise unchanged
v2: adjust to match series
---
 drivers/net/wireless/ath/ath10k/pci.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index f6988075cd83..16037e77264b 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2871,7 +2871,7 @@ static int ath10k_pci_hif_suspend(struct ath10k *ar)
 	return 0;
 }
 
-static int ath10k_pci_suspend(struct ath10k *ar)
+static void ath10k_pci_suspend(struct ath10k *ar)
 {
 	/* The grace timer can still be counting down and ar->ps_awake be true.
 	 * It is known that the device may be asleep after resuming regardless
@@ -2879,8 +2879,6 @@ static int ath10k_pci_suspend(struct ath10k *ar)
 	 * device is asleep before proceeding.
 	 */
 	ath10k_pci_sleep_sync(ar);
-
-	return 0;
 }
 
 static int ath10k_pci_hif_resume(struct ath10k *ar)
@@ -3734,13 +3732,9 @@ MODULE_DEVICE_TABLE(pci, ath10k_pci_id_table);
 static __maybe_unused int ath10k_pci_pm_suspend(struct device *dev)
 {
 	struct ath10k *ar = dev_get_drvdata(dev);
-	int ret;
 
-	ret = ath10k_pci_suspend(ar);
-	if (ret)
-		ath10k_warn(ar, "failed to suspend hif: %d\n", ret);
-
-	return ret;
+	ath10k_pci_suspend(ar);
+	return 0;
 }
 
 static __maybe_unused int ath10k_pci_pm_resume(struct device *dev)
-- 
2.41.0


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization

[Jeff Johnson]

On 8/23/2023 10:51 PM, Dmitry Antipov wrote:
> Commit 25d0dbcbd5c7 ("ath10k: split ce initialization and allocation")
> changes 'ath10k_ce_init_src_ring()' and 'ath10k_ce_init_dest_ring()'
> so these functions can't return -ENOMEM but always returns 0. This way
> both of them may be converted to 'void', and 'ath10k_ce_init_pipe()'
> may be simplified accordingly.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>

> ---
> v3: split to smaller units per Jeff's suggestion
> v2: change 'ath10k_ce_alloc_rri()' to return -ENOMEM in case
> of 'dma_alloc_coherent()' failure and fix error handling in
> 'ath10k_snoc_hif_power_up()'
> ---
>   drivers/net/wireless/ath/ath10k/ce.c | 38 ++++++++--------------------
>   1 file changed, 10 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/ce.c b/drivers/net/wireless/ath/ath10k/ce.c
> index c27b8204718a..ace92c636733 100644
> --- a/drivers/net/wireless/ath/ath10k/ce.c
> +++ b/drivers/net/wireless/ath/ath10k/ce.c
> @@ -1352,9 +1352,9 @@ void ath10k_ce_enable_interrupts(struct ath10k *ar)
>   }
>   EXPORT_SYMBOL(ath10k_ce_enable_interrupts);
>   
> -static int ath10k_ce_init_src_ring(struct ath10k *ar,
> -				   unsigned int ce_id,
> -				   const struct ce_attr *attr)
> +static void ath10k_ce_init_src_ring(struct ath10k *ar,
> +				    unsigned int ce_id,
> +				    const struct ce_attr *attr)
>   {
>   	struct ath10k_ce *ce = ath10k_ce_priv(ar);
>   	struct ath10k_ce_pipe *ce_state = &ce->ce_states[ce_id];
> @@ -1389,13 +1389,11 @@ static int ath10k_ce_init_src_ring(struct ath10k *ar,
>   	ath10k_dbg(ar, ATH10K_DBG_BOOT,
>   		   "boot init ce src ring id %d entries %d base_addr %pK\n",
>   		   ce_id, nentries, src_ring->base_addr_owner_space);
> -
> -	return 0;
>   }
>   
> -static int ath10k_ce_init_dest_ring(struct ath10k *ar,
> -				    unsigned int ce_id,
> -				    const struct ce_attr *attr)
> +static void ath10k_ce_init_dest_ring(struct ath10k *ar,
> +				     unsigned int ce_id,
> +				     const struct ce_attr *attr)
>   {
>   	struct ath10k_ce *ce = ath10k_ce_priv(ar);
>   	struct ath10k_ce_pipe *ce_state = &ce->ce_states[ce_id];
> @@ -1427,8 +1425,6 @@ static int ath10k_ce_init_dest_ring(struct ath10k *ar,
>   	ath10k_dbg(ar, ATH10K_DBG_BOOT,
>   		   "boot ce dest ring id %d entries %d base_addr %pK\n",
>   		   ce_id, nentries, dest_ring->base_addr_owner_space);
> -
> -	return 0;
>   }
>   
>   static int ath10k_ce_alloc_shadow_base(struct ath10k *ar,
> @@ -1662,25 +1658,11 @@ ath10k_ce_alloc_dest_ring_64(struct ath10k *ar, unsigned int ce_id,
>   int ath10k_ce_init_pipe(struct ath10k *ar, unsigned int ce_id,
>   			const struct ce_attr *attr)
>   {
> -	int ret;
> -
> -	if (attr->src_nentries) {
> -		ret = ath10k_ce_init_src_ring(ar, ce_id, attr);
> -		if (ret) {
> -			ath10k_err(ar, "Failed to initialize CE src ring for ID: %d (%d)\n",
> -				   ce_id, ret);
> -			return ret;
> -		}
> -	}
> +	if (attr->src_nentries)
> +		ath10k_ce_init_src_ring(ar, ce_id, attr);
>   
> -	if (attr->dest_nentries) {
> -		ret = ath10k_ce_init_dest_ring(ar, ce_id, attr);
> -		if (ret) {
> -			ath10k_err(ar, "Failed to initialize CE dest ring for ID: %d (%d)\n",
> -				   ce_id, ret);
> -			return ret;
> -		}
> -	}
> +	if (attr->dest_nentries)
> +		ath10k_ce_init_dest_ring(ar, ce_id, attr);
>   
>   	return 0;
>   }


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> Commit 25d0dbcbd5c7 ("ath10k: split ce initialization and allocation")
> changes 'ath10k_ce_init_src_ring()' and 'ath10k_ce_init_dest_ring()'
> so these functions can't return -ENOMEM but always returns 0. This way
> both of them may be converted to 'void', and 'ath10k_ce_init_pipe()'
> may be simplified accordingly.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v3: split to smaller units per Jeff's suggestion
> v2: change 'ath10k_ce_alloc_rri()' to return -ENOMEM in case
> of 'dma_alloc_coherent()' failure and fix error handling in
> 'ath10k_snoc_hif_power_up()'

[...]

> -static int ath10k_ce_init_src_ring(struct ath10k *ar,
> -				   unsigned int ce_id,
> -				   const struct ce_attr *attr)
> +static void ath10k_ce_init_src_ring(struct ath10k *ar,
> +				    unsigned int ce_id,
> +				    const struct ce_attr *attr)

I have on purpose avoided to use void functions in ath10k/ath11k/ath12k.
The problem is that if some of the functions return void and some of the
functions return int it's much harder to review the code. If most/all of
the functions return the same error value type as int it makes a lot
easier to read the code.

Is there a benefit from function returning void? Why do this in the
first place?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization

[Dmitry Antipov]

On 9/20/23 16:23, Kalle Valo wrote:

> I have on purpose avoided to use void functions in ath10k/ath11k/ath12k.
> The problem is that if some of the functions return void and some of the
> functions return int it's much harder to review the code. 

I realize that you're constantly overloaded with reviewing a lot of patches...

> If most/all of the functions return the same error value type as int  > it makes a lot easier to read the code.

...but still not sure that this is reasonable for any non-trivial piece
of the source code.

OTOH if both you and Jeff are agreed on preserving existing ath1x style,
I will definitely take this decision into account and try to redesign
this series in attempt to fit the current design as much as possible.

Dmitry


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

>> If most/all of the functions return the same error value type as int
>> it makes a lot easier to read the code.
>
> ...but still not sure that this is reasonable for any non-trivial piece
> of the source code.

What's the concrete benefit from having few functions which return void
instead of 'return 0'? For me the benefit would have to be significant
justifying the code churn and the time used.

> OTOH if both you and Jeff are agreed on preserving existing ath1x style,
> I will definitely take this decision into account and try to redesign
> this series in attempt to fit the current design as much as possible.

Please stop fixing the design (unless you are the driver maintainer or
asked specifically by one) as that doesn't really benefit anyone,
actually the opposite. Instead focus on fixing actual bugs. But if you
have no means for testing your fixes then stick to compiler warnings and
similar.

For example, didn't I suggest you about fixing all sparse warnings in
wireless code? I would be very happy to get such patches as we really
would want to be sparse warning free.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k