[alsa-devel] [PATCH] ASoC: Fix cs4270 error path
khali at linux-fr.org
Sat Sep 27 18:09:10 CEST 2008
On Mon, 22 Sep 2008 15:35:17 -0500, Timur Tabi wrote:
> Sorry I didn't get to this earlier. I just fell off my radar.
> On Sun, Aug 31, 2008 at 7:42 AM, Jean Delvare <khali at linux-fr.org> wrote:
> > The error path in cs4270_probe/cs4270_remove is pretty broken:
> > * If cs4270_probe fails, codec is leaked.
> > * If snd_soc_register_card fails, cs4270_i2c_driver stays registered.
> So far, so good.
> > * If I2C support is enabled but no I2C device is found, i2c_del_driver
> > is never called (neither in cs4270_probe nor in cs4270_remove.)
> Hmm... The only time that can happen is if the device tree is wrong or
> the hardware is broken.
The whole point of error paths is to handle cases that were not
supposed to happen.
> (...) This means that cs4270_i2c_probe() will
> return an error. What does i2c_add_driver() return in that case?
As per the device driver model, the success or failure of device probes
has zero influence on drivers. So, yes, i2c_add_driver() still succeeds
and returns 0.
> (...) If
> it still returns 0, then control_data will be NULL, but with your
> patch, i2c_del_driver() will still be called.
Of course, it will be. It has to. This is exactly the bug I am fixing!
If i2c_add_driver() succeeds then i2c_del_driver() must be called in the
cleanup path. It's really that easy. Whether an I2C device was found or
not, must not influence that.
> Also, I think there's a bug in cs4270_i2c_probe(), where it will call
> i2c_detach_driver() if it fails. It shouldn't do that. This is also
> gated on codec->control_data, so it's a related bug.
You are right, this is a bug. Apparently you forgot the error path when
converting the driver to a new-style i2c driver. A few lines below,
which should be removed.
I disagree that this is related with my initial patch though. It's a
different error path, and it's also broken, but that's hardly enough to
make both bugs "related". So I'll send a separate patch.
> Lastly, you may need to rebase the patch, since the code's changed.
My patch still applies fine (with offset, but that's OK), so I fail to
see why I would need to rebase it.
More information about the Alsa-devel