linux-kernel - Re: [PATCH] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <159562491399.3847286.17604881801973524633@swboyd.mtv.corp.google.com>
Date:   Fri, 24 Jul 2020 14:08:33 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Maulik Shah <mkshah@...eaurora.org>,
        Lina Iyer <ilina@...eaurora.org>
Subject: Re: [PATCH] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free

Quoting Doug Anderson (2020-07-24 13:31:39)
> Hi,
> 
> On Fri, Jul 24, 2020 at 1:27 PM Stephen Boyd <swboyd@...omium.org> wrote:
> >
> > Quoting Doug Anderson (2020-07-24 13:11:59)
> > >
> > > 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.
> > >
> >
> > Ok that sounds like you don't want a check for -EBUSY so I'll leave this
> > as >= 0.
> 
> To clarify, I'd be OK with either of these (slight preference towards
> #2, but not a strong one):
> 
> 1. Your current code and a REALLY OBVIOUS comment in
> claim_tcs_for_req() saying that we'd better not return any error codes
> other than -EBUSY (because we'll just blindly retry on all of them).
> 
> - or -
> 
> 2. Handling error codes other than -EBUSY, like this:
> 
> wait_event_lock_irq(drv->tcs_wait,
>                     (tcs_id = claim_tcs_for_req(drv, tcs, msg)) != -EBUSY,
>                     drv->lock);
> if (tcs_id < 0)
>   goto unlock;
> 

Ah I think I understand. You're thinking that claim_tcs_for_req() may
return an error value that isn't -EBUSY some day and then this
wait_event_lock_irq() will keep spinning forever when it isn't busy but
invalid or some such? I'd rather not do #2 because it is dead code until
claim_tcs_for_req() changes. I'll add a comment indicating that it must
return something that is claimed or the caller will keep trying. When
the code changes the call sites should be evaluated by the author to
make sure that it keeps working. I'm afraid a really big comment won't
do much to help with that in the future.