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 <[email protected]>
> ---
> 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);
On 2023/9/15 01:06, Jeff Johnson wrote:
> 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 <[email protected]>
>> ---
>> 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
>
Hi,
Sounds like a good idea, I will check other patches for this situation.
Thanks for your suggestions.
Wu Yunchuan