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@xxxxxxxxxxxx> 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. -- Jean Delvare _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
