A set of patches for bug fixes and improved logging in mhi/core/boot.c.
Verified on x86 and arm64 platforms.
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 | 15 ++++++---
drivers/bus/mhi/core/pm.c | 6 +---
include/linux/mhi.h | 2 --
6 files changed, 62 insertions(+), 41 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
---
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
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 <[email protected]>
---
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
From: Hemant Kumar <[email protected]>
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 <[email protected]>
---
drivers/bus/mhi/core/main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 23154f1..ba8afa7 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))
+ continue;
+
mhi_chan = &mhi_cntrl->mhi_chan[chan];
if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project