lkml.org 
[lkml]   [2017]   [Aug]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] ALSA: ac97c: Fix an error handling path in 'atmel_ac97c_probe()'
From
Date
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

\
 
 \ /
  Last update: 2017-08-31 21:08    [W:0.837 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site