[PATCH AUTOSEL 5.10 30/62] ASoC: rt5645: add error checking to rt5645_probe function
From: Phillip Potter phil@philpotter.co.uk
[ Upstream commit 5e70b8e22b64eed13d5bbebcb5911dae65bf8c6b ]
Check for return value from various snd_soc_dapm_* calls, as many of them can return errors and this should be handled. Also, reintroduce the allocation failure check for rt5645->eq_param as well. Make all areas where return values are checked lead to the end of the function in the case of an error. Finally, introduce a comment explaining how resources here are actually eventually cleaned up by the caller.
Cc: Mark Brown broonie@kernel.org Signed-off-by: Phillip Potter phil@philpotter.co.uk Link: https://lore.kernel.org/r/20210503115736.2104747-56-gregkh@linuxfoundation.o... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Sasha Levin sashal@kernel.org --- sound/soc/codecs/rt5645.c | 48 +++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 9 deletions(-)
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index ed4b59ba63f3..a5665e992ecb 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3376,30 +3376,44 @@ static int rt5645_probe(struct snd_soc_component *component) { struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); struct rt5645_priv *rt5645 = snd_soc_component_get_drvdata(component); + int ret = 0;
rt5645->component = component;
switch (rt5645->codec_type) { case CODEC_TYPE_RT5645: - snd_soc_dapm_new_controls(dapm, + ret = snd_soc_dapm_new_controls(dapm, rt5645_specific_dapm_widgets, ARRAY_SIZE(rt5645_specific_dapm_widgets)); - snd_soc_dapm_add_routes(dapm, + if (ret < 0) + goto exit; + + ret = snd_soc_dapm_add_routes(dapm, rt5645_specific_dapm_routes, ARRAY_SIZE(rt5645_specific_dapm_routes)); + if (ret < 0) + goto exit; + if (rt5645->v_id < 3) { - snd_soc_dapm_add_routes(dapm, + ret = snd_soc_dapm_add_routes(dapm, rt5645_old_dapm_routes, ARRAY_SIZE(rt5645_old_dapm_routes)); + if (ret < 0) + goto exit; } break; case CODEC_TYPE_RT5650: - snd_soc_dapm_new_controls(dapm, + ret = snd_soc_dapm_new_controls(dapm, rt5650_specific_dapm_widgets, ARRAY_SIZE(rt5650_specific_dapm_widgets)); - snd_soc_dapm_add_routes(dapm, + if (ret < 0) + goto exit; + + ret = snd_soc_dapm_add_routes(dapm, rt5650_specific_dapm_routes, ARRAY_SIZE(rt5650_specific_dapm_routes)); + if (ret < 0) + goto exit; break; }
@@ -3407,9 +3421,17 @@ static int rt5645_probe(struct snd_soc_component *component)
/* for JD function */ if (rt5645->pdata.jd_mode) { - snd_soc_dapm_force_enable_pin(dapm, "JD Power"); - snd_soc_dapm_force_enable_pin(dapm, "LDO2"); - snd_soc_dapm_sync(dapm); + ret = snd_soc_dapm_force_enable_pin(dapm, "JD Power"); + if (ret < 0) + goto exit; + + ret = snd_soc_dapm_force_enable_pin(dapm, "LDO2"); + if (ret < 0) + goto exit; + + ret = snd_soc_dapm_sync(dapm); + if (ret < 0) + goto exit; }
if (rt5645->pdata.long_name) @@ -3419,7 +3441,15 @@ static int rt5645_probe(struct snd_soc_component *component) RT5645_HWEQ_NUM, sizeof(struct rt5645_eq_param_s), GFP_KERNEL);
- return 0; + if (!rt5645->eq_param) + ret = -ENOMEM; +exit: + /* + * If there was an error above, everything will be cleaned up by the + * caller if we return an error here. This will be done with a later + * call to rt5645_remove(). + */ + return ret; }
static void rt5645_remove(struct snd_soc_component *component)
On Mon, May 24, 2021 at 10:47:11AM -0400, Sasha Levin wrote:
From: Phillip Potter phil@philpotter.co.uk
[ Upstream commit 5e70b8e22b64eed13d5bbebcb5911dae65bf8c6b ]
Check for return value from various snd_soc_dapm_* calls, as many of them can return errors and this should be handled. Also, reintroduce the allocation failure check for rt5645->eq_param as well. Make all
Now I've looked at the patch I don't think it's appropriate for stable, it's essentially equivalent to a patch that adds -Werror - the changes in it are upgrading things from error messages that would be generated by what are essentially static checks (even though we do do them at runtime they're on hard coded strings) to probe failures which would be a regression. Unfortunately people do ignore warnings like that in shipping stuff so it's possible it's happening, we could do an audit to see if it is but it seems like more effort than it's worth.
The only case I can think where it might help is if we're managing to OOM during probe() but that feels very unlikely to happen, and improved handling unlikely to make substantial difference compared to the risk that the routing warnings are triggering but being ignored so someone's sound stops working due to a stable update. Otherwise it won't do much so why risk it?
On Tue, May 25, 2021 at 10:49:44PM +0100, Mark Brown wrote:
On Mon, May 24, 2021 at 10:47:11AM -0400, Sasha Levin wrote:
From: Phillip Potter phil@philpotter.co.uk
[ Upstream commit 5e70b8e22b64eed13d5bbebcb5911dae65bf8c6b ]
Check for return value from various snd_soc_dapm_* calls, as many of them can return errors and this should be handled. Also, reintroduce the allocation failure check for rt5645->eq_param as well. Make all
Now I've looked at the patch I don't think it's appropriate for stable, it's essentially equivalent to a patch that adds -Werror
- the changes in it are upgrading things from error messages that
would be generated by what are essentially static checks (even though we do do them at runtime they're on hard coded strings) to probe failures which would be a regression. Unfortunately people do ignore warnings like that in shipping stuff so it's possible it's happening, we could do an audit to see if it is but it seems like more effort than it's worth.
The only case I can think where it might help is if we're managing to OOM during probe() but that feels very unlikely to happen, and improved handling unlikely to make substantial difference compared to the risk that the routing warnings are triggering but being ignored so someone's sound stops working due to a stable update. Otherwise it won't do much so why risk it?
Dear Mark,
So I frankly don't have the experience to disagree with you :-) Your reasoning certainly seems sound to me. My original motivation for the patch (after discussion with others within the mentorship process) was that some other sound SoC drivers do this, an example being the Ux500. I defer to the decision of the community as a whole of course, and am happy with whatever is decided.
Regards, Phil
On Tue, May 25, 2021 at 11:15:17PM +0100, Phillip Potter wrote:
On Tue, May 25, 2021 at 10:49:44PM +0100, Mark Brown wrote:
Now I've looked at the patch I don't think it's appropriate for stable, it's essentially equivalent to a patch that adds -Werror
So I frankly don't have the experience to disagree with you :-) Your reasoning certainly seems sound to me. My original motivation for the patch (after discussion with others within the mentorship process) was that some other sound SoC drivers do this, an example being the Ux500. I defer to the decision of the community as a whole of course, and am happy with whatever is decided.
Right, so there's multiple bits here - there's checking at all, there's adding the checks to mainline and there's backporting them to stable. For stable we want to be fairly conservative about what we're backporting since we want people to be able to just update without worrying about things breaking on them so something that increases the severity of existing checks is particularly risky, if the code were already there and people would've seen any issues it causes when integrating the kernel it's a different story since the risks are different.
participants (3)
-
Mark Brown -
Phillip Potter -
Sasha Levin