On 7/8/25 6:43 AM, Baochen Qiang wrote:
>
>
> On 7/7/2025 9:11 PM, Muhammad Usama Anjum wrote:
>>>>>>>> diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
>>>>>>>> index 4488e4cdc5e9e..bc4930fe6a367 100644
>>>>>>>> --- a/drivers/net/wireless/ath/ath11k/core.c
>>>>>>>> +++ b/drivers/net/wireless/ath/ath11k/core.c
>>>>>>>> @@ -2213,14 +2213,9 @@ static int ath11k_core_reconfigure_on_crash(struct ath11k_base *ab)
>>>>>>>> mutex_unlock(&ab->core_lock);
>>>>>>>>
>>>>>>>> ath11k_dp_free(ab);
>>>>>>>> - ath11k_hal_srng_deinit(ab);
>>>>>>>>
>>>>>>>> ab->free_vdev_map = (1LL << (ab->num_radios * TARGET_NUM_VDEVS(ab))) - 1;
>>>>>>>>
>>>>>>>> - ret = ath11k_hal_srng_init(ab);
>>>>>>>> - if (ret)
>>>>>>>> - return ret;
>>>>>>>> -
>>>>>>> while I agree there is no need of a dealloc/realloc, we can not simply remove calling the
>>>>>>> _deinit()/_init() pair. At least the memset() cleanup to hal parameters (e.g.
>>>>>> Why do is it being done in the resume handler? Shouldn't those parameters be cleaned up
>>>>>> in resume handler? So when device wakes up, its state is already correct.
>>>>>>
>>>>> Hmm... not quite understand your question. Can you elaborate?
>>>> I'm trying to understand the possibility of cleanup of hal in suspend handler. For example:
>>>> * The driver has been loaded and has been working fine.
>>>> * The user called suspend. So all devices would be suspended.
>>>> * In suspend handler of the ath11k, we should do the necessary cleanups of the states
>>>> like hal.
>>>> * When the device would resume after long time, the hal would have the correct state
>>>> already. So we'll not need to deinit and init again.
>>> The hal cleanup is not only needed by suspend/resume, but also a step of reset/recover
>>> process. So If we are moving the cleanup to suspend handler, similar stuff needs to be
>>> done for reset/recover as well.
>> It makes sense.
>>
>> So clearing the hal structure completely other than ab->hal.srn_config doesn't seem
>> right. I've also tested it and it crashes the whole system.
>>
>> On contrary, with only the current patch applied, there is no abnormality.
>>
>> num_shadow_reg_configured and avail_blk_resource are non-zero. If I make them 0,
>> driver still keeps on working.
>>
>> ab->hal.num_shadow_reg_configured = 0;
>> ab->hal.avail_blk_resource = 0;
>> ab->hal.current_blk_index = 0;
>>
>> As you have suggested setting these 3 to zero, is there any other variable in hal
>> structure which should be set to zero?
>
> IMO srng_config, rdp, wrp and srng_key may keep unchanged through suspend/reset, all other
> fields should be cleared/reinitialized.
memseting srng_list and shadow_reg_addr causes crashes. Please can you confirm why do you
think those should be memset. Here is WIP patch:
--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -2213,14 +2213,10 @@ static int ath11k_core_reconfigure_on_crash(struct ath11k_base *ab)
mutex_unlock(&ab->core_lock);
ath11k_dp_free(ab);
- ath11k_hal_srng_deinit(ab);
+ ath11k_hal_srng_clear(ab);
ab->free_vdev_map = (1LL << (ab->num_radios * TARGET_NUM_VDEVS(ab))) - 1;
- ret = ath11k_hal_srng_init(ab);
- if (ret)
- return ret;
-
clear_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags);
ret = ath11k_core_qmi_firmware_ready(ab);
diff --git a/drivers/net/wireless/ath/ath11k/hal.c b/drivers/net/wireless/ath/ath11k/hal.c
index b32de563d453a..d4be040acf2c8 100644
--- a/drivers/net/wireless/ath/ath11k/hal.c
+++ b/drivers/net/wireless/ath/ath11k/hal.c
@@ -1359,6 +1359,19 @@ void ath11k_hal_srng_deinit(struct ath11k_base *ab)
}
EXPORT_SYMBOL(ath11k_hal_srng_deinit);
+void ath11k_hal_srng_clear(struct ath11k_base *ab)
+{
+// --> both of these memset causes crashes
+// memset(ab->hal.srng_list, 0,
+// sizeof(ab->hal.srng_list) * HAL_SRNG_RING_ID_MAX);
+// memset(ab->hal.shadow_reg_addr, 0,
+// sizeof(ab->hal.shadow_reg_addr) * HAL_SHADOW_NUM_REGS);
+ ab->hal.avail_blk_resource = 0;
+ ab->hal.current_blk_index = 0;
+ ab->hal.num_shadow_reg_configured = 0;
+}
+EXPORT_SYMBOL(ath11k_hal_srng_clear);
+
void ath11k_hal_dump_srng_stats(struct ath11k_base *ab)
{
struct hal_srng *srng;
diff --git a/drivers/net/wireless/ath/ath11k/hal.h b/drivers/net/wireless/ath/ath11k/hal.h
index 601542410c752..839095af9267e 100644
--- a/drivers/net/wireless/ath/ath11k/hal.h
+++ b/drivers/net/wireless/ath/ath11k/hal.h
@@ -965,6 +965,7 @@ int ath11k_hal_srng_setup(struct ath11k_base *ab, enum hal_ring_type type,
struct hal_srng_params *params);
int ath11k_hal_srng_init(struct ath11k_base *ath11k);
void ath11k_hal_srng_deinit(struct ath11k_base *ath11k);
+void ath11k_hal_srng_clear(struct ath11k_base *ab);
void ath11k_hal_dump_srng_stats(struct ath11k_base *ab);
void ath11k_hal_srng_get_shadow_config(struct ath11k_base *ab,
u32 **cfg, u32 *len);
