Date:   Thu, 31 Aug 2017 21:08:10 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Mark Brown <broonie@...nel.org>, Takashi Iwai <tiwai@...e.de>
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org, nicolas.ferre@...rochip.com,
        garsilva@...eddedor.com, arvind.yadav.cs@...il.com,
        Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        andriy.shevchenko@...ux.intel.com, bhumirks@...il.com
Subject: Re: [PATCH] ALSA: ac97c: Fix an error handling path in
 'atmel_ac97c_probe()'

Le 31/08/2017 à 12:38, Mark Brown a écrit :
> On Thu, Aug 31, 2017 at 12:31:33PM +0200, Takashi Iwai wrote:
>
>> This is again a typical problem by such a trivial fix patch: the code
>> looks as if it were trivial and correct, buried in a patch series that
>> easily leads to the oversight by the maintainer's review.
> Right, plus the amount of context that diff shows you.
>
Hi,

My proposed patch was initially triggered using coccinelle, as you must 
have guessed.

In fact, I was surprised by the initial commit.
I don't have any strong opinion if testing the return value of 
'clk_prepare_enable()' is relevant or not, but I was surprised that the 
error handling path had not been updated at the same time.

So, before posting my patch, I have searched a bit in git history and it 
gave:

git shortlog --author="Arvind Yadav" | grep clk_prepare
       ata: sata_rcar: Handle return value of clk_prepare_enable
       hwrng: omap3-rom - Handle return value of clk_prepare_enable
       crypto: img-hash - Handle return value of clk_prepare_enable
       dmaengine: DW DMAC: Handle return value of clk_prepare_enable
       gpio: davinci: Handle return value of clk_prepare_enable
       cpufreq: kirkwood-cpufreq:- Handle return value of 
clk_prepare_enable()
       dmaengine: imx-sdma: Handle return value of clk_prepare_enable
       Input: s3c2410_ts - handle return value of clk_prepare_enable
       iio: adc: xilinx: Handle return value of clk_prepare_enable
       iio:adc:lpc32xx Handle return value of clk_prepare_enable
       memory: ti-aemif: Handle return value of clk_prepare_enable
       spi: davinci: Handle return value of clk_prepare_enable
       [media] tc358743: Handle return value of clk_prepare_enable
       mtd: nand: orion: Handle return value of clk_prepare_enable
       iio: Aspeed ADC - Handle return value of clk_prepare_enable
       PM / devfreq: exynos-nocp: Handle return value of clk_prepare_enable
       PM / devfreq: exynos-ppmu: Handle return value of clk_prepare_enable
       usb: host: ehci-exynos: Handle return value of clk_prepare_enable
       usb: mtu3: Handle return value of clk_prepare_enable
       usb: mtu3: Handle return value of clk_prepare_enable
       video: fbdev: pxafb: Handle return value of clk_prepare_enable
       usb: gadget: mv_udc: Handle return value of clk_prepare_enable.
       usb: dwc3: exynos: Handle return value of clk_prepare_enable
       i2c: at91: Handle return value of clk_prepare_enable
       i2c: emev2: Handle return value of clk_prepare_enable
       usb: host: ohci-pxa27x: Handle return value of clk_prepare_enable
       thermal: imx: Handle return value of clk_prepare_enable
       thermal: hisilicon: Handle return value of clk_prepare_enable
       PCI: rockchip: Check for clk_prepare_enable() errors during resume
       watchdog: meson: Handle return value of clk_prepare_enable
       watchdog: davinci: Handle return value of clk_prepare_enable
       mfd: tc6393xb: Handle return value of clk_prepare_enable
       ASoC: samsung: s3c2412: Handle return value of clk_prepare_enable.
       ASoC: samsung: s3c24xx: Handle return value of clk_prepare_enable.
       ASoC: samsung: pcm: Handle return value of clk_prepare_enable.
       ASoC: samsung: i2s: Handle return value of clk_prepare_enable.
       ASoC: samsung: spdif: Handle return value of clk_prepare_enable.
       ASoC: mxs-saif: Handle return value of 
clk_prepare_enable/clk_prepare.
       ASoC: jz4740: Handle return value of clk_prepare_enable.
       ASoC: sun4i-spdif: Handle return value of clk_prepare_enable.
       ASoC: atmel: ac97c: Handle return value of clk_prepare_enable.
       gpio: mb86s7x: Handle return value of clk_prepare_enable.
       memory: mtk-smi: Handle return value of clk_prepare_enable
       mmc: sdhci-st: Handle return value of clk_prepare_enable
       mmc: wmt-sdmmc: Handle return value of clk_prepare_enable
       mmc: mxcmmc: Handle return value of clk_prepare_enable
       dmaengine: at_xdmac: Handle return value of clk_prepare_enable.
       mtd: nand: denali: Handle return value of clk_prepare_enable.
       mtd: oxnas_nand: Handle clk_prepare_enable/clk_disable_unprepare.
       mtd: nand: lpc32xx_slc: Handle return value of clk_prepare_enable.
       mtd: nand: lpc32xx_mlc: Handle return value of clk_prepare_enable.
       mtd: st_spi_fsm: Handle clk_prepare_enable/clk_disable_unprepare.

Some of these are after a devm_clk_get(), which does not require a 
modification in the error handling path (at least according to the one 
I've looked at)

Some don't have any [devm_]clk_get() in the same function, and were not 
investigated further.

But several also had the same construction as the one reported in this 
thread, and needed, IMHO, an update of the error handling path to call 
through clk_put().


It was "too" surprising to me to have "all" these "obviously" incomplete 
patches merged.
I thought that I had missed something obvious and decided to propose one 
fix to see the reaction (and didn't expected all your replies!)

So now, I think we should go through the commits above to either revert 
the commit and remove the test (and document why it is not needed) or 
fix the error handling path accordingly, even if one could know that it 
cant' happen.

CJ

Powered by blists - more mailing lists