Re: [PATCH v8 7/8] wifi: ath12k: refactor core start based on hardware group

Harshitha Prem
2024-07-09T10:16:10Z

On 7/3/2024 9:58 PM, Kalle Valo wrote:
> Harshitha Prem <quic_hprem@quicinc.com> writes:
>
>>>> +static void ath12k_core_device_cleanup(struct ath12k_base *ab)
>>>> +{
>>>> + mutex_lock(&ab->core_lock);
>>>> +
>>>> + if (test_and_clear_bit(ATH12K_FLAG_CORE_HIF_IRQ_ENABLED, &ab->dev_flags))
>>>> + ath12k_hif_irq_disable(ab);
>>>> +
>>>> + if (test_bit(ATH12K_FLAG_PDEV_CREATED, &ab->dev_flags))
>>>> + ath12k_core_pdev_destroy(ab);
>>>> +
>>>> + if (test_bit(ATH12K_FLAG_REGISTERED, &ab->dev_flags)) {
>>>> + ath12k_mac_unregister(ab);
>>>> + ath12k_mac_destroy(ab);
>>>> + }
>>>> +
>>>> + mutex_unlock(&ab->core_lock);
>>>> +}
>>> This patch is just abusing flags and because of that we have
>>> spaghetti
>>> code. I have been disliking use of enum ath12k_dev_flags before but this
>>> is just looks too much. I am wondering do we need to cleanup the ath12k
>>> architecture first, reduce the usage of flags and then revisit this
>>> patchset?
>>>
>> yeah., more dev flags :( but flags were needed for the race conditions
>> when multiple devices where involved in a group, some devices would
>> have completed till pdev create some might not. Some crashes were seen
>> because hif_irq_disable was called for a device in a group but that
>> device was not even at the stage of core register. Will check the
>> possibility to reduce the flag usage but it seemed necessary for
>> multiple device group clean up.
>
> I think the core problem here is of mixing enum ath12k_hw_state and enum
> ath12k_dev_flags, it's just a mess even before this patchset. For
> example, these flags look like they should be part enum ath12k_hw_state
> instead:
>
> ATH12K_FLAG_RECOVERY,
> ATH12K_FLAG_UNREGISTERING,
> ATH12K_FLAG_REGISTERED,
> ATH12K_FLAG_QMI_FAIL,
>
> If we have an enum plus set of flags to handle the actual state then it
> will become difficult to manage it all. Instead we should just have the
> enum for tracking the state of the driver.
>

ath12k_hw_state is the driver state representation which is used to
indicate whether driver has started or in restarting from mac80211
prespective where as ath12k_dev_flags closely related to devices and its
q6 states.

So, ATH12K_FLAG_RECOVERY, ATH12K_FLAG_QMI_FAIL should be in
ath12k_dev_flags because these are specific to Q6 crashes and failure.
ATH12K_FLAG_UNREGISTERING is actually used to indicate pci_remove is
initiated and we should not process any QMI events but may be naming is
creating the confusion. ATH12K_FLAG_REGISTERED flag is used whether to
recover or not with the information available in mac80211 to reconfig.

With hardware abstraction, it can be like 3 devices (ath12k_base) in one
ath12k_hw and with ath12k_hw_states alone we might not be able to
handle. Because, say device1 is in recovery, device2 is already till QMI
firmware ready after device probing and these two devices are not
registered to mac80211 then we cannot set the ath12k_hw_state as ON/OFF
or anything else.

Hence, we may require two distinct flags, where one holds the driver
abstraction state and other is device states. With grouping complexity
would increases as we have to sync between the devices and we require
two flags. Please let me know your thoughts.

Thanks,
Harshitha