On 5/13/25 8:16 PM, Jeff Hugo wrote:
> On 5/13/2025 12:44 AM, Muhammad Usama Anjum wrote:
>> On 5/12/25 11:46 PM, Jeff Hugo wrote:
>>> On 5/6/2025 8:49 AM, Muhammad Usama Anjum wrote:
>>>> Fix dma_direct_alloc() failure at resume time during bhie_table
>>>> allocation because of memory pressure. There is a report where at
>>>> resume time, the memory from the dma doesn't get allocated and MHI
>>>> fails to re-initialize.
>>>>
>>>> To fix it, don't free the memory at power down during suspend /
>>>> hibernation. Instead, use the same allocated memory again after every
>>>> resume / hibernation. This patch has been tested with resume and
>>>> hibernation both.
>>>>
>>>> The rddm is of constant size for a given hardware. While the fbc_image
>>>> size depends on the firmware. If the firmware changes, we'll free and
>>>> allocate new memory for it.
>>>
>>> Why is it valid to load new firmware as a result of suspend? I don't
>>> users would expect that.
>> I'm not sure its valid or not. Like other users, I also don't expect
>> that firmware would get changed. It doesn't seem to be tested and hence
>> supported case.
>>
>> But other drivers have code which have implementation like this. I'd
>> mentioned previously that this patch was motivated from the ath12k [1]
>> and ath11k [2] patches. They don't free the memory and reuse the same
>> memory if new size is same.
>
> It feels like this justification needs to be detailed in the commit
> text. I suspect at some point we'll have another MHI device where the FW
> will need to be cached.
Okay. I'll add this information to the commit message. Currently I've
not seen firmware caching being used other than graphics driver.
>
>>>> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
>>>> index efa3b6dddf4d2..bc8459798bbee 100644
>>>> --- a/drivers/bus/mhi/host/boot.c
>>>> +++ b/drivers/bus/mhi/host/boot.c
>>>> @@ -584,10 +584,17 @@ void mhi_fw_load_handler(struct mhi_controller
>>>> *mhi_cntrl)
>>>> * device transitioning into MHI READY state
>>>> */
>>>> if (fw_load_type == MHI_FW_LOAD_FBC) {
>>>
>>> Why is this FBC specific?
>> It seems we allocate fbc_image only when firmware load type is
>> FW_LOAD_FBC. I'm just optimizing the buffer allocation here.
>
> We alloc bhie tables in non FBC usecases. Is this somehow an FBC
> specific issue? Perhaps you could clarify the limits of this solution in
> the commit text?
Okay. I'll add information that we are optimizing the bhie allocations.
It has nothing to do with firmware type. I've found only 2 bhie
allocations; fbc_image and rddm_image. So we are optimizing those.
>
>>
>>>
>>>> - ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image,
>>>> fw_sz);
>>>> - if (ret) {
>>>> - release_firmware(firmware);
>>>> - goto error_fw_load;
>>>> + if (mhi_cntrl->fbc_image && fw_sz != mhi_cntrl->prev_fw_sz) {
>>>> + mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
>>>> + mhi_cntrl->fbc_image = NULL;
>>>> + }
>>>> + if (!mhi_cntrl->fbc_image) {
>>>> + ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl-
>>>>> fbc_image, fw_sz);
>>>> + if (ret) {
>>>> + release_firmware(firmware);
>>>> + goto error_fw_load;
>>>> + }
>>>> + mhi_cntrl->prev_fw_sz = fw_sz;
>>>> }
>>>> /* Load the firmware into BHIE vec table */
>>>> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
>>>> index e6c3ff62bab1d..107d71b4cc51a 100644
>>>> --- a/drivers/bus/mhi/host/pm.c
>>>> +++ b/drivers/bus/mhi/host/pm.c
>>>> @@ -1259,10 +1259,19 @@ void mhi_power_down(struct mhi_controller
>>>> *mhi_cntrl, bool graceful)
>>>> }
>>>> EXPORT_SYMBOL_GPL(mhi_power_down);
>>>> +static void __mhi_power_down_unprepare_keep_dev(struct
>>>> mhi_controller *mhi_cntrl)
>>>> +{
>>>> + mhi_cntrl->bhi = NULL;
>>>> + mhi_cntrl->bhie = NULL;
>>>
>>> Why?
>> This function is shorter version of mhi_unprepare_after_power_down(). As
>> we need different code path in case of suspend/hibernation case, I was
>> adding a new API which Mani asked me remove and consolidate into
>> mhi_power_down_keep_dev() instead. So this static function has been
>> added. [3]
>
> I don't understand the need to zero these out. Also, if you are copying
> part of the functionality of mhi_unprepare_after_power_down(), shouldn't
> that functionality be moved into your new API to eliminate duplication?
This how the cleanup works mhi_unprepare_after_power_down(). Yeah, it
makes sense to use this function in mhi_unprepare_after_power_down().
Sending next version soon.
>
--
Regards,
Usama
