[alsa-devel] [PATCH] ASoC: Fix cs4270 error path
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. * 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.)
Fix the first 2 problems by implementing a clean error path in cs4270_probe and jumping to its labels as needed. Fix the 3rd problem by removing the condition to call i2c_del_driver in cs4270_remove.
Signed-off-by: Jean Delvare khali@linux-fr.org --- Timur, can you please review and test this patch? I do not have the hardware so I couldn't test it myself.
sound/soc/codecs/cs4270.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
--- linux-2.6.27-rc5.orig/sound/soc/codecs/cs4270.c 2008-08-31 13:59:17.000000000 +0200 +++ linux-2.6.27-rc5/sound/soc/codecs/cs4270.c 2008-08-31 14:23:34.000000000 +0200 @@ -727,7 +727,7 @@ static int cs4270_probe(struct platform_ ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1); if (ret < 0) { printk(KERN_ERR "cs4270: failed to create PCMs\n"); - return ret; + goto error_free_codec; }
#ifdef USE_I2C @@ -736,8 +736,7 @@ static int cs4270_probe(struct platform_ ret = i2c_add_driver(&cs4270_i2c_driver); if (ret) { printk(KERN_ERR "cs4270: failed to attach driver"); - snd_soc_free_pcms(socdev); - return ret; + goto error_free_pcms; }
/* Did we find a CS4270 on the I2C bus? */ @@ -759,10 +758,23 @@ static int cs4270_probe(struct platform_ ret = snd_soc_register_card(socdev); if (ret < 0) { printk(KERN_ERR "cs4270: failed to register card\n"); - snd_soc_free_pcms(socdev); - return ret; + goto error_del_driver; }
+ return 0; + +error_del_driver: +#ifdef USE_I2C + i2c_del_driver(&cs4270_i2c_driver); + +error_free_pcms: +#endif + snd_soc_free_pcms(socdev); + +error_free_codec: + kfree(socdev->codec); + socdev->codec = NULL; + return ret; }
@@ -773,8 +785,7 @@ static int cs4270_remove(struct platform snd_soc_free_pcms(socdev);
#ifdef USE_I2C - if (socdev->codec->control_data) - i2c_del_driver(&cs4270_i2c_driver); + i2c_del_driver(&cs4270_i2c_driver); #endif
kfree(socdev->codec);
Timur,
Any news about this fix? This should go in kernel 2.6.28.
On Sun, 31 Aug 2008 14:42:14 +0200, Jean Delvare 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.
- 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.)
Fix the first 2 problems by implementing a clean error path in cs4270_probe and jumping to its labels as needed. Fix the 3rd problem by removing the condition to call i2c_del_driver in cs4270_remove.
Signed-off-by: Jean Delvare khali@linux-fr.org
Timur, can you please review and test this patch? I do not have the hardware so I couldn't test it myself.
sound/soc/codecs/cs4270.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
--- linux-2.6.27-rc5.orig/sound/soc/codecs/cs4270.c 2008-08-31 13:59:17.000000000 +0200 +++ linux-2.6.27-rc5/sound/soc/codecs/cs4270.c 2008-08-31 14:23:34.000000000 +0200 @@ -727,7 +727,7 @@ static int cs4270_probe(struct platform_ ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1); if (ret < 0) { printk(KERN_ERR "cs4270: failed to create PCMs\n");
return ret;
goto error_free_codec;#ifdef USE_I2C @@ -736,8 +736,7 @@ static int cs4270_probe(struct platform_ ret = i2c_add_driver(&cs4270_i2c_driver); if (ret) { printk(KERN_ERR "cs4270: failed to attach driver");
snd_soc_free_pcms(socdev);return ret;
goto error_free_pcms;}
/* Did we find a CS4270 on the I2C bus? */
@@ -759,10 +758,23 @@ static int cs4270_probe(struct platform_ ret = snd_soc_register_card(socdev); if (ret < 0) { printk(KERN_ERR "cs4270: failed to register card\n");
snd_soc_free_pcms(socdev);return ret;
goto error_del_driver;}
return 0;
+error_del_driver: +#ifdef USE_I2C
- i2c_del_driver(&cs4270_i2c_driver);
+error_free_pcms: +#endif
- snd_soc_free_pcms(socdev);
+error_free_codec:
- kfree(socdev->codec);
- socdev->codec = NULL;
- return ret;
}
@@ -773,8 +785,7 @@ static int cs4270_remove(struct platform snd_soc_free_pcms(socdev);
#ifdef USE_I2C
- if (socdev->codec->control_data)
i2c_del_driver(&cs4270_i2c_driver);
- i2c_del_driver(&cs4270_i2c_driver);
#endif
kfree(socdev->codec);
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. This means that cs4270_i2c_probe() will return an error. What does i2c_add_driver() return in that case? If it still returns 0, then control_data will be NULL, but with your patch, i2c_del_driver() will still be called.
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.
Lastly, you may need to rebase the patch, since the code's changed.
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.
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.
- 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.)
Fix the first 2 problems by implementing a clean error path in cs4270_probe and jumping to its labels as needed. Fix the 3rd problem by removing the condition to call i2c_del_driver in cs4270_remove.
Signed-off-by: Jean Delvare khali@linux-fr.org
Acked-By: Timur Tabi timur@freescale.com
Takashi, this patch needs to go into 2.6.27 as well. Sorry about that. I don't know how I missed so many problems with my code.
At Mon, 29 Sep 2008 08:42:15 -0500, Timur Tabi wrote:
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.
- 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.)
Fix the first 2 problems by implementing a clean error path in cs4270_probe and jumping to its labels as needed. Fix the 3rd problem by removing the condition to call i2c_del_driver in cs4270_remove.
Signed-off-by: Jean Delvare khali@linux-fr.org
Acked-By: Timur Tabi timur@freescale.com
Takashi, this patch needs to go into 2.6.27 as well. Sorry about that. I don't know how I missed so many problems with my code.
Don't worry, I already put it to the queue, too.
Takashi
Hi Takashi,
On Mon, 29 Sep 2008 15:48:03 +0200, Takashi Iwai wrote:
At Mon, 29 Sep 2008 08:42:15 -0500, Timur Tabi wrote:
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.
- 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.)
Fix the first 2 problems by implementing a clean error path in cs4270_probe and jumping to its labels as needed. Fix the 3rd problem by removing the condition to call i2c_del_driver in cs4270_remove.
Signed-off-by: Jean Delvare khali@linux-fr.org
Acked-By: Timur Tabi timur@freescale.com
Takashi, this patch needs to go into 2.6.27 as well. Sorry about that. I don't know how I missed so many problems with my code.
Don't worry, I already put it to the queue, too.
I fear there's some confusion there. There are two different patches fixing error paths in cs4270. One fixing a fallout from the new-style i2c driver conversion (in cs4270_i2c_probe), under name "ASoC: Fix another cs4270 error path". This one you pushed to Linus last night.
But there's another one, named "ASoC: Fix cs4270 error path", originally posted by myself on August 31st, fixing the error path of cs4270_probe. This is the one Timur was just acking, but I do _not_ see it in your queue, so I suspect you missed it. I can resend it if it helps.
Thanks,
At Tue, 30 Sep 2008 10:31:37 +0200, Jean Delvare wrote:
Hi Takashi,
On Mon, 29 Sep 2008 15:48:03 +0200, Takashi Iwai wrote:
At Mon, 29 Sep 2008 08:42:15 -0500, Timur Tabi wrote:
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.
- 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.)
Fix the first 2 problems by implementing a clean error path in cs4270_probe and jumping to its labels as needed. Fix the 3rd problem by removing the condition to call i2c_del_driver in cs4270_remove.
Signed-off-by: Jean Delvare khali@linux-fr.org
Acked-By: Timur Tabi timur@freescale.com
Takashi, this patch needs to go into 2.6.27 as well. Sorry about that. I don't know how I missed so many problems with my code.
Don't worry, I already put it to the queue, too.
I fear there's some confusion there. There are two different patches fixing error paths in cs4270. One fixing a fallout from the new-style i2c driver conversion (in cs4270_i2c_probe), under name "ASoC: Fix another cs4270 error path". This one you pushed to Linus last night.
But there's another one, named "ASoC: Fix cs4270 error path", originally posted by myself on August 31st, fixing the error path of cs4270_probe. This is the one Timur was just acking, but I do _not_ see it in your queue, so I suspect you missed it. I can resend it if it helps.
Oh, OK, then I must have missed that. Could you repost? And, this *must* go to 2.6.27, or not?
Frankly, this series of cs4270 patches have been hard to handle because it was always unclear what the patch is for. The description "It's for 2.6.x" is too ambiguous because it doesn't always mean the purpose but also can mean the based version of the patch. So, a more clear sign would be really helpful for me at the next time...
thanks,
Takashi
Hi Takashi,
On Tue, 30 Sep 2008 10:53:20 +0200, Takashi Iwai wrote:
At Tue, 30 Sep 2008 10:31:37 +0200, Jean Delvare wrote:
I fear there's some confusion there. There are two different patches fixing error paths in cs4270. One fixing a fallout from the new-style i2c driver conversion (in cs4270_i2c_probe), under name "ASoC: Fix another cs4270 error path". This one you pushed to Linus last night.
But there's another one, named "ASoC: Fix cs4270 error path", originally posted by myself on August 31st, fixing the error path of cs4270_probe. This is the one Timur was just acking, but I do _not_ see it in your queue, so I suspect you missed it. I can resend it if it helps.
Oh, OK, then I must have missed that. Could you repost?
Will do in a minute.
And, this *must* go to 2.6.27, or not?
It could go in 2.6.27, certainly, but I wouldn't say it *must* go there. The patch is "only" fixing an error path, which by definition isn't supposed to be needed unless something unexpected happens, in which case the driver probably won't work anyway. So it doesn't deserve delaying 2.6.27, and sending a pull request to Linus for just that patch would probably be overkill. But if you get the opportunity to send such a pull request for another problem, then it makes sense to include this cs4270 fix as well.
Anyway it's really up to you and Timur. I just happened to notice the bug and posted a fix, but I'm not even using the driver myself.
Frankly, this series of cs4270 patches have been hard to handle because it was always unclear what the patch is for. The description "It's for 2.6.x" is too ambiguous because it doesn't always mean the purpose but also can mean the based version of the patch. So, a more clear sign would be really helpful for me at the next time...
OK, sorry about that. I'll try to make things clearer next time.
At Tue, 30 Sep 2008 11:38:55 +0200, Jean Delvare wrote:
Hi Takashi,
On Tue, 30 Sep 2008 10:53:20 +0200, Takashi Iwai wrote:
At Tue, 30 Sep 2008 10:31:37 +0200, Jean Delvare wrote:
I fear there's some confusion there. There are two different patches fixing error paths in cs4270. One fixing a fallout from the new-style i2c driver conversion (in cs4270_i2c_probe), under name "ASoC: Fix another cs4270 error path". This one you pushed to Linus last night.
But there's another one, named "ASoC: Fix cs4270 error path", originally posted by myself on August 31st, fixing the error path of cs4270_probe. This is the one Timur was just acking, but I do _not_ see it in your queue, so I suspect you missed it. I can resend it if it helps.
Oh, OK, then I must have missed that. Could you repost?
Will do in a minute.
And, this *must* go to 2.6.27, or not?
It could go in 2.6.27, certainly, but I wouldn't say it *must* go there. The patch is "only" fixing an error path, which by definition isn't supposed to be needed unless something unexpected happens, in which case the driver probably won't work anyway. So it doesn't deserve delaying 2.6.27, and sending a pull request to Linus for just that patch would probably be overkill. But if you get the opportunity to send such a pull request for another problem, then it makes sense to include this cs4270 fix as well.
There is one quirk fix for a Dell laptop, so I'm going to put it in the next pull request before 2.6.27 final.
thanks,
Takashi
Takashi Iwai wrote:
Oh, OK, then I must have missed that. Could you repost? And, this *must* go to 2.6.27, or not?
The only patch that needs to go into 2.6.27 is the one titled "alsa: make the CS4270 driver a new-style I2C driver" from me. This one is missing from Linus' tree.
I notice that "ALSA: ASoC: Fix another cs4270 error path" is in Linus' tree, but nothing else is.
Frankly, this series of cs4270 patches have been hard to handle because it was always unclear what the patch is for. The description "It's for 2.6.x" is too ambiguous because it doesn't always mean the purpose but also can mean the based version of the patch. So, a more clear sign would be really helpful for me at the next time...
I can do that, but I'm not sure how I can be any clearer. "This is for 2.6.x", to me at least, means exactly that - that this patch should be applied to the branch for 2.6.27, which is either a release candidate (i.e. 2.6.27-rcX) or a bug fix (i.e. 2.6.27.x), depending on what's next.
If you want me to use different wording, just tell me what I should say.
At Tue, 30 Sep 2008 09:25:50 -0500, Timur Tabi wrote:
Takashi Iwai wrote:
Oh, OK, then I must have missed that. Could you repost? And, this *must* go to 2.6.27, or not?
The only patch that needs to go into 2.6.27 is the one titled "alsa: make the CS4270 driver a new-style I2C driver" from me.
So, do you mean that this patch (ASoC: Fix cs4270 error path) doesn't have to go into 2.6.27? Hell, there are still things unclear to me...
This one is missing from Linus' tree.
It's already in 2.6.27-rc8: ec2cd95f340fb07b905839ee219b3846ecf58396 ALSA: make the CS4270 driver a new-style I2C driver
I notice that "ALSA: ASoC: Fix another cs4270 error path" is in Linus' tree, but nothing else is.
Frankly, this series of cs4270 patches have been hard to handle because it was always unclear what the patch is for. The description "It's for 2.6.x" is too ambiguous because it doesn't always mean the purpose but also can mean the based version of the patch. So, a more clear sign would be really helpful for me at the next time...
I can do that, but I'm not sure how I can be any clearer. "This is for 2.6.x", to me at least, means exactly that - that this patch should be applied to the branch for 2.6.27, which is either a release candidate (i.e. 2.6.27-rcX) or a bug fix (i.e. 2.6.27.x), depending on what's next.
If you want me to use different wording, just tell me what I should say.
Just suggest more clearly that your patch is to be merged as soon as possible. For example, "apply this to next 2.6.27-rc8 pull request" or "merge this to the upstream immediately", or so.
In short: "A is for B" is too passive and ambiguous. Rather say simply "Do X".
thanks,
Takashi
Takashi Iwai wrote:
So, do you mean that this patch (ASoC: Fix cs4270 error path) doesn't have to go into 2.6.27? Hell, there are still things unclear to me...
I think "ASoC: Fix cs4270 error path" should go in, but Jean is correct - it just fixes an error condition that will never happen in real life. It's your call if you want to push it up.
This one is missing from Linus' tree.
It's already in 2.6.27-rc8: ec2cd95f340fb07b905839ee219b3846ecf58396 ALSA: make the CS4270 driver a new-style I2C driver
Ah, I misread the search page. I see it now.
In short: "A is for B" is too passive and ambiguous. Rather say simply "Do X".
Got it.
At Tue, 30 Sep 2008 09:56:15 -0500, Timur Tabi wrote:
Takashi Iwai wrote:
So, do you mean that this patch (ASoC: Fix cs4270 error path) doesn't have to go into 2.6.27? Hell, there are still things unclear to me...
I think "ASoC: Fix cs4270 error path" should go in, but Jean is correct - it just fixes an error condition that will never happen in real life. It's your call if you want to push it up.
OK, thanks for checking. Then this is now in my queue for the next pull request with another patch, and will be likely into 2.6.27.
Takashi
participants (3)
-
Jean Delvare -
Takashi Iwai -
Timur Tabi