* [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior @ 2021-03-05 17:02 Loic Poulain 2021-03-05 17:02 ` [PATCH 2/2] bus: mhi: pm: reduce PM state change verbosity Loic Poulain 2021-03-31 17:12 ` [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior Manivannan Sadhasivam 0 siblings, 2 replies; 8+ messages in thread From: Loic Poulain @ 2021-03-05 17:02 UTC (permalink / raw) To: manivannan.sadhasivam, hemantk; +Cc: linux-arm-msm, Loic Poulain This change ensures that PM reference is always get during packet queueing and released either after queuing completion (RX) or once the buffer has been consumed (TX). This guarantees proper update for underlying MHI controller runtime status (e.g. last_busy timestamp) and prevents suspend to be triggered while TX packets are flying, or before we completed update of the RX ring. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/bus/mhi/core/main.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index c780234..16b9640 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -584,8 +584,11 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, /* notify client */ mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); - if (mhi_chan->dir == DMA_TO_DEVICE) + if (mhi_chan->dir == DMA_TO_DEVICE) { atomic_dec(&mhi_cntrl->pending_pkts); + /* Release the reference got from mhi_queue() */ + mhi_cntrl->runtime_put(mhi_cntrl); + } /* * Recycle the buffer if buffer is pre-allocated, @@ -1021,9 +1024,11 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info, if (unlikely(ret)) goto exit_unlock; - /* trigger M3 exit if necessary */ - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) - mhi_trigger_resume(mhi_cntrl); + /* Packet is queued, take a usage ref to exit M3 if necessary + * for host->device buffer, balanced put is done on buffer completion + * for device->host buffer, balanced put is after ringing the DB + */ + mhi_cntrl->runtime_get(mhi_cntrl); /* Assert dev_wake (to exit/prevent M1/M2)*/ mhi_cntrl->wake_toggle(mhi_cntrl); @@ -1034,6 +1039,9 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info, if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) mhi_ring_chan_db(mhi_cntrl, mhi_chan); + if (dir == DMA_FROM_DEVICE) + mhi_cntrl->runtime_put(mhi_cntrl); + exit_unlock: read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags); @@ -1431,6 +1439,10 @@ static void mhi_reset_data_chan(struct mhi_controller *mhi_cntrl, result.buf_addr = buf_info->cb_buf; mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); } + + /* Release the reference got from mhi_queue() */ + if (mhi_chan->dir == DMA_TO_DEVICE) + mhi_cntrl->runtime_put(mhi_cntrl); } } -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] bus: mhi: pm: reduce PM state change verbosity 2021-03-05 17:02 [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior Loic Poulain @ 2021-03-05 17:02 ` Loic Poulain 2021-04-05 18:51 ` Bhaumik Bhatt 2021-03-31 17:12 ` [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior Manivannan Sadhasivam 1 sibling, 1 reply; 8+ messages in thread From: Loic Poulain @ 2021-03-05 17:02 UTC (permalink / raw) To: manivannan.sadhasivam, hemantk; +Cc: linux-arm-msm, Loic Poulain Since M3 can be entered/exited quite a lot when used for runtime PM, keep the mhi suspend/resume transitions quiet. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/bus/mhi/core/pm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c index edd7794..0cd6445 100644 --- a/drivers/bus/mhi/core/pm.c +++ b/drivers/bus/mhi/core/pm.c @@ -858,7 +858,7 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl) return -EBUSY; } - dev_info(dev, "Allowing M3 transition\n"); + dev_dbg(dev, "Allowing M3 transition\n"); new_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_M3_ENTER); if (new_state != MHI_PM_M3_ENTER) { write_unlock_irq(&mhi_cntrl->pm_lock); @@ -872,7 +872,7 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl) /* Set MHI to M3 and wait for completion */ mhi_set_mhi_state(mhi_cntrl, MHI_STATE_M3); write_unlock_irq(&mhi_cntrl->pm_lock); - dev_info(dev, "Wait for M3 completion\n"); + dev_dbg(dev, "Wait for M3 completion\n"); ret = wait_event_timeout(mhi_cntrl->state_event, mhi_cntrl->dev_state == MHI_STATE_M3 || @@ -906,7 +906,7 @@ int mhi_pm_resume(struct mhi_controller *mhi_cntrl) enum mhi_pm_state cur_state; int ret; - dev_info(dev, "Entered with PM state: %s, MHI state: %s\n", + dev_dbg(dev, "Entered with PM state: %s, MHI state: %s\n", to_mhi_pm_state_str(mhi_cntrl->pm_state), TO_MHI_STATE_STR(mhi_cntrl->dev_state)); -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] bus: mhi: pm: reduce PM state change verbosity 2021-03-05 17:02 ` [PATCH 2/2] bus: mhi: pm: reduce PM state change verbosity Loic Poulain @ 2021-04-05 18:51 ` Bhaumik Bhatt 0 siblings, 0 replies; 8+ messages in thread From: Bhaumik Bhatt @ 2021-04-05 18:51 UTC (permalink / raw) To: Loic Poulain; +Cc: manivannan.sadhasivam, hemantk, linux-arm-msm On 2021-03-05 09:02 AM, Loic Poulain wrote: > Since M3 can be entered/exited quite a lot when used for runtime PM, > keep the mhi suspend/resume transitions quiet. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/bus/mhi/core/pm.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c > index edd7794..0cd6445 100644 > --- a/drivers/bus/mhi/core/pm.c > +++ b/drivers/bus/mhi/core/pm.c > @@ -858,7 +858,7 @@ int mhi_pm_suspend(struct mhi_controller > *mhi_cntrl) > return -EBUSY; > } > > - dev_info(dev, "Allowing M3 transition\n"); > + dev_dbg(dev, "Allowing M3 transition\n"); > new_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_M3_ENTER); > if (new_state != MHI_PM_M3_ENTER) { > write_unlock_irq(&mhi_cntrl->pm_lock); > @@ -872,7 +872,7 @@ int mhi_pm_suspend(struct mhi_controller > *mhi_cntrl) > /* Set MHI to M3 and wait for completion */ > mhi_set_mhi_state(mhi_cntrl, MHI_STATE_M3); > write_unlock_irq(&mhi_cntrl->pm_lock); > - dev_info(dev, "Wait for M3 completion\n"); > + dev_dbg(dev, "Wait for M3 completion\n"); > > ret = wait_event_timeout(mhi_cntrl->state_event, > mhi_cntrl->dev_state == MHI_STATE_M3 || > @@ -906,7 +906,7 @@ int mhi_pm_resume(struct mhi_controller *mhi_cntrl) > enum mhi_pm_state cur_state; > int ret; > > - dev_info(dev, "Entered with PM state: %s, MHI state: %s\n", > + dev_dbg(dev, "Entered with PM state: %s, MHI state: %s\n", > to_mhi_pm_state_str(mhi_cntrl->pm_state), > TO_MHI_STATE_STR(mhi_cntrl->dev_state)); Since we go from 4 chars (info) to 3 chars (dbg), there is need to adjust the two parameters here in the next two lines, right? If that's needed and is done, you can add my tag for: Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> We can just push this patch as a standalone one not dependent on the 1/2 in this series. Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior 2021-03-05 17:02 [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior Loic Poulain 2021-03-05 17:02 ` [PATCH 2/2] bus: mhi: pm: reduce PM state change verbosity Loic Poulain @ 2021-03-31 17:12 ` Manivannan Sadhasivam [not found] ` <CAMZdPi-M5fYPs7QfsBx4Gy6ywCLue5yqJLn0XthGhTeON1wWfw@mail.gmail.com> 1 sibling, 1 reply; 8+ messages in thread From: Manivannan Sadhasivam @ 2021-03-31 17:12 UTC (permalink / raw) To: Loic Poulain; +Cc: hemantk, linux-arm-msm On Fri, Mar 05, 2021 at 06:02:23PM +0100, Loic Poulain wrote: > This change ensures that PM reference is always get during packet > queueing and released either after queuing completion (RX) or once > the buffer has been consumed (TX). This guarantees proper update for > underlying MHI controller runtime status (e.g. last_busy timestamp) > and prevents suspend to be triggered while TX packets are flying, > or before we completed update of the RX ring. > Any reason why you didn't wait for RX completion also? Thanks, Mani > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/bus/mhi/core/main.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index c780234..16b9640 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -584,8 +584,11 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, > /* notify client */ > mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); > > - if (mhi_chan->dir == DMA_TO_DEVICE) > + if (mhi_chan->dir == DMA_TO_DEVICE) { > atomic_dec(&mhi_cntrl->pending_pkts); > + /* Release the reference got from mhi_queue() */ > + mhi_cntrl->runtime_put(mhi_cntrl); > + } > > /* > * Recycle the buffer if buffer is pre-allocated, > @@ -1021,9 +1024,11 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info, > if (unlikely(ret)) > goto exit_unlock; > > - /* trigger M3 exit if necessary */ > - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) > - mhi_trigger_resume(mhi_cntrl); > + /* Packet is queued, take a usage ref to exit M3 if necessary > + * for host->device buffer, balanced put is done on buffer completion > + * for device->host buffer, balanced put is after ringing the DB > + */ > + mhi_cntrl->runtime_get(mhi_cntrl); > > /* Assert dev_wake (to exit/prevent M1/M2)*/ > mhi_cntrl->wake_toggle(mhi_cntrl); > @@ -1034,6 +1039,9 @@ static int mhi_queue(struct mhi_device *mhi_dev, struct mhi_buf_info *buf_info, > if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) > mhi_ring_chan_db(mhi_cntrl, mhi_chan); > > + if (dir == DMA_FROM_DEVICE) > + mhi_cntrl->runtime_put(mhi_cntrl); > + > exit_unlock: > read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags); > > @@ -1431,6 +1439,10 @@ static void mhi_reset_data_chan(struct mhi_controller *mhi_cntrl, > result.buf_addr = buf_info->cb_buf; > mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); > } > + > + /* Release the reference got from mhi_queue() */ > + if (mhi_chan->dir == DMA_TO_DEVICE) > + mhi_cntrl->runtime_put(mhi_cntrl); > } > } > > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <CAMZdPi-M5fYPs7QfsBx4Gy6ywCLue5yqJLn0XthGhTeON1wWfw@mail.gmail.com>]
* Re: [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior [not found] ` <CAMZdPi-M5fYPs7QfsBx4Gy6ywCLue5yqJLn0XthGhTeON1wWfw@mail.gmail.com> @ 2021-03-31 18:27 ` Manivannan Sadhasivam 2021-04-05 18:46 ` Bhaumik Bhatt 0 siblings, 1 reply; 8+ messages in thread From: Manivannan Sadhasivam @ 2021-03-31 18:27 UTC (permalink / raw) To: Loic Poulain; +Cc: hemantk, linux-arm-msm On Wed, Mar 31, 2021 at 07:38:55PM +0200, Loic Poulain wrote: > Hi Mani, > > Le mer. 31 mars 2021 à 19:12, Manivannan Sadhasivam < > manivannan.sadhasivam@linaro.org> a écrit : > > > On Fri, Mar 05, 2021 at 06:02:23PM +0100, Loic Poulain wrote: > > > This change ensures that PM reference is always get during packet > > > queueing and released either after queuing completion (RX) or once > > > the buffer has been consumed (TX). This guarantees proper update for > > > underlying MHI controller runtime status (e.g. last_busy timestamp) > > > and prevents suspend to be triggered while TX packets are flying, > > > or before we completed update of the RX ring. > > > > > > > Any reason why you didn't wait for RX completion also? > > > Because on TX we know the buffer completion is going to happen really > quickly (we send data) whereas we never know when when RX packet will be > completed (we are waiting for data), so we want to be able to put the MHI > device in suspend while RX is pending (the device will wake up the host on > incoming data) > Device wakeup will only happen for device initiated suspend (M1) but for host initiated suspend (M3), device will check for pending data to host and will initiate wakeup request before going for suspend. So I think it is safe to wait for RX data. Hemant/Bhaumik, any thoughts? Thanks, Mani > > > > > Thanks, > > Mani > > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > > --- > > > drivers/bus/mhi/core/main.c | 20 ++++++++++++++++---- > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > > > index c780234..16b9640 100644 > > > --- a/drivers/bus/mhi/core/main.c > > > +++ b/drivers/bus/mhi/core/main.c > > > @@ -584,8 +584,11 @@ static int parse_xfer_event(struct mhi_controller > > *mhi_cntrl, > > > /* notify client */ > > > mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); > > > > > > - if (mhi_chan->dir == DMA_TO_DEVICE) > > > + if (mhi_chan->dir == DMA_TO_DEVICE) { > > > atomic_dec(&mhi_cntrl->pending_pkts); > > > + /* Release the reference got from > > mhi_queue() */ > > > + mhi_cntrl->runtime_put(mhi_cntrl); > > > + } > > > > > > /* > > > * Recycle the buffer if buffer is pre-allocated, > > > @@ -1021,9 +1024,11 @@ static int mhi_queue(struct mhi_device *mhi_dev, > > struct mhi_buf_info *buf_info, > > > if (unlikely(ret)) > > > goto exit_unlock; > > > > > > - /* trigger M3 exit if necessary */ > > > - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) > > > - mhi_trigger_resume(mhi_cntrl); > > > + /* Packet is queued, take a usage ref to exit M3 if necessary > > > + * for host->device buffer, balanced put is done on buffer > > completion > > > + * for device->host buffer, balanced put is after ringing the DB > > > + */ > > > + mhi_cntrl->runtime_get(mhi_cntrl); > > > > > > /* Assert dev_wake (to exit/prevent M1/M2)*/ > > > mhi_cntrl->wake_toggle(mhi_cntrl); > > > @@ -1034,6 +1039,9 @@ static int mhi_queue(struct mhi_device *mhi_dev, > > struct mhi_buf_info *buf_info, > > > if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) > > > mhi_ring_chan_db(mhi_cntrl, mhi_chan); > > > > > > + if (dir == DMA_FROM_DEVICE) > > > + mhi_cntrl->runtime_put(mhi_cntrl); > > > + > > > exit_unlock: > > > read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags); > > > > > > @@ -1431,6 +1439,10 @@ static void mhi_reset_data_chan(struct > > mhi_controller *mhi_cntrl, > > > result.buf_addr = buf_info->cb_buf; > > > mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); > > > } > > > + > > > + /* Release the reference got from mhi_queue() */ > > > + if (mhi_chan->dir == DMA_TO_DEVICE) > > > + mhi_cntrl->runtime_put(mhi_cntrl); > > > } > > > } > > > > > > -- > > > 2.7.4 > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior 2021-03-31 18:27 ` Manivannan Sadhasivam @ 2021-04-05 18:46 ` Bhaumik Bhatt 2021-04-06 3:54 ` Hemant Kumar 0 siblings, 1 reply; 8+ messages in thread From: Bhaumik Bhatt @ 2021-04-05 18:46 UTC (permalink / raw) To: Manivannan Sadhasivam; +Cc: Loic Poulain, hemantk, linux-arm-msm On 2021-03-31 11:27 AM, Manivannan Sadhasivam wrote: > On Wed, Mar 31, 2021 at 07:38:55PM +0200, Loic Poulain wrote: >> Hi Mani, >> >> Le mer. 31 mars 2021 à 19:12, Manivannan Sadhasivam < >> manivannan.sadhasivam@linaro.org> a écrit : >> >> > On Fri, Mar 05, 2021 at 06:02:23PM +0100, Loic Poulain wrote: >> > > This change ensures that PM reference is always get during packet >> > > queueing and released either after queuing completion (RX) or once >> > > the buffer has been consumed (TX). This guarantees proper update for >> > > underlying MHI controller runtime status (e.g. last_busy timestamp) >> > > and prevents suspend to be triggered while TX packets are flying, >> > > or before we completed update of the RX ring. >> > > >> > >> > Any reason why you didn't wait for RX completion also? >> >> >> Because on TX we know the buffer completion is going to happen really >> quickly (we send data) whereas we never know when when RX packet will >> be >> completed (we are waiting for data), so we want to be able to put the >> MHI >> device in suspend while RX is pending (the device will wake up the >> host on >> incoming data) >> > > Device wakeup will only happen for device initiated suspend (M1) but > for > host initiated suspend (M3), device will check for pending data to host > and will initiate wakeup request before going for suspend. So I think > it > is safe to wait for RX data. > > Hemant/Bhaumik, any thoughts? > > Thanks, > Mani > Agree with Loic here. Let's not depend on the device to determine host side behavior and instead, assume that the device may or may not be following protocol so as to reduce chances of higher power draw by host. Host should not care when RX comes, but host should care about TX completion as that's where our requirement ends. There have been instances of delayed RX and in some cases, no TX completion from a certain client (I think DIAG), where device thinks they have received garbage and decide not to respond with a TX completion. We want to be able to put device in suspend or at least initiate it while host waits for incoming data. Once RX comes in, host will wake up to process it. What Loic does in this patch is done in one way using patch [1]. However, that does not update the last_busy timestamp. I am mostly in favor of this patch going in but would like Loic to answer one question: In mhi_reset_data_chan(), why not do a runtime_put(mhi_cntrl) inside the pre-existing if (mhi_chan->dir == DMA_TO_DEVICE) at the start of the while loop: while (tre_ring->rp != tre_ring->wp)? This would be balanced for each TX. >> >> > >> > Thanks, >> > Mani >> > >> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >> > > --- >> > > drivers/bus/mhi/core/main.c | 20 ++++++++++++++++---- >> > > 1 file changed, 16 insertions(+), 4 deletions(-) >> > > >> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c >> > > index c780234..16b9640 100644 >> > > --- a/drivers/bus/mhi/core/main.c >> > > +++ b/drivers/bus/mhi/core/main.c >> > > @@ -584,8 +584,11 @@ static int parse_xfer_event(struct mhi_controller >> > *mhi_cntrl, >> > > /* notify client */ >> > > mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); >> > > >> > > - if (mhi_chan->dir == DMA_TO_DEVICE) >> > > + if (mhi_chan->dir == DMA_TO_DEVICE) { >> > > atomic_dec(&mhi_cntrl->pending_pkts); >> > > + /* Release the reference got from >> > mhi_queue() */ >> > > + mhi_cntrl->runtime_put(mhi_cntrl); >> > > + } >> > > >> > > /* >> > > * Recycle the buffer if buffer is pre-allocated, >> > > @@ -1021,9 +1024,11 @@ static int mhi_queue(struct mhi_device *mhi_dev, >> > struct mhi_buf_info *buf_info, >> > > if (unlikely(ret)) >> > > goto exit_unlock; >> > > >> > > - /* trigger M3 exit if necessary */ >> > > - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) >> > > - mhi_trigger_resume(mhi_cntrl); >> > > + /* Packet is queued, take a usage ref to exit M3 if necessary >> > > + * for host->device buffer, balanced put is done on buffer >> > completion >> > > + * for device->host buffer, balanced put is after ringing the DB >> > > + */ >> > > + mhi_cntrl->runtime_get(mhi_cntrl); >> > > >> > > /* Assert dev_wake (to exit/prevent M1/M2)*/ >> > > mhi_cntrl->wake_toggle(mhi_cntrl); >> > > @@ -1034,6 +1039,9 @@ static int mhi_queue(struct mhi_device *mhi_dev, >> > struct mhi_buf_info *buf_info, >> > > if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) >> > > mhi_ring_chan_db(mhi_cntrl, mhi_chan); >> > > >> > > + if (dir == DMA_FROM_DEVICE) >> > > + mhi_cntrl->runtime_put(mhi_cntrl); >> > > + >> > > exit_unlock: >> > > read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags); >> > > >> > > @@ -1431,6 +1439,10 @@ static void mhi_reset_data_chan(struct >> > mhi_controller *mhi_cntrl, >> > > result.buf_addr = buf_info->cb_buf; >> > > mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); >> > > } >> > > + >> > > + /* Release the reference got from mhi_queue() */ >> > > + if (mhi_chan->dir == DMA_TO_DEVICE) >> > > + mhi_cntrl->runtime_put(mhi_cntrl); Can this runtime_put(mhi_cntrl); be moved to the if at the top of this while loop? >> > > } >> > > } >> > > >> > > -- >> > > 2.7.4 >> > > >> > Thanks, Bhaumik [1] https://lore.kernel.org/r/20200929175218.8178-4-manivannan.sadhasivam@linaro.org --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior 2021-04-05 18:46 ` Bhaumik Bhatt @ 2021-04-06 3:54 ` Hemant Kumar 2021-04-06 8:29 ` Loic Poulain 0 siblings, 1 reply; 8+ messages in thread From: Hemant Kumar @ 2021-04-06 3:54 UTC (permalink / raw) To: bbhatt, Manivannan Sadhasivam; +Cc: Loic Poulain, linux-arm-msm Hi Loic, On 4/5/21 11:46 AM, Bhaumik Bhatt wrote: > On 2021-03-31 11:27 AM, Manivannan Sadhasivam wrote: >> On Wed, Mar 31, 2021 at 07:38:55PM +0200, Loic Poulain wrote: >>> Hi Mani, >>> >>> Le mer. 31 mars 2021 à 19:12, Manivannan Sadhasivam < >>> manivannan.sadhasivam@linaro.org> a écrit : >>> >>> > On Fri, Mar 05, 2021 at 06:02:23PM +0100, Loic Poulain wrote: >>> > > This change ensures that PM reference is always get during packet >>> > > queueing and released either after queuing completion (RX) or once >>> > > the buffer has been consumed (TX). This guarantees proper update for >>> > > underlying MHI controller runtime status (e.g. last_busy timestamp) >>> > > and prevents suspend to be triggered while TX packets are flying, >>> > > or before we completed update of the RX ring. >>> > > >>> > >>> > Any reason why you didn't wait for RX completion also? >>> >>> >>> Because on TX we know the buffer completion is going to happen really >>> quickly (we send data) whereas we never know when when RX packet will be >>> completed (we are waiting for data), so we want to be able to put the >>> MHI >>> device in suspend while RX is pending (the device will wake up the >>> host on >>> incoming data) >>> >> >> Device wakeup will only happen for device initiated suspend (M1) but for >> host initiated suspend (M3), device will check for pending data to host >> and will initiate wakeup request before going for suspend. So I think it >> is safe to wait for RX data. >> >> Hemant/Bhaumik, any thoughts? >> >> Thanks, >> Mani >> > Agree with Loic here. Let's not depend on the device to determine host side > behavior and instead, assume that the device may or may not be following > protocol so as to reduce chances of higher power draw by host. Host should > not care when RX comes, but host should care about TX completion as that's > where our requirement ends. > > There have been instances of delayed RX and in some cases, no TX completion > from a certain client (I think DIAG), where device thinks they have > received > garbage and decide not to respond with a TX completion. > > We want to be able to put device in suspend or at least initiate it while > host waits for incoming data. Once RX comes in, host will wake up to > process it. Agree with Bhaumik and Loic about not waiting for Rx data. > > What Loic does in this patch is done in one way using patch [1]. > However, that > does not update the last_busy timestamp. I am mostly in favor of this patch > going in but would like Loic to answer one question: > > In mhi_reset_data_chan(), why not do a runtime_put(mhi_cntrl) inside the > pre-existing if (mhi_chan->dir == DMA_TO_DEVICE) at the start of the > while loop: > while (tre_ring->rp != tre_ring->wp)? This would be balanced for each TX. I got same question when i looked at the patch. >>> >>> > >>> > Thanks, >>> > Mani >>> > >>> > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >>> > > --- >>> > > drivers/bus/mhi/core/main.c | 20 ++++++++++++++++---- >>> > > 1 file changed, 16 insertions(+), 4 deletions(-) >>> > > >>> > > diff --git a/drivers/bus/mhi/core/main.c >>> b/drivers/bus/mhi/core/main.c >>> > > index c780234..16b9640 100644 >>> > > --- a/drivers/bus/mhi/core/main.c >>> > > +++ b/drivers/bus/mhi/core/main.c >>> > > @@ -584,8 +584,11 @@ static int parse_xfer_event(struct >>> mhi_controller >>> > *mhi_cntrl, >>> > > /* notify client */ >>> > > mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); >>> > > >>> > > - if (mhi_chan->dir == DMA_TO_DEVICE) >>> > > + if (mhi_chan->dir == DMA_TO_DEVICE) { >>> > > atomic_dec(&mhi_cntrl->pending_pkts); >>> > > + /* Release the reference got from >>> > mhi_queue() */ >>> > > + mhi_cntrl->runtime_put(mhi_cntrl); >>> > > + } >>> > > >>> > > /* >>> > > * Recycle the buffer if buffer is >>> pre-allocated, >>> > > @@ -1021,9 +1024,11 @@ static int mhi_queue(struct mhi_device >>> *mhi_dev, >>> > struct mhi_buf_info *buf_info, >>> > > if (unlikely(ret)) >>> > > goto exit_unlock; >>> > > >>> > > - /* trigger M3 exit if necessary */ >>> > > - if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) >>> > > - mhi_trigger_resume(mhi_cntrl); >>> > > + /* Packet is queued, take a usage ref to exit M3 if necessary >>> > > + * for host->device buffer, balanced put is done on buffer >>> > completion >>> > > + * for device->host buffer, balanced put is after ringing >>> the DB >>> > > + */ >>> > > + mhi_cntrl->runtime_get(mhi_cntrl); >>> > > >>> > > /* Assert dev_wake (to exit/prevent M1/M2)*/ >>> > > mhi_cntrl->wake_toggle(mhi_cntrl); >>> > > @@ -1034,6 +1039,9 @@ static int mhi_queue(struct mhi_device >>> *mhi_dev, >>> > struct mhi_buf_info *buf_info, >>> > > if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl))) >>> > > mhi_ring_chan_db(mhi_cntrl, mhi_chan); >>> > > >>> > > + if (dir == DMA_FROM_DEVICE) >>> > > + mhi_cntrl->runtime_put(mhi_cntrl); >>> > > + >>> > > exit_unlock: >>> > > read_unlock_irqrestore(&mhi_cntrl->pm_lock, flags); >>> > > >>> > > @@ -1431,6 +1439,10 @@ static void mhi_reset_data_chan(struct >>> > mhi_controller *mhi_cntrl, >>> > > result.buf_addr = buf_info->cb_buf; >>> > > mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result); >>> > > } >>> > > + >>> > > + /* Release the reference got from mhi_queue() */ >>> > > + if (mhi_chan->dir == DMA_TO_DEVICE) >>> > > + mhi_cntrl->runtime_put(mhi_cntrl); > Can this runtime_put(mhi_cntrl); be moved to the if at the top of this > while > loop? >>> > > } >>> > > } >>> > > >>> > > -- >>> > > 2.7.4 >>> > > >>> > > > Thanks, > Bhaumik > > [1] > https://lore.kernel.org/r/20200929175218.8178-4-manivannan.sadhasivam@linaro.org > > --- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project Thanks, Hemant -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior 2021-04-06 3:54 ` Hemant Kumar @ 2021-04-06 8:29 ` Loic Poulain 0 siblings, 0 replies; 8+ messages in thread From: Loic Poulain @ 2021-04-06 8:29 UTC (permalink / raw) To: Hemant Kumar; +Cc: Bhaumik Bhatt, Manivannan Sadhasivam, linux-arm-msm Hi Hemant, Bhaumik, On Tue, 6 Apr 2021 at 05:54, Hemant Kumar <hemantk@codeaurora.org> wrote: > > Hi Loic, > > On 4/5/21 11:46 AM, Bhaumik Bhatt wrote: > > On 2021-03-31 11:27 AM, Manivannan Sadhasivam wrote: > >> On Wed, Mar 31, 2021 at 07:38:55PM +0200, Loic Poulain wrote: > >>> Hi Mani, > >>> > >>> Le mer. 31 mars 2021 à 19:12, Manivannan Sadhasivam < > >>> manivannan.sadhasivam@linaro.org> a écrit : > >>> > >>> > On Fri, Mar 05, 2021 at 06:02:23PM +0100, Loic Poulain wrote: > >>> > > This change ensures that PM reference is always get during packet > >>> > > queueing and released either after queuing completion (RX) or once > >>> > > the buffer has been consumed (TX). This guarantees proper update for > >>> > > underlying MHI controller runtime status (e.g. last_busy timestamp) > >>> > > and prevents suspend to be triggered while TX packets are flying, > >>> > > or before we completed update of the RX ring. > >>> > > > >>> > > >>> > Any reason why you didn't wait for RX completion also? > >>> > >>> > >>> Because on TX we know the buffer completion is going to happen really > >>> quickly (we send data) whereas we never know when when RX packet will be > >>> completed (we are waiting for data), so we want to be able to put the > >>> MHI > >>> device in suspend while RX is pending (the device will wake up the > >>> host on > >>> incoming data) > >>> > >> > >> Device wakeup will only happen for device initiated suspend (M1) but for > >> host initiated suspend (M3), device will check for pending data to host > >> and will initiate wakeup request before going for suspend. So I think it > >> is safe to wait for RX data. > >> > >> Hemant/Bhaumik, any thoughts? > >> > >> Thanks, > >> Mani > >> > > Agree with Loic here. Let's not depend on the device to determine host side > > behavior and instead, assume that the device may or may not be following > > protocol so as to reduce chances of higher power draw by host. Host should > > not care when RX comes, but host should care about TX completion as that's > > where our requirement ends. > > > > There have been instances of delayed RX and in some cases, no TX completion > > from a certain client (I think DIAG), where device thinks they have > > received > > garbage and decide not to respond with a TX completion. > > > > We want to be able to put device in suspend or at least initiate it while > > host waits for incoming data. Once RX comes in, host will wake up to > > process it. > Agree with Bhaumik and Loic about not waiting for Rx data. > > > > What Loic does in this patch is done in one way using patch [1]. > > However, that > > does not update the last_busy timestamp. I am mostly in favor of this patch > > going in but would like Loic to answer one question: > > > > In mhi_reset_data_chan(), why not do a runtime_put(mhi_cntrl) inside the > > pre-existing if (mhi_chan->dir == DMA_TO_DEVICE) at the start of the > > while loop: > > while (tre_ring->rp != tre_ring->wp)? This would be balanced for each TX. > I got same question when i looked at the patch. Very true, I've not seen any issue, because there is 'usually' not TX pending on reset/remove but we indeed need to take care of balancing that here as well. I'm going to follow up with a new patch. Thanks, Loic ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-06 8:21 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 17:02 [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior Loic Poulain
2021-03-05 17:02 ` [PATCH 2/2] bus: mhi: pm: reduce PM state change verbosity Loic Poulain
2021-04-05 18:51 ` Bhaumik Bhatt
2021-03-31 17:12 ` [PATCH 1/2] bus: mhi: core: Fix MHI runtime_pm behavior Manivannan Sadhasivam
[not found] ` <CAMZdPi-M5fYPs7QfsBx4Gy6ywCLue5yqJLn0XthGhTeON1wWfw@mail.gmail.com>
2021-03-31 18:27 ` Manivannan Sadhasivam
2021-04-05 18:46 ` Bhaumik Bhatt
2021-04-06 3:54 ` Hemant Kumar
2021-04-06 8:29 ` Loic Poulain
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).