| Message ID | 1674597444-24543-3-git-send-email-quic_jhugo@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MHI host syserr fixes | expand |
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 <quic_jhugo@quicinc.com> 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 <mani@kernel.org> - Mani > Reviewed-by: Carl Vanderlip <quic_carlv@quicinc.com> > --- > 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 >
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;