Date:   Sat, 13 Nov 2021 20:06:42 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Michael Walle <michael@walle.cc>
Cc:     linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH] spi: fsl-dspi: use devm_spi_alloc_master()
Message-ID: <20211113180642.7doe6dc5iotw5yez@skbuf>
References: <20211110161754.3312805-1-michael@walle.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211110161754.3312805-1-michael@walle.cc>
Precedence: bulk

On Wed, Nov 10, 2021 at 05:17:54PM +0100, Michael Walle wrote:
> Commit 530b5affc675 ("spi: fsl-dspi: fix use-after-free in remove path")
> fixed the use-after-free by manually allocating memory. Nowadays, since
> commit 5e844cc37a5c ("spi: Introduce device-managed SPI controller
> allocation"), there is a devres version of spi_alloc_master() for exactly
> this purpose. Revert the commit which introduced the manual allocation
> and use the new devm_spi_alloc_master().
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Btw, using the devm_ version of spi_controller_register() doesn't seem to

you mean spi_register_controller.

> be a good idea, see commit 8d559a64f00b ("spi: stm32: drop devres version
> of spi_register_master").

And you mention this because? The dspi driver doesn't use
devm_spi_register_controller(). In fact, if it could be made to not use
devres at all, I couldn't be happier. At this stage, the devres wrappers
for SPI are doing more harm than good.

>  drivers/spi/spi-fsl-dspi.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index fd004c9db9dc..29f8a596c8ee 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -1227,17 +1227,11 @@ static int dspi_probe(struct platform_device *pdev)
>  	void __iomem *base;
>  	bool big_endian;
>  
> -	dspi = devm_kzalloc(&pdev->dev, sizeof(*dspi), GFP_KERNEL);
> -	if (!dspi)
> -		return -ENOMEM;
> -
> -	ctlr = spi_alloc_master(&pdev->dev, 0);
> +	ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(struct fsl_dspi));
>  	if (!ctlr)
>  		return -ENOMEM;
>  
> -	spi_controller_set_devdata(ctlr, dspi);
> -	platform_set_drvdata(pdev, dspi);
> -
> +	dspi = spi_controller_get_devdata(ctlr);
>  	dspi->pdev = pdev;
>  	dspi->ctlr = ctlr;
>  
> @@ -1373,6 +1367,8 @@ static int dspi_probe(struct platform_device *pdev)
>  	if (dspi->devtype_data->trans_mode != DSPI_DMA_MODE)
>  		ctlr->ptp_sts_supported = true;
>  
> +	platform_set_drvdata(pdev, ctlr);
> +

Why do you feel a need to change the drvdata from "dspi" to "ctrl"?

>  	ret = spi_register_controller(ctlr);
>  	if (ret != 0) {
>  		dev_err(&pdev->dev, "Problem registering DSPI ctlr\n");
> @@ -1396,7 +1392,8 @@ static int dspi_probe(struct platform_device *pdev)
>  
>  static int dspi_remove(struct platform_device *pdev)
>  {
> -	struct fsl_dspi *dspi = platform_get_drvdata(pdev);
> +	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> +	struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
>  
>  	/* Disconnect from the SPI framework */
>  	spi_unregister_controller(dspi->ctlr);
> -- 
> 2.30.2
>