Harshitha Prem <quic_hprem@xxxxxxxxxxx> writes:
> On 7/3/2024 9:58 PM, Kalle Valo wrote:
>> 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.
>>
>
> 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.
Can you elaborate, for example provide exact state machine description
for all these states? I just can't understand how using several flags in
addition of an enum as different states makes anything easier to manage.
To me that sounds just like spaghetti code.
What I'm thinking is we should cleanup ath12k_core_start(). Now it's
basically asynchronous (ie. qmi.c calls
ath12k_core_qmi_firmware_ready()) but if we change it to be synchronous
(using completions etc.) I believe we could get rid of lots of these
annoying flags. In other words, if ath12k_core_start() returns with zero
then the firmware has booted succesfully.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
