linux-kernel - Re: [PATCH] remoteproc: qcom: q6v5: shore up resource probe handling

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+ASDXP5iBWgX1K3=QHke4ZoqO72hLk6Q09=cP3X-esrTOhk+w@mail.gmail.com>
Date:   Tue, 9 Oct 2018 16:48:31 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     ohad@...ery.com, bjorn.andersson@...aro.org,
        linux-remoteproc@...r.kernel.org,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org, sibis@...eaurora.org
Subject: Re: [PATCH] remoteproc: qcom: q6v5: shore up resource probe handling

On Tue, Oct 9, 2018 at 4:34 PM Doug Anderson <dianders@...omium.org> wrote:
> On Tue, Oct 9, 2018 at 3:33 PM Brian Norris <briannorris@...omium.org> wrote:
> > +       if (q6v5->wdog_irq < 0) {
> > +               if (q6v5->wdog_irq != -EPROBE_DEFER)
> > +                       dev_err(&pdev->dev,
> > +                               "failed to retrieve wdog IRQ: %d\n",
> > +                               q6v5->wdog_irq);
> > +               return q6v5->wdog_irq;
> > +       }
>
> optional: Since there's the same pattern 5 times here, I wonder if we
> should abstract it out to a helper function that would print the
> error?

I'll leave that up to Bjorn. I don't personally care to do this, but I
also acknowledge that there are a bunch of drivers that do this wrong.

> ...in the ideal case it would be somewhere that all Linux drivers
> could use since this is a super common pattern, but that might be a
> bit too much yak shaving...

Yeah, I'll pass on shaving that yak.

> > @@ -237,8 +260,13 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
> >         disable_irq(q6v5->handover_irq);
> >
> >         q6v5->stop_irq = platform_get_irq_byname(pdev, "stop-ack");
> > -       if (q6v5->stop_irq == -EPROBE_DEFER)
> > -               return -EPROBE_DEFER;
> > +       if (q6v5->stop_irq < 0) {
> > +               if (q6v5->stop_irq != -EPROBE_DEFER)
> > +                       dev_err(&pdev->dev,
> > +                               "failed to retrieve stop IRQ: %d\n",
> > +                               q6v5->stop_irq);
> > +               return q6v5->stop_irq;
>
> Nitty nit that it's the "stop-ack" IRQ, not the "stop" IRQ.  The error
> message below this one calls it stop-ack too so it would be ideal to
> keep it consistent between the two error messages.

Ack. I thought I double checked on comparing the error messages.

Bjorn already has this on his radar, so I'll let him decide if he
wants other changes, or if he wants to make his own transformations
before applying it.

> Reviewed-by: Douglas Anderson <dianders@...omium.org>

Thanks,
Brian