Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 22 Jul 2022 19:38:09 +0100
From:   Mark Brown <broonie@kernel.org>
To:     =?utf-8?B?5ZGo55Cw5p2wIChaaG91IFlhbmppZSk=?= 
        <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.
Message-ID: <YtrukeLk9fInqQIL@sirena.org.uk>
References: <1658508510-15400-1-git-send-email-zhouyanjie@wanyeetech.com>
 <1658508510-15400-4-git-send-email-zhouyanjie@wanyeetech.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="u7Z7lZkjhnzZyt/z"
Content-Disposition: inline
In-Reply-To: <1658508510-15400-4-git-send-email-zhouyanjie@wanyeetech.com>
Precedence: bulk


--u7Z7lZkjhnzZyt/z
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Jul 23, 2022 at 12:48:30AM +0800, =E5=91=A8=E7=90=B0=E6=9D=B0 (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 =3D 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
=66rom the device...

> +static int ingenic_sfc_setup(struct spi_device *spi)
> +{
> +	struct ingenic_sfc *sfc =3D spi_controller_get_devdata(spi->master);
> +	unsigned long rate;
> +	int ret, val;
> +
> +	if (!spi->max_speed_hz)
> +		return -EINVAL;
> +
> +	ret =3D 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 =3D 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 =3D 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 =3D %d\n", sfc->irq,=
 ret);
> +		goto err_put_master;
> +	}

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

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

=2E..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.

--u7Z7lZkjhnzZyt/z
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmLa7pAACgkQJNaLcl1U
h9BufAf+ML3z6pqB+9VRx2uVE2vDguPss14L7VVj8Szqp0T4Dmx4QF9s0u6QKqxP
9UUQHAEdO1N6Vh2SSdBGvOPPrChYaAWW3eA/t7ODFbynEkJ1+bq/UPQrVfOln8hs
8hEnsS1N3XHn4CRSfVBGsw4+FxsIG3t+sSXSHEqI8uk9aBJ2GUgWZepEJheNFK4a
NF6bAJnK896B73olJr0deNlCfhsvSiUqaAE5fxsLuWY7Ur1LNBG8JV5WvHGNEJkR
dknDfQL0Y4nWWlffbKz6VReB6dFvFGETgGfIDepjsxvznoXqAMdNYtwixpu5dVuO
hh/q4B5v95rS4/BjakpMCs+Jmw3Psw==
=c0js
-----END PGP SIGNATURE-----

--u7Z7lZkjhnzZyt/z--