[PATCH AUTOSEL 5.12 32/63] ASoC: cs43130: handle errors in cs43130_probe() properly
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
[ Upstream commit 2da441a6491d93eff8ffff523837fd621dc80389 ]
cs43130_probe() does not do any valid error checking of things it initializes, OR what it does, it does not unwind properly if there are errors.
Fix this up by moving the sysfs files to an attribute group so the driver core will correctly add/remove them all at once and handle errors with them, and correctly check for creating a new workqueue and unwinding if that fails.
Cc: Mark Brown broonie@kernel.org Link: https://lore.kernel.org/r/20210503115736.2104747-58-gregkh@linuxfoundation.o... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Sasha Levin sashal@kernel.org --- sound/soc/codecs/cs43130.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c index c2b6f0ae6d57..80cd3ea0c157 100644 --- a/sound/soc/codecs/cs43130.c +++ b/sound/soc/codecs/cs43130.c @@ -1735,6 +1735,14 @@ static DEVICE_ATTR(hpload_dc_r, 0444, cs43130_show_dc_r, NULL); static DEVICE_ATTR(hpload_ac_l, 0444, cs43130_show_ac_l, NULL); static DEVICE_ATTR(hpload_ac_r, 0444, cs43130_show_ac_r, NULL);
+static struct attribute *hpload_attrs[] = { + &dev_attr_hpload_dc_l.attr, + &dev_attr_hpload_dc_r.attr, + &dev_attr_hpload_ac_l.attr, + &dev_attr_hpload_ac_r.attr, +}; +ATTRIBUTE_GROUPS(hpload); + static struct reg_sequence hp_en_cal_seq[] = { {CS43130_INT_MASK_4, CS43130_INT_MASK_ALL}, {CS43130_HP_MEAS_LOAD_1, 0}, @@ -2302,23 +2310,15 @@ static int cs43130_probe(struct snd_soc_component *component)
cs43130->hpload_done = false; if (cs43130->dc_meas) { - ret = device_create_file(component->dev, &dev_attr_hpload_dc_l); - if (ret < 0) - return ret; - - ret = device_create_file(component->dev, &dev_attr_hpload_dc_r); - if (ret < 0) - return ret; - - ret = device_create_file(component->dev, &dev_attr_hpload_ac_l); - if (ret < 0) - return ret; - - ret = device_create_file(component->dev, &dev_attr_hpload_ac_r); - if (ret < 0) + ret = sysfs_create_groups(&component->dev->kobj, hpload_groups); + if (ret) return ret;
cs43130->wq = create_singlethread_workqueue("cs43130_hp"); + if (!cs43130->wq) { + sysfs_remove_groups(&component->dev->kobj, hpload_groups); + return -ENOMEM; + } INIT_WORK(&cs43130->work, cs43130_imp_meas); }
On Mon, May 24, 2021 at 10:45:49AM -0400, Sasha Levin wrote:
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
[ Upstream commit 2da441a6491d93eff8ffff523837fd621dc80389 ]
cs43130_probe() does not do any valid error checking of things it initializes, OR what it does, it does not unwind properly if there are errors.
I don't have this commit and can't see any sign of it having been submitted upstream. Where is it being backported from? The last commit I can see in -next to this driver is d2912cb15bdda8ba4a5dd73396ad62641af2f520 (treewide: Replace GPLv2 boilerplate/reference with SPDX - rule 500) from 2019.
Fix this up by moving the sysfs files to an attribute group so the driver core will correctly add/remove them all at once and handle errors with them, and correctly check for creating a new workqueue and unwinding if that fails.
Cc: Mark Brown broonie@kernel.org Link: https://lore.kernel.org/r/20210503115736.2104747-58-gregkh@linuxfoundation.o... Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Sasha Levin sashal@kernel.org
sound/soc/codecs/cs43130.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c index c2b6f0ae6d57..80cd3ea0c157 100644 --- a/sound/soc/codecs/cs43130.c +++ b/sound/soc/codecs/cs43130.c @@ -1735,6 +1735,14 @@ static DEVICE_ATTR(hpload_dc_r, 0444, cs43130_show_dc_r, NULL); static DEVICE_ATTR(hpload_ac_l, 0444, cs43130_show_ac_l, NULL); static DEVICE_ATTR(hpload_ac_r, 0444, cs43130_show_ac_r, NULL);
+static struct attribute *hpload_attrs[] = {
- &dev_attr_hpload_dc_l.attr,
- &dev_attr_hpload_dc_r.attr,
- &dev_attr_hpload_ac_l.attr,
- &dev_attr_hpload_ac_r.attr,
+}; +ATTRIBUTE_GROUPS(hpload);
static struct reg_sequence hp_en_cal_seq[] = { {CS43130_INT_MASK_4, CS43130_INT_MASK_ALL}, {CS43130_HP_MEAS_LOAD_1, 0}, @@ -2302,23 +2310,15 @@ static int cs43130_probe(struct snd_soc_component *component)
cs43130->hpload_done = false; if (cs43130->dc_meas) {
ret = device_create_file(component->dev, &dev_attr_hpload_dc_l);if (ret < 0)return ret;ret = device_create_file(component->dev, &dev_attr_hpload_dc_r);if (ret < 0)return ret;ret = device_create_file(component->dev, &dev_attr_hpload_ac_l);if (ret < 0)return ret;ret = device_create_file(component->dev, &dev_attr_hpload_ac_r);if (ret < 0)
ret = sysfs_create_groups(&component->dev->kobj, hpload_groups);if (ret) return ret;cs43130->wq = create_singlethread_workqueue("cs43130_hp");
if (!cs43130->wq) {sysfs_remove_groups(&component->dev->kobj, hpload_groups);return -ENOMEM;}INIT_WORK(&cs43130->work, cs43130_imp_meas); }
-- 2.30.2
On Tue, May 25, 2021 at 03:00:28PM +0100, Mark Brown wrote:
On Mon, May 24, 2021 at 10:45:49AM -0400, Sasha Levin wrote:
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
[ Upstream commit 2da441a6491d93eff8ffff523837fd621dc80389 ]
cs43130_probe() does not do any valid error checking of things it initializes, OR what it does, it does not unwind properly if there are errors.
I don't have this commit and can't see any sign of it having been submitted upstream. Where is it being backported from? The last commit I can see in -next to this driver is d2912cb15bdda8ba4a5dd73396ad62641af2f520 (treewide: Replace GPLv2 boilerplate/reference with SPDX - rule 500) from 2019.
This is now in 5.13-rc3.
You should have been cc:ed on it a few times already.
thanks,
greg k-h
On Tue, May 25, 2021 at 04:43:52PM +0200, Greg Kroah-Hartman wrote:
On Tue, May 25, 2021 at 03:00:28PM +0100, Mark Brown wrote:
On Mon, May 24, 2021 at 10:45:49AM -0400, Sasha Levin wrote:
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
[ Upstream commit 2da441a6491d93eff8ffff523837fd621dc80389 ]
This is now in 5.13-rc3.
You should have been cc:ed on it a few times already.
Hrm, I've managed to find a *single* copy mixed in with a revert as part of a huge series (it was almost 70 patches) with no cover letter that got copied to me - I think what happened here is that this looked like this was something where you'd done a revert and then dropped that revert (which was what things I'd heard from other sources suggested was what was going on with that series). I'd certainly have expected to get a standalone patch submission or other communication for something that was entirely new code, and if you're not getting review for new code like this that isn't super urgent I'd expect some attempts to get it before bypassing.
This sort of stuff is not great, especially when half of what you were doing was to address bad practice on the part of the UMN people - I would have really expected any completely new changes like these that came up to be sent as new patches through the normal process rather than mixed in with what look like mechanical, treewide changes. It's a recipe for things getting missed, as I said in followup to the copy of the patch I found there's some issues with the rt5645 changes. On rechecking everything the only issue I actually spotted with any of that code (use of devm at the component level, which realistically is at worst very minor) is not fixed by the additional patch.
participants (3)
-
Greg Kroah-Hartman -
Mark Brown -
Sasha Levin