If firmware loading fails, the controller's pm_state is updated to
MHI_PM_FW_DL_ERR unconditionally. This can corrupt the pm_state as the
update is not done under the proper lock, and also does not validate
the state transition. The firmware loading can fail due to a detected
syserr, but if MHI_PM_FW_DL_ERR is unconditionally set as the pm_state,
the handling of the syserr can break when it attempts to transition from
syserr detect, to syserr process.
By grabbing the lock, we ensure we don't race with some other pm_state
update. By using mhi_try_set_pm_state(), we check that the transition
to MHI_PM_FW_DL_ERR is valid via the state machine logic. If it is not
valid, then some other transition is occurring like syserr processing, and
we assume that will resolve the firmware loading error.
Signed-off-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Carl Vanderlip <[email protected]>
---
drivers/bus/mhi/host/boot.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
index 1c69fee..d2a19b07 100644
--- a/drivers/bus/mhi/host/boot.c
+++ b/drivers/bus/mhi/host/boot.c
@@ -391,6 +391,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
{
const struct firmware *firmware = NULL;
struct device *dev = &mhi_cntrl->mhi_dev->dev;
+ enum mhi_pm_state new_state;
const char *fw_name;
void *buf;
dma_addr_t dma_addr;
@@ -508,14 +509,18 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
}
error_fw_load:
- mhi_cntrl->pm_state = MHI_PM_FW_DL_ERR;
- wake_up_all(&mhi_cntrl->state_event);
+ write_lock_irq(&mhi_cntrl->pm_lock);
+ new_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_FW_DL_ERR);
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+ if (new_state == MHI_PM_FW_DL_ERR)
+ wake_up_all(&mhi_cntrl->state_event);
}
int mhi_download_amss_image(struct mhi_controller *mhi_cntrl)
{
struct image_info *image_info = mhi_cntrl->fbc_image;
struct device *dev = &mhi_cntrl->mhi_dev->dev;
+ enum mhi_pm_state new_state;
int ret;
if (!image_info)
@@ -526,8 +531,11 @@ int mhi_download_amss_image(struct mhi_controller *mhi_cntrl)
&image_info->mhi_buf[image_info->entries - 1]);
if (ret) {
dev_err(dev, "MHI did not load AMSS, ret:%d\n", ret);
- mhi_cntrl->pm_state = MHI_PM_FW_DL_ERR;
- wake_up_all(&mhi_cntrl->state_event);
+ write_lock_irq(&mhi_cntrl->pm_lock);
+ new_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_FW_DL_ERR);
+ write_unlock_irq(&mhi_cntrl->pm_lock);
+ if (new_state == MHI_PM_FW_DL_ERR)
+ wake_up_all(&mhi_cntrl->state_event);
}
return ret;
--
2.7.4
On Tue, Jan 24, 2023 at 02:57:24PM -0700, Jeffrey Hugo wrote:
> If firmware loading fails, the controller's pm_state is updated to
> MHI_PM_FW_DL_ERR unconditionally. This can corrupt the pm_state as the
> update is not done under the proper lock, and also does not validate
> the state transition. The firmware loading can fail due to a detected
> syserr, but if MHI_PM_FW_DL_ERR is unconditionally set as the pm_state,
> the handling of the syserr can break when it attempts to transition from
> syserr detect, to syserr process.
>
> By grabbing the lock, we ensure we don't race with some other pm_state
> update. By using mhi_try_set_pm_state(), we check that the transition
> to MHI_PM_FW_DL_ERR is valid via the state machine logic. If it is not
> valid, then some other transition is occurring like syserr processing, and
> we assume that will resolve the firmware loading error.
>
> Signed-off-by: Jeffrey Hugo <[email protected]>
This looks like a legitimate fix. So please add the fixes tag and CC stable
for backporting (please add a hint on how far this patch has to be backported).
With that,
Reviewed-by: Manivannan Sadhasivam <[email protected]>
- Mani
> Reviewed-by: Carl Vanderlip <[email protected]>
> ---
> drivers/bus/mhi/host/boot.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bus/mhi/host/boot.c b/drivers/bus/mhi/host/boot.c
> index 1c69fee..d2a19b07 100644
> --- a/drivers/bus/mhi/host/boot.c
> +++ b/drivers/bus/mhi/host/boot.c
> @@ -391,6 +391,7 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
> {
> const struct firmware *firmware = NULL;
> struct device *dev = &mhi_cntrl->mhi_dev->dev;
> + enum mhi_pm_state new_state;
> const char *fw_name;
> void *buf;
> dma_addr_t dma_addr;
> @@ -508,14 +509,18 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
> }
>
> error_fw_load:
> - mhi_cntrl->pm_state = MHI_PM_FW_DL_ERR;
> - wake_up_all(&mhi_cntrl->state_event);
> + write_lock_irq(&mhi_cntrl->pm_lock);
> + new_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_FW_DL_ERR);
> + write_unlock_irq(&mhi_cntrl->pm_lock);
> + if (new_state == MHI_PM_FW_DL_ERR)
> + wake_up_all(&mhi_cntrl->state_event);
> }
>
> int mhi_download_amss_image(struct mhi_controller *mhi_cntrl)
> {
> struct image_info *image_info = mhi_cntrl->fbc_image;
> struct device *dev = &mhi_cntrl->mhi_dev->dev;
> + enum mhi_pm_state new_state;
> int ret;
>
> if (!image_info)
> @@ -526,8 +531,11 @@ int mhi_download_amss_image(struct mhi_controller *mhi_cntrl)
> &image_info->mhi_buf[image_info->entries - 1]);
> if (ret) {
> dev_err(dev, "MHI did not load AMSS, ret:%d\n", ret);
> - mhi_cntrl->pm_state = MHI_PM_FW_DL_ERR;
> - wake_up_all(&mhi_cntrl->state_event);
> + write_lock_irq(&mhi_cntrl->pm_lock);
> + new_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_FW_DL_ERR);
> + write_unlock_irq(&mhi_cntrl->pm_lock);
> + if (new_state == MHI_PM_FW_DL_ERR)
> + wake_up_all(&mhi_cntrl->state_event);
> }
>
> return ret;
> --
> 2.7.4
>
--
மணிவண்ணன் சதாசிவம்