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

Kees Bakker
2024-12-10T21:01:02Z

Op 04-12-2024 om 17:32 schreef Kalle Valo:
> From: Karthikeyan Periyasamy <quic_periyasa@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