[PATCH 5/7] wifi: ath12k: move struct ath12k_hw from per device to group

[Kees Bakker]

Op 04-12-2024 om 17:32 schreef Kalle Valo:
> From: Karthikeyan Periyasamy <quic_periyasa at quicinc.com>
>
> Currently, hardware abstractions (ah) of different radio bands are tightly
> coupled to a single device (ab). But, with hardware device group abstraction
> (ag), multiple radios across different devices in a group can form different
> combinations of hardware abstractions (ah) within the group. Hence, the mapping
> between ah to ab can be removed and instead it can be mapped with struct
> ath12k_hw_group (ag).
> [...]
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> [...]
> -void ath12k_mac_destroy(struct ath12k_base *ab)
> +void ath12k_mac_destroy(struct ath12k_hw_group *ag)
>   {
>   	struct ath12k_pdev *pdev;
> +	struct ath12k_base *ab = ag->ab[0];
> +	int i, j;
>   	struct ath12k_hw *ah;
> -	int i;
>   
> -	for (i = 0; i < ab->num_radios; i++) {
> -		pdev = &ab->pdevs[i];
> -		if (!pdev->ar)
> +	for (i = 0; i < ag->num_devices; i++) {
> +		ab = ag->ab[i];
> +		if (!ab)
>   			continue;
>   
> -		pdev->ar = NULL;
> +		for (j = 0; j < ab->num_radios; j++) {
> +			pdev = &ab->pdevs[j];
> +			if (!pdev->ar)
> +				continue;
> +			pdev->ar = NULL;
> +		}
>   	}
>   
>   	for (i = 0; i < ath12k_get_num_hw(ab); i++) {
> @@ -10942,26 +10945,59 @@ void ath12k_mac_destroy(struct ath12k_base *ab)
>   	}
>   }
>   
The new ath12k_mac_destroy() looks suspicious with respect to "ab".
Here is the new function with comments.

void ath12k_mac_destroy(struct ath12k_hw_group *ag)
{
         struct ath12k_pdev *pdev;
         struct ath12k_base *ab = ag->ab[0];
==> here ag->ab[0] is used before checking if ag->num_devices is greater 
than zero
==> If ag->num_devices will never be zero then this first assignment 
will be repeated in the next for-loop
==> if ag->num_devices can be zero then you need to confirm that 
ag->ab[0] is non-NULL

         int i, j;
         struct ath12k_hw *ah;

         for (i = 0; i < ag->num_devices; i++) {
                 ab = ag->ab[i];
                 if (!ab)
=>> This if check indicates that ab can be NULL
                         continue;

                 for (j = 0; j < ab->num_radios; j++) {
                         pdev = &ab->pdevs[j];
                         if (!pdev->ar)
                                 continue;
                         pdev->ar = NULL;
                 }
         }

==> Now ab is going to be used, but as shown above, ab can be NULL
         for (i = 0; i < ath12k_get_num_hw(ab); i++) {
                 ah = ath12k_ab_to_ah(ab, i);
                 if (!ah)
                         continue;

                 ath12k_mac_hw_destroy(ah);
                 ath12k_ab_set_ah(ab, i, NULL);
         }
}

-- 
Kees