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.
Thanks,
Harshitha
