Hi Timur,
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@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, there's also: kfree(i2c_client); 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.