[PATCH 0/6] add improvements to mhi driver

[PATCH 0/6] add improvements to mhi driver

[Gokul Sriram Palanisamy]

The below patches address
- memory fragmentation issues: when several iterations of mhi controller
  register/unregister happens. In our case, we have a PCIe WiFi device for
  which fw is loaded everytime when 'wifi load' happens. For this, we need
  to allocate 9MiB of coherent memory with 512K segment size. After several
  iterations, when the memory gets fragmented and 512K segment becomes
  unavailable causing alloc failure.
  
- debug requirements by printing critical mhi debug registers
- bug fixes to mhi_irq_handler
- RDDM timeout issue
- memory carveout for RDDM segments.

Gokul Sriram Palanisamy (2):
  drivers: bus: mhi: Added shared-dma-pool support for mhi_dev
  bus: mhi: dump debug registers in critical sections

Karthick Shanmugham (1):
  bus: mhi: change IRQ_HANDLED to IRQ_NONE in mhi_irq_handler

Praveenkumar I (1):
  bus: mhi: increase RDDM timeout

Rajkumar Ayyasamy (1):
  bus: mhi: check for RDDM cookie set by device to indicate readiness

Ram Kumar D (1):
  bus: mhi: add support to allocate rddm memory during crash time

 drivers/bus/mhi/host/boot.c     | 209 ++++++++++++++++++++++++--------
 drivers/bus/mhi/host/init.c     |  70 ++++++++++-
 drivers/bus/mhi/host/internal.h |  37 +++++-
 drivers/bus/mhi/host/main.c     | 115 +++++++++++++++++-
 drivers/bus/mhi/host/pm.c       |   6 +-
 include/linux/mhi.h             |  20 +++
 6 files changed, 397 insertions(+), 60 deletions(-)

-- 
2.34.1

[PATCH 1/6] drivers: bus: mhi: Added shared-dma-pool support for mhi_dev

[Gokul Sriram Palanisamy]

When using default memory for coherent memory allocation without
reservation, memory gets fragmented after several mhi
register/unregister cycles and no coherent reservation was possible.

Client driver registering MHI shall reserve a dedicated region as
shared-dma-pool for mhi to help avoid this situation. On boards
which doesn't reserve this memory, it will continue to allocate
memory from default memory.

DMA pool is reserved for coherent allocations of size SZ_512K
(mhi_cntrl->seg_len) to avoid fragmentation and always ensure
allocations of SZ_512K succeeds. Allocations of lower order from the
reserved memory would lead to fragmentation on multiple alloc/frees.
So use dma_alloc_coherent from mhi_cntrl->cntrl_dev for allocations
lower than mhi_cntrl->seg_len. If coherent pool is not reserved, all
reservations go through mhi_cntrl->cntrl_dev.

Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 drivers/bus/mhi/host/boot.c     | 19 ++++++------
 drivers/bus/mhi/host/init.c     | 51 +++++++++++++++++++++++++++++++++
 drivers/bus/mhi/host/internal.h | 26 +++++++++++++++++
 include/linux/mhi.h             |  5 ++++
 4 files changed, 91 insertions(+), 10 deletions(-)

diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index dedd29ca8db3..ca842facf820 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -303,8 +303,8 @@ void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
 	struct mhi_buf *mhi_buf = image_info->mhi_buf;
 
 	for (i = 0; i < image_info->entries; i++, mhi_buf++)
-		dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
-				  mhi_buf->buf, mhi_buf->dma_addr);
+		mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
+				     mhi_buf->buf, mhi_buf->dma_addr);
 
 	kfree(image_info->mhi_buf);
 	kfree(image_info);
@@ -340,9 +340,9 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
 			vec_size = sizeof(struct bhi_vec_entry) * i;
 
 		mhi_buf->len = vec_size;
-		mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev,
-						  vec_size, &mhi_buf->dma_addr,
-						  GFP_KERNEL);
+		mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl, vec_size,
+						     &mhi_buf->dma_addr,
+						     GFP_KERNEL);
 		if (!mhi_buf->buf)
 			goto error_alloc_segment;
 	}
@@ -355,8 +355,8 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
 
 error_alloc_segment:
 	for (--i, --mhi_buf; i >= 0; i--, mhi_buf--)
-		dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
-				  mhi_buf->buf, mhi_buf->dma_addr);
+		mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
+				     mhi_buf->buf, mhi_buf->dma_addr);
 
 error_alloc_mhi_buf:
 	kfree(img_info);
@@ -452,8 +452,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	fw_sz = firmware->size;
 
 skip_req_fw:
-	buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr,
-				 GFP_KERNEL);
+	buf = mhi_fw_alloc_coherent(mhi_cntrl, size, &dma_addr, GFP_KERNEL);
 	if (!buf) {
 		release_firmware(firmware);
 		goto error_fw_load;
@@ -462,7 +461,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	/* Download image using BHI */
 	memcpy(buf, fw_data, size);
 	ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size);
-	dma_free_coherent(mhi_cntrl->cntrl_dev, size, buf, dma_addr);
+	mhi_fw_free_coherent(mhi_cntrl, size, buf, dma_addr);
 
 	/* Error or in EDL mode, we're done */
 	if (ret) {
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index ce7d2e62c2f1..c1e1412c43e2 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -8,9 +8,12 @@
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/dma-direction.h>
+#include <linux/dma-map-ops.h>
 #include <linux/dma-mapping.h>
 #include <linux/idr.h>
 #include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/list.h>
 #include <linux/mhi.h>
 #include <linux/mod_devicetable.h>
@@ -929,6 +932,51 @@ static int parse_config(struct mhi_controller *mhi_cntrl,
 	return ret;
 }
 
+static void mhi_init_fw_coherent_memory(struct mhi_controller *mhi_cntrl,
+					struct mhi_device *mhi_dev)
+{
+	struct reserved_mem *mhi_rmem = NULL;
+	struct device *dev = &mhi_dev->dev;
+	struct device_node *cma_node = mhi_cntrl->cma_node;
+	int ret;
+
+	dev->coherent_dma_mask = mhi_cntrl->cntrl_dev->coherent_dma_mask;
+
+	if (!cma_node) {
+		dev_err(mhi_cntrl->cntrl_dev, "mhi coherent pool is not reserved");
+		return;
+	}
+
+	mhi_rmem = of_reserved_mem_lookup(cma_node);
+	of_node_put(cma_node);
+
+	if (!mhi_rmem) {
+		dev_err(mhi_cntrl->cntrl_dev, "Failed to get DMA reserved memory");
+		return;
+	}
+
+	mhi_cntrl->cma_base = mhi_rmem->base;
+	mhi_cntrl->cma_size = mhi_rmem->size;
+
+	ret = dma_declare_coherent_memory(dev, mhi_cntrl->cma_base,
+					  mhi_cntrl->cma_base,
+					  mhi_cntrl->cma_size);
+	if (ret)
+		dev_err(mhi_cntrl->cntrl_dev, "Failed to declare dma coherent memory");
+	else
+		dev_info(mhi_cntrl->cntrl_dev, "DMA Memory initialized at %pa, size %ld MiB",
+			 &mhi_cntrl->cma_base,
+			 (unsigned long)mhi_cntrl->cma_size / SZ_1M);
+}
+
+static void mhi_deinit_fw_coherent_memory(struct mhi_controller *mhi_cntrl)
+{
+	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
+
+	dma_release_coherent_memory(&mhi_dev->dev);
+	mhi_dev->dev.dma_mem = NULL;
+}
+
 int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 			    const struct mhi_controller_config *config)
 {
@@ -1028,6 +1076,7 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 		goto error_setup_irq;
 	}
 
+	mhi_init_fw_coherent_memory(mhi_cntrl, mhi_dev);
 	mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;
 	mhi_dev->mhi_cntrl = mhi_cntrl;
 	dev_set_name(&mhi_dev->dev, "mhi%d", mhi_cntrl->index);
@@ -1053,6 +1102,7 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	return 0;
 
 err_release_dev:
+	mhi_deinit_fw_coherent_memory(mhi_cntrl);
 	put_device(&mhi_dev->dev);
 error_setup_irq:
 	mhi_deinit_free_irq(mhi_cntrl);
@@ -1095,6 +1145,7 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
 	}
 	vfree(mhi_cntrl->mhi_chan);
 
+	mhi_deinit_fw_coherent_memory(mhi_cntrl);
 	device_del(&mhi_dev->dev);
 	put_device(&mhi_dev->dev);
 
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index aaad40a07f69..41ce100d87d2 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -396,6 +396,32 @@ void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl,
 void mhi_reset_chan(struct mhi_controller *mhi_cntrl,
 		    struct mhi_chan *mhi_chan);
 
+static inline void *mhi_fw_alloc_coherent(struct mhi_controller *mhi_cntrl,
+					  size_t size, dma_addr_t *dma_handle,
+					  gfp_t gfp)
+{
+	if (size < mhi_cntrl->seg_len || !mhi_cntrl->cma_base) {
+		return dma_alloc_coherent(mhi_cntrl->cntrl_dev,
+					  size, dma_handle, gfp);
+	} else {
+		return dma_alloc_coherent(&mhi_cntrl->mhi_dev->dev,
+					  size, dma_handle, gfp);
+	}
+}
+
+static inline void mhi_fw_free_coherent(struct mhi_controller *mhi_cntrl,
+					size_t size, void *vaddr,
+					dma_addr_t dma_handle)
+{
+	if (size < mhi_cntrl->seg_len || !mhi_cntrl->cma_base) {
+		dma_free_coherent(mhi_cntrl->cntrl_dev, size, vaddr,
+				  dma_handle);
+	} else {
+		dma_free_coherent(&mhi_cntrl->mhi_dev->dev, size, vaddr,
+				  dma_handle);
+	}
+}
+
 /* Event processing methods */
 void mhi_ctrl_ev_task(unsigned long data);
 void mhi_ev_task(unsigned long data);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 059dc94d20bb..c788c12039b5 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -362,6 +362,8 @@ struct mhi_controller_config {
  * @wake_set: Device wakeup set flag
  * @irq_flags: irq flags passed to request_irq (optional)
  * @mru: the default MRU for the MHI device
+ * @cma_base: Base address of the cohernet memory pool reserved
+ * @cma_size: Size of the cohernel memory pool reserved
  *
  * Fields marked as (required) need to be populated by the controller driver
  * before calling mhi_register_controller(). For the fields marked as (optional)
@@ -447,6 +449,9 @@ struct mhi_controller {
 	bool wake_set;
 	unsigned long irq_flags;
 	u32 mru;
+	struct device_node *cma_node;
+	phys_addr_t cma_base;
+	size_t cma_size;
 };
 
 /**
-- 
2.34.1

Re: [PATCH 1/6] drivers: bus: mhi: Added shared-dma-pool support for mhi_dev

[Jeffrey Hugo]

$SUBJECT looks wrong

On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
> When using default memory for coherent memory allocation without
> reservation, memory gets fragmented after several mhi
> register/unregister cycles and no coherent reservation was possible.
> 
> Client driver registering MHI shall reserve a dedicated region as
> shared-dma-pool for mhi to help avoid this situation. On boards
> which doesn't reserve this memory, it will continue to allocate
> memory from default memory.
> 
> DMA pool is reserved for coherent allocations of size SZ_512K
> (mhi_cntrl->seg_len) to avoid fragmentation and always ensure
> allocations of SZ_512K succeeds. Allocations of lower order from the
> reserved memory would lead to fragmentation on multiple alloc/frees.
> So use dma_alloc_coherent from mhi_cntrl->cntrl_dev for allocations
> lower than mhi_cntrl->seg_len. If coherent pool is not reserved, all
> reservations go through mhi_cntrl->cntrl_dev.
> 
> Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
> ---
>   drivers/bus/mhi/host/boot.c     | 19 ++++++------
>   drivers/bus/mhi/host/init.c     | 51 +++++++++++++++++++++++++++++++++
>   drivers/bus/mhi/host/internal.h | 26 +++++++++++++++++
>   include/linux/mhi.h             |  5 ++++
>   4 files changed, 91 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index dedd29ca8db3..ca842facf820 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -303,8 +303,8 @@ void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
>   	struct mhi_buf *mhi_buf = image_info->mhi_buf;
>   
>   	for (i = 0; i < image_info->entries; i++, mhi_buf++)
> -		dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
> -				  mhi_buf->buf, mhi_buf->dma_addr);
> +		mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
> +				     mhi_buf->buf, mhi_buf->dma_addr);
>   
>   	kfree(image_info->mhi_buf);
>   	kfree(image_info);
> @@ -340,9 +340,9 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>   			vec_size = sizeof(struct bhi_vec_entry) * i;
>   
>   		mhi_buf->len = vec_size;
> -		mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev,
> -						  vec_size, &mhi_buf->dma_addr,
> -						  GFP_KERNEL);
> +		mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl, vec_size,
> +						     &mhi_buf->dma_addr,
> +						     GFP_KERNEL);
>   		if (!mhi_buf->buf)
>   			goto error_alloc_segment;
>   	}
> @@ -355,8 +355,8 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>   
>   error_alloc_segment:
>   	for (--i, --mhi_buf; i >= 0; i--, mhi_buf--)
> -		dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
> -				  mhi_buf->buf, mhi_buf->dma_addr);
> +		mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
> +				     mhi_buf->buf, mhi_buf->dma_addr);
>   
>   error_alloc_mhi_buf:
>   	kfree(img_info);
> @@ -452,8 +452,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>   	fw_sz = firmware->size;
>   
>   skip_req_fw:
> -	buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr,
> -				 GFP_KERNEL);
> +	buf = mhi_fw_alloc_coherent(mhi_cntrl, size, &dma_addr, GFP_KERNEL);
>   	if (!buf) {
>   		release_firmware(firmware);
>   		goto error_fw_load;
> @@ -462,7 +461,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>   	/* Download image using BHI */
>   	memcpy(buf, fw_data, size);
>   	ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size);
> -	dma_free_coherent(mhi_cntrl->cntrl_dev, size, buf, dma_addr);
> +	mhi_fw_free_coherent(mhi_cntrl, size, buf, dma_addr);
>   
>   	/* Error or in EDL mode, we're done */
>   	if (ret) {
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index ce7d2e62c2f1..c1e1412c43e2 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -8,9 +8,12 @@
>   #include <linux/debugfs.h>
>   #include <linux/device.h>
>   #include <linux/dma-direction.h>
> +#include <linux/dma-map-ops.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/idr.h>
>   #include <linux/interrupt.h>
> +#include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>

NACK.  Not all platforms that use MHI have devicetree.

Re: [PATCH 1/6] drivers: bus: mhi: Added shared-dma-pool support for mhi_dev

[Gokul Sriram P]

On 7/18/2024 9:47 PM, Jeffrey Hugo wrote:
> $SUBJECT looks wrong
>
> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>> When using default memory for coherent memory allocation without
>> reservation, memory gets fragmented after several mhi
>> register/unregister cycles and no coherent reservation was possible.
>>
>> Client driver registering MHI shall reserve a dedicated region as
>> shared-dma-pool for mhi to help avoid this situation. On boards
>> which doesn't reserve this memory, it will continue to allocate
>> memory from default memory.
>>
>> DMA pool is reserved for coherent allocations of size SZ_512K
>> (mhi_cntrl->seg_len) to avoid fragmentation and always ensure
>> allocations of SZ_512K succeeds. Allocations of lower order from the
>> reserved memory would lead to fragmentation on multiple alloc/frees.
>> So use dma_alloc_coherent from mhi_cntrl->cntrl_dev for allocations
>> lower than mhi_cntrl->seg_len. If coherent pool is not reserved, all
>> reservations go through mhi_cntrl->cntrl_dev.
>>
>> Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/boot.c     | 19 ++++++------
>>   drivers/bus/mhi/host/init.c     | 51 +++++++++++++++++++++++++++++++++
>>   drivers/bus/mhi/host/internal.h | 26 +++++++++++++++++
>>   include/linux/mhi.h             |  5 ++++
>>   4 files changed, 91 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
>> index dedd29ca8db3..ca842facf820 100644
>> --- a/drivers/bus/mhi/host/boot.c
>> +++ b/drivers/bus/mhi/host/boot.c
>> @@ -303,8 +303,8 @@ void mhi_free_bhie_table(struct mhi_controller 
>> *mhi_cntrl,
>>       struct mhi_buf *mhi_buf = image_info->mhi_buf;
>>         for (i = 0; i < image_info->entries; i++, mhi_buf++)
>> -        dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
>> -                  mhi_buf->buf, mhi_buf->dma_addr);
>> +        mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
>> +                     mhi_buf->buf, mhi_buf->dma_addr);
>>         kfree(image_info->mhi_buf);
>>       kfree(image_info);
>> @@ -340,9 +340,9 @@ int mhi_alloc_bhie_table(struct mhi_controller 
>> *mhi_cntrl,
>>               vec_size = sizeof(struct bhi_vec_entry) * i;
>>             mhi_buf->len = vec_size;
>> -        mhi_buf->buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev,
>> -                          vec_size, &mhi_buf->dma_addr,
>> -                          GFP_KERNEL);
>> +        mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl, vec_size,
>> +                             &mhi_buf->dma_addr,
>> +                             GFP_KERNEL);
>>           if (!mhi_buf->buf)
>>               goto error_alloc_segment;
>>       }
>> @@ -355,8 +355,8 @@ int mhi_alloc_bhie_table(struct mhi_controller 
>> *mhi_cntrl,
>>     error_alloc_segment:
>>       for (--i, --mhi_buf; i >= 0; i--, mhi_buf--)
>> -        dma_free_coherent(mhi_cntrl->cntrl_dev, mhi_buf->len,
>> -                  mhi_buf->buf, mhi_buf->dma_addr);
>> +        mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
>> +                     mhi_buf->buf, mhi_buf->dma_addr);
>>     error_alloc_mhi_buf:
>>       kfree(img_info);
>> @@ -452,8 +452,7 @@ void mhi_fw_load_handler(struct mhi_controller 
>> *mhi_cntrl)
>>       fw_sz = firmware->size;
>>     skip_req_fw:
>> -    buf = dma_alloc_coherent(mhi_cntrl->cntrl_dev, size, &dma_addr,
>> -                 GFP_KERNEL);
>> +    buf = mhi_fw_alloc_coherent(mhi_cntrl, size, &dma_addr, 
>> GFP_KERNEL);
>>       if (!buf) {
>>           release_firmware(firmware);
>>           goto error_fw_load;
>> @@ -462,7 +461,7 @@ void mhi_fw_load_handler(struct mhi_controller 
>> *mhi_cntrl)
>>       /* Download image using BHI */
>>       memcpy(buf, fw_data, size);
>>       ret = mhi_fw_load_bhi(mhi_cntrl, dma_addr, size);
>> -    dma_free_coherent(mhi_cntrl->cntrl_dev, size, buf, dma_addr);
>> +    mhi_fw_free_coherent(mhi_cntrl, size, buf, dma_addr);
>>         /* Error or in EDL mode, we're done */
>>       if (ret) {
>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>> index ce7d2e62c2f1..c1e1412c43e2 100644
>> --- a/drivers/bus/mhi/host/init.c
>> +++ b/drivers/bus/mhi/host/init.c
>> @@ -8,9 +8,12 @@
>>   #include <linux/debugfs.h>
>>   #include <linux/device.h>
>>   #include <linux/dma-direction.h>
>> +#include <linux/dma-map-ops.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/idr.h>
>>   #include <linux/interrupt.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_reserved_mem.h>
>
> NACK.  Not all platforms that use MHI have devicetree.
>
Got it. Removed this patch. This can be addressed from client driver 
that need this feature.

Regards,

Gokul

[PATCH 2/6] bus: mhi: add support to allocate rddm memory during crash time

[Gokul Sriram Palanisamy]

From: Ram Kumar D <quic_ramd@quicinc.com>

Currently, MHI bus pre-allocates the RDDM buffer for crash dump
collection during MHI power up.

To avoid carving out memory for RDDM buffers even if it is unutilized,
add support to allocate memory at runtime during the RDDM download
after target crash.

This feature can be controlled by the client driver registering the MHI
controller by setting the rddm_prealloc flag to false in mhi_cntrl.
By default rddm_prealloc is true, retaining the existing behaviour.

By default, rddm_seg_len will be same as seg_len. The client driver
can override the mhi_cntrl->rddm_seg_len.

Signed-off-by: Ram Kumar D <quic_ramd@quicinc.com>
Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 drivers/bus/mhi/host/boot.c     | 149 +++++++++++++++++++++++++++-----
 drivers/bus/mhi/host/init.c     |  19 ++--
 drivers/bus/mhi/host/internal.h |  11 ++-
 drivers/bus/mhi/host/main.c     |   4 +-
 drivers/bus/mhi/host/pm.c       |   2 +-
 include/linux/mhi.h             |   2 +
 6 files changed, 156 insertions(+), 31 deletions(-)

diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index ca842facf820..1a918e340424 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -35,6 +35,16 @@ int mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
 		bhi_vec->size = mhi_buf->len;
 	}
 
+	if (!mhi_cntrl->rddm_prealloc) {
+		mhi_buf->dma_addr = dma_map_single(mhi_cntrl->cntrl_dev,
+						   mhi_buf->buf, mhi_buf->len,
+						   DMA_TO_DEVICE);
+		if (dma_mapping_error(mhi_cntrl->cntrl_dev, mhi_buf->dma_addr)) {
+			dev_err(dev, "dma mapping failed\n");
+			return -ENOMEM;
+		}
+	}
+
 	dev_dbg(dev, "BHIe programming for RDDM\n");
 
 	mhi_write_reg(mhi_cntrl, base, BHIE_RXVECADDR_HIGH_OFFS,
@@ -158,10 +168,35 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
 {
 	void __iomem *base = mhi_cntrl->bhie;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	struct mhi_buf *mhi_buf = NULL;
 	u32 rx_status;
+	int ret;
 
-	if (in_panic)
-		return __mhi_download_rddm_in_panic(mhi_cntrl);
+	/*
+	 * Allocate RDDM table if specified, this table is for debugging purpose
+	 */
+	if (!mhi_cntrl->rddm_prealloc && mhi_cntrl->rddm_size) {
+		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->rddm_image,
+					   mhi_cntrl->rddm_size, IMG_TYPE_RDDM);
+		if (ret) {
+			dev_err(dev, "Failed to allocate RDDM table memory\n");
+			return ret;
+		}
+
+		/* setup the RX vector table */
+		ret = mhi_rddm_prepare(mhi_cntrl, mhi_cntrl->rddm_image);
+		if (ret) {
+			dev_err(dev, "Failed to prepare RDDM\n");
+			mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->rddm_image,
+					    IMG_TYPE_RDDM);
+			return ret;
+		}
+	}
+
+	if (in_panic) {
+		ret = __mhi_download_rddm_in_panic(mhi_cntrl);
+		goto out;
+	}
 
 	dev_dbg(dev, "Waiting for RDDM image download via BHIe\n");
 
@@ -173,7 +208,16 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
 					      &rx_status) || rx_status,
 			   msecs_to_jiffies(mhi_cntrl->timeout_ms));
 
-	return (rx_status == BHIE_RXVECSTATUS_STATUS_XFER_COMPL) ? 0 : -EIO;
+	ret = (rx_status == BHIE_RXVECSTATUS_STATUS_XFER_COMPL) ? 0 : -EIO;
+
+out:
+	mhi_buf = &mhi_cntrl->rddm_image->mhi_buf[mhi_cntrl->rddm_image->entries - 1];
+
+	if (!mhi_cntrl->rddm_prealloc)
+		dma_unmap_single(mhi_cntrl->cntrl_dev, mhi_buf->dma_addr,
+				 mhi_buf->len, DMA_TO_DEVICE);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(mhi_download_rddm_image);
 
@@ -297,14 +341,25 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
 }
 
 void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
-			 struct image_info *image_info)
+			 struct image_info *image_info,
+			 enum image_type img_type)
 {
 	int i;
 	struct mhi_buf *mhi_buf = image_info->mhi_buf;
 
-	for (i = 0; i < image_info->entries; i++, mhi_buf++)
-		mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
-				     mhi_buf->buf, mhi_buf->dma_addr);
+	for (i = 0; i < image_info->entries; i++, mhi_buf++) {
+		if (img_type == IMG_TYPE_RDDM && !mhi_cntrl->rddm_prealloc) {
+			if (i == (image_info->entries - 1))
+				dma_unmap_single(mhi_cntrl->cntrl_dev,
+						 mhi_buf->dma_addr,
+						 mhi_buf->len,
+						 DMA_FROM_DEVICE);
+			kfree(mhi_buf->buf);
+		} else {
+			mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
+					     mhi_buf->buf, mhi_buf->dma_addr);
+		}
+	}
 
 	kfree(image_info->mhi_buf);
 	kfree(image_info);
@@ -312,21 +367,31 @@ void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
 
 int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
 			 struct image_info **image_info,
-			 size_t alloc_size)
+			 size_t alloc_size, enum image_type img_type)
 {
 	size_t seg_size = mhi_cntrl->seg_len;
-	int segments = DIV_ROUND_UP(alloc_size, seg_size) + 1;
+	int segments;
 	int i;
 	struct image_info *img_info;
 	struct mhi_buf *mhi_buf;
+	/* Masked __GFP_DIRECT_RECLAIM flag for non-interrupt context
+	 * to avoid rcu context sleep issue in kmalloc during kernel panic
+	 */
+	gfp_t gfp = (in_interrupt() ? GFP_ATOMIC :
+		((GFP_KERNEL | __GFP_NORETRY) & ~__GFP_DIRECT_RECLAIM));
+
+	if (img_type == IMG_TYPE_RDDM)
+		seg_size = mhi_cntrl->rddm_seg_len;
 
-	img_info = kzalloc(sizeof(*img_info), GFP_KERNEL);
+	segments = DIV_ROUND_UP(alloc_size, seg_size) + 1;
+
+	img_info = kzalloc(sizeof(*img_info), gfp);
 	if (!img_info)
 		return -ENOMEM;
 
 	/* Allocate memory for entries */
 	img_info->mhi_buf = kcalloc(segments, sizeof(*img_info->mhi_buf),
-				    GFP_KERNEL);
+				    gfp);
 	if (!img_info->mhi_buf)
 		goto error_alloc_mhi_buf;
 
@@ -340,11 +405,42 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
 			vec_size = sizeof(struct bhi_vec_entry) * i;
 
 		mhi_buf->len = vec_size;
-		mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl, vec_size,
-						     &mhi_buf->dma_addr,
-						     GFP_KERNEL);
-		if (!mhi_buf->buf)
-			goto error_alloc_segment;
+
+		if (img_type == IMG_TYPE_RDDM && !mhi_cntrl->rddm_prealloc) {
+			/* Vector table is the last entry */
+			if (i == segments - 1) {
+				mhi_buf->buf = kzalloc(PAGE_ALIGN(vec_size),
+						       gfp);
+				if (!mhi_buf->buf)
+					goto error_alloc_segment;
+
+				/* Vector table entry will be dma_mapped during
+				 * rddm prepare with DMA_TO_DEVICE and unmapped
+				 * once the target completes the RDDM XFER.
+				 */
+				continue;
+			}
+			mhi_buf->buf = kmalloc(vec_size, gfp);
+			if (!mhi_buf->buf)
+				goto error_alloc_segment;
+
+			mhi_buf->dma_addr = dma_map_single(mhi_cntrl->cntrl_dev,
+							   mhi_buf->buf,
+							   vec_size,
+							   DMA_FROM_DEVICE);
+			if (dma_mapping_error(mhi_cntrl->cntrl_dev,
+					      mhi_buf->dma_addr)) {
+				kfree(mhi_buf->buf);
+				goto error_alloc_segment;
+			}
+		} else {
+			mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl,
+							     vec_size,
+							     &mhi_buf->dma_addr,
+							     GFP_KERNEL);
+			if (!mhi_buf->buf)
+				goto error_alloc_segment;
+		}
 	}
 
 	img_info->bhi_vec = img_info->mhi_buf[segments - 1].buf;
@@ -354,9 +450,18 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
 	return 0;
 
 error_alloc_segment:
-	for (--i, --mhi_buf; i >= 0; i--, mhi_buf--)
-		mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
-				     mhi_buf->buf, mhi_buf->dma_addr);
+	for (--i, --mhi_buf; i >= 0; i--, mhi_buf--) {
+		if (img_type == IMG_TYPE_RDDM && !mhi_cntrl->rddm_prealloc) {
+			dma_unmap_single(mhi_cntrl->cntrl_dev,
+					 mhi_buf->dma_addr, mhi_buf->len,
+					 DMA_FROM_DEVICE);
+			kfree(mhi_buf->buf);
+
+		} else {
+			mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
+					     mhi_buf->buf, mhi_buf->dma_addr);
+		}
+	}
 
 error_alloc_mhi_buf:
 	kfree(img_info);
@@ -485,7 +590,8 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	 * device transitioning into MHI READY state
 	 */
 	if (mhi_cntrl->fbc_download) {
-		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, fw_sz);
+		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, fw_sz,
+					   IMG_TYPE_FBC);
 		if (ret) {
 			release_firmware(firmware);
 			goto error_fw_load;
@@ -510,7 +616,8 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 
 error_ready_state:
 	if (mhi_cntrl->fbc_download) {
-		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
+		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image,
+				    IMG_TYPE_FBC);
 		mhi_cntrl->fbc_image = NULL;
 	}
 
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index c1e1412c43e2..8a47c3354560 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -1058,6 +1058,9 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 		mhi_cntrl->unmap_single = mhi_unmap_single_no_bb;
 	}
 
+	mhi_cntrl->rddm_prealloc = true;
+	mhi_cntrl->rddm_seg_len = mhi_cntrl->seg_len;
+
 	mhi_cntrl->index = ida_alloc(&mhi_controller_ida, GFP_KERNEL);
 	if (mhi_cntrl->index < 0) {
 		ret = mhi_cntrl->index;
@@ -1224,14 +1227,18 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)
 		/*
 		 * Allocate RDDM table for debugging purpose if specified
 		 */
-		mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->rddm_image,
-				     mhi_cntrl->rddm_size);
+		if (mhi_cntrl->rddm_prealloc)
+			mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->rddm_image,
+					     mhi_cntrl->rddm_size,
+					     IMG_TYPE_RDDM);
+
 		if (mhi_cntrl->rddm_image) {
 			ret = mhi_rddm_prepare(mhi_cntrl,
 					       mhi_cntrl->rddm_image);
 			if (ret) {
 				mhi_free_bhie_table(mhi_cntrl,
-						    mhi_cntrl->rddm_image);
+						    mhi_cntrl->rddm_image,
+						    IMG_TYPE_RDDM);
 				goto error_reg_offset;
 			}
 		}
@@ -1254,12 +1261,14 @@ EXPORT_SYMBOL_GPL(mhi_prepare_for_power_up);
 void mhi_unprepare_after_power_down(struct mhi_controller *mhi_cntrl)
 {
 	if (mhi_cntrl->fbc_image) {
-		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
+		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image,
+				    IMG_TYPE_FBC);
 		mhi_cntrl->fbc_image = NULL;
 	}
 
 	if (mhi_cntrl->rddm_image) {
-		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->rddm_image);
+		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->rddm_image,
+				    IMG_TYPE_RDDM);
 		mhi_cntrl->rddm_image = NULL;
 	}
 
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index 41ce100d87d2..c7946f81be38 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -176,6 +176,11 @@ enum mhi_er_type {
 	MHI_ER_TYPE_VALID = 0x1,
 };
 
+enum image_type {
+	IMG_TYPE_FBC,
+	IMG_TYPE_RDDM,
+};
+
 struct db_cfg {
 	bool reset_req;
 	bool db_mode;
@@ -314,9 +319,11 @@ int mhi_destroy_device(struct device *dev, void *data);
 void mhi_create_devices(struct mhi_controller *mhi_cntrl);
 
 int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
-			 struct image_info **image_info, size_t alloc_size);
+			 struct image_info **image_info, size_t alloc_size,
+			 enum image_type img_type);
 void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
-			 struct image_info *image_info);
+			 struct image_info *image_info,
+			 enum image_type img_type);
 
 /* Power management APIs */
 enum mhi_pm_state __must_check mhi_tryset_pm_state(
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 4de75674f193..2f44f11fa5a6 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -503,13 +503,13 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
 	}
 	write_unlock_irq(&mhi_cntrl->pm_lock);
 
-	if (pm_state != MHI_PM_SYS_ERR_DETECT)
+	if (pm_state != MHI_PM_SYS_ERR_DETECT && ee != MHI_EE_RDDM)
 		goto exit_intvec;
 
 	switch (ee) {
 	case MHI_EE_RDDM:
 		/* proceed if power down is not already in progress */
-		if (mhi_cntrl->rddm_image && mhi_is_active(mhi_cntrl)) {
+		if (mhi_cntrl->rddm_size && mhi_is_active(mhi_cntrl)) {
 			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
 			mhi_cntrl->ee = ee;
 			wake_up_all(&mhi_cntrl->state_event);
diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
index 11c0e751f223..68524e27e76c 100644
--- a/drivers/bus/mhi/host/pm.c
+++ b/drivers/bus/mhi/host/pm.c
@@ -767,7 +767,7 @@ void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl)
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 
 	/* skip if controller supports RDDM */
-	if (mhi_cntrl->rddm_image) {
+	if (mhi_cntrl->rddm_size) {
 		dev_dbg(dev, "Controller supports RDDM, skip SYS_ERROR\n");
 		return;
 	}
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index c788c12039b5..ce229a6a2b9a 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -452,6 +452,8 @@ struct mhi_controller {
 	struct device_node *cma_node;
 	phys_addr_t cma_base;
 	size_t cma_size;
+	bool rddm_prealloc;
+	u32 rddm_seg_len;
 };
 
 /**
-- 
2.34.1

Re: [PATCH 2/6] bus: mhi: add support to allocate rddm memory during crash time

[Jeffrey Hugo]

On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
> From: Ram Kumar D <quic_ramd@quicinc.com>
> 
> Currently, MHI bus pre-allocates the RDDM buffer for crash dump
> collection during MHI power up.
> 
> To avoid carving out memory for RDDM buffers even if it is unutilized,
> add support to allocate memory at runtime during the RDDM download
> after target crash.
> 
> This feature can be controlled by the client driver registering the MHI
> controller by setting the rddm_prealloc flag to false in mhi_cntrl.
> By default rddm_prealloc is true, retaining the existing behaviour.
> 
> By default, rddm_seg_len will be same as seg_len. The client driver
> can override the mhi_cntrl->rddm_seg_len.
> 
> Signed-off-by: Ram Kumar D <quic_ramd@quicinc.com>
> Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
> ---
>   drivers/bus/mhi/host/boot.c     | 149 +++++++++++++++++++++++++++-----
>   drivers/bus/mhi/host/init.c     |  19 ++--
>   drivers/bus/mhi/host/internal.h |  11 ++-
>   drivers/bus/mhi/host/main.c     |   4 +-
>   drivers/bus/mhi/host/pm.c       |   2 +-
>   include/linux/mhi.h             |   2 +

NACK.  None of this gets used, making it dead code.

>   6 files changed, 156 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index ca842facf820..1a918e340424 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -35,6 +35,16 @@ int mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
>   		bhi_vec->size = mhi_buf->len;
>   	}
>   
> +	if (!mhi_cntrl->rddm_prealloc) {
> +		mhi_buf->dma_addr = dma_map_single(mhi_cntrl->cntrl_dev,
> +						   mhi_buf->buf, mhi_buf->len,
> +						   DMA_TO_DEVICE);
> +		if (dma_mapping_error(mhi_cntrl->cntrl_dev, mhi_buf->dma_addr)) {
> +			dev_err(dev, "dma mapping failed\n");
> +			return -ENOMEM;
> +		}
> +	}
> +
>   	dev_dbg(dev, "BHIe programming for RDDM\n");
>   
>   	mhi_write_reg(mhi_cntrl, base, BHIE_RXVECADDR_HIGH_OFFS,
> @@ -158,10 +168,35 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
>   {
>   	void __iomem *base = mhi_cntrl->bhie;
>   	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	struct mhi_buf *mhi_buf = NULL;
>   	u32 rx_status;
> +	int ret;
>   
> -	if (in_panic)
> -		return __mhi_download_rddm_in_panic(mhi_cntrl);
> +	/*
> +	 * Allocate RDDM table if specified, this table is for debugging purpose
> +	 */
> +	if (!mhi_cntrl->rddm_prealloc && mhi_cntrl->rddm_size) {
> +		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->rddm_image,
> +					   mhi_cntrl->rddm_size, IMG_TYPE_RDDM);
> +		if (ret) {
> +			dev_err(dev, "Failed to allocate RDDM table memory\n");
> +			return ret;
> +		}
> +
> +		/* setup the RX vector table */
> +		ret = mhi_rddm_prepare(mhi_cntrl, mhi_cntrl->rddm_image);
> +		if (ret) {
> +			dev_err(dev, "Failed to prepare RDDM\n");
> +			mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->rddm_image,
> +					    IMG_TYPE_RDDM);
> +			return ret;
> +		}
> +	}
> +
> +	if (in_panic) {
> +		ret = __mhi_download_rddm_in_panic(mhi_cntrl);
> +		goto out;
> +	}
>   
>   	dev_dbg(dev, "Waiting for RDDM image download via BHIe\n");
>   
> @@ -173,7 +208,16 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
>   					      &rx_status) || rx_status,
>   			   msecs_to_jiffies(mhi_cntrl->timeout_ms));
>   
> -	return (rx_status == BHIE_RXVECSTATUS_STATUS_XFER_COMPL) ? 0 : -EIO;
> +	ret = (rx_status == BHIE_RXVECSTATUS_STATUS_XFER_COMPL) ? 0 : -EIO;
> +
> +out:
> +	mhi_buf = &mhi_cntrl->rddm_image->mhi_buf[mhi_cntrl->rddm_image->entries - 1];
> +
> +	if (!mhi_cntrl->rddm_prealloc)
> +		dma_unmap_single(mhi_cntrl->cntrl_dev, mhi_buf->dma_addr,
> +				 mhi_buf->len, DMA_TO_DEVICE);
> +
> +	return ret;
>   }
>   EXPORT_SYMBOL_GPL(mhi_download_rddm_image);
>   
> @@ -297,14 +341,25 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
>   }
>   
>   void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
> -			 struct image_info *image_info)
> +			 struct image_info *image_info,
> +			 enum image_type img_type)
>   {
>   	int i;
>   	struct mhi_buf *mhi_buf = image_info->mhi_buf;
>   
> -	for (i = 0; i < image_info->entries; i++, mhi_buf++)
> -		mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
> -				     mhi_buf->buf, mhi_buf->dma_addr);
> +	for (i = 0; i < image_info->entries; i++, mhi_buf++) {
> +		if (img_type == IMG_TYPE_RDDM && !mhi_cntrl->rddm_prealloc) {
> +			if (i == (image_info->entries - 1))
> +				dma_unmap_single(mhi_cntrl->cntrl_dev,
> +						 mhi_buf->dma_addr,
> +						 mhi_buf->len,
> +						 DMA_FROM_DEVICE);
> +			kfree(mhi_buf->buf);
> +		} else {
> +			mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
> +					     mhi_buf->buf, mhi_buf->dma_addr);
> +		}
> +	}
>   
>   	kfree(image_info->mhi_buf);
>   	kfree(image_info);
> @@ -312,21 +367,31 @@ void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
>   
>   int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>   			 struct image_info **image_info,
> -			 size_t alloc_size)
> +			 size_t alloc_size, enum image_type img_type)
>   {
>   	size_t seg_size = mhi_cntrl->seg_len;
> -	int segments = DIV_ROUND_UP(alloc_size, seg_size) + 1;
> +	int segments;
>   	int i;
>   	struct image_info *img_info;
>   	struct mhi_buf *mhi_buf;
> +	/* Masked __GFP_DIRECT_RECLAIM flag for non-interrupt context
> +	 * to avoid rcu context sleep issue in kmalloc during kernel panic
> +	 */

Incorrect comment style.

Also, why would RDDM be relevant to a Linux kernel panic?  This makes me 
suspect a lot is wrong with this patch.

> +	gfp_t gfp = (in_interrupt() ? GFP_ATOMIC :
> +		((GFP_KERNEL | __GFP_NORETRY) & ~__GFP_DIRECT_RECLAIM));
> +
> +	if (img_type == IMG_TYPE_RDDM)
> +		seg_size = mhi_cntrl->rddm_seg_len;
>   
> -	img_info = kzalloc(sizeof(*img_info), GFP_KERNEL);
> +	segments = DIV_ROUND_UP(alloc_size, seg_size) + 1;
> +
> +	img_info = kzalloc(sizeof(*img_info), gfp);
>   	if (!img_info)
>   		return -ENOMEM;
>   
>   	/* Allocate memory for entries */
>   	img_info->mhi_buf = kcalloc(segments, sizeof(*img_info->mhi_buf),
> -				    GFP_KERNEL);
> +				    gfp);
>   	if (!img_info->mhi_buf)
>   		goto error_alloc_mhi_buf;
>   
> @@ -340,11 +405,42 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>   			vec_size = sizeof(struct bhi_vec_entry) * i;
>   
>   		mhi_buf->len = vec_size;
> -		mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl, vec_size,
> -						     &mhi_buf->dma_addr,
> -						     GFP_KERNEL);
> -		if (!mhi_buf->buf)
> -			goto error_alloc_segment;
> +
> +		if (img_type == IMG_TYPE_RDDM && !mhi_cntrl->rddm_prealloc) {
> +			/* Vector table is the last entry */
> +			if (i == segments - 1) {
> +				mhi_buf->buf = kzalloc(PAGE_ALIGN(vec_size),
> +						       gfp);
> +				if (!mhi_buf->buf)
> +					goto error_alloc_segment;
> +
> +				/* Vector table entry will be dma_mapped during
> +				 * rddm prepare with DMA_TO_DEVICE and unmapped
> +				 * once the target completes the RDDM XFER.

Incorrect comment style

> +				 */
> +				continue;
> +			}
> +			mhi_buf->buf = kmalloc(vec_size, gfp);
> +			if (!mhi_buf->buf)
> +				goto error_alloc_segment;
> +
> +			mhi_buf->dma_addr = dma_map_single(mhi_cntrl->cntrl_dev,
> +							   mhi_buf->buf,
> +							   vec_size,
> +							   DMA_FROM_DEVICE);
> +			if (dma_mapping_error(mhi_cntrl->cntrl_dev,
> +					      mhi_buf->dma_addr)) {
> +				kfree(mhi_buf->buf);
> +				goto error_alloc_segment;
> +			}
> +		} else {
> +			mhi_buf->buf = mhi_fw_alloc_coherent(mhi_cntrl,
> +							     vec_size,
> +							     &mhi_buf->dma_addr,
> +							     GFP_KERNEL);
> +			if (!mhi_buf->buf)
> +				goto error_alloc_segment;
> +		}
>   	}
>   
>   	img_info->bhi_vec = img_info->mhi_buf[segments - 1].buf;
> @@ -354,9 +450,18 @@ int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>   	return 0;
>   
>   error_alloc_segment:
> -	for (--i, --mhi_buf; i >= 0; i--, mhi_buf--)
> -		mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
> -				     mhi_buf->buf, mhi_buf->dma_addr);
> +	for (--i, --mhi_buf; i >= 0; i--, mhi_buf--) {
> +		if (img_type == IMG_TYPE_RDDM && !mhi_cntrl->rddm_prealloc) {
> +			dma_unmap_single(mhi_cntrl->cntrl_dev,
> +					 mhi_buf->dma_addr, mhi_buf->len,
> +					 DMA_FROM_DEVICE);
> +			kfree(mhi_buf->buf);
> +
> +		} else {
> +			mhi_fw_free_coherent(mhi_cntrl, mhi_buf->len,
> +					     mhi_buf->buf, mhi_buf->dma_addr);
> +		}
> +	}
>   
>   error_alloc_mhi_buf:
>   	kfree(img_info);
> @@ -485,7 +590,8 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>   	 * device transitioning into MHI READY state
>   	 */
>   	if (mhi_cntrl->fbc_download) {
> -		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, fw_sz);
> +		ret = mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->fbc_image, fw_sz,
> +					   IMG_TYPE_FBC);
>   		if (ret) {
>   			release_firmware(firmware);
>   			goto error_fw_load;
> @@ -510,7 +616,8 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>   
>   error_ready_state:
>   	if (mhi_cntrl->fbc_download) {
> -		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
> +		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image,
> +				    IMG_TYPE_FBC);
>   		mhi_cntrl->fbc_image = NULL;
>   	}
>   
> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index c1e1412c43e2..8a47c3354560 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -1058,6 +1058,9 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>   		mhi_cntrl->unmap_single = mhi_unmap_single_no_bb;
>   	}
>   
> +	mhi_cntrl->rddm_prealloc = true;
> +	mhi_cntrl->rddm_seg_len = mhi_cntrl->seg_len;
> +
>   	mhi_cntrl->index = ida_alloc(&mhi_controller_ida, GFP_KERNEL);
>   	if (mhi_cntrl->index < 0) {
>   		ret = mhi_cntrl->index;
> @@ -1224,14 +1227,18 @@ int mhi_prepare_for_power_up(struct mhi_controller *mhi_cntrl)
>   		/*
>   		 * Allocate RDDM table for debugging purpose if specified
>   		 */
> -		mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->rddm_image,
> -				     mhi_cntrl->rddm_size);
> +		if (mhi_cntrl->rddm_prealloc)
> +			mhi_alloc_bhie_table(mhi_cntrl, &mhi_cntrl->rddm_image,
> +					     mhi_cntrl->rddm_size,
> +					     IMG_TYPE_RDDM);
> +
>   		if (mhi_cntrl->rddm_image) {
>   			ret = mhi_rddm_prepare(mhi_cntrl,
>   					       mhi_cntrl->rddm_image);
>   			if (ret) {
>   				mhi_free_bhie_table(mhi_cntrl,
> -						    mhi_cntrl->rddm_image);
> +						    mhi_cntrl->rddm_image,
> +						    IMG_TYPE_RDDM);
>   				goto error_reg_offset;
>   			}
>   		}
> @@ -1254,12 +1261,14 @@ EXPORT_SYMBOL_GPL(mhi_prepare_for_power_up);
>   void mhi_unprepare_after_power_down(struct mhi_controller *mhi_cntrl)
>   {
>   	if (mhi_cntrl->fbc_image) {
> -		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image);
> +		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->fbc_image,
> +				    IMG_TYPE_FBC);
>   		mhi_cntrl->fbc_image = NULL;
>   	}
>   
>   	if (mhi_cntrl->rddm_image) {
> -		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->rddm_image);
> +		mhi_free_bhie_table(mhi_cntrl, mhi_cntrl->rddm_image,
> +				    IMG_TYPE_RDDM);
>   		mhi_cntrl->rddm_image = NULL;
>   	}
>   
> diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
> index 41ce100d87d2..c7946f81be38 100644
> --- a/drivers/bus/mhi/host/internal.h
> +++ b/drivers/bus/mhi/host/internal.h
> @@ -176,6 +176,11 @@ enum mhi_er_type {
>   	MHI_ER_TYPE_VALID = 0x1,
>   };
>   
> +enum image_type {
> +	IMG_TYPE_FBC,
> +	IMG_TYPE_RDDM,
> +};
> +
>   struct db_cfg {
>   	bool reset_req;
>   	bool db_mode;
> @@ -314,9 +319,11 @@ int mhi_destroy_device(struct device *dev, void *data);
>   void mhi_create_devices(struct mhi_controller *mhi_cntrl);
>   
>   int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
> -			 struct image_info **image_info, size_t alloc_size);
> +			 struct image_info **image_info, size_t alloc_size,
> +			 enum image_type img_type);
>   void mhi_free_bhie_table(struct mhi_controller *mhi_cntrl,
> -			 struct image_info *image_info);
> +			 struct image_info *image_info,
> +			 enum image_type img_type);
>   
>   /* Power management APIs */
>   enum mhi_pm_state __must_check mhi_tryset_pm_state(
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index 4de75674f193..2f44f11fa5a6 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -503,13 +503,13 @@ irqreturn_t mhi_intvec_threaded_handler(int irq_number, void *priv)
>   	}
>   	write_unlock_irq(&mhi_cntrl->pm_lock);
>   
> -	if (pm_state != MHI_PM_SYS_ERR_DETECT)
> +	if (pm_state != MHI_PM_SYS_ERR_DETECT && ee != MHI_EE_RDDM)
>   		goto exit_intvec;
>   
>   	switch (ee) {
>   	case MHI_EE_RDDM:
>   		/* proceed if power down is not already in progress */
> -		if (mhi_cntrl->rddm_image && mhi_is_active(mhi_cntrl)) {
> +		if (mhi_cntrl->rddm_size && mhi_is_active(mhi_cntrl)) {
>   			mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_RDDM);
>   			mhi_cntrl->ee = ee;
>   			wake_up_all(&mhi_cntrl->state_event);
> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
> index 11c0e751f223..68524e27e76c 100644
> --- a/drivers/bus/mhi/host/pm.c
> +++ b/drivers/bus/mhi/host/pm.c
> @@ -767,7 +767,7 @@ void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl)
>   	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>   
>   	/* skip if controller supports RDDM */
> -	if (mhi_cntrl->rddm_image) {
> +	if (mhi_cntrl->rddm_size) {
>   		dev_dbg(dev, "Controller supports RDDM, skip SYS_ERROR\n");
>   		return;
>   	}
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index c788c12039b5..ce229a6a2b9a 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -452,6 +452,8 @@ struct mhi_controller {
>   	struct device_node *cma_node;
>   	phys_addr_t cma_base;
>   	size_t cma_size;
> +	bool rddm_prealloc;
> +	u32 rddm_seg_len;
>   };
>   
>   /**

Re: [PATCH 2/6] bus: mhi: add support to allocate rddm memory during crash time

[Gokul Sriram P]

On 7/18/2024 9:52 PM, Jeffrey Hugo wrote:
> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>> From: Ram Kumar D <quic_ramd@quicinc.com>
>>
>> Currently, MHI bus pre-allocates the RDDM buffer for crash dump
>> collection during MHI power up.
>>
>> To avoid carving out memory for RDDM buffers even if it is unutilized,
>> add support to allocate memory at runtime during the RDDM download
>> after target crash.
>>
>> This feature can be controlled by the client driver registering the MHI
>> controller by setting the rddm_prealloc flag to false in mhi_cntrl.
>> By default rddm_prealloc is true, retaining the existing behaviour.
>>
>> By default, rddm_seg_len will be same as seg_len. The client driver
>> can override the mhi_cntrl->rddm_seg_len.
>>
>> Signed-off-by: Ram Kumar D <quic_ramd@quicinc.com>
>> Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/boot.c     | 149 +++++++++++++++++++++++++++-----
>>   drivers/bus/mhi/host/init.c     |  19 ++--
>>   drivers/bus/mhi/host/internal.h |  11 ++-
>>   drivers/bus/mhi/host/main.c     |   4 +-
>>   drivers/bus/mhi/host/pm.c       |   2 +-
>>   include/linux/mhi.h             |   2 +
>
> NACK.  None of this gets used, making it dead code.
I didn't get it. Do you mean to say "mhi_cntrl->rddm_prealloc = true"  
by default, so its dead code?
>
>>   6 files changed, 156 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
>> index ca842facf820..1a918e340424 100644
>> --- a/drivers/bus/mhi/host/boot.c
>> +++ b/drivers/bus/mhi/host/boot.c
>> @@ -35,6 +35,16 @@ int mhi_rddm_prepare(struct mhi_controller 
>> *mhi_cntrl,
>>           bhi_vec->size = mhi_buf->len;
>>       }
>>   +    if (!mhi_cntrl->rddm_prealloc) {
>> +        mhi_buf->dma_addr = dma_map_single(mhi_cntrl->cntrl_dev,
>> +                           mhi_buf->buf, mhi_buf->len,
...
>> @@ -312,21 +367,31 @@ void mhi_free_bhie_table(struct mhi_controller 
>> *mhi_cntrl,
>>     int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>>                struct image_info **image_info,
>> -             size_t alloc_size)
>> +             size_t alloc_size, enum image_type img_type)
>>   {
>>       size_t seg_size = mhi_cntrl->seg_len;
>> -    int segments = DIV_ROUND_UP(alloc_size, seg_size) + 1;
>> +    int segments;
>>       int i;
>>       struct image_info *img_info;
>>       struct mhi_buf *mhi_buf;
>> +    /* Masked __GFP_DIRECT_RECLAIM flag for non-interrupt context
>> +     * to avoid rcu context sleep issue in kmalloc during kernel panic
>> +     */
>
> Incorrect comment style.
>
Will correct this.


> Also, why would RDDM be relevant to a Linux kernel panic?  This makes 
> me suspect a lot is wrong with this patch.
>
When mhi device crashed, kernel also could be in panic, so during panic, 
its necessary to collect RDDM. This state is already supported in mhi 
driver.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bus/mhi/host/boot.c?h=v6.10#n163

...

>> +            /* Vector table is the last entry */
>> +            if (i == segments - 1) {
>> +                mhi_buf->buf = kzalloc(PAGE_ALIGN(vec_size),
>> +                               gfp);
>> +                if (!mhi_buf->buf)
>> +                    goto error_alloc_segment;
>> +
>> +                /* Vector table entry will be dma_mapped during
>> +                 * rddm prepare with DMA_TO_DEVICE and unmapped
>> +                 * once the target completes the RDDM XFER.
>
> Incorrect comment style
>
will correct this.

Regards,

Gokul

>> +                 */
>> +                continue;
>> +            }
>> +            mhi_buf->buf = kmalloc(vec_size, gfp);
>> +            if (!mhi_buf->buf)
>> +                goto error_alloc_segment;
>> +

Re: [PATCH 2/6] bus: mhi: add support to allocate rddm memory during crash time

[Jeffrey Hugo]

On 7/28/2024 7:40 AM, Gokul Sriram P wrote:
> 
> On 7/18/2024 9:52 PM, Jeffrey Hugo wrote:
>> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>>> From: Ram Kumar D <quic_ramd@quicinc.com>
>>>
>>> Currently, MHI bus pre-allocates the RDDM buffer for crash dump
>>> collection during MHI power up.
>>>
>>> To avoid carving out memory for RDDM buffers even if it is unutilized,
>>> add support to allocate memory at runtime during the RDDM download
>>> after target crash.
>>>
>>> This feature can be controlled by the client driver registering the MHI
>>> controller by setting the rddm_prealloc flag to false in mhi_cntrl.
>>> By default rddm_prealloc is true, retaining the existing behaviour.
>>>
>>> By default, rddm_seg_len will be same as seg_len. The client driver
>>> can override the mhi_cntrl->rddm_seg_len.
>>>
>>> Signed-off-by: Ram Kumar D <quic_ramd@quicinc.com>
>>> Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>>> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>>> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
>>> ---
>>>   drivers/bus/mhi/host/boot.c     | 149 +++++++++++++++++++++++++++-----
>>>   drivers/bus/mhi/host/init.c     |  19 ++--
>>>   drivers/bus/mhi/host/internal.h |  11 ++-
>>>   drivers/bus/mhi/host/main.c     |   4 +-
>>>   drivers/bus/mhi/host/pm.c       |   2 +-
>>>   include/linux/mhi.h             |   2 +
>>
>> NACK.  None of this gets used, making it dead code.
> I didn't get it. Do you mean to say "mhi_cntrl->rddm_prealloc = true" by 
> default, so its dead code?

For example, this patch adds rddm_prealloc to the mhi_cntrl struct.  You 
do not add a user for this.  No MHI controller in the kernel consumes 
this, and your series doesn't add one, therefore all the code you add in 
this change is dead because it will never get exercised.

This is not acceptable.

>>
>>>   6 files changed, 156 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
>>> index ca842facf820..1a918e340424 100644
>>> --- a/drivers/bus/mhi/host/boot.c
>>> +++ b/drivers/bus/mhi/host/boot.c
>>> @@ -35,6 +35,16 @@ int mhi_rddm_prepare(struct mhi_controller 
>>> *mhi_cntrl,
>>>           bhi_vec->size = mhi_buf->len;
>>>       }
>>>   +    if (!mhi_cntrl->rddm_prealloc) {
>>> +        mhi_buf->dma_addr = dma_map_single(mhi_cntrl->cntrl_dev,
>>> +                           mhi_buf->buf, mhi_buf->len,
> ...
>>> @@ -312,21 +367,31 @@ void mhi_free_bhie_table(struct mhi_controller 
>>> *mhi_cntrl,
>>>     int mhi_alloc_bhie_table(struct mhi_controller *mhi_cntrl,
>>>                struct image_info **image_info,
>>> -             size_t alloc_size)
>>> +             size_t alloc_size, enum image_type img_type)
>>>   {
>>>       size_t seg_size = mhi_cntrl->seg_len;
>>> -    int segments = DIV_ROUND_UP(alloc_size, seg_size) + 1;
>>> +    int segments;
>>>       int i;
>>>       struct image_info *img_info;
>>>       struct mhi_buf *mhi_buf;
>>> +    /* Masked __GFP_DIRECT_RECLAIM flag for non-interrupt context
>>> +     * to avoid rcu context sleep issue in kmalloc during kernel panic
>>> +     */
>>
>> Incorrect comment style.
>>
> Will correct this.
> 
> 
>> Also, why would RDDM be relevant to a Linux kernel panic?  This makes 
>> me suspect a lot is wrong with this patch.
>>
> When mhi device crashed, kernel also could be in panic, so during panic, 
> its necessary to collect RDDM. This state is already supported in mhi 
> driver.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bus/mhi/host/boot.c?h=v6.10#n163

That appears to be unused, so I fail to see the connection to a Linux 
kernel panic.

[PATCH 3/6] bus: mhi: increase RDDM timeout

[Gokul Sriram Palanisamy]

From: Praveenkumar I <quic_ipkumar@quicinc.com>

Host sometimes misses the EE RDDM during kernel panic causing
RDDM failure. Increase RDDM timeout to overcome this issue.

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 drivers/bus/mhi/host/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index 1a918e340424..324510d2c7fd 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -77,7 +77,7 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl)
 	enum mhi_ee_type ee;
 	const u32 delayus = 2000;
 	u32 retry = (mhi_cntrl->timeout_ms * 1000) / delayus;
-	const u32 rddm_timeout_us = 200000;
+	const u32 rddm_timeout_us = 400000;
 	int rddm_retry = rddm_timeout_us / delayus;
 	void __iomem *base = mhi_cntrl->bhie;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
-- 
2.34.1

Re: [PATCH 3/6] bus: mhi: increase RDDM timeout

[Jeffrey Hugo]

On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
> From: Praveenkumar I <quic_ipkumar@quicinc.com>
> 
> Host sometimes misses the EE RDDM during kernel panic causing
> RDDM failure. Increase RDDM timeout to overcome this issue.

If we are in the middle of a Linux kernel panic, the host is going down, 
so why do we care about RDDM?

How did you determine the new value?

Really seems like significantly more detail is needed here.

Re: [PATCH 3/6] bus: mhi: increase RDDM timeout

[Gokul Sriram P]

On 7/18/2024 9:53 PM, Jeffrey Hugo wrote:
> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>> From: Praveenkumar I <quic_ipkumar@quicinc.com>
>>
>> Host sometimes misses the EE RDDM during kernel panic causing
>> RDDM failure. Increase RDDM timeout to overcome this issue.
>
> If we are in the middle of a Linux kernel panic, the host is going 
> down, so why do we care about RDDM?
>
> How did you determine the new value?
>
> Really seems like significantly more detail is needed here.

If kernel panic was due to mhi, we need to have the RDDM dump and this 
is already supported in upstream MHI.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bus/mhi/host/boot.c?h=v6.10#n163

Regards,

Gokul

Re: [PATCH 3/6] bus: mhi: increase RDDM timeout

[Jeffrey Hugo]

On 7/28/2024 7:41 AM, Gokul Sriram P wrote:
> 
> On 7/18/2024 9:53 PM, Jeffrey Hugo wrote:
>> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>>> From: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>
>>> Host sometimes misses the EE RDDM during kernel panic causing
>>> RDDM failure. Increase RDDM timeout to overcome this issue.
>>
>> If we are in the middle of a Linux kernel panic, the host is going 
>> down, so why do we care about RDDM?
>>
>> How did you determine the new value?
>>
>> Really seems like significantly more detail is needed here.
> 
> If kernel panic was due to mhi, we need to have the RDDM dump and this 
> is already supported in upstream MHI.

Why would mhi cause a linux kernel panic?  That just sounds insane to 
me.  Also what you point to is not used, so I fail to see how it relates 
to your point here.

I'm still unconvinced that this change brings any value.

[PATCH 4/6] bus: mhi: dump debug registers in critical sections

[Gokul Sriram Palanisamy]

ERRCODE and ERRDBG registers are dumped during BHI failure.
Extend this to BHIe and RDDM failures. Also, add additional status
registers essential for debug and make this debug function available
for client driver.

Co-developed-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 drivers/bus/mhi/host/boot.c | 53 +++++++++++++++++-------------
 drivers/bus/mhi/host/main.c | 65 +++++++++++++++++++++++++++++++++++++
 drivers/bus/mhi/host/pm.c   |  4 ++-
 include/linux/mhi.h         |  5 +++
 4 files changed, 103 insertions(+), 24 deletions(-)

diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index 324510d2c7fd..b403890d873b 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -159,6 +159,7 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl)
 error_exit_rddm:
 	dev_err(dev, "RDDM transfer failed. Current EE: %s\n",
 		TO_MHI_EXEC_STR(ee));
+	mhi_debug_reg_dump(mhi_cntrl);
 
 	return -EIO;
 }
@@ -168,6 +169,7 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
 {
 	void __iomem *base = mhi_cntrl->bhie;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
 	struct mhi_buf *mhi_buf = NULL;
 	u32 rx_status;
 	int ret;
@@ -217,6 +219,15 @@ int mhi_download_rddm_image(struct mhi_controller *mhi_cntrl, bool in_panic)
 		dma_unmap_single(mhi_cntrl->cntrl_dev, mhi_buf->dma_addr,
 				 mhi_buf->len, DMA_TO_DEVICE);
 
+	if (ret) {
+		dev_err(dev, "RDDM transfer failed. RXVEC_STATUS: 0x%x\n",
+			rx_status);
+		read_lock_bh(pm_lock);
+		if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
+			mhi_debug_reg_dump(mhi_cntrl);
+		read_unlock_bh(pm_lock);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mhi_download_rddm_image);
@@ -263,8 +274,22 @@ static int mhi_fw_load_bhie(struct mhi_controller *mhi_cntrl,
 						   &tx_status) || tx_status,
 				 msecs_to_jiffies(mhi_cntrl->timeout_ms));
 	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
-	    tx_status != BHIE_TXVECSTATUS_STATUS_XFER_COMPL)
+	    tx_status != BHIE_TXVECSTATUS_STATUS_XFER_COMPL) {
+		dev_err(dev, "Upper:0x%x Lower:0x%x len:0x%zx sequence:%u\n",
+			upper_32_bits(mhi_buf->dma_addr),
+			lower_32_bits(mhi_buf->dma_addr),
+			mhi_buf->len, sequence_id);
+
+		dev_err(dev, "MHI pm_state: %s tx_status: %d ee: %s\n",
+			to_mhi_pm_state_str(mhi_cntrl->pm_state), tx_status,
+			TO_MHI_EXEC_STR(mhi_get_exec_env(mhi_cntrl)));
+
+		read_lock_bh(pm_lock);
+		if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
+			mhi_debug_reg_dump(mhi_cntrl);
+		read_unlock_bh(pm_lock);
 		return -EIO;
+	}
 
 	return (!ret) ? -ETIMEDOUT : 0;
 }
@@ -273,21 +298,11 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
 			   dma_addr_t dma_addr,
 			   size_t size)
 {
-	u32 tx_status, val, session_id;
-	int i, ret;
+	u32 tx_status, session_id;
+	int ret;
 	void __iomem *base = mhi_cntrl->bhi;
 	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
-	struct {
-		char *name;
-		u32 offset;
-	} error_reg[] = {
-		{ "ERROR_CODE", BHI_ERRCODE },
-		{ "ERROR_DBG1", BHI_ERRDBG1 },
-		{ "ERROR_DBG2", BHI_ERRDBG2 },
-		{ "ERROR_DBG3", BHI_ERRDBG3 },
-		{ NULL },
-	};
 
 	read_lock_bh(pm_lock);
 	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
@@ -319,16 +334,8 @@ static int mhi_fw_load_bhi(struct mhi_controller *mhi_cntrl,
 	if (tx_status == BHI_STATUS_ERROR) {
 		dev_err(dev, "Image transfer failed\n");
 		read_lock_bh(pm_lock);
-		if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
-			for (i = 0; error_reg[i].name; i++) {
-				ret = mhi_read_reg(mhi_cntrl, base,
-						   error_reg[i].offset, &val);
-				if (ret)
-					break;
-				dev_err(dev, "Reg: %s value: 0x%x\n",
-					error_reg[i].name, val);
-			}
-		}
+		if (MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
+			mhi_debug_reg_dump(mhi_cntrl);
 		read_unlock_bh(pm_lock);
 		goto invalid_pm_state;
 	}
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 2f44f11fa5a6..26baa04badf4 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -1707,3 +1707,68 @@ int mhi_get_channel_doorbell_offset(struct mhi_controller *mhi_cntrl, u32 *chdb_
 	return 0;
 }
 EXPORT_SYMBOL_GPL(mhi_get_channel_doorbell_offset);
+
+void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl)
+{
+	enum mhi_state state;
+	enum mhi_ee_type ee;
+	int i, ret;
+	u32 val;
+	void __iomem *mhi_base = mhi_cntrl->regs;
+	void __iomem *bhi_base = mhi_cntrl->bhi;
+	void __iomem *bhie_base = mhi_cntrl->bhie;
+	void __iomem *wake_db = mhi_cntrl->wake_db;
+	struct {
+		const char *name;
+		int offset;
+		void *base;
+	} debug_reg[] = {
+		{ "MHI_CNTRL", MHICTRL, mhi_base},
+		{ "MHI_STATUS", MHISTATUS, mhi_base},
+		{ "MHI_WAKE_DB", 0, wake_db},
+		{ "BHI_EXECENV", BHI_EXECENV, bhi_base},
+		{ "BHI_STATUS", BHI_STATUS, bhi_base},
+		{ "BHI_ERRCODE", BHI_ERRCODE, bhi_base},
+		{ "BHI_ERRDBG1", BHI_ERRDBG1, bhi_base},
+		{ "BHI_ERRDBG2", BHI_ERRDBG2, bhi_base},
+		{ "BHI_ERRDBG3", BHI_ERRDBG3, bhi_base},
+		{ "BHIE_TXVEC_DB", BHIE_TXVECDB_OFFS, bhie_base},
+		{ "BHIE_TXVEC_STATUS", BHIE_TXVECSTATUS_OFFS, bhie_base},
+		{ "BHIE_RXVEC_DB", BHIE_RXVECDB_OFFS, bhie_base},
+		{ "BHIE_RXVEC_STATUS", BHIE_RXVECSTATUS_OFFS, bhie_base},
+		{ "BHIE_IMGTXDB", BHI_IMGTXDB, bhie_base},
+		{ NULL },
+	};
+
+	dev_info(&mhi_cntrl->mhi_dev->dev,
+		 "host pm_state:%s dev_state:%s ee:%s\n",
+		 to_mhi_pm_state_str(mhi_cntrl->pm_state),
+		 mhi_state_str(mhi_cntrl->dev_state),
+		 TO_MHI_EXEC_STR(mhi_cntrl->ee));
+
+	state = mhi_get_mhi_state(mhi_cntrl);
+
+	if (!mhi_cntrl->bhi) {
+		dev_err(&mhi_cntrl->mhi_dev->dev,
+			"BHI not initialized, failed to dump debug registers\n");
+		return;
+	}
+
+	ee = mhi_get_exec_env(mhi_cntrl);
+
+	dev_info(&mhi_cntrl->mhi_dev->dev,
+		 "device ee:%s dev_state:%s\n", TO_MHI_EXEC_STR(ee),
+		 mhi_state_str(state));
+
+	for (i = 0; debug_reg[i].name; i++) {
+		if (!debug_reg[i].base)
+			continue;
+
+		ret = mhi_read_reg(mhi_cntrl, debug_reg[i].base,
+				   debug_reg[i].offset, &val);
+		dev_info(&mhi_cntrl->mhi_dev->dev,
+			 "reg:%s val:0x%x, ret:%d\n", debug_reg[i].name, val,
+			 ret);
+	}
+}
+EXPORT_SYMBOL_GPL(mhi_debug_reg_dump);
diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
index 68524e27e76c..5db99e092dbe 100644
--- a/drivers/bus/mhi/host/pm.c
+++ b/drivers/bus/mhi/host/pm.c
@@ -1267,8 +1267,10 @@ int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
 			   msecs_to_jiffies(timeout_ms));
 
 	ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -ETIMEDOUT;
-	if (ret)
+	if (ret) {
+		mhi_debug_reg_dump(mhi_cntrl);
 		mhi_power_down(mhi_cntrl, false);
+	}
 
 	return ret;
 }
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index ce229a6a2b9a..c0c9bfc28e4a 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -834,4 +834,9 @@ bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
  */
 int mhi_get_channel_doorbell_offset(struct mhi_controller *mhi_cntrl, u32 *chdb_offset);
 
+/**
+ * mhi_debug_reg_dump - dump MHI registers for debug purpose
+ * @mhi_cntrl: MHI controller
+ */
+void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl);
 #endif /* _MHI_H_ */
-- 
2.34.1

Re: [PATCH 4/6] bus: mhi: dump debug registers in critical sections

[Jeffrey Hugo]

On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -834,4 +834,9 @@ bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
>    */
>   int mhi_get_channel_doorbell_offset(struct mhi_controller *mhi_cntrl, u32 *chdb_offset);
>   
> +/**
> + * mhi_debug_reg_dump - dump MHI registers for debug purpose
> + * @mhi_cntrl: MHI controller
> + */
> +void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl);
>   #endif /* _MHI_H_ */

NACK.  This is not used.

Re: [PATCH 4/6] bus: mhi: dump debug registers in critical sections

[Gokul Sriram P]

On 7/18/2024 9:55 PM, Jeffrey Hugo wrote:
> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -834,4 +834,9 @@ bool mhi_queue_is_full(struct mhi_device 
>> *mhi_dev, enum dma_data_direction dir);
>>    */
>>   int mhi_get_channel_doorbell_offset(struct mhi_controller 
>> *mhi_cntrl, u32 *chdb_offset);
>>   +/**
>> + * mhi_debug_reg_dump - dump MHI registers for debug purpose
>> + * @mhi_cntrl: MHI controller
>> + */
>> +void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl);
>>   #endif /* _MHI_H_ */
>
> NACK.  This is not used.
mhi_debug_reg_dump - this is added in 3 places, mhi_fw_load_bhi( ), 
mhi_fw_load_bhie( ) and mhi_download_rddm_image( ) to print error codes 
on failure scenarios.

Re: [PATCH 4/6] bus: mhi: dump debug registers in critical sections

[Jeffrey Hugo]

On 7/28/2024 11:47 PM, Gokul Sriram P wrote:
> 
> On 7/18/2024 9:55 PM, Jeffrey Hugo wrote:
>> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>>> --- a/include/linux/mhi.h
>>> +++ b/include/linux/mhi.h
>>> @@ -834,4 +834,9 @@ bool mhi_queue_is_full(struct mhi_device 
>>> *mhi_dev, enum dma_data_direction dir);
>>>    */
>>>   int mhi_get_channel_doorbell_offset(struct mhi_controller 
>>> *mhi_cntrl, u32 *chdb_offset);
>>>   +/**
>>> + * mhi_debug_reg_dump - dump MHI registers for debug purpose
>>> + * @mhi_cntrl: MHI controller
>>> + */
>>> +void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl);
>>>   #endif /* _MHI_H_ */
>>
>> NACK.  This is not used.
> mhi_debug_reg_dump - this is added in 3 places, mhi_fw_load_bhi( ), 
> mhi_fw_load_bhie( ) and mhi_download_rddm_image( ) to print error codes 
> on failure scenarios.

You add this to the MHI API and export it as a symbol.  Nothing outside 
MHI uses this.

[PATCH 5/6] bus: mhi: check for RDDM cookie set by device to indicate readiness

[Gokul Sriram Palanisamy]

From: Rajkumar Ayyasamy <quic_arajkuma@quicinc.com>

If the device is unable to send the mission mode execution
environment change event but has already entered mission mode
with the ability to allow ramdump collection, it can set a unique
cookie pattern to indicate the availability of ramdumps. Allow
the controller to query for this unique pattern upon any bootup
failure or timeout.

Signed-off-by: Rajkumar Ayyasamy <quic_arajkuma@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 drivers/bus/mhi/host/main.c | 44 +++++++++++++++++++++++++++++++++++++
 include/linux/mhi.h         |  8 +++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 26baa04badf4..de804a701b85 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -1772,3 +1772,47 @@ void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl)
 	}
 }
 EXPORT_SYMBOL_GPL(mhi_debug_reg_dump);
+
+bool mhi_scan_rddm_cookie(struct mhi_controller *mhi_cntrl, u32 cookie)
+{
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	int ret;
+	int i;
+	u32 val;
+	bool result = false;
+	struct {
+		char *name;
+		u32 offset;
+	} error_reg[] = {
+		{ "ERROR_DBG1", BHI_ERRDBG1 },
+		{ "ERROR_DBG2", BHI_ERRDBG2 },
+		{ "ERROR_DBG3", BHI_ERRDBG3 },
+		{ NULL },
+	};
+
+	if (!mhi_cntrl->rddm_size || !cookie)
+		return false;
+
+	dev_dbg(dev, "Checking BHI debug register for 0x%x\n", cookie);
+
+	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
+		return false;
+
+	/* look for an RDDM cookie match in any of the error debug registers */
+	for (i = 0; error_reg[i].name; i++) {
+		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi,
+				   error_reg[i].offset, &val);
+		if (ret)
+			break;
+		dev_dbg(dev, "reg:%s value:0x%x\n", error_reg[i].name, val);
+
+		if (!(val ^ cookie)) {
+			dev_err(dev, "RDDM cookie found in %s\n",
+				error_reg[i].name);
+			return true;
+		}
+	}
+	dev_dbg(dev, "RDDM cookie not found\n");
+	return result;
+}
+EXPORT_SYMBOL_GPL(mhi_scan_rddm_cookie);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index c0c9bfc28e4a..2f90de8616f3 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -839,4 +839,12 @@ int mhi_get_channel_doorbell_offset(struct mhi_controller *mhi_cntrl, u32 *chdb_
  * @mhi_cntrl: MHI controller
  */
 void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl);
+
+/**
+ * mhi_scan_rddm_cookie - Look for supplied cookie value in the BHI debug
+ * registers set by device to indicate rddm readiness for debugging purposes.
+ * @mhi_cntrl: MHI controller
+ * @cookie: cookie/pattern value to match
+ */
+bool mhi_scan_rddm_cookie(struct mhi_controller *mhi_cntrl, u32 cookie);
 #endif /* _MHI_H_ */
-- 
2.34.1

Re: [PATCH 5/6] bus: mhi: check for RDDM cookie set by device to indicate readiness

[Jeffrey Hugo]

On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index c0c9bfc28e4a..2f90de8616f3 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -839,4 +839,12 @@ int mhi_get_channel_doorbell_offset(struct mhi_controller *mhi_cntrl, u32 *chdb_
>    * @mhi_cntrl: MHI controller
>    */
>   void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl);
> +
> +/**
> + * mhi_scan_rddm_cookie - Look for supplied cookie value in the BHI debug
> + * registers set by device to indicate rddm readiness for debugging purposes.
> + * @mhi_cntrl: MHI controller
> + * @cookie: cookie/pattern value to match
> + */
> +bool mhi_scan_rddm_cookie(struct mhi_controller *mhi_cntrl, u32 cookie);
>   #endif /* _MHI_H_ */

NACK.  This is not used.

Re: [PATCH 5/6] bus: mhi: check for RDDM cookie set by device to indicate readiness

[Gokul Sriram P]

On 7/18/2024 9:55 PM, Jeffrey Hugo wrote:
> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index c0c9bfc28e4a..2f90de8616f3 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -839,4 +839,12 @@ int mhi_get_channel_doorbell_offset(struct 
>> mhi_controller *mhi_cntrl, u32 *chdb_
>>    * @mhi_cntrl: MHI controller
>>    */
>>   void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl);
>> +
>> +/**
>> + * mhi_scan_rddm_cookie - Look for supplied cookie value in the BHI 
>> debug
>> + * registers set by device to indicate rddm readiness for debugging 
>> purposes.
>> + * @mhi_cntrl: MHI controller
>> + * @cookie: cookie/pattern value to match
>> + */
>> +bool mhi_scan_rddm_cookie(struct mhi_controller *mhi_cntrl, u32 
>> cookie);
>>   #endif /* _MHI_H_ */
>
> NACK.  This is not used.

mhi_debug_reg_dump - this is added in 3 places, mhi_fw_load_bhi( ), 
mhi_fw_load_bhie( ) and mhi_download_rddm_image( ) to print error codes 
on failure scenarios.

What do you mean by not used?

Regards,

Gokul

Re: [PATCH 5/6] bus: mhi: check for RDDM cookie set by device to indicate readiness

[Jeffrey Hugo]

On 7/28/2024 7:41 AM, Gokul Sriram P wrote:
> 
> On 7/18/2024 9:55 PM, Jeffrey Hugo wrote:
>> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>>> index c0c9bfc28e4a..2f90de8616f3 100644
>>> --- a/include/linux/mhi.h
>>> +++ b/include/linux/mhi.h
>>> @@ -839,4 +839,12 @@ int mhi_get_channel_doorbell_offset(struct 
>>> mhi_controller *mhi_cntrl, u32 *chdb_
>>>    * @mhi_cntrl: MHI controller
>>>    */
>>>   void mhi_debug_reg_dump(struct mhi_controller *mhi_cntrl);
>>> +
>>> +/**
>>> + * mhi_scan_rddm_cookie - Look for supplied cookie value in the BHI 
>>> debug
>>> + * registers set by device to indicate rddm readiness for debugging 
>>> purposes.
>>> + * @mhi_cntrl: MHI controller
>>> + * @cookie: cookie/pattern value to match
>>> + */
>>> +bool mhi_scan_rddm_cookie(struct mhi_controller *mhi_cntrl, u32 
>>> cookie);
>>>   #endif /* _MHI_H_ */
>>
>> NACK.  This is not used.
> 
> mhi_debug_reg_dump - this is added in 3 places, mhi_fw_load_bhi( ), 
> mhi_fw_load_bhie( ) and mhi_download_rddm_image( ) to print error codes 
> on failure scenarios.
> 
> What do you mean by not used?

You add an API to other drivers, mhi_scan_rddm_cookie(), but no code 
ever calls it.  This entire patch is useless as all it does is add dead 
code.

[PATCH 6/6] bus: mhi: change IRQ_HANDLED to IRQ_NONE in mhi_irq_handler

[Gokul Sriram Palanisamy]

From: Karthick Shanmugham <quic_kartshan@quicinc.com>

When mhi_irq_handler is a shared interrupt handler. It is the shared
interrupt handlers responsibility to identify its own interrupt and exit
quickly if it is not. If there is no pending event in the event ring
handled, exit the interrupt context returning IRQ_NONE denoting the
interrupt either doesn't belong to this event ring or not handled.

Signed-off-by: Karthick Shanmugham <quic_kartshan@quicinc.com>
Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 drivers/bus/mhi/host/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index de804a701b85..f87eb0c2b01a 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -462,7 +462,7 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
 
 	/* Only proceed if event ring has pending events */
 	if (ev_ring->rp == dev_rp)
-		return IRQ_HANDLED;
+		return IRQ_NONE;
 
 	/* For client managed event ring, notify pending data */
 	if (mhi_event->cl_manage) {
-- 
2.34.1

Re: [PATCH 6/6] bus: mhi: change IRQ_HANDLED to IRQ_NONE in mhi_irq_handler

[Jeffrey Hugo]

On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
> From: Karthick Shanmugham <quic_kartshan@quicinc.com>
> 
> When mhi_irq_handler is a shared interrupt handler. It is the shared
> interrupt handlers responsibility to identify its own interrupt and exit
> quickly if it is not. If there is no pending event in the event ring
> handled, exit the interrupt context returning IRQ_NONE denoting the
> interrupt either doesn't belong to this event ring or not handled.

How was this found?

Re: [PATCH 6/6] bus: mhi: change IRQ_HANDLED to IRQ_NONE in mhi_irq_handler

[Gokul Sriram P]

On 7/18/2024 9:59 PM, Jeffrey Hugo wrote:
> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>> From: Karthick Shanmugham <quic_kartshan@quicinc.com>
>>
>> When mhi_irq_handler is a shared interrupt handler. It is the shared
>> interrupt handlers responsibility to identify its own interrupt and exit
>> quickly if it is not. If there is no pending event in the event ring
>> handled, exit the interrupt context returning IRQ_NONE denoting the
>> interrupt either doesn't belong to this event ring or not handled.
>
> How was this found?

Internally, the proprietary cnss drivers shares the same msi interrupt 
for streaming certain debug information.

In that scenario, the interrupt didn't reach the corresponding shared 
handler since the mhi_irq_handler( ) returned IRQ_HANDLED when there is 
actually no pending mhi event.

This resulted in debug information not collected.

Regards,

Gokul

Re: [PATCH 6/6] bus: mhi: change IRQ_HANDLED to IRQ_NONE in mhi_irq_handler

[Jeffrey Hugo]

On 7/28/2024 7:41 AM, Gokul Sriram P wrote:
> 
> On 7/18/2024 9:59 PM, Jeffrey Hugo wrote:
>> On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
>>> From: Karthick Shanmugham <quic_kartshan@quicinc.com>
>>>
>>> When mhi_irq_handler is a shared interrupt handler. It is the shared
>>> interrupt handlers responsibility to identify its own interrupt and exit
>>> quickly if it is not. If there is no pending event in the event ring
>>> handled, exit the interrupt context returning IRQ_NONE denoting the
>>> interrupt either doesn't belong to this event ring or not handled.
>>
>> How was this found?
> 
> Internally, the proprietary cnss drivers shares the same msi interrupt 
> for streaming certain debug information.
> 
> In that scenario, the interrupt didn't reach the corresponding shared 
> handler since the mhi_irq_handler( ) returned IRQ_HANDLED when there is 
> actually no pending mhi event.
> 
> This resulted in debug information not collected.

You should probably explain this in the commit text.

Also, are you sure other parts of the irq handler don't need to be updated?

Re: [PATCH 0/6] add improvements to mhi driver

[Jeffrey Hugo]

On 7/18/2024 12:13 AM, Gokul Sriram Palanisamy wrote:
> The below patches address
> - memory fragmentation issues: when several iterations of mhi controller
>    register/unregister happens. In our case, we have a PCIe WiFi device for
>    which fw is loaded everytime when 'wifi load' happens. For this, we need
>    to allocate 9MiB of coherent memory with 512K segment size. After several
>    iterations, when the memory gets fragmented and 512K segment becomes
>    unavailable causing alloc failure.
>    
> - debug requirements by printing critical mhi debug registers
> - bug fixes to mhi_irq_handler
> - RDDM timeout issue
> - memory carveout for RDDM segments.
> 
> Gokul Sriram Palanisamy (2):
>    drivers: bus: mhi: Added shared-dma-pool support for mhi_dev
>    bus: mhi: dump debug registers in critical sections
> 
> Karthick Shanmugham (1):
>    bus: mhi: change IRQ_HANDLED to IRQ_NONE in mhi_irq_handler
> 
> Praveenkumar I (1):
>    bus: mhi: increase RDDM timeout
> 
> Rajkumar Ayyasamy (1):
>    bus: mhi: check for RDDM cookie set by device to indicate readiness
> 
> Ram Kumar D (1):
>    bus: mhi: add support to allocate rddm memory during crash time
> 
>   drivers/bus/mhi/host/boot.c     | 209 ++++++++++++++++++++++++--------
>   drivers/bus/mhi/host/init.c     |  70 ++++++++++-
>   drivers/bus/mhi/host/internal.h |  37 +++++-
>   drivers/bus/mhi/host/main.c     | 115 +++++++++++++++++-
>   drivers/bus/mhi/host/pm.c       |   6 +-
>   include/linux/mhi.h             |  20 +++
>   6 files changed, 397 insertions(+), 60 deletions(-)
> 

Please post v2 to the internal review list before posting to the public 
lists.