linux-kernel - Re: [PATCH 3/3] spi: apple: Add driver for Apple SPI controller

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 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]

Date:   Sat, 1 Jan 2022 08:25:48 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     Hector Martin <marcan@...can.st>
Cc:     Sven Peter <sven@...npeter.dev>, Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        linux-arm-kernel@...ts.infradead.org, linux-spi@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] spi: apple: Add driver for Apple SPI controller

On Sun, Dec 12, 2021 at 12:47:26PM +0900, Hector Martin wrote:
> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
[...]
> +static int apple_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	struct apple_spi *spi = spi_controller_get_devdata(ctlr);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	/* Disable all the interrupts just in case */
> +	reg_write(spi, APPLE_SPI_IE_FIFO, 0);
> +	reg_write(spi, APPLE_SPI_IE_XFER, 0);
> +
> +	clk_disable_unprepare(spi->clk);

You need to use spi_register_controller() in apple_spi_probe()
(*not* the devm variant) and invoke spi_unregister_controller()
first thing in apple_spi_remove().

Otherwise you've got an ordering issue on driver unbind:
__device_release_driver() first calls the ->remove hook and only
then devres_release_all().  You're disabling the clock and interrupts
in the ->remove hook, rendering the controller unusable.  Yet the
expectation is that until spi_unregister_controller() returns,
the controller works and its slaves are accessible.

This is especially important if there are slaves attached to the
controller which perform SPI transfers in their ->remove hooks,
e.g. to quiesce interrupts on the slaves.  Those transfers won't
work the way you've structured the code now.

spi-sifive.c, from which you've derived this, is broken.  As are
a couple dozen other drivers.  I've fixed some of them,
but haven't gotten around to fixing them all.  Just trying to
prevent more brokenness from entering the codebase.

Thanks,

Lukas