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

Christophe JAILLET
2017-08-31T19:08:10Z

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

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html