linux-kernel - Re: [PATCH 2/4] spi: sprd: Add the SPI irq function for the SPI DMA mode

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: <CAMz4ku+nky4YSFjjQ5BGJRCFQHCnXysh+_C7FmFF5YpdpDbZuA@mail.gmail.com>
Date:   Wed, 16 Jan 2019 10:21:42 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Orson Zhai <orsonzhai@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>, lanqing.liu@...soc.com,
        linux-spi@...r.kernel.org, DTML <devicetree@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] spi: sprd: Add the SPI irq function for the SPI DMA mode

Hi Mark,

On Tue, 15 Jan 2019 at 22:25, Mark Brown <broonie@...nel.org> wrote:
>
> On Tue, Jan 15, 2019 at 09:46:51PM +0800, Baolin Wang wrote:
>
> This looks good, just one small issue and a thing to check:
>
> > +static irqreturn_t sprd_spi_handle_irq(int irq, void *data)
> > +{
> > +     struct sprd_spi *ss = (struct sprd_spi *)data;
> > +     u32 val = readl_relaxed(ss->base + SPRD_SPI_INT_MASK_STS);
> > +
> > +     if (val & SPRD_SPI_MASK_TX_END) {
> > +             writel_relaxed(SPRD_SPI_TX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> > +             if (!(ss->trans_mode & SPRD_SPI_RX_MODE))
> > +                     complete(&ss->xfer_completion);
> > +             return IRQ_HANDLED;
> > +     }
> > +
> > +     if (val & SPRD_SPI_MASK_RX_END) {
> > +             writel_relaxed(SPRD_SPI_RX_END_CLR, ss->base + SPRD_SPI_INT_CLR);
> > +             complete(&ss->xfer_completion);
> > +     }
> > +
> > +     return IRQ_HANDLED;
> > +}
>
> This will return IRQ_HANDLED no matter if there was an interrupt
> actually handled.  That means that if something goes wrong due to some
> bug or a hardware change (eg, a new version of the IP) and there's
> another interrupt fired we won't clear it and the interrupt core won't
> be able to detect that it's a spurious interrupt and use its own error
> handling.  It's better to return IRQ_NONE in that case.

Yes, you are correct and will fix in next version.

> > +     ret = devm_request_irq(&pdev->dev, ss->irq, sprd_spi_handle_irq,
> > +                             0, pdev->name, ss);
> > +     if (ret)
> > +             dev_err(&pdev->dev, "failed to request spi irq %d, ret = %d\n",
> > +                     ss->irq, ret);
>
> Are you sure it's safe to use devm_request_irq(), especially when
> unloading the driver?  Using it means that we will only disable the
> interrupt after the driver's remove function has finished so there's a
> danger of an interrupt firing when some of the resources the hander has
> used are still in use.  I didn't spot any issues, just something to
> check especially with the later patches building on top of this.

Yes, we missed this issue, thanks for pointing out this. After some
discussion with Lanqing, we think we need call the
spi_controller_suspend() manually in remove function to avoid this
issue.

Thanks for your comments.

-- 
Baolin Wang
Best Regards