Re: [PATCH v2 1/6] drivers: qcom: rpmh-rsc: return if the controller is idle

[Lina Iyer]

On Tue, Sep 11 2018 at 16:39 -0600, Matthias Kaehlcke wrote:
> Hi Raju/Lina,
>
> On Fri, Jul 27, 2018 at 03:34:44PM +0530, Raju P L S S S N wrote:
> > From: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> >
> > Allow the controller status be queried. The controller is busy if it is
> > actively processing request.
> >
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> > Signed-off-by: Raju P.L.S.S.S.N <rplsssn@xxxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >      - Remove unnecessary EXPORT_SYMBOL
> > ---
> >  drivers/soc/qcom/rpmh-internal.h |  1 +
> >  drivers/soc/qcom/rpmh-rsc.c      | 20 ++++++++++++++++++++
> >  2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> > index a7bbbb6..4ff43bf 100644
> > --- a/drivers/soc/qcom/rpmh-internal.h
> > +++ b/drivers/soc/qcom/rpmh-internal.h
> > @@ -108,6 +108,7 @@ struct rsc_drv {
> >  int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
> >  			     const struct tcs_request *msg);
> >  int rpmh_rsc_invalidate(struct rsc_drv *drv);
> > +bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv);
> >
> >  void rpmh_tx_done(const struct tcs_request *msg, int r);
> >
> > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > index 33fe9f9..42d0041 100644
> > --- a/drivers/soc/qcom/rpmh-rsc.c
> > +++ b/drivers/soc/qcom/rpmh-rsc.c
> > @@ -496,6 +496,26 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
> >  }
> >
> >  /**
> > + *  rpmh_rsc_ctrlr_is_idle: Check if any of the AMCs are busy.
> > + *
> > + *  @drv: The controller
> > + *
> > + *  Returns true if the TCSes are engaged in handling requests.
> > + */
> > +bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv)
> > +{
> > +	int m;
> > +	struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS);
> > +
> > +	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
> > +		if (!tcs_is_free(drv, m))
> > +			return false;
> > +	}
> > +
> > +	return true;
> > +}
>
> This looks racy, tcs_write() could be running simultaneously and use
> TCSes that were seen as free by _is_idle(). This could be fixed by
> holding tcs->lock (assuming this doesn't cause lock ordering problems).
> However even with this tcs_write() could run right after releasing the
> lock, using TCSes and the caller of _is_idle() would consider the
> controller to be idle.

We could run this without the lock, since we are only reading a status.
Generally, this function is called from the idle code of the last CPU
and no CPU or active TCS request should be in progress, but if it were,
then this function would let the caller know we are not ready to do
idle. If there were no requests that were running at that time we read
the registers, we would not be making one after, since we are already
in the idle code and no requests are made there.

I understand, how it might appear racy, the context of the callling
function helps resolve that.

-- Lina