On 7/8/2025 5:12 PM, Muhammad Usama Anjum wrote:
> 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:
We need to make sure they have a clean state while resume/recover.
>
>
> --- 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);
You are memset too much, just sizeof(ab->hal.srng_list) is OK.
> +// memset(ab->hal.shadow_reg_addr, 0,
> +// sizeof(ab->hal.shadow_reg_addr) * HAL_SHADOW_NUM_REGS);
same here
> + 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);
>
>
