linux-kernel - Re: [PATCH 2/3] spi: axiado: Add driver for Axiado SPI DB 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 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: <9b6aaad6-3dca-49e3-9a59-8e8a0179a517@sirena.org.uk>
Date: Mon, 15 Sep 2025 15:30:25 +0100
From: Mark Brown <broonie@...nel.org>
To: Vladimir Moravcevic <vmoravcevic@...ado.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Harshit Shah <hshah@...ado.com>,
	Tzu-Hao Wei <twei@...ado.com>,
	Axiado Reviewers <linux-maintainer@...ado.com>,
	linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] spi: axiado: Add driver for Axiado SPI DB controller

On Mon, Sep 15, 2025 at 06:11:56AM -0700, Vladimir Moravcevic wrote:

> +	/*Calculate the maximum data payload that can fit into the FIFO. */
> +	if (fifo_total_bytes <= protocol_overhead_bytes) {
> +		max_transfer_payload_bytes = 0;
> +		dev_warn(&spi->dev, "SPI FIFO (%zu bytes) is too small for protocol overhead (%zu bytes)! Max data size forced to 0.\n",
> +			 fifo_total_bytes, protocol_overhead_bytes);

This might be a good fit for dev_warn_once(), I imagine if gets
triggered lots of whatever operation triggers it will happen and the
current code would spam the logs.

> +	ret = devm_request_irq(&pdev->dev, irq, ax_spi_irq,
> +			       0, pdev->name, ctlr);
> +	if (ret != 0) {
> +		ret = -ENXIO;
> +		dev_err(&pdev->dev, "request_irq failed\n");
> +		goto clk_dis_all;
> +	}

None of the other allocations are managed using devm, you most likely
have unsafe race conditions especially if the interrupt line is shared.

> +static void ax_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	struct ax_spi *xspi = spi_controller_get_devdata(ctlr);
> +
> +	clk_disable_unprepare(xspi->ref_clk);
> +	clk_disable_unprepare(xspi->pclk);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +
> +	spi_unregister_controller(ctlr);
> +}

This will do a bunch of teardown before unregistering the controller
meaning that new operations might be submitted after the clocks are
disabled which I imagine won't go well.  You should unregister from the
subsystem first, then tear down the other resources.

> +
> +static struct platform_driver ax_spi_driver = {
> +	.probe	= ax_spi_probe,
> +	.remove	= ax_spi_remove,
> +	.driver = {
> +		.name = AX_SPI_NAME,
> +		.of_match_table = ax_spi_of_match,
> +	},
> +};

There were a bunch of runtime PM calls but there are no PM operations
here at all.  That's not specifically a problem, for example power
domain level PM with full state retention would work here, but it seems
like at least stopping and starting the clocks would be a good idea.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)