From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E3160C47087 for ; Fri, 28 May 2021 12:52:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C8DB7613E6 for ; Fri, 28 May 2021 12:52:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229552AbhE1MyX (ORCPT ); Fri, 28 May 2021 08:54:23 -0400 Received: from bmailout2.hostsharing.net ([83.223.78.240]:39953 "EHLO bmailout2.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230261AbhE1MyX (ORCPT ); Fri, 28 May 2021 08:54:23 -0400 Received: from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.hostsharing.net", Issuer "RapidSSL TLS DV RSA Mixed SHA256 2020 CA-1" (verified OK)) by bmailout2.hostsharing.net (Postfix) with ESMTPS id C88CC2800B3C2; Fri, 28 May 2021 14:52:45 +0200 (CEST) Received: by h08.hostsharing.net (Postfix, from userid 100393) id BA67986D7E; Fri, 28 May 2021 14:52:45 +0200 (CEST) Date: Fri, 28 May 2021 14:52:45 +0200 From: Lukas Wunner To: Andy Shevchenko Cc: Mark Brown , Saravana Kannan , kernel-team@android.com, linux-spi@vger.kernel.org, Vignesh Raghavendra , Jarkko Nikula Subject: Re: [PATCH for-5.13] spi: Cleanup on failure of initial setup Message-ID: <20210528125245.GC17551@wunner.de> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-spi@vger.kernel.org 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 # 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