Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 02DBD60256
Date:   Wed, 24 Jul 2019 08:54:52 -0600
From:   Lina Iyer <ilina@codeaurora.org>
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     agross@kernel.org, bjorn.andersson@linaro.org,
        linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
        rnayak@codeaurora.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, dianders@chromium.org,
        mkshah@codeaurora.org, "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
Subject: Re: [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking
Message-ID: <20190724145452.GC18620@codeaurora.org>
References: <20190722215340.3071-1-ilina@codeaurora.org>
 <5d375054.1c69fb81.7ce3f.3591@mx.google.com>
 <20190723192159.GA18620@codeaurora.org>
 <5d376bb3.1c69fb81.2bb4e.7771@mx.google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <5d376bb3.1c69fb81.2bb4e.7771@mx.google.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 23 2019 at 14:19 -0600, Stephen Boyd wrote:
>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@codeaurora.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.
>
Oh, right.
>>
>> >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.
>
May be I should club this with the following patch. Instead of adding
irqsave and restore to drv->lock and then remvoing them again in the
following patch.

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