Unverified Commit 2ec6f20b authored by Lukas Wunner's avatar Lukas Wunner Committed by Mark Brown
Browse files

spi: Cleanup on failure of initial setup 



Commit c7299fea ("spi: Fix spi device unregister flow") changed the
SPI core's behavior if the ->setup() hook returns an error upon adding
an spi_device:  Before, the ->cleanup() hook was invoked to free any
allocations that were made by ->setup().  With the commit, that's no
longer the case, so the ->setup() hook is expected to free the
allocations itself.

I've identified 5 drivers which depend on the old behavior and am fixing
them up hereinafter: spi-bitbang.c spi-fsl-spi.c spi-omap-uwire.c
spi-omap2-mcspi.c spi-pxa2xx.c

Importantly, ->setup() is not only invoked on spi_device *addition*:
It may subsequently be called to *change* SPI parameters.  If changing
these SPI parameters fails, freeing memory allocations would be wrong.
That should only be done if the spi_device is finally destroyed.
I am therefore using a bool "initial_setup" in 4 of the affected drivers
to differentiate between the invocation on *adding* the spi_device and
any subsequent invocations: spi-bitbang.c spi-fsl-spi.c spi-omap-uwire.c
spi-omap2-mcspi.c

In spi-pxa2xx.c, it seems the ->setup() hook can only fail on spi_device
addition, not any subsequent calls.  It therefore doesn't need the bool.

It's worth noting that 5 other drivers already perform a cleanup if the
->setup() hook fails.  Before c7299fea, they caused a double-free
if ->setup() failed on spi_device addition.  Since the commit, they're
fine.  These drivers are: spi-mpc512x-psc.c spi-pl022.c spi-s3c64xx.c
spi-st-ssc4.c spi-tegra114.c

(spi-pxa2xx.c also already performs a cleanup, but only in one of
several error paths.)

Fixes: c7299fea ("spi: Fix spi device unregister flow")
Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
Cc: Saravana Kannan <saravanak@google.com>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> # pxa2xx
Link: https://lore.kernel.org/r/f76a0599469f265b69c371538794101fa37b5536.1622149321.git.lukas@wunner.de


Signed-off-by: default avatarMark Brown <broonie@kernel.org>
parent 13817d46

drivers/spi/spi-bitbang.c

+14 −4
Original line number Original line Diff line number Diff line
@@ -184,6 +184,8 @@ int spi_bitbang_setup(struct spi_device *spi) 
{

{

	struct spi_bitbang_cs	*cs = spi->controller_state;

	struct spi_bitbang_cs	*cs = spi->controller_state;

	struct spi_bitbang	*bitbang;

	struct spi_bitbang	*bitbang;

	bool			initial_setup = false;

	int			retval;





	bitbang = spi_master_get_devdata(spi->master);

	bitbang = spi_master_get_devdata(spi->master);
@@ -192,22 +194,30 @@ int spi_bitbang_setup(struct spi_device *spi) 
		if (!cs)

		if (!cs)

			return -ENOMEM;

			return -ENOMEM;

		spi->controller_state = cs;

		spi->controller_state = cs;

		initial_setup = true;

	}

	}





	/* per-word shift register access, in hardware or bitbanging */

	/* per-word shift register access, in hardware or bitbanging */

	cs->txrx_word = bitbang->txrx_word[spi->mode & (SPI_CPOL|SPI_CPHA)];

	cs->txrx_word = bitbang->txrx_word[spi->mode & (SPI_CPOL|SPI_CPHA)];

	if (!cs->txrx_word)

	if (!cs->txrx_word) {

		return -EINVAL;

		retval = -EINVAL;

		goto err_free;

	}





	if (bitbang->setup_transfer) {

	if (bitbang->setup_transfer) {

		int retval = bitbang->setup_transfer(spi, NULL);

		retval = bitbang->setup_transfer(spi, NULL);

		if (retval < 0)

		if (retval < 0)

			return retval;

			goto err_free;

	}

	}





	dev_dbg(&spi->dev, "%s, %u nsec/bit\n", __func__, 2 * cs->nsecs);

	dev_dbg(&spi->dev, "%s, %u nsec/bit\n", __func__, 2 * cs->nsecs);





	return 0;

	return 0;



err_free:

	if (initial_setup)

		kfree(cs);

	return retval;

}

}

EXPORT_SYMBOL_GPL(spi_bitbang_setup);

EXPORT_SYMBOL_GPL(spi_bitbang_setup);

drivers/spi/spi-fsl-spi.c

+4 −0
Original line number Original line Diff line number Diff line
@@ -440,6 +440,7 @@ static int fsl_spi_setup(struct spi_device *spi) 
{

{

	struct mpc8xxx_spi *mpc8xxx_spi;

	struct mpc8xxx_spi *mpc8xxx_spi;

	struct fsl_spi_reg __iomem *reg_base;

	struct fsl_spi_reg __iomem *reg_base;

	bool initial_setup = false;

	int retval;

	int retval;

	u32 hw_mode;

	u32 hw_mode;

	struct spi_mpc8xxx_cs *cs = spi_get_ctldata(spi);

	struct spi_mpc8xxx_cs *cs = spi_get_ctldata(spi);
@@ -452,6 +453,7 @@ static int fsl_spi_setup(struct spi_device *spi) 
		if (!cs)

		if (!cs)

			return -ENOMEM;

			return -ENOMEM;

		spi_set_ctldata(spi, cs);

		spi_set_ctldata(spi, cs);

		initial_setup = true;

	}

	}

	mpc8xxx_spi = spi_master_get_devdata(spi->master);

	mpc8xxx_spi = spi_master_get_devdata(spi->master);
@@ -475,6 +477,8 @@ static int fsl_spi_setup(struct spi_device *spi) 
	retval = fsl_spi_setup_transfer(spi, NULL);

	retval = fsl_spi_setup_transfer(spi, NULL);

	if (retval < 0) {

	if (retval < 0) {

		cs->hw_mode = hw_mode; /* Restore settings */

		cs->hw_mode = hw_mode; /* Restore settings */

		if (initial_setup)

			kfree(cs);

		return retval;

		return retval;

	}

	}

drivers/spi/spi-omap-uwire.c

+8 −1
Original line number Original line Diff line number Diff line
@@ -424,15 +424,22 @@ static int uwire_setup_transfer(struct spi_device *spi, struct spi_transfer *t) 
static int uwire_setup(struct spi_device *spi)

static int uwire_setup(struct spi_device *spi)

{

{

	struct uwire_state *ust = spi->controller_state;

	struct uwire_state *ust = spi->controller_state;

	bool initial_setup = false;

	int status;





	if (ust == NULL) {

	if (ust == NULL) {

		ust = kzalloc(sizeof(*ust), GFP_KERNEL);

		ust = kzalloc(sizeof(*ust), GFP_KERNEL);

		if (ust == NULL)

		if (ust == NULL)

			return -ENOMEM;

			return -ENOMEM;

		spi->controller_state = ust;

		spi->controller_state = ust;

		initial_setup = true;

	}

	}





	return uwire_setup_transfer(spi, NULL);

	status = uwire_setup_transfer(spi, NULL);

	if (status && initial_setup)

		kfree(ust);



	return status;

}

}





static void uwire_cleanup(struct spi_device *spi)

static void uwire_cleanup(struct spi_device *spi)

drivers/spi/spi-omap2-mcspi.c

+20 −13
Original line number Original line Diff line number Diff line
@@ -1032,8 +1032,22 @@ static void omap2_mcspi_release_dma(struct spi_master *master) 
	}

	}

}

}





static void omap2_mcspi_cleanup(struct spi_device *spi)

{

	struct omap2_mcspi_cs	*cs;



	if (spi->controller_state) {

		/* Unlink controller state from context save list */

		cs = spi->controller_state;

		list_del(&cs->node);



		kfree(cs);

	}

}



static int omap2_mcspi_setup(struct spi_device *spi)

static int omap2_mcspi_setup(struct spi_device *spi)

{

{

	bool			initial_setup = false;

	int			ret;

	int			ret;

	struct omap2_mcspi	*mcspi = spi_master_get_devdata(spi->master);

	struct omap2_mcspi	*mcspi = spi_master_get_devdata(spi->master);

	struct omap2_mcspi_regs	*ctx = &mcspi->ctx;

	struct omap2_mcspi_regs	*ctx = &mcspi->ctx;
@@ -1051,35 +1065,28 @@ static int omap2_mcspi_setup(struct spi_device *spi) 
		spi->controller_state = cs;

		spi->controller_state = cs;

		/* Link this to context save list */

		/* Link this to context save list */

		list_add_tail(&cs->node, &ctx->cs);

		list_add_tail(&cs->node, &ctx->cs);

		initial_setup = true;

	}

	}





	ret = pm_runtime_get_sync(mcspi->dev);

	ret = pm_runtime_get_sync(mcspi->dev);

	if (ret < 0) {

	if (ret < 0) {

		pm_runtime_put_noidle(mcspi->dev);

		pm_runtime_put_noidle(mcspi->dev);

		if (initial_setup)

			omap2_mcspi_cleanup(spi);





		return ret;

		return ret;

	}

	}





	ret = omap2_mcspi_setup_transfer(spi, NULL);

	ret = omap2_mcspi_setup_transfer(spi, NULL);

	if (ret && initial_setup)

		omap2_mcspi_cleanup(spi);



	pm_runtime_mark_last_busy(mcspi->dev);

	pm_runtime_mark_last_busy(mcspi->dev);

	pm_runtime_put_autosuspend(mcspi->dev);

	pm_runtime_put_autosuspend(mcspi->dev);





	return ret;

	return ret;

}

}





static void omap2_mcspi_cleanup(struct spi_device *spi)

{

	struct omap2_mcspi_cs	*cs;



	if (spi->controller_state) {

		/* Unlink controller state from context save list */

		cs = spi->controller_state;

		list_del(&cs->node);



		kfree(cs);

	}

}



static irqreturn_t omap2_mcspi_irq_handler(int irq, void *data)

static irqreturn_t omap2_mcspi_irq_handler(int irq, void *data)

{

{

	struct omap2_mcspi *mcspi = data;

	struct omap2_mcspi *mcspi = data;

drivers/spi/spi-pxa2xx.c

+8 −1
Original line number Original line Diff line number Diff line
@@ -1254,6 +1254,8 @@ static int setup_cs(struct spi_device *spi, struct chip_data *chip, 
		chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;

		chip->gpio_cs_inverted = spi->mode & SPI_CS_HIGH;





		err = gpiod_direction_output(gpiod, !chip->gpio_cs_inverted);

		err = gpiod_direction_output(gpiod, !chip->gpio_cs_inverted);

		if (err)

			gpiod_put(chip->gpiod_cs);

	}

	}





	return err;

	return err;
@@ -1267,6 +1269,7 @@ static int setup(struct spi_device *spi) 
	struct driver_data *drv_data =

	struct driver_data *drv_data =

		spi_controller_get_devdata(spi->controller);

		spi_controller_get_devdata(spi->controller);

	uint tx_thres, tx_hi_thres, rx_thres;

	uint tx_thres, tx_hi_thres, rx_thres;

	int err;





	switch (drv_data->ssp_type) {

	switch (drv_data->ssp_type) {

	case QUARK_X1000_SSP:

	case QUARK_X1000_SSP:
@@ -1413,7 +1416,11 @@ static int setup(struct spi_device *spi) 
	if (drv_data->ssp_type == CE4100_SSP)

	if (drv_data->ssp_type == CE4100_SSP)

		return 0;

		return 0;





	return setup_cs(spi, chip, chip_info);

	err = setup_cs(spi, chip, chip_info);

	if (err)

		kfree(chip);



	return err;

}

}





static void cleanup(struct spi_device *spi)

static void cleanup(struct spi_device *spi)