| Message ID | 20230824055117.42309-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/6,v3] wifi: ath10k: cleanup CE ring initialization | expand |
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; > }
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?
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
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.
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; }
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(-)