Hi,
Static analysis with Coverity has detected an uninitialized value being
used in a comparison. The error was detected on a recent change to
drivers/staging/greybus/audio_topology.c however the issue actually
dates back to the original commit:
commit 6339d2322c47f4b8ebabf9daf0130328ed72648b
Author: Vaibhav Agarwal <vaibhav.agarwal(a)linaro.org>
Date: Wed Jan 13 14:07:51 2016 -0700
greybus: audio: Add topology parser for GB codec
The analysis is as follows:
425 static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
426 struct snd_ctl_elem_value
*ucontrol)
427 {
428 int ret, wi, max, connect;
429 unsigned int mask, val;
430 struct gb_audio_ctl_elem_info *info;
431 struct gbaudio_ctl_pvt *data;
1. var_decl: Declaring variable gbvalue without initializer.
432 struct gb_audio_ctl_elem_value gbvalue;
433 struct gbaudio_module_info *module;
434 struct snd_soc_dapm_widget_list *wlist =
snd_kcontrol_chip(kcontrol);
435 struct snd_soc_dapm_widget *widget = wlist->widgets[0];
436 struct device *codec_dev = widget->dapm->dev;
437 struct gbaudio_codec_info *gb = dev_get_drvdata(codec_dev);
438 struct gb_bundle *bundle;
439
2. Condition 0 /* __builtin_types_compatible_p() */, taking false branch.
3. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
4. Falling through to end of if statement.
5. Condition !!branch, taking false branch.
6. Condition ({...; !!branch;}), taking false branch.
440 dev_dbg(codec_dev, "Entered %s:%s\n", __func__,
kcontrol->id.name);
441 module = find_gb_module(gb, kcontrol->id.name);
7. Condition !module, taking false branch.
442 if (!module)
443 return -EINVAL;
444
445 data = (struct gbaudio_ctl_pvt *)kcontrol->private_value;
446 info = (struct gb_audio_ctl_elem_info *)data->info;
8. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
447 bundle = to_gb_bundle(module->dev);
448
9. Condition data->vcount == 2, taking true branch.
449 if (data->vcount == 2)
450 dev_warn(widget->dapm->dev,
451 "GB: Control '%s' is stereo, which is not
supported\n",
452 kcontrol->id.name);
453
454 max = le32_to_cpu(info->value.integer.max);
455 mask = (1 << fls(max)) - 1;
456 val = ucontrol->value.integer.value[0] & mask;
10. Condition !!val, taking true branch.
457 connect = !!val;
458
459 /* update ucontrol */
Uninitialized scalar variable (UNINIT)
11. uninit_use: Using uninitialized value gbvalue.value.integer_value[0].
460 if (gbvalue.value.integer_value[0] != val) {
The gbvalue.value.integer_value[0] read is bogus since gbvalue was
declared on the stack but was not initialized. There seems to be no
where that sets this data. I'm assuming most of the time that the
comparison works because the garbage value is different from val and so
the code in the if stanza is executed.
Anyhow, I'm unsure what the original intent of the code was, so I've not
attempted to fix this.
Colin
The existing GB Audio codec driver is dependent on MSM8994 Audio driver.
During the development stage, this dependency was configured due to
various changes involved in MSM Audio driver to enable addtional codec
card and some of the changes proposed in mainline ASoC framework.
However, these are not the real dependencies and some of them can be
easily removed.
The folowing patch series includes the changes to resolve unnecessary
depedencies and make the codec driver functional with the latest kernel.
Patch 1,2: Incudes jack framework related changes.
Patch 3,4,5: Resolves compilation error observed with the latest kernel and
also provides helper APIs required to allow dynamic addition/removal of
modules.
Patch 6: Finally provides config options and related Makefile changes to
enable GB Codec driver.
Thanks to Alexandre for raising the headsup [1] and motivating me to provide
the necessary changes.
This patchset is intended to resolve the componentization issue only.
And as per the suggestion [2] from Mark, I'll share a separate patch series
aligned to ASoC tree. Once the relevant changes are accepted in snd-soc
framework, I'll share relevant patches to pull GB Audio out of the
staging tree.
v1:
- Include the changes for the review comments suggested by Dan
- Rebase to latest staging-next
v2:
- Avoid defining unused 'update' pointer
- Fix the missing connect bool value required during mixer_update_power
- Added Reviewed-by tag from Dan
- Rebase to latest staging-next
v3:
- Fix the warnings reported by lkp[3]
- Rebase to latest staging-next
[1] https://lore.kernel.org/lkml/20200507212912.599433-1-alexandre.belloni@boot…
[2] https://lore.kernel.org/alsa-devel/20200612160620.GK5396@sirena.org.uk/
[3] https://lore.kernel.org/lkml/202006200150.mNVj1Duq%25lkp@intel.com/
Vaibhav Agarwal (7):
staging: greybus: audio: Update snd_jack FW usage as per new APIs
staging: greybus: audio: Maintain jack list within GB Audio module
staging: greybus: audio: Resolve compilation errors for GB codec
module
staging: greybus: audio: Resolve compilation error in topology parser
staging: greybus: audio: Add helper APIs for dynamic audio modules
staging: greybus: audio: Enable GB codec, audio module compilation.
drivers: staging: audio: Fix the missing header file for helper file
drivers/staging/greybus/Kconfig | 14 +-
drivers/staging/greybus/Makefile | 6 +-
drivers/staging/greybus/audio_codec.c | 178 +++++++++++---------
drivers/staging/greybus/audio_codec.h | 12 +-
drivers/staging/greybus/audio_helper.c | 198 +++++++++++++++++++++++
drivers/staging/greybus/audio_helper.h | 17 ++
drivers/staging/greybus/audio_module.c | 15 +-
drivers/staging/greybus/audio_topology.c | 131 +++++++--------
8 files changed, 414 insertions(+), 157 deletions(-)
create mode 100644 drivers/staging/greybus/audio_helper.c
create mode 100644 drivers/staging/greybus/audio_helper.h
base-commit: 14442181d20490945f341644bb8257e334b01447
--
2.27.0
The "err" variable is not meaningful so there is no need to print it.
It's uninitialized on the first iteration through the loop.
Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules")
Signed-off-by: Dan Carpenter <dan.carpenter(a)oracle.com>
---
drivers/staging/greybus/audio_helper.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
index 8b100a71f02e..237531ba60f3 100644
--- a/drivers/staging/greybus/audio_helper.c
+++ b/drivers/staging/greybus/audio_helper.c
@@ -173,8 +173,7 @@ static int gbaudio_remove_controls(struct snd_card *card, struct device *dev,
id.index = control->index;
kctl = snd_ctl_find_id(card, &id);
if (!kctl) {
- dev_err(dev, "%d: Failed to find %s\n", err,
- control->name);
+ dev_err(dev, "Failed to find %s\n", control->name);
continue;
}
err = snd_ctl_remove(card, kctl);
--
2.27.0
Hi list,
I made a PR to GitHub before realizing that it's probably preferred to send
a patch to the ML.
https://github.com/projectara/manifesto/pull/1
Patch is attached as well.
Is anyone able to merge the PR or patch?
Thanks,
C
The existing GB Audio codec driver is dependent on MSM8994 Audio driver.
During the development stage, this dependency was configured due to
various changes involved in MSM Audio driver to enable addtional codec
card and some of the changes proposed in mainline ASoC framework.
However, these are not the real dependencies and some of them can be
easily removed.
The folowing patch series includes the changes to resolve unnecessary
depedencies and make the codec driver functional with the latest kernel.
Patch 1,2: Incudes jack framework related changes.
Patch 3,4,5: Resolves compilation error observed with the latest kernel and
also provides helper APIs required to allow dynamic addition/removal of
modules.
Patch 6: Finally provides config options and related Makefile changes to
enable GB Codec driver.
Thanks to Alexandre for raising the headsup [1] and motivating me to provide
the necessary changes.
This patchset is intended to resolve the componentization issue only.
And as per the suggestion [2] from Mark, I'll share a separate patch series
aligned to ASoC tree. Once the relevant changes are accepted in snd-soc
framework, I'll share relevant patches to pull GB Audio out of the
staging tree.
[1] https://lore.kernel.org/lkml/20200507212912.599433-1-alexandre.belloni@boot…
[2] https://lore.kernel.org/alsa-devel/20200612160620.GK5396@sirena.org.uk/
v1:
- Include the changes for the review comments suggested by Dan
- Rebase to latest staging-next
v2:
- Avoid defining unused 'update' pointer
- Fix the missing connect bool value required during mixer_update_power
- Added Reviewed-by tag from Dan
- Rebase to latest staging-next
Vaibhav Agarwal (6):
staging: greybus: audio: Update snd_jack FW usage as per new APIs
staging: greybus: audio: Maintain jack list within GB Audio module
staging: greybus: audio: Resolve compilation errors for GB codec
module
staging: greybus: audio: Resolve compilation error in topology parser
staging: greybus: audio: Add helper APIs for dynamic audio modules
staging: greybus: audio: Enable GB codec, audio module compilation.
drivers/staging/greybus/Kconfig | 14 +-
drivers/staging/greybus/Makefile | 6 +-
drivers/staging/greybus/audio_codec.c | 178 +++++++++++---------
drivers/staging/greybus/audio_codec.h | 12 +-
drivers/staging/greybus/audio_helper.c | 197 +++++++++++++++++++++++
drivers/staging/greybus/audio_helper.h | 17 ++
drivers/staging/greybus/audio_module.c | 15 +-
drivers/staging/greybus/audio_topology.c | 123 +++++++-------
8 files changed, 409 insertions(+), 153 deletions(-)
create mode 100644 drivers/staging/greybus/audio_helper.c
create mode 100644 drivers/staging/greybus/audio_helper.h
base-commit: 98fe05e21a6e0ca242e974650ed58b64813cb2dc
--
2.26.2
From: Zenyu Sy <zenyu(a)tuta.io>
Fix typos ("an" -> "a", "chose" -> "choose") in Kconfig
Signed-off-by: Zenyu Sy <zenyu(a)tuta.io>
---
base-commit: f5e94d10e4c468357019e5c28d48499f677b284f
Fixed the white-space formatting problem.
drivers/greybus/Kconfig | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
index b84fcaf8b..aeea082f1 100644
--- a/drivers/greybus/Kconfig
+++ b/drivers/greybus/Kconfig
@@ -3,7 +3,7 @@ menuconfig GREYBUS
tristate "Greybus support"
depends on SYSFS
---help---
- This option enables the Greybus driver core. Greybus is an
+ This option enables the Greybus driver core. Greybus is a
hardware protocol that was designed to provide Unipro with a
sane application layer. It was originally designed for the
ARA project, a module phone system, but has shown up in other
@@ -12,7 +12,7 @@ menuconfig GREYBUS
Say Y here to enable support for these types of drivers.
- To compile this code as a module, chose M here: the module
+ To compile this code as a module, choose M here: the module
will be called greybus.ko
if GREYBUS
@@ -25,7 +25,7 @@ config GREYBUS_ES2
acts as a Greybus "host controller". This device is a bridge
from a USB device to a Unipro network.
- To compile this code as a module, chose M here: the module
+ To compile this code as a module, choose M here: the module
will be called gb-es2.ko
endif # GREYBUS
--
2.26.1
From: Ulf Hansson <ulf.hansson(a)linaro.org>
[ Upstream commit a389087ee9f195fcf2f31cd771e9ec5f02c16650 ]
Using a fixed 1s timeout for all commands is a bit problematic.
For some commands it means waiting longer than needed for the timeout to
expire, which may not a big issue, but still. For other commands, like for
an erase (CMD38) that uses a R1B response, may require longer timeouts than
1s. In these cases, we may end up treating the command as it failed, while
it just needed some more time to complete successfully.
Fix the problem by respecting the cmd->busy_timeout, which is provided by
the mmc core.
Cc: Rui Miguel Silva <rmfrfs(a)gmail.com>
Cc: Johan Hovold <johan(a)kernel.org>
Cc: Alex Elder <elder(a)kernel.org>
Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
Cc: greybus-dev(a)lists.linaro.org
Signed-off-by: Ulf Hansson <ulf.hansson(a)linaro.org>
Acked-by: Rui Miguel Silva <rmfrfs(a)gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>
Link: https://lore.kernel.org/r/20200414161413.3036-20-ulf.hansson@linaro.org
Signed-off-by: Ulf Hansson <ulf.hansson(a)linaro.org>
Signed-off-by: Sasha Levin <sashal(a)kernel.org>
---
drivers/staging/greybus/sdio.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/greybus/sdio.c b/drivers/staging/greybus/sdio.c
index 5649ef1e379d..82a1c2cf6687 100644
--- a/drivers/staging/greybus/sdio.c
+++ b/drivers/staging/greybus/sdio.c
@@ -413,6 +413,7 @@ static int gb_sdio_command(struct gb_sdio_host *host, struct mmc_command *cmd)
struct gb_sdio_command_request request = {0};
struct gb_sdio_command_response response;
struct mmc_data *data = host->mrq->data;
+ unsigned int timeout_ms;
u8 cmd_flags;
u8 cmd_type;
int i;
@@ -471,9 +472,12 @@ static int gb_sdio_command(struct gb_sdio_host *host, struct mmc_command *cmd)
request.data_blksz = cpu_to_le16(data->blksz);
}
- ret = gb_operation_sync(host->connection, GB_SDIO_TYPE_COMMAND,
- &request, sizeof(request), &response,
- sizeof(response));
+ timeout_ms = cmd->busy_timeout ? cmd->busy_timeout :
+ GB_OPERATION_TIMEOUT_DEFAULT;
+
+ ret = gb_operation_sync_timeout(host->connection, GB_SDIO_TYPE_COMMAND,
+ &request, sizeof(request), &response,
+ sizeof(response), timeout_ms);
if (ret < 0)
goto out;
--
2.25.1