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

[Harshitha Prem]

On 7/3/2024 9:58 PM, Kalle Valo wrote:
> Harshitha Prem <quic_hprem at 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