linux-kernel - Re: [PATCH 2/3] bus: mhi: don't deinitialize and re-initialize again

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fdb9f1e7-bf8f-4018-b0ac-ac8a70d9b8ec@oss.qualcomm.com>
Date: Tue, 8 Jul 2025 09:43:49 +0800
From: Baochen Qiang <baochen.qiang@....qualcomm.com>
To: Muhammad Usama Anjum <usama.anjum@...labora.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Jeff Johnson <jjohnson@...nel.org>,
        Jeff Hugo <jeff.hugo@....qualcomm.com>,
        Youssef Samir <quic_yabdulra@...cinc.com>,
        Matthew Leung <quic_mattleun@...cinc.com>, Yan Zhen <yanzhen@...o.com>,
        Alexander Wilhelm <alexander.wilhelm@...termo.com>,
        Alex Elder <elder@...nel.org>, Kunwu Chan <chentao@...inos.cn>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Siddartha Mohanadoss <smohanad@...eaurora.org>,
        Sujeev Dias <sdias@...eaurora.org>,
        Julia Lawall <julia.lawall@...6.fr>, John Crispin <john@...ozen.org>,
        Muna Sinada <quic_msinada@...cinc.com>,
        Venkateswara Naralasetty <quic_vnaralas@...cinc.com>,
        Maharaja Kennadyrajan <quic_mkenna@...cinc.com>, mhi@...ts.linux.dev,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-wireless@...r.kernel.org, ath11k@...ts.infradead.org
Cc: kernel@...labora.com
Subject: Re: [PATCH 2/3] bus: mhi: don't deinitialize and re-initialize again



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.