linux-kernel - Re: [PATCH 4/5] mmc: sdhci-of-arasan: Obviously always return success in remove callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Jun 2022 10:35:32 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Adrian Hunter <adrian.hunter@...el.com>
Cc:     Michal Simek <michal.simek@...inx.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        linux-mmc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        kernel@...gutronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] mmc: sdhci-of-arasan: Obviously always return
 success in remove callback

On Mon, Jun 13, 2022 at 10:13:16AM +0300, Adrian Hunter wrote:
> On 11/06/22 00:12, Uwe Kleine-König wrote:
> > sdhci_pltfm_unregister() returns 0 unconditionally and returning an
> > error in a platform remove callback isn't very sensible. (The only
> > effect of the latter is that the device core emits a generic warning and
> > then removes the device anyhow.)
> > 
> > So return 0 unconditionally to make it obvious there is no error
> > forwarded to the upper layers.
> > 
> > This is a preparation for making platform remove callbacks return void.
> 
> This preparation seems a bit unnatural.

IMHO it's not. I have to adapt all ~4800 platform drivers together in a
single commit to change the prototype of the return callback.

Now assume you want to review that commit and make sure there are no
relevant changes in behaviour. Without the preparation in the commit
under discussion, the change to sdhci-of-arasan.c would look as follows:

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -1733,7 +1733,4 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 
 static int sdhci_arasan_remove(struct platform_device *pdev)
 {
-	int ret;
 	struct sdhci_host *host = platform_get_drvdata(pdev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
@@ -1747,11 +1746,11 @@ static int sdhci_arasan_remove(struct platform_device *pdev)
 
 	sdhci_arasan_unregister_sdclk(&pdev->dev);
 
-	ret = sdhci_pltfm_unregister(pdev);
+	sdhci_pltfm_unregister(pdev);
 
 	clk_disable_unprepare(clk_ahb);
- 
-	return ret;
 }
 
 static struct platform_driver sdhci_arasan_driver = {

So you have to look up then what sdhci_pltfm_unregister() does and if
it's ok to ignore the return value. Should I mention that in the commit
log? What about the other drivers?

If however the change to the sdhci-arasan driver is only

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 3997cad1f793..84c949bd99c8 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -1749,8 +1749,6 @@ static int sdhci_arasan_remove(struct platform_device *pdev)
 	sdhci_pltfm_unregister(pdev);
 
 	clk_disable_unprepare(clk_ahb);
-
-	return 0;
 }
 
 static struct platform_driver sdhci_arasan_driver = {

, it's trivial to see that there is no relevant driver specific change.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)