Re: [PATCH v2 1/3] soc: qcom: rpmh-rsc: Remove tcs_is_free() and find_free_tcs() APIs

[Stephen Boyd <swboyd@chromium.org>]

Quoting Doug Anderson (2020-04-27 17:13:04)
> On Sat, Apr 25, 2020 at 10:53 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > index 571aa1012f23..3f4951840365 100644
> > --- a/drivers/soc/qcom/rpmh-rsc.c
> > +++ b/drivers/soc/qcom/rpmh-rsc.c
> > @@ -492,33 +476,37 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
> >   *
> >   * This will walk through the TCSes in the group and check if any of them
> >   * appear to be sending to addresses referenced in the message. If it finds
> > - * one it'll return -EBUSY.
> > + * one it'll return -EBUSY because the hardware can't handle more than
> > + * one of the same address being changed at the same time.
> >   *
> > - * Only for use for active-only transfers.
> > + * Only for use with active-only transfers.
> >   *
> >   * Must be called with the drv->lock held since that protects tcs_in_use.
> >   *
> > - * Return: 0 if nothing in flight or -EBUSY if we should try again later.
> > + * Return: offset` of free slot if nothing in flight and a free slot is found
> 
> Why the back tick after "offset"?

Must be some copy/paste bug!

> 
> 
> > + *         or -EBUSY if we should try again later.
> >   *         The caller must re-enable interrupts between tries since that's
> > - *         the only way tcs_is_free() will ever return true and the only way
> > + *         the only way tcs_in_use will ever be updated and the only way
> >   *         RSC_DRV_CMD_ENABLE will ever be cleared.
> >   */
> > -static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
> > -                                 const struct tcs_request *msg)
> > +static int check_for_req_inflight_and_find_free(struct rsc_drv *drv,
> > +       const struct tcs_group *tcs, const struct tcs_request *msg)
> >  {
> >         unsigned long curr_enabled;
> >         u32 addr;
> > -       int i, j, k;
> > -       int tcs_id = tcs->offset;
> > -
> > -       for (i = 0; i < tcs->num_tcs; i++, tcs_id++) {
> > -               if (tcs_is_free(drv, tcs_id))
> > -                       continue;
> > +       int j, k;
> > +       int i = tcs->offset;
> > +       unsigned long max = tcs->offset + tcs->num_tcs;
> > +       int first_free = i;
> 
> The way "first_free" is calculated definitely adds complexity to this
> function.  Are we sure it's justified compared to just calling
> find_next_zero_bit() if the function doesn't return -EBUSY?  If you
> really like it this way I won't object too strongly, but I'm not
> convinced that it makes the code size smaller (vs. jumping to a common
> implementation in the kernel) and it seems unlikely to have any
> real-world speed impact.

I was trying to coalesce the double loop over the free bits here by
adding a couple more lines to keep track of the first free bit and to
set the bit when it's found. It almost feels like it would be better to
inline this whole function into the one call site too.

> 
> 
> > -               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
> > +       for_each_set_bit_from(i, drv->tcs_in_use, max) {
> > +               /* Find a free tcs to use in this group */
> > +               if (first_free == i)
> > +                       first_free = i + 1; /* Maybe the next one is free? */
> >
> > +               curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i);
> >                 for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
> > -                       addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
> > +                       addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, i, j);
> >                         for (k = 0; k < msg->num_cmds; k++) {
> >                                 if (addr == msg->cmds[k].addr)
> >                                         return -EBUSY;
> > @@ -526,28 +514,11 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
> >                 }
> >         }
> >
> > -       return 0;
> > -}
> > +       if (first_free >= max)
> > +               return -EBUSY;
> >
> > -/**
> > - * find_free_tcs() - Find free tcs in the given tcs_group; only for active.
> > - * @tcs: A pointer to the active-only tcs_group (or the wake tcs_group if
> > - *       we borrowed it because there are zero active-only ones).
> > - *
> > - * Must be called with the drv->lock held since that protects tcs_in_use.
> > - *
> > - * Return: The first tcs that's free.
> > - */
> > -static int find_free_tcs(struct tcs_group *tcs)
> > -{
> > -       int i;
> > -
> > -       for (i = 0; i < tcs->num_tcs; i++) {
> > -               if (tcs_is_free(tcs->drv, tcs->offset + i))
> > -                       return tcs->offset + i;
> > -       }
> > -
> > -       return -EBUSY;
> > +       set_bit(first_free, drv->tcs_in_use);
> 
> Function is not documented to also set the bit.  Do we really gain
> anything by setting it in this function, or can it just stay with the
> caller?  I'd hate to call this function
> check_for_req_inflight_and_find_free_and_claim_it().

Maybe the function can be named claim_tcs_for_req() or something like
that. Anything to make it shorter would be good!

> 
> 
> > +       return first_free;
> >  }
> >
> >  /**
> > @@ -580,17 +551,14 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> >          * The h/w does not like if we send a request to the same address,
> >          * when one is already in-flight or being processed.
> >          */
> > -       ret = check_for_req_inflight(drv, tcs, msg);
> > -       if (ret)
> > -               goto unlock;
> > -
> > -       ret = find_free_tcs(tcs);
> > -       if (ret < 0)
> > +       tcs_id = check_for_req_inflight_and_find_free(drv, tcs, msg);
> > +       if (tcs_id < 0) {
> > +               ret = tcs_id;
> >                 goto unlock;
> > -       tcs_id = ret;
> > +       }
> >
> > +       ret = 0;
> 
> nit: Cleaner to just init to 0 when the function is declared now?
> 

Ok.