Re: [RFC v4 2/8] spi: rockchip-sfc: add rockchip serial flash controller

[Lukas Wunner <lukas@xxxxxxxxx>]

On Fri, Jun 04, 2021 at 10:10:49AM -0500, Chris Morgan wrote:
> +static int rockchip_sfc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct spi_master *master;
> +	struct resource *res;
> +	struct rockchip_sfc *sfc;
> +	int ret;
> +
> +	master = spi_alloc_master(&pdev->dev, sizeof(*sfc));

You need to use devm_spi_alloc_master() here because you're not
freeing the allocation in any of the probe error paths.

> +	if (!master) {
> +		dev_err(&pdev->dev, "spi_alloc_master failed\n");
> +		return -ENOMEM;
> +	}

The dev_err() is superfluous as the memory allocator will dump a
stack trace anyway on allocation failure.

> +	ret = clk_prepare_enable(sfc->hclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to enable ahb clk\n");
> +		goto err_hclk;
> +	}

Return directly here and get rid of the err_hclk label.

> +	ret = devm_spi_register_master(dev, master);

You need to use spi_register_master() here (*not* the devm variant)
and add spi_unregister_master() at the top of rockchip_sfc_remove().

The reason is that ->remove() is executed *before* devres resources
are freed and rockchip_sfc_remove() disables the clocks, presumably
rendering the chip inaccessible.

However the chip may be performing SPI transfers until
spi_unregister_master() returns, so the chip needs to be accessible
as long.

Because you're using devm_spi_register_master(), the chip may try
to perform SPI transfers even though its clocks have been disabled.
So you've got an ordering problem with the devm variant.

> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Rockchip Serial Flash Controller Driver");
> +MODULE_AUTHOR("Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>");

Why isn't Shawn Lin cc'ed or gets commit authorship even though they're
the driver author?

Thanks,

Lukas