Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200723010137.3127584-1-swboyd@chromium.org> <CAD=FV=WtjyYY+bmocc17S9NbRs6inkAWjj7=c9qBsVf3LtG99Q@mail.gmail.com>
 <159561988523.3847286.14763422711224252201@swboyd.mtv.corp.google.com>
 <CAD=FV=WH1vKKe=MPVdtBJZWnSzxNLO0uyM02GFG6oCJfSEwehQ@mail.gmail.com> <159562087212.3847286.9484527206999948907@swboyd.mtv.corp.google.com>
In-Reply-To: <159562087212.3847286.9484527206999948907@swboyd.mtv.corp.google.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 24 Jul 2020 13:11:59 -0700
Message-ID: <CAD=FV=VUSwsP_xrHsufLjZqbWdn5V8rybtv2DWad2nBfU+VJ9w@mail.gmail.com>
Subject: Re: [PATCH] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Maulik Shah <mkshah@codeaurora.org>,
        Lina Iyer <ilina@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Fri, Jul 24, 2020 at 1:01 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-07-24 12:49:56)
> > Hi,
> >
> > On Fri, Jul 24, 2020 at 12:44 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > > > -       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.
> >
> > Ah, right.  Yes, you got it right.  Of course then we have to add a
> > "if (tcd_id < 0) goto unlock", too.  If you think it's not worth
> > adding this then we just need to make sure it's super obvious in
> > claim_tcs_for_req() that it's not allowed to return other errors.
> >
>
> Hmm right now the code will wait forever for the condition to become
> true, so it will only ever continue past this point if tcs_id >= 0. We
> could add a timeout here in another patch, but I didn't want to change
> the behavior of what is there in this patch. I don't really care if >= 0
> or != -EBUSY is used here so I can change it to -EBUSY to provide
> clarity.
>
> If we add a timeout here, it would be better to change this driver to
> use a pull model instead of the push model it is using today so that the
> timeout isn't necessary. That would entail making a proper kthread that
> pulls requests off a queue of messages and then this asnyc call would
> append messages to the end of the queue and return immediately. That
> would be necessary if we want the async calls to work from non-sleeping
> contexts for example. I think Lina was alluding to this earlier in this
> thread.

I wasn't suggesting adding a timeout.  I was just saying that if
claim_tcs_for_req() were to ever return an error code other than
-EBUSY that we'd need a check for it because otherwise we'd interpret
the result as a tcs_id.

-Doug