From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Lukas Wunner <lukas@wunner.de>
Cc: Mark Brown <broonie@kernel.org>,
Saravana Kannan <saravanak@google.com>,
kernel-team@android.com, linux-spi@vger.kernel.org,
Vignesh Raghavendra <vigneshr@ti.com>,
Jarkko Nikula <jarkko.nikula@linux.intel.com>
Subject: Re: [PATCH for-5.13] spi: Cleanup on failure of initial setup
Date: Fri, 28 May 2021 12:31:09 +0300 [thread overview]
Message-ID: <YLC4XWxfmjgizyvz@smile.fi.intel.com> (raw)
In-Reply-To: <f76a0599469f265b69c371538794101fa37b5536.1622149321.git.lukas@wunner.de>
On Thu, May 27, 2021 at 11:10:56PM +0200, Lukas Wunner wrote:
> Commit c7299fea6769 ("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 c7299fea6769, 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.)
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> # pxa2xx
I'm not sure how this can be applied now without reconsidering what is in
for-5.14.
> Fixes: c7299fea6769 ("spi: Fix spi device unregister flow")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Saravana Kannan <saravanak@google.com>
> ---
> Compile-tested only! Please review and test.
> drivers/spi/spi-bitbang.c | 18 ++++++++++++++----
> drivers/spi/spi-fsl-spi.c | 4 ++++
> drivers/spi/spi-omap-uwire.c | 9 ++++++++-
> drivers/spi/spi-omap2-mcspi.c | 33 ++++++++++++++++++++-------------
> drivers/spi/spi-pxa2xx.c | 9 ++++++++-
> 5 files changed, 54 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> index 6a6af85aebfd..27d0087f8688 100644
> --- a/drivers/spi/spi-bitbang.c
> +++ b/drivers/spi/spi-bitbang.c
> @@ -184,6 +184,8 @@ int spi_bitbang_setup(struct spi_device *spi)
> {
> struct spi_bitbang_cs *cs = spi->controller_state;
> struct spi_bitbang *bitbang;
> + bool initial_setup = false;
> + int retval;
>
> bitbang = spi_master_get_devdata(spi->master);
>
> @@ -192,22 +194,30 @@ int spi_bitbang_setup(struct spi_device *spi)
> if (!cs)
> return -ENOMEM;
> spi->controller_state = cs;
> + initial_setup = true;
> }
>
> /* per-word shift register access, in hardware or bitbanging */
> cs->txrx_word = bitbang->txrx_word[spi->mode & (SPI_CPOL|SPI_CPHA)];
> - if (!cs->txrx_word)
> - return -EINVAL;
> + if (!cs->txrx_word) {
> + retval = -EINVAL;
> + goto err_free;
> + }
>
> if (bitbang->setup_transfer) {
> - int retval = bitbang->setup_transfer(spi, NULL);
> + retval = bitbang->setup_transfer(spi, NULL);
> if (retval < 0)
> - return retval;
> + goto err_free;
> }
>
> dev_dbg(&spi->dev, "%s, %u nsec/bit\n", __func__, 2 * cs->nsecs);
>
> return 0;
> +
> +err_free:
> + if (initial_setup)
> + kfree(cs);
> + return retval;
> }
> EXPORT_SYMBOL_GPL(spi_bitbang_setup);
>
> diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
> index d0e5aa18b7ba..bdf94cc7be1a 100644
> --- a/drivers/spi/spi-fsl-spi.c
> +++ b/drivers/spi/spi-fsl-spi.c
> @@ -440,6 +440,7 @@ static int fsl_spi_setup(struct spi_device *spi)
> {
> struct mpc8xxx_spi *mpc8xxx_spi;
> struct fsl_spi_reg __iomem *reg_base;
> + bool initial_setup = false;
> int retval;
> u32 hw_mode;
> struct spi_mpc8xxx_cs *cs = spi_get_ctldata(spi);
> @@ -452,6 +453,7 @@ static int fsl_spi_setup(struct spi_device *spi)
> if (!cs)
> return -ENOMEM;
> spi_set_ctldata(spi, cs);
> + initial_setup = true;
> }
> 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);
> if (retval < 0) {
> cs->hw_mode = hw_mode; /* Restore settings */
> + if (initial_setup)
> + kfree(cs);
> return retval;
> }
>
> diff --git a/drivers/spi/spi-omap-uwire.c b/drivers/spi/spi-omap-uwire.c
> index 71402f71ddd8..df28c6664aba 100644
> --- a/drivers/spi/spi-omap-uwire.c
> +++ b/drivers/spi/spi-omap-uwire.c
> @@ -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)
> {
> struct uwire_state *ust = spi->controller_state;
> + bool initial_setup = false;
> + int status;
>
> if (ust == NULL) {
> ust = kzalloc(sizeof(*ust), GFP_KERNEL);
> if (ust == NULL)
> return -ENOMEM;
> 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)
> diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> index 999c22736416..ede7f05e5ced 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -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)
> {
> + bool initial_setup = false;
> int ret;
> struct omap2_mcspi *mcspi = spi_master_get_devdata(spi->master);
> struct omap2_mcspi_regs *ctx = &mcspi->ctx;
> @@ -1051,35 +1065,28 @@ static int omap2_mcspi_setup(struct spi_device *spi)
> spi->controller_state = cs;
> /* Link this to context save list */
> list_add_tail(&cs->node, &ctx->cs);
> + initial_setup = true;
> }
>
> ret = pm_runtime_get_sync(mcspi->dev);
> if (ret < 0) {
> pm_runtime_put_noidle(mcspi->dev);
> + if (initial_setup)
> + omap2_mcspi_cleanup(spi);
>
> return ret;
> }
>
> ret = omap2_mcspi_setup_transfer(spi, NULL);
> + if (ret && initial_setup)
> + omap2_mcspi_cleanup(spi);
> +
> pm_runtime_mark_last_busy(mcspi->dev);
> pm_runtime_put_autosuspend(mcspi->dev);
>
> 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)
> {
> struct omap2_mcspi *mcspi = data;
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index 5e59ba075bc7..8ee0cc071777 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -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;
>
> err = gpiod_direction_output(gpiod, !chip->gpio_cs_inverted);
> + if (err)
> + gpiod_put(chip->gpiod_cs);
> }
>
> return err;
> @@ -1267,6 +1269,7 @@ static int setup(struct spi_device *spi)
> struct driver_data *drv_data =
> spi_controller_get_devdata(spi->controller);
> uint tx_thres, tx_hi_thres, rx_thres;
> + int err;
>
> switch (drv_data->ssp_type) {
> case QUARK_X1000_SSP:
> @@ -1413,7 +1416,11 @@ static int setup(struct spi_device *spi)
> if (drv_data->ssp_type == CE4100_SSP)
> 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)
> --
> 2.31.1
>
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2021-05-28 9:31 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-27 21:10 [PATCH for-5.13] spi: Cleanup on failure of initial setup Lukas Wunner
2021-05-28 9:31 ` Andy Shevchenko [this message]
2021-05-28 12:52 ` Lukas Wunner
2021-06-01 17:37 ` Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YLC4XWxfmjgizyvz@smile.fi.intel.com \
--to=andriy.shevchenko@linux.intel.com \
--cc=broonie@kernel.org \
--cc=jarkko.nikula@linux.intel.com \
--cc=kernel-team@android.com \
--cc=linux-spi@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=saravanak@google.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.