[alsa-devel] [PATCH 0/3] ASoC: da7218: Adjustments for two function implementations
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 21:00:12 +0100
Three update suggestions were taken into account from static source code analysis.
Markus Elfring (3): Delete two error messages for a failed memory allocation in da7218_of_to_pdata() Use common error handling code in da7218_of_to_pdata() Improve a size determination in da7218_i2c_probe()
sound/soc/codecs/da7218.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 20:42:20 +0100
Omit extra messages for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- sound/soc/codecs/da7218.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c index 56564ce90cb6..25ab7443d803 100644 --- a/sound/soc/codecs/da7218.c +++ b/sound/soc/codecs/da7218.c @@ -2455,10 +2455,8 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec) u32 of_val32;
pdata = devm_kzalloc(codec->dev, sizeof(*pdata), GFP_KERNEL); - if (!pdata) { - dev_warn(codec->dev, "Failed to allocate memory for pdata\n"); + if (!pdata) return NULL; - }
if (of_property_read_u32(np, "dlg,micbias1-lvl-millivolt", &of_val32) >= 0) pdata->micbias1_lvl = da7218_of_micbias_lvl(codec, of_val32); @@ -2527,8 +2525,6 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec) hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata), GFP_KERNEL); if (!hpldet_pdata) { - dev_warn(codec->dev, - "Failed to allocate memory for hpldet pdata\n"); of_node_put(hpldet_np); return pdata; }
On 23 November 2017 20:05, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 20:42:20 +0100
Omit extra messages for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
Acked-by: Adam Thomson Adam.Thomson.Opensource@diasemi.com
sound/soc/codecs/da7218.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c index 56564ce90cb6..25ab7443d803 100644 --- a/sound/soc/codecs/da7218.c +++ b/sound/soc/codecs/da7218.c @@ -2455,10 +2455,8 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec) u32 of_val32;
pdata = devm_kzalloc(codec->dev, sizeof(*pdata), GFP_KERNEL);
- if (!pdata) {
dev_warn(codec->dev, "Failed to allocate memory for pdata\n");
- if (!pdata) return NULL;
}
if (of_property_read_u32(np, "dlg,micbias1-lvl-millivolt", &of_val32) >= 0) pdata->micbias1_lvl = da7218_of_micbias_lvl(codec, of_val32);
@@ -2527,8 +2525,6 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec) hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata), GFP_KERNEL); if (!hpldet_pdata) {
dev_warn(codec->dev,"Failed to allocate memory for hpldet pdata\n"); of_node_put(hpldet_np); return pdata;-- 2.15.0
The patch
ASoC: da7218: Delete two error messages for a failed memory allocation in da7218_of_to_pdata()
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 14a07f1d8c4c64af29566316df0415052e8bdfe4 Mon Sep 17 00:00:00 2001
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 20:42:20 +0100 Subject: [PATCH] ASoC: da7218: Delete two error messages for a failed memory allocation in da7218_of_to_pdata()
Omit extra messages for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net Acked-by: Adam Thomson Adam.Thomson.Opensource@diasemi.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/da7218.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c index 56564ce90cb6..25ab7443d803 100644 --- a/sound/soc/codecs/da7218.c +++ b/sound/soc/codecs/da7218.c @@ -2455,10 +2455,8 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec) u32 of_val32;
pdata = devm_kzalloc(codec->dev, sizeof(*pdata), GFP_KERNEL); - if (!pdata) { - dev_warn(codec->dev, "Failed to allocate memory for pdata\n"); + if (!pdata) return NULL; - }
if (of_property_read_u32(np, "dlg,micbias1-lvl-millivolt", &of_val32) >= 0) pdata->micbias1_lvl = da7218_of_micbias_lvl(codec, of_val32); @@ -2527,8 +2525,6 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec) hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata), GFP_KERNEL); if (!hpldet_pdata) { - dev_warn(codec->dev, - "Failed to allocate memory for hpldet pdata\n"); of_node_put(hpldet_np); return pdata; }
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 20:48:05 +0100
Add a jump target so that a bit of exception handling can be better reused in an if branch of this function.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- sound/soc/codecs/da7218.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c index 25ab7443d803..93a0cb741751 100644 --- a/sound/soc/codecs/da7218.c +++ b/sound/soc/codecs/da7218.c @@ -2524,10 +2524,9 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec)
hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata), GFP_KERNEL); - if (!hpldet_pdata) { - of_node_put(hpldet_np); - return pdata; - } + if (!hpldet_pdata) + goto put_node; + pdata->hpldet_pdata = hpldet_pdata;
if (of_property_read_u32(hpldet_np, "dlg,jack-rate-us", @@ -2561,6 +2560,7 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec) if (of_property_read_bool(hpldet_np, "dlg,discharge")) hpldet_pdata->discharge = true;
+put_node: of_node_put(hpldet_np); }
On 23 November 2017 20:06, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 20:48:05 +0100
Add a jump target so that a bit of exception handling can be better reused in an if branch of this function.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
Hmm. Doesn't really gain an awful lot this. Would understand if there were multiple return paths, but in that case I'd have implemented something like this anyway. Also your patch description isn't really correct. You're re-using code from the sunny day scenario to handle an exception.
sound/soc/codecs/da7218.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c index 25ab7443d803..93a0cb741751 100644 --- a/sound/soc/codecs/da7218.c +++ b/sound/soc/codecs/da7218.c @@ -2524,10 +2524,9 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec)
hpldet_pdata = devm_kzalloc(codec->dev, sizeof(*hpldet_pdata), GFP_KERNEL);
if (!hpldet_pdata) {of_node_put(hpldet_np);return pdata;}
if (!hpldet_pdata)goto put_node;pdata->hpldet_pdata = hpldet_pdata;
if (of_property_read_u32(hpldet_np, "dlg,jack-rate-us",
@@ -2561,6 +2560,7 @@ static struct da7218_pdata *da7218_of_to_pdata(struct snd_soc_codec *codec) if (of_property_read_bool(hpldet_np, "dlg,discharge")) hpldet_pdata->discharge = true;
+put_node: of_node_put(hpldet_np); }
-- 2.15.0
Add a jump target so that a bit of exception handling can be better reused in an if branch of this function.
…
Hmm. Doesn't really gain an awful lot this.
I show just another small change possibility.
Would understand if there were multiple return paths, but in that case I'd have implemented something like this anyway.
Where?
Can the suggested software refactoring become useful also for this function implementation?
Also your patch description isn't really correct.
Which wording would you find more appropriate?
You're re-using code from the sunny day scenario to handle an exception.
Can this detail be better?
Regards, Markus
On Fri, Nov 24, 2017 at 12:03:32PM +0000, Adam Thomson wrote:
On 23 November 2017 20:06, SF Markus Elfring wrote:
Add a jump target so that a bit of exception handling can be better reused in an if branch of this function.
Hmm. Doesn't really gain an awful lot this. Would understand if there were multiple return paths, but in that case I'd have implemented something like this anyway. Also your patch description isn't really correct. You're re-using code from the sunny day scenario to handle an exception.
Other people have given him similar feedback on his other patches with this pattern. I'm not intending to apply any of them.
Add a jump target so that a bit of exception handling can be better reused in an if branch of this function.
Hmm. Doesn't really gain an awful lot this. Would understand if there were multiple return paths, but in that case I'd have implemented something like this anyway. Also your patch description isn't really correct. You're re-using code from the sunny day scenario to handle an exception.
Other people have given him similar feedback on his other patches with this pattern. I'm not intending to apply any of them.
I can offer another bit of information for a software development discussion. 💭
The affected source file can be compiled for the processor architecture “x86_64” by a tool like “GCC 6.4.1+r251631-1.3” from the software distribution “openSUSE Tumbleweed” with the following command example.
my_cc=/usr/bin/gcc-6 \ && my_module=sound/soc/codecs/da7218.o \ && for XYZ in 0 s 3; do echo " _____ $XYZ _____" \ && my_extra="-O$XYZ" \ && git checkout ':/^ASoC: da7218: Delete two error messages for a failed memory allocation in da7218_of_to_pdata' \ && make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS="${my_extra}" allmodconfig "${my_module}" \ && size "${my_module}" \ && git checkout ':/^ASoC: da7218: Use common error handling code in da7218_of_to_pdata' \ && make -j4 CC="${my_cc}" HOSTCC="${my_cc}" EXTRA_CFLAGS="${my_extra}" allmodconfig "${my_module}" \ && size "${my_module}"; done
🔮 Do you find the following differences worth for further clarification?
╔═════════╤══════╗ ║ setting │ text ║ ╠═════════╪══════╣ ║ O0 │ -25 ║ ║ Os │ -14 ║ ║ O3 │ +11 ║ ╚═════════╧══════╝
Regards, Markus
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 20:50:44 +0100
Replace the specification of a data structure by a pointer dereference as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net --- sound/soc/codecs/da7218.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c index 93a0cb741751..0c1933a98995 100644 --- a/sound/soc/codecs/da7218.c +++ b/sound/soc/codecs/da7218.c @@ -3269,8 +3269,7 @@ static int da7218_i2c_probe(struct i2c_client *i2c, struct da7218_priv *da7218; int ret;
- da7218 = devm_kzalloc(&i2c->dev, sizeof(struct da7218_priv), - GFP_KERNEL); + da7218 = devm_kzalloc(&i2c->dev, sizeof(*da7218), GFP_KERNEL); if (!da7218) return -ENOMEM;
On 23 November 2017 20:08, SF Markus Elfring wrote:
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 20:50:44 +0100
Replace the specification of a data structure by a pointer dereference as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net
Acked-by: Adam Thomson Adam.Thomson.Opensource@diasemi.com
sound/soc/codecs/da7218.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c index 93a0cb741751..0c1933a98995 100644 --- a/sound/soc/codecs/da7218.c +++ b/sound/soc/codecs/da7218.c @@ -3269,8 +3269,7 @@ static int da7218_i2c_probe(struct i2c_client *i2c, struct da7218_priv *da7218; int ret;
- da7218 = devm_kzalloc(&i2c->dev, sizeof(struct da7218_priv),
GFP_KERNEL);
- da7218 = devm_kzalloc(&i2c->dev, sizeof(*da7218), GFP_KERNEL); if (!da7218) return -ENOMEM;
-- 2.15.0
The patch
ASoC: da7218: Improve a size determination in da7218_i2c_probe()
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 392b79e20b41cfdc174d31bd4b004bbd874de4d9 Mon Sep 17 00:00:00 2001
From: Markus Elfring elfring@users.sourceforge.net Date: Thu, 23 Nov 2017 20:50:44 +0100 Subject: [PATCH] ASoC: da7218: Improve a size determination in da7218_i2c_probe()
Replace the specification of a data structure by a pointer dereference as the parameter for the operator "sizeof" to make the corresponding size determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring elfring@users.sourceforge.net Acked-by: Adam Thomson Adam.Thomson.Opensource@diasemi.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/da7218.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/codecs/da7218.c b/sound/soc/codecs/da7218.c index 25ab7443d803..96c644a15b11 100644 --- a/sound/soc/codecs/da7218.c +++ b/sound/soc/codecs/da7218.c @@ -3269,8 +3269,7 @@ static int da7218_i2c_probe(struct i2c_client *i2c, struct da7218_priv *da7218; int ret;
- da7218 = devm_kzalloc(&i2c->dev, sizeof(struct da7218_priv), - GFP_KERNEL); + da7218 = devm_kzalloc(&i2c->dev, sizeof(*da7218), GFP_KERNEL); if (!da7218) return -ENOMEM;
participants (3)
-
Adam Thomson -
Mark Brown -
SF Markus Elfring