Re: [PATCH] spi: fsl-dspi: use devm_spi_alloc_master()

[Michael Walle]

Am 2021-11-13 19:06, schrieb Vladimir Oltean:
> 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@xxxxxxxx>
> > ---
> > 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?

just in case someone thinks it is a good idea to convert the
spi_register_controller() call.

> 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"?

It was just a revert and that was the way it was done before
the patch mentioned above. You're right, dspi is better
here and then the hunk below will be superfluous.

Let me know if I should respin the patch or if you like to
keep the devm_kzalloc() as it is now, because you've mentioned
you don't like the spi devres mappers. During earlier debugging
I just noticed the following comment in drivers/spi/spi.c and
noticed that this driver isn't converted:

/* Release the last reference on the controller if its driver
 * has not yet been converted to devm_spi_alloc_master/slave().
 */

-michael

> >  	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