All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kalle Valo <kvalo@kernel.org>
To: Dmitry Antipov <dmantipov@yandex.ru>
Cc: Jeff Johnson <quic_jjohnson@quicinc.com>,
	linux-wireless@vger.kernel.org,  lvc-project@linuxtesting.org,
	ath10k@lists.infradead.org
Subject: Re: [PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization
Date: Wed, 20 Sep 2023 16:23:16 +0300	[thread overview]
Message-ID: <874jjpashn.fsf@kernel.org> (raw)
In-Reply-To: <20230824055117.42309-1-dmantipov@yandex.ru> (Dmitry Antipov's message of "Thu, 24 Aug 2023 08:51:06 +0300")

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

WARNING: multiple messages have this Message-ID (diff)
From: Kalle Valo <kvalo@kernel.org>
To: Dmitry Antipov <dmantipov@yandex.ru>
Cc: Jeff Johnson <quic_jjohnson@quicinc.com>,
	linux-wireless@vger.kernel.org, lvc-project@linuxtesting.org,
	ath10k@lists.infradead.org
Subject: Re: [PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization
Date: Wed, 20 Sep 2023 16:23:16 +0300	[thread overview]
Message-ID: <874jjpashn.fsf@kernel.org> (raw)
In-Reply-To: <20230824055117.42309-1-dmantipov@yandex.ru> (Dmitry Antipov's message of "Thu, 24 Aug 2023 08:51:06 +0300")

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

  parent reply	other threads:[~2023-09-20 13:23 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-23  9:50 [PATCH] [v2] wifi: ath10k: cleanup CE initialization Dmitry Antipov
2023-08-23  9:50 ` Dmitry Antipov
2023-08-23  9:50 ` [PATCH 2/3] [v2] wifi: ath10k: simplify ath10k_peer_assoc_h_vht() Dmitry Antipov
2023-08-23  9:50   ` Dmitry Antipov
2023-08-23 14:55   ` Jeff Johnson
2023-08-23 14:55     ` Jeff Johnson
2023-08-23  9:50 ` [PATCH 3/3] [v2] wifi: ath10k: simplify ath10k_pci_pm_suspend() Dmitry Antipov
2023-08-23  9:50   ` Dmitry Antipov
2023-08-23 14:57   ` Jeff Johnson
2023-08-23 14:57     ` Jeff Johnson
2023-08-23 10:03 ` [lvc-project] [PATCH] [v2] wifi: ath10k: cleanup CE initialization Alexey Khoroshilov
2023-08-23 10:03   ` Alexey Khoroshilov
2023-08-23 10:13   ` Antipov, Dmitriy
2023-08-23 10:13     ` Antipov, Dmitriy
2023-08-25  7:47     ` Kalle Valo
2023-08-25  7:47       ` Kalle Valo
2023-08-23 14:53 ` Jeff Johnson
2023-08-23 14:53   ` Jeff Johnson
2023-08-24  5:51   ` [PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization Dmitry Antipov
2023-08-24  5:51     ` Dmitry Antipov
2023-08-24  5:51     ` [PATCH 2/6] [v3] wifi: ath10k: simplify ath10k_ce_init_pipe() Dmitry Antipov
2023-08-24  5:51       ` Dmitry Antipov
2023-08-24 15:26       ` Jeff Johnson
2023-08-24 15:26         ` Jeff Johnson
2023-08-24  5:51     ` [PATCH 3/6] [v3] wifi: ath10k: cleanup CE pipes initialization Dmitry Antipov
2023-08-24  5:51       ` Dmitry Antipov
2023-08-24 15:27       ` Jeff Johnson
2023-08-24 15:27         ` Jeff Johnson
2023-08-24  5:51     ` [PATCH 4/6] [v3] wifi: ath10k: do not ignore possible dma_alloc_coherent() error Dmitry Antipov
2023-08-24  5:51       ` Dmitry Antipov
2023-08-24 15:27       ` Jeff Johnson
2023-08-24 15:27         ` Jeff Johnson
2023-08-24  5:51     ` [PATCH 5/6] [v3] wifi: ath10k: simplify ath10k_peer_assoc_h_vht() Dmitry Antipov
2023-08-24  5:51       ` Dmitry Antipov
2023-08-24  5:51     ` [PATCH 6/6] [v3] wifi: ath10k: simplify ath10k_pci_pm_suspend() Dmitry Antipov
2023-08-24  5:51       ` Dmitry Antipov
2023-08-24 15:26     ` [PATCH 1/6] [v3] wifi: ath10k: cleanup CE ring initialization Jeff Johnson
2023-08-24 15:26       ` Jeff Johnson
2023-09-20 13:23     ` Kalle Valo [this message]
2023-09-20 13:23       ` Kalle Valo
2023-09-21  8:58       ` Dmitry Antipov
2023-09-21  8:58         ` Dmitry Antipov
2023-09-21  9:33         ` Kalle Valo
2023-09-21  9:33           ` Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=874jjpashn.fsf@kernel.org \
    --to=kvalo@kernel.org \
    --cc=ath10k@lists.infradead.org \
    --cc=dmantipov@yandex.ru \
    --cc=linux-wireless@vger.kernel.org \
    --cc=lvc-project@linuxtesting.org \
    --cc=quic_jjohnson@quicinc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.