Quoting Doug Anderson (2020-07-24 10:42:55)
> Hi,
>
> On Wed, Jul 22, 2020 at 6:01 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> > diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> > index ef60e790a750..9a325bac58fe 100644
> > --- a/drivers/soc/qcom/rpmh-internal.h
> > +++ b/drivers/soc/qcom/rpmh-internal.h
> > @@ -118,6 +119,7 @@ struct rsc_drv {
> > struct tcs_group tcs[TCS_TYPE_NR];
> > DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
> > spinlock_t lock;
> > + wait_queue_head_t tcs_wait;
>
> nit: this structure has a kernel-doc comment above it describing the
> elements. Could you add yours?
Sure.
>
>
> > struct rpmh_ctrlr client;
> > };
> >
> > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > index 076fd27f3081..6c758b052c95 100644
> > --- a/drivers/soc/qcom/rpmh-rsc.c
> > +++ b/drivers/soc/qcom/rpmh-rsc.c
> > @@ -19,6 +19,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/slab.h>
> > #include <linux/spinlock.h>
> > +#include <linux/wait.h>
> >
> > #include <soc/qcom/cmd-db.h>
> > #include <soc/qcom/tcs.h>
> > @@ -444,6 +445,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
> > */
> > if (!drv->tcs[ACTIVE_TCS].num_tcs)
> > enable_tcs_irq(drv, i, false);
> > + wake_up(&drv->tcs_wait);
> > spin_unlock(&drv->lock);
>
> nit: I think it's slightly better to do the wake_up() after the
> spin_unlock(), no? The first thing the other task will do is to try
> to grab the spinlock and we might as well give it a chance of
> succeeding without looping. I don't see any reason why we'd need to
> be holding the lock while calling wake_up().
Right that's better.
>
>
> > if (req)
> > rpmh_tx_done(req, err);
> > @@ -562,44 +564,59 @@ static int find_free_tcs(struct tcs_group *tcs)
> > return -EBUSY;
> > }
> >
> > +static int claim_tcs_for_req(struct rsc_drv *drv, struct tcs_group *tcs,
> > + const struct tcs_request *msg)
>
> nit: I know this is a short function and kernel convention doesn't
> strictly require comments in front of all functions. However, every
> other function in this file has a comment and I had a really hard time
> dealing with the rpmh-rsc code before the comments. Could you add one
> for your function, even if it's short? One thing that would be nice
> to note is that the only error it returns is -EBUSY. See below.
Sure I'll write up some kernel-doc.
>
> > - if (ret)
> > - goto unlock;
> >
> > - ret = find_free_tcs(tcs);
> > - if (ret < 0)
> > - goto unlock;
> > - tcs_id = ret;
> > + wait_event_lock_irq(drv->tcs_wait,
> > + (tcs_id = claim_tcs_for_req(drv, tcs, msg)) >= 0,
>
> Even though claim_tcs_for_req() only returns 0 or -EBUSY today (IOW it
> never returns error codes other than -EBUSY), should we handle it? If
> we don't, claim_tcs_for_req() should be very clear that it shouldn't
> return any errors other than -EBUSY.
Do you mean you want to change it to be
(tcs_id = claim_tcs_for_req(drv, tcs, msg)) != -EBUSY
instead of >= 0? It should return the tcs_id that was claimed, not just
0 or -EBUSY.
