Re: [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking

[Lina Iyer]

On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
> Quoting Lina Iyer (2019-07-22 14:53:37)
> > From: "Raju P.L.S.S.S.N" <rplsssn@xxxxxxxxxxxxxx>
> >
> > The tcs->lock was introduced to serialize access with in TCS group. But,
> > drv->lock is still needed to synchronize core aspects of the
> > communication. This puts the drv->lock in the critical and high latency
> > path of sending a request. drv->lock provides the all necessary
> > synchronization. So remove locking around TCS group and simply use the
> > drv->lock instead.
>
> This doesn't talk about removing the irq saving and restoring though.
You mean for drv->lock? It was not an _irqsave/_irqrestore anyways and
we were only removing the tcs->lock.

> Can you keep irq saving and restoring in this patch and then remove that
> in the next patch with reasoning? It probably isn't safe if the lock is
> taken in interrupt context anyway.
Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
been changed by this patch.

> > Signed-off-by: Raju P.L.S.S.S.N <rplsssn@xxxxxxxxxxxxxx>
> > [ilina: split patch into multiple files, update commit text]
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
>
> > diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> > index a7bbbb67991c..969d5030860e 100644
> > --- a/drivers/soc/qcom/rpmh-internal.h
> > +++ b/drivers/soc/qcom/rpmh-internal.h
> > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> > index e278fc11fe5c..5ede8d6de3ad 100644
> > --- a/drivers/soc/qcom/rpmh-rsc.c
> > +++ b/drivers/soc/qcom/rpmh-rsc.c
> > @@ -106,26 +106,26 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
> >  {
> >         int m;
> >         struct tcs_group *tcs;
> > +       int ret = 0;
> >
> >         tcs = get_tcs_of_type(drv, type);
> >
> > -       spin_lock(&tcs->lock);
> > -       if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
> > -               spin_unlock(&tcs->lock);
> > -               return 0;
> > -       }
> > +       spin_lock(&drv->lock);
> > +       if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
> > +               goto done_invalidate;
> >
> >         for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
> >                 if (!tcs_is_free(drv, m)) {
> > -                       spin_unlock(&tcs->lock);
> > -                       return -EAGAIN;
> > +                       ret = -EAGAIN;
> > +                       goto done_invalidate;
> >                 }
> >                 write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
> >                 write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
> >         }
> >         bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
> > -       spin_unlock(&tcs->lock);
> >
> > +done_invalidate:
> > +       spin_unlock(&drv->lock);
> >         return 0;
>
> return ret now?
Yes, will do.
> >  }
> >
> > @@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
> >  {
> >         struct tcs_group *tcs;
> >         int tcs_id;
> > -       unsigned long flags;
> >         int ret;
> >
> >         tcs = get_tcs_for_msg(drv, msg);
> >         if (IS_ERR(tcs))
> >                 return PTR_ERR(tcs);
> >
> > -       spin_lock_irqsave(&tcs->lock, flags);
> >         spin_lock(&drv->lock);
> >         /*
> >          * 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) {
> > -               spin_unlock(&drv->lock);
> > +       if (ret)
> >                 goto done_write;
> > -       }
> >
> >         tcs_id = find_free_tcs(tcs);
> >         if (tcs_id < 0) {
> >                 ret = tcs_id;
> > -               spin_unlock(&drv->lock);
> >                 goto done_write;
> >         }
> >
> >         tcs->req[tcs_id - tcs->offset] = msg;
> >         set_bit(tcs_id, drv->tcs_in_use);
> > -       spin_unlock(&drv->lock);
> >
> >         __tcs_buffer_write(drv, tcs_id, 0, msg);
> >         __tcs_trigger(drv, tcs_id);
> >
> >  done_write:
> > -       spin_unlock_irqrestore(&tcs->lock, flags);
> > +       spin_unlock(&drv->lock);
> >         return ret;
> >  }
> >
> > @@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
> >  {
> >         struct tcs_group *tcs;
> >         int tcs_id = 0, cmd_id = 0;
> > -       unsigned long flags;
> >         int ret;
> >
> >         tcs = get_tcs_for_msg(drv, msg);
> >         if (IS_ERR(tcs))
> >                 return PTR_ERR(tcs);
> >
> > -       spin_lock_irqsave(&tcs->lock, flags);
> > +       spin_lock(&drv->lock);
> >         /* find the TCS id and the command in the TCS to write to */
> >         ret = find_slots(tcs, msg, &tcs_id, &cmd_id);
> >         if (!ret)
> >                 __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
> > -       spin_unlock_irqrestore(&tcs->lock, flags);
> > +       spin_unlock(&drv->lock);
>
> These ones, just leave them doing the irq save restore for now?
drv->lock ??

--Lina