linux-kernel - Re: [RFC PATCH 06/17] net: ipa: Add timeout for ipa_cmd_pipeline_clear

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: <CF2P11HZE0H2.S4II3PH6QLCF@skynet-linux>
Date:   Mon, 18 Oct 2021 22:32:31 +0530
From:   "Sireesh Kodali" <sireeshkodali1@...il.com>
To:     "Alex Elder" <elder@...e.org>, <phone-devel@...r.kernel.org>,
        <~postmarketos/upstreaming@...ts.sr.ht>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <elder@...nel.org>
Cc:     "Vladimir Lypak" <vladimir.lypak@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        "Jakub Kicinski" <kuba@...nel.org>
Subject: Re: [RFC PATCH 06/17] net: ipa: Add timeout for
 ipa_cmd_pipeline_clear_wait

On Thu Oct 14, 2021 at 3:59 AM IST, Alex Elder wrote:
> On 9/19/21 10:08 PM, Sireesh Kodali wrote:
> > From: Vladimir Lypak <vladimir.lypak@...il.com>
> > 
> > Sometimes the pipeline clear fails, and when it does, having a hang in
> > kernel is ugly. The timeout gives us a nice error message. Note that
> > this shouldn't actually hang, ever. It only hangs if there is a mistake
> > in the config, and the timeout is only useful when debugging.
> > 
> > Signed-off-by: Vladimir Lypak <vladimir.lypak@...il.com>
> > Signed-off-by: Sireesh Kodali <sireeshkodali1@...il.com>
>
> This is actually an item on my to-do list. All of the waits
> for GSI completions should have timeouts. The only reason it
> hasn't been implemented already is that I would like to be sure
> all paths that could have a timeout actually have a reasonable
> recovery.
>
> I'd say an error message after a timeout is better than a hung
> task panic, but if this does time out, I'm not sure the state
> of the hardware is well-defined.

Early on while wiring up BAM support, I handn't quite figured out the
IPA init sequence, and some of the BAM opcode stuff. This caused the
driver to hang when it would reach the completion. Since this particular
completion was waited for just before the probe function retured, it
prevented hung up the kernel thread, and prevented the module from being
`modprobe -r`ed.

Since then, I've properly fixed the BAM code, the completion always
returns, making the patch kinda useless for now. Since its only for
debugging, I'll just drop this patch. I think the only error handling we
can do at this stage is to return -EIO, and get the callee to handle
de-initing everything.

Regards,
Sireesh

>
> -Alex
>
> > ---
> >   drivers/net/ipa/ipa_cmd.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ipa/ipa_cmd.c b/drivers/net/ipa/ipa_cmd.c
> > index 3db9e94e484f..0bdbc331fa78 100644
> > --- a/drivers/net/ipa/ipa_cmd.c
> > +++ b/drivers/net/ipa/ipa_cmd.c
> > @@ -658,7 +658,10 @@ u32 ipa_cmd_pipeline_clear_count(void)
> >   
> >   void ipa_cmd_pipeline_clear_wait(struct ipa *ipa)
> >   {
> > -	wait_for_completion(&ipa->completion);
> > +	unsigned long timeout_jiffies = msecs_to_jiffies(1000);
> > +
> > +	if (!wait_for_completion_timeout(&ipa->completion, timeout_jiffies))
> > +		dev_err(&ipa->pdev->dev, "%s time out\n", __func__);
> >   }
> >   
> >   void ipa_cmd_pipeline_clear(struct ipa *ipa)
> >