linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI
@ 2020-04-28  2:59 Bhaumik Bhatt
  2020-04-28  2:59 ` [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel Bhaumik Bhatt
                   ` (7 more replies)
  0 siblings, 8 replies; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

A set of patches for bug fixes and improved logging in mhi/core/boot.c.
Verified on x86 and arm64 platforms.

v2:
-Fix channel ID range check potential infinite loop
-Add appropriate signed-off-by tags

Bhaumik Bhatt (5):
  bus: mhi: core: Handle firmware load using state worker
  bus: mhi: core: WARN_ON for malformed vector table
  bus: mhi: core: Return appropriate error codes for AMSS load failure
  bus: mhi: core: Improve debug logs for loading firmware
  bus: mhi: core: Ensure non-zero session or sequence ID values

Hemant Kumar (3):
  bus: mhi: core: Cache intmod from mhi event to mhi channel
  bus: mhi: core: Add range check for channel id received in event ring
  bus: mhi: core: Read transfer length from an event properly

 drivers/bus/mhi/core/boot.c     | 74 +++++++++++++++++++++++++----------------
 drivers/bus/mhi/core/init.c     |  5 ++-
 drivers/bus/mhi/core/internal.h |  1 +
 drivers/bus/mhi/core/main.c     | 18 +++++++---
 drivers/bus/mhi/core/pm.c       |  6 +---
 include/linux/mhi.h             |  2 --
 6 files changed, 65 insertions(+), 41 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel
  2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
@ 2020-04-28  2:59 ` Bhaumik Bhatt
  2020-04-28 14:39   ` Jeffrey Hugo
  2020-04-28  2:59 ` [PATCH v2 2/8] bus: mhi: core: Add range check for channel id received in event ring Bhaumik Bhatt
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

From: Hemant Kumar <hemantk@codeaurora.org>

Driver is using zero initialized intmod value from mhi channel when
configuring TRE for bei field. This prevents interrupt moderation to
take effect in case it is supported by an event ring. Fix this by
copying intmod value from associated event ring to mhi channel upon
registering mhi controller.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/init.c | 4 ++++
 drivers/bus/mhi/core/main.c | 9 ++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index b38359c..4dc7f22 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -864,6 +864,10 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 		mutex_init(&mhi_chan->mutex);
 		init_completion(&mhi_chan->completion);
 		rwlock_init(&mhi_chan->lock);
+
+		/* used in setting bei field of TRE */
+		mhi_event = &mhi_cntrl->mhi_event[mhi_chan->er_index];
+		mhi_chan->intmod = mhi_event->intmod;
 	}
 
 	if (mhi_cntrl->bounce_buf) {
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index eb4256b..23154f1 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -929,7 +929,7 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
 	struct mhi_ring *buf_ring = &mhi_chan->buf_ring;
 	struct mhi_buf_info *buf_info;
 	struct mhi_tre *mhi_tre;
-	int ret;
+	int ret, bei;
 
 	/* If MHI host pre-allocates buffers then client drivers cannot queue */
 	if (mhi_chan->pre_alloc)
@@ -966,10 +966,11 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
 		goto map_error;
 
 	mhi_tre = tre_ring->wp;
+	bei = !!(mhi_chan->intmod);
 
 	mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr);
 	mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(buf_info->len);
-	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(1, 1, 0, 0);
+	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, 1, 0, 0);
 
 	/* increment WP */
 	mhi_add_ring_element(mhi_cntrl, tre_ring);
@@ -1006,6 +1007,7 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
 	struct mhi_ring *buf_ring = &mhi_chan->buf_ring;
 	struct mhi_buf_info *buf_info;
 	struct mhi_tre *mhi_tre;
+	int bei;
 
 	/* If MHI host pre-allocates buffers then client drivers cannot queue */
 	if (mhi_chan->pre_alloc)
@@ -1043,10 +1045,11 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
 	buf_info->len = len;
 
 	mhi_tre = tre_ring->wp;
+	bei = !!(mhi_chan->intmod);
 
 	mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr);
 	mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(buf_info->len);
-	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(1, 1, 0, 0);
+	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, 1, 0, 0);
 
 	/* increment WP */
 	mhi_add_ring_element(mhi_cntrl, tre_ring);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 2/8] bus: mhi: core: Add range check for channel id received in event ring
  2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
  2020-04-28  2:59 ` [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel Bhaumik Bhatt
@ 2020-04-28  2:59 ` Bhaumik Bhatt
  2020-04-28 14:44   ` Jeffrey Hugo
  2020-04-28  2:59 ` [PATCH v2 3/8] bus: mhi: core: Read transfer length from an event properly Bhaumik Bhatt
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

From: Hemant Kumar <hemantk@codeaurora.org>

MHI data completion handler function reads channel id from event
ring element. Value is under the control of MHI devices and can be
any value between 0 and 255. In order to prevent out of bound access
add a bound check against the max channel supported by controller
and skip processing of that event ring element.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 23154f1..1ccd4cc 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -827,6 +827,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
 		enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp);
 
 		chan = MHI_TRE_GET_EV_CHID(local_rp);
+		if (WARN_ON(chan >= mhi_cntrl->max_chan))
+			goto next_event;
+
 		mhi_chan = &mhi_cntrl->mhi_chan[chan];
 
 		if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
@@ -837,6 +840,7 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
 			event_quota--;
 		}
 
+next_event:
 		mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
 		local_rp = ev_ring->rp;
 		dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 3/8] bus: mhi: core: Read transfer length from an event properly
  2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
  2020-04-28  2:59 ` [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel Bhaumik Bhatt
  2020-04-28  2:59 ` [PATCH v2 2/8] bus: mhi: core: Add range check for channel id received in event ring Bhaumik Bhatt
@ 2020-04-28  2:59 ` Bhaumik Bhatt
  2020-04-28 14:50   ` Jeffrey Hugo
  2020-04-28  2:59 ` [PATCH v2 4/8] bus: mhi: core: Handle firmware load using state worker Bhaumik Bhatt
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

From: Hemant Kumar <hemantk@codeaurora.org>

When MHI Driver receives an EOT event, it reads xfer_len from the
event in the last TRE. The value is under control of the MHI device
and never validated by Host MHI driver. The value should never be
larger than the real size of the buffer but a malicious device can
set the value 0xFFFF as maximum. This causes device to memory
overflow (both read or write). Fix this issue by reading minimum of
transfer length from event and the buffer length provided.

Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 1ccd4cc..3d468d9 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -521,7 +521,10 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
 				mhi_cntrl->unmap_single(mhi_cntrl, buf_info);
 
 			result.buf_addr = buf_info->cb_buf;
-			result.bytes_xferd = xfer_len;
+
+			/* truncate to buf len if xfer_len is larger */
+			result.bytes_xferd =
+				min_t(u16, xfer_len, buf_info->len);
 			mhi_del_ring_element(mhi_cntrl, buf_ring);
 			mhi_del_ring_element(mhi_cntrl, tre_ring);
 			local_rp = tre_ring->rp;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 4/8] bus: mhi: core: Handle firmware load using state worker
  2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
                   ` (2 preceding siblings ...)
  2020-04-28  2:59 ` [PATCH v2 3/8] bus: mhi: core: Read transfer length from an event properly Bhaumik Bhatt
@ 2020-04-28  2:59 ` Bhaumik Bhatt
  2020-04-28 14:27   ` Jeffrey Hugo
  2020-04-28  2:59 ` [PATCH v2 5/8] bus: mhi: core: WARN_ON for malformed vector table Bhaumik Bhatt
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

Upon power up, driver queues firmware worker thread if the execution
environment is PBL. Firmware worker is blocked with a timeout until
state worker gets a chance to run and unblock firmware worker. An
endpoint power up failure can be seen if state worker gets a chance to
run after firmware worker has timed out. Remove this dependency and
handle firmware load directly using state worker thread.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c     | 18 +++---------------
 drivers/bus/mhi/core/init.c     |  1 -
 drivers/bus/mhi/core/internal.h |  1 +
 drivers/bus/mhi/core/pm.c       |  6 +-----
 include/linux/mhi.h             |  2 --
 5 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index ebad5eb..17c636b 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -377,30 +377,18 @@ static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,
 	}
 }
 
-void mhi_fw_load_worker(struct work_struct *work)
+void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 {
-	struct mhi_controller *mhi_cntrl;
 	const struct firmware *firmware = NULL;
 	struct image_info *image_info;
-	struct device *dev;
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 	const char *fw_name;
 	void *buf;
 	dma_addr_t dma_addr;
 	size_t size;
 	int ret;
 
-	mhi_cntrl = container_of(work, struct mhi_controller, fw_worker);
-	dev = &mhi_cntrl->mhi_dev->dev;
-
-	dev_dbg(dev, "Waiting for device to enter PBL from: %s\n",
-		TO_MHI_EXEC_STR(mhi_cntrl->ee));
-
-	ret = wait_event_timeout(mhi_cntrl->state_event,
-				 MHI_IN_PBL(mhi_cntrl->ee) ||
-				 MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
-				 msecs_to_jiffies(mhi_cntrl->timeout_ms));
-
-	if (!ret || MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
+	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
 		dev_err(dev, "Device MHI is not in valid state\n");
 		return;
 	}
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 4dc7f22..e92a20a 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -836,7 +836,6 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	spin_lock_init(&mhi_cntrl->wlock);
 	INIT_WORK(&mhi_cntrl->st_worker, mhi_pm_st_worker);
 	INIT_WORK(&mhi_cntrl->syserr_worker, mhi_pm_sys_err_worker);
-	INIT_WORK(&mhi_cntrl->fw_worker, mhi_fw_load_worker);
 	init_waitqueue_head(&mhi_cntrl->state_event);
 
 	mhi_cmd = mhi_cntrl->mhi_cmd;
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 5deadfa..4919a43 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -630,6 +630,7 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl,
 void mhi_deinit_free_irq(struct mhi_controller *mhi_cntrl);
 void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
 		      struct image_info *img_info);
+void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl);
 int mhi_prepare_channel(struct mhi_controller *mhi_cntrl,
 			struct mhi_chan *mhi_chan);
 int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 52690cb..dc90a71 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -528,7 +528,6 @@ static void mhi_pm_disable_transition(struct mhi_controller *mhi_cntrl,
 	dev_dbg(dev, "Waiting for all pending threads to complete\n");
 	wake_up_all(&mhi_cntrl->state_event);
 	flush_work(&mhi_cntrl->st_worker);
-	flush_work(&mhi_cntrl->fw_worker);
 
 	dev_dbg(dev, "Reset all active channels and remove MHI devices\n");
 	device_for_each_child(mhi_cntrl->cntrl_dev, NULL, mhi_destroy_device);
@@ -643,7 +642,7 @@ void mhi_pm_st_worker(struct work_struct *work)
 				mhi_cntrl->ee = mhi_get_exec_env(mhi_cntrl);
 			write_unlock_irq(&mhi_cntrl->pm_lock);
 			if (MHI_IN_PBL(mhi_cntrl->ee))
-				wake_up_all(&mhi_cntrl->state_event);
+				mhi_fw_load_handler(mhi_cntrl);
 			break;
 		case DEV_ST_TRANSITION_SBL:
 			write_lock_irq(&mhi_cntrl->pm_lock);
@@ -833,9 +832,6 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
 	next_state = MHI_IN_PBL(current_ee) ?
 		DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
 
-	if (next_state == DEV_ST_TRANSITION_PBL)
-		schedule_work(&mhi_cntrl->fw_worker);
-
 	mhi_queue_state_transition(mhi_cntrl, next_state);
 
 	mutex_unlock(&mhi_cntrl->pm_mutex);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index ad19960..cda7305 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -331,7 +331,6 @@ struct mhi_controller_config {
  * @wlock: Lock for protecting device wakeup
  * @mhi_link_info: Device bandwidth info
  * @st_worker: State transition worker
- * @fw_worker: Firmware download worker
  * @syserr_worker: System error worker
  * @state_event: State change event
  * @status_cb: CB function to notify power states of the device (required)
@@ -411,7 +410,6 @@ struct mhi_controller {
 	spinlock_t wlock;
 	struct mhi_link_info mhi_link_info;
 	struct work_struct st_worker;
-	struct work_struct fw_worker;
 	struct work_struct syserr_worker;
 	wait_queue_head_t state_event;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 5/8] bus: mhi: core: WARN_ON for malformed vector table
  2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
                   ` (3 preceding siblings ...)
  2020-04-28  2:59 ` [PATCH v2 4/8] bus: mhi: core: Handle firmware load using state worker Bhaumik Bhatt
@ 2020-04-28  2:59 ` Bhaumik Bhatt
  2020-04-28  2:59 ` [PATCH v2 6/8] bus: mhi: core: Return appropriate error codes for AMSS load failure Bhaumik Bhatt
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

Add a bounds check in the firmware copy routine to exit if a malformed
vector table is found while attempting to load the firmware in to the
BHIe vector table.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 17c636b..bc70edc 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -362,8 +362,14 @@ static void mhi_firmware_copy(struct mhi_controller *mhi_cntrl,
 	int i = 0;
 	struct mhi_buf *mhi_buf = img_info->mhi_buf;
 	struct bhi_vec_entry *bhi_vec = img_info->bhi_vec;
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 
 	while (remainder) {
+		if (WARN_ON(i >= img_info->entries)) {
+			dev_err(dev, "Malformed vector table\n");
+			return;
+		}
+
 		to_cpy = min(remainder, mhi_buf->len);
 		memcpy(mhi_buf->buf, buf, to_cpy);
 		bhi_vec->dma_addr = mhi_buf->dma_addr;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 6/8] bus: mhi: core: Return appropriate error codes for AMSS load failure
  2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
                   ` (4 preceding siblings ...)
  2020-04-28  2:59 ` [PATCH v2 5/8] bus: mhi: core: WARN_ON for malformed vector table Bhaumik Bhatt
@ 2020-04-28  2:59 ` Bhaumik Bhatt
  2020-04-28  2:59 ` [PATCH v2 7/8] bus: mhi: core: Improve debug logs for loading firmware Bhaumik Bhatt
  2020-04-28  2:59 ` [PATCH v2 8/8] bus: mhi: core: Ensure non-zero session or sequence ID values Bhaumik Bhatt
  7 siblings, 0 replies; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

When loading AMSS firmware using BHIe protocol, return -ETIMEDOUT if no
response is received within the timeout or return -EIO in case of a
protocol returned failure or an MHI error state.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index bc70edc..4e49a0e 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -176,6 +176,7 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
 	void __iomem *base = mhi_cntrl->bhie;
 	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
 	u32 tx_status, sequence_id;
+	int ret;
 
 	read_lock_bh(pm_lock);
 	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
@@ -198,19 +199,19 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
 	read_unlock_bh(pm_lock);
 
 	/* Wait for the image download to complete */
-	wait_event_timeout(mhi_cntrl->state_event,
-			   MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
-			   mhi_read_reg_field(mhi_cntrl, base,
-					      BHIE_TXVECSTATUS_OFFS,
-					      BHIE_TXVECSTATUS_STATUS_BMSK,
-					      BHIE_TXVECSTATUS_STATUS_SHFT,
-					      &tx_status) || tx_status,
-			   msecs_to_jiffies(mhi_cntrl->timeout_ms));
-
-	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state))
+	ret = wait_event_timeout(mhi_cntrl->state_event,
+				 MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
+				 mhi_read_reg_field(mhi_cntrl, base,
+						    BHIE_TXVECSTATUS_OFFS,
+						   BHIE_TXVECSTATUS_STATUS_BMSK,
+						   BHIE_TXVECSTATUS_STATUS_SHFT,
+						    &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)
 		return -EIO;
 
-	return (tx_status == BHIE_TXVECSTATUS_STATUS_XFER_COMPL) ? 0 : -EIO;
+	return (!ret) ? -ETIMEDOUT : 0;
 }
 
 static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 7/8] bus: mhi: core: Improve debug logs for loading firmware
  2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
                   ` (5 preceding siblings ...)
  2020-04-28  2:59 ` [PATCH v2 6/8] bus: mhi: core: Return appropriate error codes for AMSS load failure Bhaumik Bhatt
@ 2020-04-28  2:59 ` Bhaumik Bhatt
  2020-04-28  2:59 ` [PATCH v2 8/8] bus: mhi: core: Ensure non-zero session or sequence ID values Bhaumik Bhatt
  7 siblings, 0 replies; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

Add log messages to track boot flow errors and timeouts in SBL or AMSS
firmware loading to aid in debug.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 4e49a0e..0bc9c50 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -121,7 +121,8 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl)
 		ee = mhi_get_exec_env(mhi_cntrl);
 	}
 
-	dev_dbg(dev, "Waiting for image download completion, current EE: %s\n",
+	dev_dbg(dev,
+		"Waiting for RDDM image download via BHIe, current EE:%s\n",
 		TO_MHI_EXEC_STR(ee));
 
 	while (retry--) {
@@ -152,11 +153,14 @@ static int __mhi_download_rddm_in_panic(struct mhi_controller *mhi_cntrl)
 int mhi_download_rddm_img(struct mhi_controller *mhi_cntrl, bool in_panic)
 {
 	void __iomem *base = mhi_cntrl->bhie;
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 	u32 rx_status;
 
 	if (in_panic)
 		return __mhi_download_rddm_in_panic(mhi_cntrl);
 
+	dev_dbg(dev, "Waiting for RDDM image download via BHIe\n");
+
 	/* Wait for the image download to complete */
 	wait_event_timeout(mhi_cntrl->state_event,
 			   mhi_read_reg_field(mhi_cntrl, base,
@@ -174,6 +178,7 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
 			    const struct mhi_buf *mhi_buf)
 {
 	void __iomem *base = mhi_cntrl->bhie;
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
 	rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
 	u32 tx_status, sequence_id;
 	int ret;
@@ -184,6 +189,7 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
 		return -EIO;
 	}
 
+	dev_dbg(dev, "Starting AMSS download via BHIe\n");
 	mhi_write_reg(mhi_cntrl, base, BHIE_TXVECADDR_HIGH_OFFS,
 		      upper_32_bits(mhi_buf->dma_addr));
 
@@ -441,7 +447,12 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 		release_firmware(firmware);
 
 	/* Error or in EDL mode, we're done */
-	if (ret || mhi_cntrl->ee == MHI_EE_EDL)
+	if (ret) {
+		dev_err(dev, "MHI did not load SBL, ret:%d\n", ret);
+		return;
+	}
+
+	if (mhi_cntrl->ee == MHI_EE_EDL)
 		return;
 
 	write_lock_irq(&mhi_cntrl->pm_lock);
@@ -469,8 +480,10 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	if (!mhi_cntrl->fbc_download)
 		return;
 
-	if (ret)
+	if (ret) {
+		dev_err(dev, "MHI did not enter READY state\n");
 		goto error_read;
+	}
 
 	/* Wait for the SBL event */
 	ret = wait_event_timeout(mhi_cntrl->state_event,
@@ -488,6 +501,8 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	ret = mhi_fw_load_amss(mhi_cntrl,
 			       /* Vector table is the last entry */
 			       &image_info->mhi_buf[image_info->entries - 1]);
+	if (ret)
+		dev_err(dev, "MHI did not load AMSS, ret:%d\n", ret);
 
 	release_firmware(firmware);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 8/8] bus: mhi: core: Ensure non-zero session or sequence ID values
  2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
                   ` (6 preceding siblings ...)
  2020-04-28  2:59 ` [PATCH v2 7/8] bus: mhi: core: Improve debug logs for loading firmware Bhaumik Bhatt
@ 2020-04-28  2:59 ` Bhaumik Bhatt
  7 siblings, 0 replies; 16+ messages in thread
From: Bhaumik Bhatt @ 2020-04-28  2:59 UTC (permalink / raw)
  To: mani; +Cc: linux-arm-msm, linux-kernel, hemantk, Bhaumik Bhatt

While writing any sequence or session identifiers, it is possible that
the host could write a zero value, whereas only non-zero values are
supported writes to those registers. Ensure that host does not write a
non-zero value for those cases.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 0bc9c50..c9971d4 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -199,6 +199,9 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
 	mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS, mhi_buf->len);
 
 	sequence_id = prandom_u32() & BHIE_TXVECSTATUS_SEQNUM_BMSK;
+	if (unlikely(!sequence_id))
+		sequence_id = 1;
+
 	mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS,
 			    BHIE_TXVECDB_SEQNUM_BMSK, BHIE_TXVECDB_SEQNUM_SHFT,
 			    sequence_id);
@@ -254,6 +257,9 @@ static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
 		      lower_32_bits(dma_addr));
 	mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, size);
 	session_id = prandom_u32() & BHI_TXDB_SEQNUM_BMSK;
+	if (unlikely(!session_id))
+		session_id = 1;
+
 	mhi_write_reg(mhi_cntrl, base, BHI_IMGTXDB, session_id);
 	read_unlock_bh(pm_lock);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 4/8] bus: mhi: core: Handle firmware load using state worker
  2020-04-28  2:59 ` [PATCH v2 4/8] bus: mhi: core: Handle firmware load using state worker Bhaumik Bhatt
@ 2020-04-28 14:27   ` Jeffrey Hugo
  0 siblings, 0 replies; 16+ messages in thread
From: Jeffrey Hugo @ 2020-04-28 14:27 UTC (permalink / raw)
  To: Bhaumik Bhatt, mani; +Cc: linux-arm-msm, linux-kernel, hemantk

Err, you fixed the SOB on patches 1-3, but now broke 4+.

On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:

No "From: ..." means you are the author.

> Upon power up, driver queues firmware worker thread if the execution
> environment is PBL. Firmware worker is blocked with a timeout until
> state worker gets a chance to run and unblock firmware worker. An
> endpoint power up failure can be seen if state worker gets a chance to
> run after firmware worker has timed out. Remove this dependency and
> handle firmware load directly using state worker thread.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

SOB from you meets the developers certificate of origin.

> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>

SOB from Hemant here, is confusing.  This would normally signal that 
Hemant is sending a patch you authored (ie patches 1-3 are authored by 
Hemant but you posted them to list), and he is attesting that he has the 
right to do so (certificate of origin), but this email came from you. 
This doesn't add up.

If you are trying to imply that Hemant wrote this change with you, there 
is a Co-authored-by tag you can use (goes before the SOB) to indicate 
multiple authors since git only has a single author field.  The 
Documentation/process/submitting-patches I pointed out on v1 should have 
the details for this.

Otherwise, you are listed as the author, and the patch has your SOB, 
therefore nothing else is needed.

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel
  2020-04-28  2:59 ` [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel Bhaumik Bhatt
@ 2020-04-28 14:39   ` Jeffrey Hugo
  2020-04-29 17:27     ` Hemant Kumar
  0 siblings, 1 reply; 16+ messages in thread
From: Jeffrey Hugo @ 2020-04-28 14:39 UTC (permalink / raw)
  To: Bhaumik Bhatt, mani; +Cc: linux-arm-msm, linux-kernel, hemantk

On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
> From: Hemant Kumar <hemantk@codeaurora.org>
> 
> Driver is using zero initialized intmod value from mhi channel when
> configuring TRE for bei field. This prevents interrupt moderation to
> take effect in case it is supported by an event ring. Fix this by
> copying intmod value from associated event ring to mhi channel upon
> registering mhi controller.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>   drivers/bus/mhi/core/init.c | 4 ++++
>   drivers/bus/mhi/core/main.c | 9 ++++++---
>   2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index b38359c..4dc7f22 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -864,6 +864,10 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>   		mutex_init(&mhi_chan->mutex);
>   		init_completion(&mhi_chan->completion);
>   		rwlock_init(&mhi_chan->lock);
> +
> +		/* used in setting bei field of TRE */
> +		mhi_event = &mhi_cntrl->mhi_event[mhi_chan->er_index];
> +		mhi_chan->intmod = mhi_event->intmod;
>   	}
>   
>   	if (mhi_cntrl->bounce_buf) {
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index eb4256b..23154f1 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -929,7 +929,7 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>   	struct mhi_ring *buf_ring = &mhi_chan->buf_ring;
>   	struct mhi_buf_info *buf_info;
>   	struct mhi_tre *mhi_tre;
> -	int ret;
> +	int ret, bei;
>   
>   	/* If MHI host pre-allocates buffers then client drivers cannot queue */
>   	if (mhi_chan->pre_alloc)
> @@ -966,10 +966,11 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>   		goto map_error;
>   
>   	mhi_tre = tre_ring->wp;
> +	bei = !!(mhi_chan->intmod);
>   
>   	mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr);
>   	mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(buf_info->len);
> -	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(1, 1, 0, 0);
> +	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, 1, 0, 0);

Why are we not using mhi_gen_tre() here?  Its used in mhi_queue_buf(), 
which seems to be why the issue doesn't appear there.  It looks like we 
have 3 instances of the same code, which all need to be kept in sync. 
Seems like this bug exists because they were not in sync.

Simplifying the code by removing the duplication seems to be the better 
approach by not only addressing this issue, but also preventing future ones.

>   
>   	/* increment WP */
>   	mhi_add_ring_element(mhi_cntrl, tre_ring);
> @@ -1006,6 +1007,7 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>   	struct mhi_ring *buf_ring = &mhi_chan->buf_ring;
>   	struct mhi_buf_info *buf_info;
>   	struct mhi_tre *mhi_tre;
> +	int bei;
>   
>   	/* If MHI host pre-allocates buffers then client drivers cannot queue */
>   	if (mhi_chan->pre_alloc)
> @@ -1043,10 +1045,11 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>   	buf_info->len = len;
>   
>   	mhi_tre = tre_ring->wp;
> +	bei = !!(mhi_chan->intmod);
>   
>   	mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr);
>   	mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(buf_info->len);
> -	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(1, 1, 0, 0);
> +	mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, 1, 0, 0);
>   
>   	/* increment WP */
>   	mhi_add_ring_element(mhi_cntrl, tre_ring);
> 


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/8] bus: mhi: core: Add range check for channel id received in event ring
  2020-04-28  2:59 ` [PATCH v2 2/8] bus: mhi: core: Add range check for channel id received in event ring Bhaumik Bhatt
@ 2020-04-28 14:44   ` Jeffrey Hugo
  2020-04-29 17:29     ` Hemant Kumar
  0 siblings, 1 reply; 16+ messages in thread
From: Jeffrey Hugo @ 2020-04-28 14:44 UTC (permalink / raw)
  To: Bhaumik Bhatt, mani; +Cc: linux-arm-msm, linux-kernel, hemantk

On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
> From: Hemant Kumar <hemantk@codeaurora.org>
> 
> MHI data completion handler function reads channel id from event
> ring element. Value is under the control of MHI devices and can be
> any value between 0 and 255. In order to prevent out of bound access
> add a bound check against the max channel supported by controller
> and skip processing of that event ring element.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>   drivers/bus/mhi/core/main.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 23154f1..1ccd4cc 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -827,6 +827,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
>   		enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp);
>   
>   		chan = MHI_TRE_GET_EV_CHID(local_rp);
> +		if (WARN_ON(chan >= mhi_cntrl->max_chan))
> +			goto next_event;
> +
>   		mhi_chan = &mhi_cntrl->mhi_chan[chan];
>   
>   		if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
> @@ -837,6 +840,7 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
>   			event_quota--;
>   		}
>   
> +next_event:
>   		mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
>   		local_rp = ev_ring->rp;
>   		dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp);
> 

It looks like the same issue exists in mhi_process_ctrl_ev_ring(), and 
thus some form of this solution needs to be applied there as well. 
Would you please fix that too?

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 3/8] bus: mhi: core: Read transfer length from an event properly
  2020-04-28  2:59 ` [PATCH v2 3/8] bus: mhi: core: Read transfer length from an event properly Bhaumik Bhatt
@ 2020-04-28 14:50   ` Jeffrey Hugo
  2020-04-29 17:30     ` Hemant Kumar
  0 siblings, 1 reply; 16+ messages in thread
From: Jeffrey Hugo @ 2020-04-28 14:50 UTC (permalink / raw)
  To: Bhaumik Bhatt, mani; +Cc: linux-arm-msm, linux-kernel, hemantk

On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
> From: Hemant Kumar <hemantk@codeaurora.org>
> 
> When MHI Driver receives an EOT event, it reads xfer_len from the
> event in the last TRE. The value is under control of the MHI device
> and never validated by Host MHI driver. The value should never be
> larger than the real size of the buffer but a malicious device can
> set the value 0xFFFF as maximum. This causes device to memory

The device will overflow, or the driver?

> overflow (both read or write). Fix this issue by reading minimum of
> transfer length from event and the buffer length provided.
> 
> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>   drivers/bus/mhi/core/main.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 1ccd4cc..3d468d9 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -521,7 +521,10 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
>   				mhi_cntrl->unmap_single(mhi_cntrl, buf_info);
>   
>   			result.buf_addr = buf_info->cb_buf;
> -			result.bytes_xferd = xfer_len;
> +
> +			/* truncate to buf len if xfer_len is larger */
> +			result.bytes_xferd =
> +				min_t(u16, xfer_len, buf_info->len);
>   			mhi_del_ring_element(mhi_cntrl, buf_ring);
>   			mhi_del_ring_element(mhi_cntrl, tre_ring);
>   			local_rp = tre_ring->rp;
> 


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel
  2020-04-28 14:39   ` Jeffrey Hugo
@ 2020-04-29 17:27     ` Hemant Kumar
  0 siblings, 0 replies; 16+ messages in thread
From: Hemant Kumar @ 2020-04-29 17:27 UTC (permalink / raw)
  To: Jeffrey Hugo, Bhaumik Bhatt, mani; +Cc: linux-arm-msm, linux-kernel

Hi Jeff

On 4/28/20 7:39 AM, Jeffrey Hugo wrote:
> On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
>> From: Hemant Kumar <hemantk@codeaurora.org>
>>
>> Driver is using zero initialized intmod value from mhi channel when
>> configuring TRE for bei field. This prevents interrupt moderation to
>> take effect in case it is supported by an event ring. Fix this by
>> copying intmod value from associated event ring to mhi channel upon
>> registering mhi controller.
>>
>> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/init.c | 4 ++++
>>   drivers/bus/mhi/core/main.c | 9 ++++++---
>>   2 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index b38359c..4dc7f22 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -864,6 +864,10 @@ int mhi_register_controller(struct mhi_controller 
>> *mhi_cntrl,
>>           mutex_init(&mhi_chan->mutex);
>>           init_completion(&mhi_chan->completion);
>>           rwlock_init(&mhi_chan->lock);
>> +
>> +        /* used in setting bei field of TRE */
>> +        mhi_event = &mhi_cntrl->mhi_event[mhi_chan->er_index];
>> +        mhi_chan->intmod = mhi_event->intmod;
>>       }
>>       if (mhi_cntrl->bounce_buf) {
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index eb4256b..23154f1 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -929,7 +929,7 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum 
>> dma_data_direction dir,
>>       struct mhi_ring *buf_ring = &mhi_chan->buf_ring;
>>       struct mhi_buf_info *buf_info;
>>       struct mhi_tre *mhi_tre;
>> -    int ret;
>> +    int ret, bei;
>>       /* If MHI host pre-allocates buffers then client drivers cannot 
>> queue */
>>       if (mhi_chan->pre_alloc)
>> @@ -966,10 +966,11 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, 
>> enum dma_data_direction dir,
>>           goto map_error;
>>       mhi_tre = tre_ring->wp;
>> +    bei = !!(mhi_chan->intmod);
>>       mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr);
>>       mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(buf_info->len);
>> -    mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(1, 1, 0, 0);
>> +    mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, 1, 0, 0);
> 
> Why are we not using mhi_gen_tre() here?  Its used in mhi_queue_buf(), 
> which seems to be why the issue doesn't appear there.  It looks like we 
> have 3 instances of the same code, which all need to be kept in sync. 
> Seems like this bug exists because they were not in sync.
> 
> Simplifying the code by removing the duplication seems to be the better 
> approach by not only addressing this issue, but also preventing future 
> ones.
> 
Agree, i came up with a  re-factor change to have all queue APIs calling
mhi_gen_tre(). Will include as part of V3.
>>       /* increment WP */
>>       mhi_add_ring_element(mhi_cntrl, tre_ring);
>> @@ -1006,6 +1007,7 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, 
>> enum dma_data_direction dir,
>>       struct mhi_ring *buf_ring = &mhi_chan->buf_ring;
>>       struct mhi_buf_info *buf_info;
>>       struct mhi_tre *mhi_tre;
>> +    int bei;
>>       /* If MHI host pre-allocates buffers then client drivers cannot 
>> queue */
>>       if (mhi_chan->pre_alloc)
>> @@ -1043,10 +1045,11 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, 
>> enum dma_data_direction dir,
>>       buf_info->len = len;
>>       mhi_tre = tre_ring->wp;
>> +    bei = !!(mhi_chan->intmod);
>>       mhi_tre->ptr = MHI_TRE_DATA_PTR(buf_info->p_addr);
>>       mhi_tre->dword[0] = MHI_TRE_DATA_DWORD0(buf_info->len);
>> -    mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(1, 1, 0, 0);
>> +    mhi_tre->dword[1] = MHI_TRE_DATA_DWORD1(bei, 1, 0, 0);
>>       /* increment WP */
>>       mhi_add_ring_element(mhi_cntrl, tre_ring);
>>
> 
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/8] bus: mhi: core: Add range check for channel id received in event ring
  2020-04-28 14:44   ` Jeffrey Hugo
@ 2020-04-29 17:29     ` Hemant Kumar
  0 siblings, 0 replies; 16+ messages in thread
From: Hemant Kumar @ 2020-04-29 17:29 UTC (permalink / raw)
  To: Jeffrey Hugo, Bhaumik Bhatt, mani; +Cc: linux-arm-msm, linux-kernel

Hi Jeff

On 4/28/20 7:44 AM, Jeffrey Hugo wrote:
> On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
>> From: Hemant Kumar <hemantk@codeaurora.org>
>>
>> MHI data completion handler function reads channel id from event
>> ring element. Value is under the control of MHI devices and can be
>> any value between 0 and 255. In order to prevent out of bound access
>> add a bound check against the max channel supported by controller
>> and skip processing of that event ring element.
>>
>> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/main.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 23154f1..1ccd4cc 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -827,6 +827,9 @@ int mhi_process_data_event_ring(struct 
>> mhi_controller *mhi_cntrl,
>>           enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp);
>>           chan = MHI_TRE_GET_EV_CHID(local_rp);
>> +        if (WARN_ON(chan >= mhi_cntrl->max_chan))
>> +            goto next_event;
>> +
>>           mhi_chan = &mhi_cntrl->mhi_chan[chan];
>>           if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
>> @@ -837,6 +840,7 @@ int mhi_process_data_event_ring(struct 
>> mhi_controller *mhi_cntrl,
>>               event_quota--;
>>           }
>> +next_event:
>>           mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
>>           local_rp = ev_ring->rp;
>>           dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp);
>>
> 
> It looks like the same issue exists in mhi_process_ctrl_ev_ring(), and 
> thus some form of this solution needs to be applied there as well. Would 
> you please fix that too?
> 
As discussed with you off line, spec allows to have just event ring to 
be used for both data and control. Updating this in V3.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 3/8] bus: mhi: core: Read transfer length from an event properly
  2020-04-28 14:50   ` Jeffrey Hugo
@ 2020-04-29 17:30     ` Hemant Kumar
  0 siblings, 0 replies; 16+ messages in thread
From: Hemant Kumar @ 2020-04-29 17:30 UTC (permalink / raw)
  To: Jeffrey Hugo, Bhaumik Bhatt, mani; +Cc: linux-arm-msm, linux-kernel

Hi Jeff

On 4/28/20 7:50 AM, Jeffrey Hugo wrote:
> On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
>> From: Hemant Kumar <hemantk@codeaurora.org>
>>
>> When MHI Driver receives an EOT event, it reads xfer_len from the
>> event in the last TRE. The value is under control of the MHI device
>> and never validated by Host MHI driver. The value should never be
>> larger than the real size of the buffer but a malicious device can
>> set the value 0xFFFF as maximum. This causes device to memory
> 
> The device will overflow, or the driver?
Done.
> 
>> overflow (both read or write). Fix this issue by reading minimum of
>> transfer length from event and the buffer length provided.
>>
>> Signed-off-by: Hemant Kumar <hemantk@codeaurora.org>
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/main.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 1ccd4cc..3d468d9 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -521,7 +521,10 @@ static int parse_xfer_event(struct mhi_controller 
>> *mhi_cntrl,
>>                   mhi_cntrl->unmap_single(mhi_cntrl, buf_info);
>>               result.buf_addr = buf_info->cb_buf;
>> -            result.bytes_xferd = xfer_len;
>> +
>> +            /* truncate to buf len if xfer_len is larger */
>> +            result.bytes_xferd =
>> +                min_t(u16, xfer_len, buf_info->len);
>>               mhi_del_ring_element(mhi_cntrl, buf_ring);
>>               mhi_del_ring_element(mhi_cntrl, tre_ring);
>>               local_rp = tre_ring->rp;
>>
> 
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-04-29 17:30 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28  2:59 [RESEND PATCH v2 0/8] Bug fixes and improved logging in MHI Bhaumik Bhatt
2020-04-28  2:59 ` [PATCH v2 1/8] bus: mhi: core: Cache intmod from mhi event to mhi channel Bhaumik Bhatt
2020-04-28 14:39   ` Jeffrey Hugo
2020-04-29 17:27     ` Hemant Kumar
2020-04-28  2:59 ` [PATCH v2 2/8] bus: mhi: core: Add range check for channel id received in event ring Bhaumik Bhatt
2020-04-28 14:44   ` Jeffrey Hugo
2020-04-29 17:29     ` Hemant Kumar
2020-04-28  2:59 ` [PATCH v2 3/8] bus: mhi: core: Read transfer length from an event properly Bhaumik Bhatt
2020-04-28 14:50   ` Jeffrey Hugo
2020-04-29 17:30     ` Hemant Kumar
2020-04-28  2:59 ` [PATCH v2 4/8] bus: mhi: core: Handle firmware load using state worker Bhaumik Bhatt
2020-04-28 14:27   ` Jeffrey Hugo
2020-04-28  2:59 ` [PATCH v2 5/8] bus: mhi: core: WARN_ON for malformed vector table Bhaumik Bhatt
2020-04-28  2:59 ` [PATCH v2 6/8] bus: mhi: core: Return appropriate error codes for AMSS load failure Bhaumik Bhatt
2020-04-28  2:59 ` [PATCH v2 7/8] bus: mhi: core: Improve debug logs for loading firmware Bhaumik Bhatt
2020-04-28  2:59 ` [PATCH v2 8/8] bus: mhi: core: Ensure non-zero session or sequence ID values Bhaumik Bhatt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).