Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <7427d5aa-902a-4013-8aa9-f986bb5218a4@quicinc.com>
Date:   Thu, 14 Sep 2023 10:06:51 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH wireless-next 7/9] wifi: ath12k: Remove unnecessary
 (void*) conversions
Content-Language: en-US
To:     Wu Yunchuan <yunchuan@nfschina.com>, <kvalo@kernel.org>
CC:     <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <kernel-janitors@vger.kernel.org>
References: <20230914040525.1170102-1-yunchuan@nfschina.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20230914040525.1170102-1-yunchuan@nfschina.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/13/2023 9:05 PM, Wu Yunchuan wrote:
> No need cast (void*) to (struct hal_rx_ppdu_end_user_stats *),
> (struct ath12k_rx_desc_info *) or (struct hal_tx_msdu_ext_desc *).
> 
> Signed-off-by: Wu Yunchuan <yunchuan@nfschina.com>
> ---
>   drivers/net/wireless/ath/ath12k/dp_mon.c | 6 ++----
>   drivers/net/wireless/ath/ath12k/dp_rx.c  | 2 +-
>   drivers/net/wireless/ath/ath12k/dp_tx.c  | 2 +-
>   3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/dp_mon.c b/drivers/net/wireless/ath/ath12k/dp_mon.c
> index f1e57e98bdc6..41cfe7bd865f 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_mon.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_mon.c
> @@ -13,8 +13,7 @@
>   static void ath12k_dp_mon_rx_handle_ofdma_info(void *rx_tlv,
>   					       struct hal_rx_user_status *rx_user_status)
>   {
> -	struct hal_rx_ppdu_end_user_stats *ppdu_end_user =
> -				(struct hal_rx_ppdu_end_user_stats *)rx_tlv;
> +	struct hal_rx_ppdu_end_user_stats *ppdu_end_user = rx_tlv;
>   
>   	rx_user_status->ul_ofdma_user_v0_word0 =
>   		__le32_to_cpu(ppdu_end_user->usr_resp_ref);
> @@ -26,8 +25,7 @@ static void
>   ath12k_dp_mon_rx_populate_byte_count(void *rx_tlv, void *ppduinfo,
>   				     struct hal_rx_user_status *rx_user_status)
>   {
> -	struct hal_rx_ppdu_end_user_stats *ppdu_end_user =
> -		(struct hal_rx_ppdu_end_user_stats *)rx_tlv;
> +	struct hal_rx_ppdu_end_user_stats *ppdu_end_user = rx_tlv;
>   	u32 mpdu_ok_byte_count = __le32_to_cpu(ppdu_end_user->mpdu_ok_cnt);
>   	u32 mpdu_err_byte_count = __le32_to_cpu(ppdu_end_user->mpdu_err_cnt);
>   

for both of the above IMO the better solution is to change the prototype 
to replace void *rx_tlv with struct hal_rx_ppdu_end_user_stats 
*ppdu_end_user and to remove the local variable

further, I think you can add const to that since the TLV is only read, 
not written

this better describes that the function requires a specific flavor of RX 
TLV rather than handling any RX TLV

> diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
> index e6e64d437c47..ff5f66ca7783 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_rx.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
> @@ -3730,7 +3730,7 @@ int ath12k_dp_rx_process_wbm_err(struct ath12k_base *ab,
>   			continue;
>   		}
>   
> -		desc_info = (struct ath12k_rx_desc_info *)err_info.rx_desc;
> +		desc_info = err_info.rx_desc;

this is ok

>   
>   		/* retry manual desc retrieval if hw cc is not done */
>   		if (!desc_info) {
> diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
> index 8874c815d7fa..98ddf46b3bb9 100644
> --- a/drivers/net/wireless/ath/ath12k/dp_tx.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
> @@ -109,7 +109,7 @@ static struct ath12k_tx_desc_info *ath12k_dp_tx_assign_buffer(struct ath12k_dp *
>   static void ath12k_hal_tx_cmd_ext_desc_setup(struct ath12k_base *ab, void *cmd,

This function is only called from one place, and it is already passing a 
variable that is of type struct hal_tx_msdu_ext_desc *
so IMO a better solution is to change the prototype to replace void *cmd 
with struct hal_tx_msdu_ext_desc *tcl_ext_desc and remove the local variable

again this better describes that the function requires a specific 
payload rather than a generic opaque payload

as a general rule, use void * when the payload has some level of 
opaqueness and use a specific struct * when the payload must be of a 
specific type

>   					     struct hal_tx_info *ti)
>   {
> -	struct hal_tx_msdu_ext_desc *tcl_ext_cmd = (struct hal_tx_msdu_ext_desc *)cmd;
> +	struct hal_tx_msdu_ext_desc *tcl_ext_cmd = cmd;
>   
>   	tcl_ext_cmd->info0 = le32_encode_bits(ti->paddr,
>   					      HAL_TX_MSDU_EXT_INFO0_BUF_PTR_LO);