On 8/6/2024 11:33 AM, Kalle Valo wrote:
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.
No new flags are introduced in this patchset, I have revisited and
removed it any new flags. But, there are some existing flags which seems
to be necessary in case of firmware crash or QMI handshake failure
scenario.
Please find the details of how each flags/state in device
(ath12k_dev_flags) is used currently :
1. ATH12K_CAC_RUNNING - This is used to indicate whether any ongoing
CAC is in progress for the radio in device, if yes, then we should not
handle any data packets or management packets that are received over dp
rx or wmi event respectively during this state.
The state is set during vdev start that is initiated via
assign_vif_chanctx or switch_vif_chanctx. here, the vif_chanctx is from
mac80211 but the data rx or wmi event is from the device which are both
different context. We cannot sync between them, so this flag helps in
avoiding processing the packets.
2. ATH12K_FLAG_CRASH_FLUSH - this is used to indicate that the QMI
connection between the host driver and Q6 is lost. This is set as soon
as driver receives an QMI server exit event. So, once this is set we
have to ensure that no data packets to be queued for hardware and no WMI
commands are posted to firmware. All the calls to send any WMI command
or data packet would be from mac80211.
3. ATH12K_FLAG_RAW_MODE - this flag indicates that the firmware or
hardware operates in RAW mode. Based on this we have to set the rx/tx
decap type to indicate firmware how to handle packets from this type of
vif. (Do we necessary need to keep in dev_flags? may be dev_mode)
4. ATH12K_FLAG_HW_CRYPTO_DISABLED - this indicates whether software
encryption is set or not in raw mode.
5. ATH12K_FLAG_RECOVERY - This is set during QMI server exit but not
used anywhere. (Unused. Remove?)
6. ATH12K_FLAG_UNREGISTERING - this is set during pci remove. if this
flag is set driver should avoid handling any mhi events/QMI events
received from the target/hardware. (May be rename to pci shutdown?)
7. ATH12K_FLAG_REGISTERED - this is set once the device is successfully
registered to mac80211. So, later if this device encounters any firmware
crash, it can be recovered smoothly without registering. Also, this flag
is used during pci_suspend scenario. If device crashes even before
registering to mac80211 we have to avoid recovering it and this flags
helps the same.
Once firmware crash is occurred, after collecting the dumps again the
QMI handshake for that device will start from QMI server arrive to
ATH12K_QMI_EVENT_FW_READY and in firmware ready we should not start the
core again rather init only few and try to reconfigure the device with
available data from mac80211.
8. ATH12K_FLAG_QMI_FAIL - this is set when any of the QMI handshakes
fail and tested in rmmod case to skip the cleanup process.
The following flags are added by MCC team, I am not so clear on the use
case seems like there are few cases in suspend use case where they need
these states.
9. ATH12K_FLAG_HTC_SUSPEND_COMPLETE - Used for htc suspend case.
10. ATH12K_FLAG_CE_IRQ_ENABLED
11. ATH12K_FLAG_EXT_IRQ_ENABLED
So, these device states are more closely related to the
hardware/firmware in which state it is whether firmware crashed after
mac80211 register or before and based on it whether driver can process
the dp tx/rx or wmi tx/rx as these would be from different context. Not
sure, how we will be able to sync among various contexts/threads without
these flags, please let me know your thoughts?
Now, on the otherhand, we have ath12k_hw_state which is used by mac80211
during recovery or driver start or stop.
In multiple device grouping, say if we have 3 pci devices connected and
grouped, each of them will be probed separately and core_init would be
done for each. During core_init, QMI init would be called. So, now
device1, device2 and device3 would have invoked the QMI init. Since the
QMI handshake happens between host and Q6 processor separately for each
device (QMI handshake would be in order for that particular device but
is asynchronous between the devices in group), driver would not have
control on which device (whether device1/device2/device3) will receive
the QMI server arrive event first. Hence, we are using mutex as well as
defer mechanism to sync the devices as some of the MLO partner device
params has to be filled in QMI host cap. Kindly refer below patchset.
https://patchwork.kernel.org/project/linux-wireless/patch/20240520131409.676931-1-quic_hprem@xxxxxxxxxxx/
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.
Sorry, I could not get your question, because already
ath12k_core_start() is called from ath12k_core_qmi_firmware_ready()
which means firmware has booted successfully and in core_start we
register to mac80211 and enable the irqs.
Thanks,
Harshitha
