On Fri, May 28, 2021 at 12:31:09PM +0300, Andy Shevchenko wrote: > 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@xxxxxxxxxxxxxxx> # pxa2xx > > I'm not sure how this can be applied now without reconsidering what is in > for-5.14. I originally developed this patch against for-5.14, then realized that c7299fea6769 went into v5.13-rc3, so I backported it to for-5.13. I was able to cherry-pick the patch cleanly from 5.14 to 5.13, so it seems there won't be any merge conflicts. And I did go through spi-pxa2xx.c's ->setup() hook once more when backporting and double-checked all the error paths. That said, it would definitely help if you or someone else at Intel could test the patch, if only to raise the confidence. Thanks! Lukas