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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5d376bb3.1c69fb81.2bb4e.7771@mx.google.com>
Date:   Tue, 23 Jul 2019 13:18:58 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Lina Iyer <ilina@...eaurora.org>
Cc:     agross@...nel.org, bjorn.andersson@...aro.org,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        rnayak@...eaurora.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, dianders@...omium.org,
        mkshah@...eaurora.org, "Raju P.L.S.S.S.N" <rplsssn@...eaurora.org>
Subject: Re: [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking

Quoting Lina Iyer (2019-07-23 12:21:59)
> 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@...eaurora.org>
> >>
> >> 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.

Yes drv->lock wasn't an irqsave/restore variant because it was a
spinlock inside of an obviously already irqsaved region of code because
the tcs->lock was outside the drv->lock and that was saving the irq
flags.

> 
> >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.

It needs to be changed to maintain the irqsaving/restoring of the code.

> >> @@ -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 ??
> 

Yes, it should have irq save/restore still.