linux-kernel - Re: [PATCH] spi: bcm2835: Enable shared interrupt support

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: <20200529174358.som3snunfxch6phi@wunner.de>
Date:   Fri, 29 May 2020 19:43:58 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
Cc:     Mark Brown <broonie@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        bcm-kernel-feedback-list@...adcom.com,
        Martin Sperl <kernel@...tin.sperl.org>,
        linux-spi@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: bcm2835: Enable shared interrupt support

On Thu, May 28, 2020 at 08:58:04PM +0200, Nicolas Saenz Julienne wrote:
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -379,6 +379,10 @@ static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
>  	if (bs->tx_len && cs & BCM2835_SPI_CS_DONE)
>  		bcm2835_wr_fifo_blind(bs, BCM2835_SPI_FIFO_SIZE);
>  
> +	/* check if we got interrupt enabled */
> +	if (!(bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_INTR))
> +		return IRQ_NONE;
> +
>  	/* Read as many bytes as possible from FIFO */
>  	bcm2835_rd_fifo(bs);
>  	/* Write as many bytes as possible to FIFO */

This definitely looks wrong.  The check whether the interrupt is enabled
should be moved *before* the conditional calls to bcm2835_rd_fifo_blind()
and bcm2835_wr_fifo_blind(), i.e. to the top of the function.

Otherwise if an interrupt is raised by another SPI controller,
this function may perform read/write accesses to the FIFO and
interfere with an ongoing transfer in DMA or poll mode.

Also, instead of performing an MMIO read, just use the "cs" variable
which was assigned at the top of the function.

The code comment should explain that the check is necessary because the
interrupt may be shared with other controllers on the BCM2711.

Finally, it would be nice if the check would be optimized away when
compiling for pre-RasPi4 products, maybe something like:

+	if (IS_ENABLED(CONFIG_ARM_LPAE) && !(cs & BCM2835_SPI_CS_INTR))
+		return IRQ_NONE;

Thanks,

Lukas