As mhi_poll function can be called by mhi client drivers
which will call process_event, which will ring DB even if
there no ring elements to process. This could cause
doorbell event to be processed by MHI device to check for
any ring elements even it is none and also it will occupy
lot of bandwidth on peripheral when mhi_poll() is called in
aggressive loop.
Signed-off-by: Vivek Pernamitta <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
---
changes since v4:
updating the commit text with more information.
changes since v3:
- Updating commit text for multiple versions of patches.
changes since v2:
- Updated comments in code.
changes since v1:
- Add an check to avoid ringing EV DB in mhi_process_ctrl_ev_ring().
---
drivers/bus/mhi/host/main.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index df0fbfe..1bbdb75 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -961,7 +961,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
}
read_lock_bh(&mhi_cntrl->pm_lock);
- if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))
+
+ /* Ring EV DB only if there is any pending element to process */
+ if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)) && count)
mhi_ring_er_db(mhi_event);
read_unlock_bh(&mhi_cntrl->pm_lock);
@@ -1031,7 +1033,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
count++;
}
read_lock_bh(&mhi_cntrl->pm_lock);
- if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))
+
+ /* Ring EV DB only if there is any pending element to process */
+ if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)) && count)
mhi_ring_er_db(mhi_event);
read_unlock_bh(&mhi_cntrl->pm_lock);
--
2.7.4
On Wed, Feb 22, 2023 at 11:07:48PM +0530, Vivek Pernamitta wrote:
> As mhi_poll function can be called by mhi client drivers
> which will call process_event, which will ring DB even if
> there no ring elements to process. This could cause
> doorbell event to be processed by MHI device to check for
> any ring elements even it is none and also it will occupy
> lot of bandwidth on peripheral when mhi_poll() is called in
> aggressive loop.
>
The change looks good to me but who is the actual in-kernel user of
mhi_poll() API? It is being exported and if there is no upstream client
driver making use of it, then it shouldn't be.
I'm gonna submit a patch to remove this API altogether.
> Signed-off-by: Vivek Pernamitta <[email protected]>
> Reviewed-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Thanks,
Mani
>
> ---
> changes since v4:
> updating the commit text with more information.
> changes since v3:
> - Updating commit text for multiple versions of patches.
> changes since v2:
> - Updated comments in code.
> changes since v1:
> - Add an check to avoid ringing EV DB in mhi_process_ctrl_ev_ring().
> ---
> drivers/bus/mhi/host/main.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index df0fbfe..1bbdb75 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -961,7 +961,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
> }
>
> read_lock_bh(&mhi_cntrl->pm_lock);
> - if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))
> +
> + /* Ring EV DB only if there is any pending element to process */
> + if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)) && count)
> mhi_ring_er_db(mhi_event);
> read_unlock_bh(&mhi_cntrl->pm_lock);
>
> @@ -1031,7 +1033,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
> count++;
> }
> read_lock_bh(&mhi_cntrl->pm_lock);
> - if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))
> +
> + /* Ring EV DB only if there is any pending element to process */
> + if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)) && count)
> mhi_ring_er_db(mhi_event);
> read_unlock_bh(&mhi_cntrl->pm_lock);
>
> --
> 2.7.4
>
On Thu, Feb 23, 2023 at 09:18:36PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Feb 22, 2023 at 11:07:48PM +0530, Vivek Pernamitta wrote:
> > As mhi_poll function can be called by mhi client drivers
> > which will call process_event, which will ring DB even if
> > there no ring elements to process. This could cause
> > doorbell event to be processed by MHI device to check for
> > any ring elements even it is none and also it will occupy
> > lot of bandwidth on peripheral when mhi_poll() is called in
> > aggressive loop.
> >
>
> The change looks good to me but who is the actual in-kernel user of
> mhi_poll() API? It is being exported and if there is no upstream client
> driver making use of it, then it shouldn't be.
>
> I'm gonna submit a patch to remove this API altogether.
>
The patch removing mhi_poll() API is now merged. Can you please respin this
patch removing the note to the API?
I still think this patch is relevant without that API.
Thanks,
Mani
> > Signed-off-by: Vivek Pernamitta <[email protected]>
> > Reviewed-by: Jeffrey Hugo <[email protected]>
>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>
>
> Thanks,
> Mani
>
> >
> > ---
> > changes since v4:
> > updating the commit text with more information.
> > changes since v3:
> > - Updating commit text for multiple versions of patches.
> > changes since v2:
> > - Updated comments in code.
> > changes since v1:
> > - Add an check to avoid ringing EV DB in mhi_process_ctrl_ev_ring().
> > ---
> > drivers/bus/mhi/host/main.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> > index df0fbfe..1bbdb75 100644
> > --- a/drivers/bus/mhi/host/main.c
> > +++ b/drivers/bus/mhi/host/main.c
> > @@ -961,7 +961,9 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
> > }
> >
> > read_lock_bh(&mhi_cntrl->pm_lock);
> > - if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))
> > +
> > + /* Ring EV DB only if there is any pending element to process */
> > + if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)) && count)
> > mhi_ring_er_db(mhi_event);
> > read_unlock_bh(&mhi_cntrl->pm_lock);
> >
> > @@ -1031,7 +1033,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
> > count++;
> > }
> > read_lock_bh(&mhi_cntrl->pm_lock);
> > - if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)))
> > +
> > + /* Ring EV DB only if there is any pending element to process */
> > + if (likely(MHI_DB_ACCESS_VALID(mhi_cntrl)) && count)
> > mhi_ring_er_db(mhi_event);
> > read_unlock_bh(&mhi_cntrl->pm_lock);
> >
> > --
> > 2.7.4
> >
>
--
மணிவண்ணன் சதாசிவம்