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;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAD=FV=WH1vKKe=MPVdtBJZWnSzxNLO0uyM02GFG6oCJfSEwehQ@mail.gmail.com>
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>
Subject: Re: [PATCH] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free
From:   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>
To:     Doug Anderson <dianders@chromium.org>
Date:   Fri, 24 Jul 2020 13:01:12 -0700
Message-ID: <159562087212.3847286.9484527206999948907@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9.1
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Doug Anderson (2020-07-24 12:49:56)
> Hi,
>=20
> On Fri, Jul 24, 2020 at 12:44 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > > -       if (ret)
> > > > -               goto unlock;
> > > >
> > > > -       ret =3D find_free_tcs(tcs);
> > > > -       if (ret < 0)
> > > > -               goto unlock;
> > > > -       tcs_id =3D ret;
> > > > +       wait_event_lock_irq(drv->tcs_wait,
> > > > +                           (tcs_id =3D claim_tcs_for_req(drv, tcs,=
 msg)) >=3D 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 =3D claim_tcs_for_req(drv, tcs, msg)) !=3D -EBUSY
> >
> > instead of >=3D 0? It should return the tcs_id that was claimed, not ju=
st
> > 0 or -EBUSY.
>=20
> 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.
>=20

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 >=3D 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 >=3D 0
or !=3D -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.