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

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]

Date:   Fri, 22 Jul 2022 19:38:09 +0100
From:   Mark Brown <broonie@...nel.org>
To:     周琰杰 (Zhou Yanjie) 
        <zhouyanjie@...yeetech.com>
Cc:     tudor.ambarus@...rochip.com, p.yadav@...com, michael@...le.cc,
        miquel.raynal@...tlin.com, richard@....at, vigneshr@...com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        linux-mtd@...ts.infradead.org, linux-spi@...r.kernel.org,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, aidanmacdonald.0x0@...il.com,
        tmn505@...il.com, paul@...pouillou.net, dongsheng.qiu@...enic.com,
        aric.pzqi@...enic.com, rick.tyliu@...enic.com,
        jinghui.liu@...enic.com, sernia.zhou@...mail.com,
        reimu@...omaker.com
Subject: Re: [PATCH 3/3] SPI: Ingenic: Add SFC support for Ingenic SoCs.

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.

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