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.
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?
- 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?
