Harshitha Prem <quic_hprem@xxxxxxxxxxx> 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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
