Re: [PATCH 3/3] SPI: Ingenic: Add SFC support for Ingenic SoCs.

From: Mark Brown <broonie@kernel.org>
To: "周琰杰 (Zhou Yanjie)" <zhouyanjie@wanyeetech.com>
Cc: tudor.ambarus@microchip.com, p.yadav@ti.com, michael@walle.cc,
	miquel.raynal@bootlin.com, richard@nod.at, vigneshr@ti.com,
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org,
	linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, aidanmacdonald.0x0@gmail.com,
	tmn505@gmail.com, paul@crapouillou.net,
	dongsheng.qiu@ingenic.com, aric.pzqi@ingenic.com,
	rick.tyliu@ingenic.com, jinghui.liu@ingenic.com,
	sernia.zhou@foxmail.com, reimu@sudomaker.com
Subject: Re: [PATCH 3/3] SPI: Ingenic: Add SFC support for Ingenic SoCs.
Date: Fri, 22 Jul 2022 19:38:09 +0100	[thread overview]
Message-ID: <YtrukeLk9fInqQIL@sirena.org.uk> (raw)
In-Reply-To: <1658508510-15400-4-git-send-email-zhouyanjie@wanyeetech.com>

[-- Attachment #1.1: Type: text/plain, Size: 2329 bytes --]

On Sat, Jul 23, 2022 at 12:48:30AM +0800, 周琰杰 (Zhou Yanjie) wrote:

This looks mostly good, a few small issues though:

> +++ b/drivers/spi/spi-ingenic-sfc.c
> @@ -0,0 +1,662 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Ingenic SoCs SPI Flash Controller Driver

Please make the entire comment a C++ one so things look more
intentional.

> +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
> +{
> +	struct ingenic_sfc *sfc = data;
> +
> +	writel(0x1f, sfc->base + SFC_REG_INTC);
> +
> +	complete(&sfc->completion);
> +
> +	return IRQ_HANDLED;
> +}

This doesn't pay any attention to any status registers in the chip so
won't work if the interrupt is shared and won't notice any error reports
from the device...

> +static int ingenic_sfc_setup(struct spi_device *spi)
> +{
> +	struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
> +	unsigned long rate;
> +	int ret, val;
> +
> +	if (!spi->max_speed_hz)
> +		return -EINVAL;
> +
> +	ret = clk_set_rate(sfc->clk, spi->max_speed_hz * 2);
> +	if (ret)
> +		return -EINVAL;

The setup() operation should be safe for use on one device while another
device is active.  It's not going to be a problem until there's a
version of the IP with more than one chip select, but that could happen
some time (and someone might decide to make a board using GPIO chip
selects...) but this should really go into the data path.

> +	ret = clk_prepare_enable(sfc->clk);
> +	if (ret)
> +		goto err_put_master;

Nothing ever disables this clock.  It might also be nice to enable the
clock only when the controller is in use, that bit is not super
important though.

> +	ret = devm_request_irq(&pdev->dev, sfc->irq, ingenic_sfc_irq_handler, 0,
> +			dev_name(&pdev->dev), sfc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq%d, ret = %d\n", sfc->irq, ret);
> +		goto err_put_master;
> +	}

It's not safe to use devm here...

> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret)
> +		goto err_put_master;

...unregistering the controller may free the driver data structure and
the interrupt handler uses it so we could attempt to use freed data in
the window between the controller being unregistered and the interrupt
being freed.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/