Re: [PATCH] ALSA: ac97c: Fix an error handling path in 'atmel_ac97c_probe()'

[Alexandre Belloni]

Hi,

On 31/08/2017 at 21:08:10 +0200, Christophe JAILLET wrote:
> 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!)
> 

You didn't miss anything, that's exactly what I am complaining about
some of the patches were OK, some aren't and all the real work is left
to the maintainer.

> 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.

I think you should go ahead and fix those now...

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com