[v1] sound: fix uninit-value in i2s_dma_isr

[PATCH] sound: fix uninit-value in i2s_dma_isr

Posted by Suraj Sonawane 3 weeks, 4 days ago

Fix an issue detected by the Smatch tool:

sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
error: uninitialized symbol 'val_1'.
sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
error: uninitialized symbol 'val_2'.

These errors occurred because the variables 'val_1' and 'val_2' are
declared but may not be assigned a value before they are used.
Specifically, if the loop that assigns values to 'val_1' and 'val_2'
does not execute (for example, when 'offlevel' is zero), these
variables remain uninitialized, leading to potential undefined
behavior.

To resolve this issue, initialize 'val_1' and 'val_2' to 0 at the
point of declaration. This ensures that 'val_1' and 'val_2' have
defined values before they are used in subsequent calculations,
preventing any warnings or undefined behavior in cases where the
loop does not run.

Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
---
 sound/soc/bcm/bcm63xx-pcm-whistler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/bcm/bcm63xx-pcm-whistler.c b/sound/soc/bcm/bcm63xx-pcm-whistler.c
index 018f2372e..5c23c228e 100644
--- a/sound/soc/bcm/bcm63xx-pcm-whistler.c
+++ b/sound/soc/bcm/bcm63xx-pcm-whistler.c
@@ -232,7 +232,7 @@ static int bcm63xx_pcm_close(struct snd_soc_component *component,
 
 static irqreturn_t i2s_dma_isr(int irq, void *bcm_i2s_priv)
 {
-	unsigned int availdepth, ifflevel, offlevel, int_status, val_1, val_2;
+	unsigned int availdepth, ifflevel, offlevel, int_status, val_1 = 0, val_2 = 0;
 	struct bcm63xx_runtime_data *prtd;
 	struct snd_pcm_substream *substream;
 	struct snd_pcm_runtime *runtime;
-- 
2.34.1

Re: [PATCH] sound: fix uninit-value in i2s_dma_isr

Posted by Mark Brown 3 weeks, 4 days ago

On Wed, Oct 30, 2024 at 10:38:29PM +0530, Suraj Sonawane wrote:
> Fix an issue detected by the Smatch tool:
> 
> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
> error: uninitialized symbol 'val_1'.
> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
> error: uninitialized symbol 'val_2'.
> 
> These errors occurred because the variables 'val_1' and 'val_2' are
> declared but may not be assigned a value before they are used.
> Specifically, if the loop that assigns values to 'val_1' and 'val_2'
> does not execute (for example, when 'offlevel' is zero), these
> variables remain uninitialized, leading to potential undefined
> behavior.
> 
> To resolve this issue, initialize 'val_1' and 'val_2' to 0 at the
> point of declaration. This ensures that 'val_1' and 'val_2' have
> defined values before they are used in subsequent calculations,
> preventing any warnings or undefined behavior in cases where the
> loop does not run.

This will shut the warning up, but why are these values valid?  Are we
handling the cases where the loops do not execute properly?

Re: [PATCH] sound: fix uninit-value in i2s_dma_isr

Posted by Suraj Sonawane 3 weeks, 3 days ago

On 30/10/24 22:44, Mark Brown wrote:
> On Wed, Oct 30, 2024 at 10:38:29PM +0530, Suraj Sonawane wrote:
>> Fix an issue detected by the Smatch tool:
>>
>> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
>> error: uninitialized symbol 'val_1'.
>> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
>> error: uninitialized symbol 'val_2'.
>>
>> These errors occurred because the variables 'val_1' and 'val_2' are
>> declared but may not be assigned a value before they are used.
>> Specifically, if the loop that assigns values to 'val_1' and 'val_2'
>> does not execute (for example, when 'offlevel' is zero), these
>> variables remain uninitialized, leading to potential undefined
>> behavior.
>>
>> To resolve this issue, initialize 'val_1' and 'val_2' to 0 at the
>> point of declaration. This ensures that 'val_1' and 'val_2' have
>> defined values before they are used in subsequent calculations,
>> preventing any warnings or undefined behavior in cases where the
>> loop does not run.
> 
> This will shut the warning up, but why are these values valid?  Are we
> handling the cases where the loops do not execute properly?

Thank you for the feedback and your time.

The uninitialized warning for val_1 and val_2 arises because, in some 
cases, the offlevel value is zero, and as a result, the loop does not 
execute, leaving these variables potentially undefined. The subsequent 
code calculates prtd->dma_addr_next using val_1 + val_2, so it's 
necessary to have val_1 and val_2 initialized to a known value, even 
when the loop does not run.

Initializing them to zero ensures prtd->dma_addr_next has a defined 
value without triggering undefined behavior. However, if a zero 
initialization could cause unintended behavior in dma_addr_next, I could 
alternatively handle this case by setting dma_addr_next conditionally 
when offlevel is non-zero.

Let me know if there’s a preferred approach, or if you'd suggest a 
different initial value for these variables based on the expected use.

Re: [PATCH] sound: fix uninit-value in i2s_dma_isr

Posted by Mark Brown 3 weeks, 3 days ago

On Thu, Oct 31, 2024 at 12:17:56PM +0530, Suraj Sonawane wrote:
> On 30/10/24 22:44, Mark Brown wrote:

> > This will shut the warning up, but why are these values valid?  Are we
> > handling the cases where the loops do not execute properly?

> Thank you for the feedback and your time.

> The uninitialized warning for val_1 and val_2 arises because, in some cases,
> the offlevel value is zero, and as a result, the loop does not execute,
> leaving these variables potentially undefined. The subsequent code
> calculates prtd->dma_addr_next using val_1 + val_2, so it's necessary to
> have val_1 and val_2 initialized to a known value, even when the loop does
> not run.

> Initializing them to zero ensures prtd->dma_addr_next has a defined value
> without triggering undefined behavior. However, if a zero initialization
> could cause unintended behavior in dma_addr_next, I could alternatively
> handle this case by setting dma_addr_next conditionally when offlevel is
> non-zero.

This is describing again what the patch does, which basically just boils
down to shutting up the warning.

> Let me know if there’s a preferred approach, or if you'd suggest a different
> initial value for these variables based on the expected use.

We need to understand what the change is doing - what do the values
mean, is the real issue that we're missing some error handling for the
case that lets us fall through without initialisation?

Re: [PATCH] sound: fix uninit-value in i2s_dma_isr

Posted by Suraj Sonawane 3 weeks, 2 days ago

On 31/10/24 21:47, Mark Brown wrote:
> On Thu, Oct 31, 2024 at 12:17:56PM +0530, Suraj Sonawane wrote:
>> On 30/10/24 22:44, Mark Brown wrote:
> 
>>> This will shut the warning up, but why are these values valid?  Are we
>>> handling the cases where the loops do not execute properly?
> 
>> Thank you for the feedback and your time.
> 
>> The uninitialized warning for val_1 and val_2 arises because, in some cases,
>> the offlevel value is zero, and as a result, the loop does not execute,
>> leaving these variables potentially undefined. The subsequent code
>> calculates prtd->dma_addr_next using val_1 + val_2, so it's necessary to
>> have val_1 and val_2 initialized to a known value, even when the loop does
>> not run.
> 
>> Initializing them to zero ensures prtd->dma_addr_next has a defined value
>> without triggering undefined behavior. However, if a zero initialization
>> could cause unintended behavior in dma_addr_next, I could alternatively
>> handle this case by setting dma_addr_next conditionally when offlevel is
>> non-zero.
> 
> This is describing again what the patch does, which basically just boils
> down to shutting up the warning.
> 
>> Let me know if there’s a preferred approach, or if you'd suggest a different
>> initial value for these variables based on the expected use.
> 
> We need to understand what the change is doing - what do the values
> mean, is the real issue that we're missing some error handling for the
> case that lets us fall through without initialisation?
Thank you for clarifying.

I reviewed the context around val_1 and val_2 in dma_addr_next. These 
values are expected to come from the registers when offlevel is 
non-zero, representing the next DMA address and length information. If 
offlevel is zero, it means there’s no offset data to process, and 
dma_addr_next might not need updating in that case.

A more precise solution would be to conditionally update 
prtd->dma_addr_next only when offlevel is non-zero, as this would 
reflect the intended logic without relying on an arbitrary initialization.

Would it be better to revise the patch to conditionally update 
prtd->dma_addr_next only when offlevel is non-zero?

Let me know if this approach aligns better with the expected behavior.

Thank you again for your time.

Thanks,
Suraj

Re: [PATCH] sound: fix uninit-value in i2s_dma_isr

Posted by Mark Brown 3 weeks, 2 days ago

On Fri, Nov 01, 2024 at 02:32:43PM +0530, Suraj Sonawane wrote:

> I reviewed the context around val_1 and val_2 in dma_addr_next. These values
> are expected to come from the registers when offlevel is non-zero,
> representing the next DMA address and length information. If offlevel is
> zero, it means there’s no offset data to process, and dma_addr_next might
> not need updating in that case.

> A more precise solution would be to conditionally update prtd->dma_addr_next
> only when offlevel is non-zero, as this would reflect the intended logic
> without relying on an arbitrary initialization.

> Would it be better to revise the patch to conditionally update
> prtd->dma_addr_next only when offlevel is non-zero?

> Let me know if this approach aligns better with the expected behavior.

That seems like a reasonable approach, just skip the update when we
didn't read the values.

Re: [PATCH] sound: fix uninit-value in i2s_dma_isr

Posted by Suraj Sonawane 3 weeks, 1 day ago

On 01/11/24 18:45, Mark Brown wrote:
> On Fri, Nov 01, 2024 at 02:32:43PM +0530, Suraj Sonawane wrote:
> 
>> I reviewed the context around val_1 and val_2 in dma_addr_next. These values
>> are expected to come from the registers when offlevel is non-zero,
>> representing the next DMA address and length information. If offlevel is
>> zero, it means there’s no offset data to process, and dma_addr_next might
>> not need updating in that case.
> 
>> A more precise solution would be to conditionally update prtd->dma_addr_next
>> only when offlevel is non-zero, as this would reflect the intended logic
>> without relying on an arbitrary initialization.
> 
>> Would it be better to revise the patch to conditionally update
>> prtd->dma_addr_next only when offlevel is non-zero?
> 
>> Let me know if this approach aligns better with the expected behavior.
> 
> That seems like a reasonable approach, just skip the update when we
> didn't read the values.
Thanks for the feedback.

I've updated the patch based on your suggestion to conditionally update 
prtd->dma_addr_next only when offlevel is non-zero, effectively skipping 
the update if values were not read. This approach aligns with the 
expected logic, as it ensures dma_addr_next is only updated when valid 
data is available, preventing any unintended updates.

I’ve just sent the revised patch. Please let me know if any additional 
adjustments are needed.

Best regards,
Suraj Sonawane

[PATCH v2] sound: fix uninit-value in i2s_dma_isr

Posted by Suraj Sonawane 3 weeks, 1 day ago

Fix an issue detected by the Smatch tool:

sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
error: uninitialized symbol 'val_1'.
sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
error: uninitialized symbol 'val_2'.

These errors were triggered because the variables 'val_1' and 'val_2'
could remain uninitialized if 'offlevel' is zero, meaning the loop
that assigns values to them does not execute. In this case,
'dma_addr_next' would use uninitialized data, potentially leading
to undefined behavior.

To resolve this, a conditional update for 'dma_addr_next' is added,
ensuring it is assigned only when 'val_1' and 'val_2' are read.
A new boolean variable 'val_read' flags when the values have been
retrieved, setting 'dma_addr_next' only if valid data is available.

This solution prevents the use of uninitialized data, maintaining
defined behavior for 'dma_addr_next' in all cases, and aligns with
expected usage of I2S RX descriptor data.

Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
---
V1: Initialize 'val_1' and 'val_2' to 0.
V2: Add conditional update for 'dma_addr_next' based on read status to
skip the update when values haven’t been read.

 sound/soc/bcm/bcm63xx-pcm-whistler.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/soc/bcm/bcm63xx-pcm-whistler.c b/sound/soc/bcm/bcm63xx-pcm-whistler.c
index 018f2372e..e3a4fcc63 100644
--- a/sound/soc/bcm/bcm63xx-pcm-whistler.c
+++ b/sound/soc/bcm/bcm63xx-pcm-whistler.c
@@ -256,12 +256,16 @@ static irqreturn_t i2s_dma_isr(int irq, void *bcm_i2s_priv)
 
 		offlevel = (int_status & I2S_RX_DESC_OFF_LEVEL_MASK) >>
 			   I2S_RX_DESC_OFF_LEVEL_SHIFT;
+		bool val_read = false;
 		while (offlevel) {
 			regmap_read(regmap_i2s, I2S_RX_DESC_OFF_ADDR, &val_1);
 			regmap_read(regmap_i2s, I2S_RX_DESC_OFF_LEN, &val_2);
+			val_read = true;
 			offlevel--;
 		}
-		prtd->dma_addr_next = val_1 + val_2;
+		if (val_read)
+			prtd->dma_addr_next = val_1 + val_2;
+
 		ifflevel = (int_status & I2S_RX_DESC_IFF_LEVEL_MASK) >>
 			   I2S_RX_DESC_IFF_LEVEL_SHIFT;
 
-- 
2.34.1

Re: [PATCH v2] sound: fix uninit-value in i2s_dma_isr

Posted by Mark Brown 2 weeks, 5 days ago

On Sat, 02 Nov 2024 18:06:30 +0530, Suraj Sonawane wrote:
> Fix an issue detected by the Smatch tool:
> 
> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
> error: uninitialized symbol 'val_1'.
> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
> error: uninitialized symbol 'val_2'.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] sound: fix uninit-value in i2s_dma_isr
      commit: 28f7aa0c015036db260adbec37891984a31ed4c2

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

Re: [PATCH v2] sound: fix uninit-value in i2s_dma_isr

Posted by Suraj Sonawane 2 weeks, 4 days ago

On 05/11/24 22:08, Mark Brown wrote:
> On Sat, 02 Nov 2024 18:06:30 +0530, Suraj Sonawane wrote:
>> Fix an issue detected by the Smatch tool:
>>
>> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
>> error: uninitialized symbol 'val_1'.
>> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
>> error: uninitialized symbol 'val_2'.
>>
>> [...]
> 
> Applied to
> 
>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
> 
> Thanks!
> 
> [1/1] sound: fix uninit-value in i2s_dma_isr
>        commit: 28f7aa0c015036db260adbec37891984a31ed4c2
> 
> 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
> 
Thank you so much!
I appreciate the update.
I’ll monitor for any follow-up feedback.

Best regards,
Suraj Sonawane

Re: [PATCH v2] sound: fix uninit-value in i2s_dma_isr

Posted by Mark Brown 2 weeks, 6 days ago

On Sat, Nov 02, 2024 at 06:06:30PM +0530, Suraj Sonawane wrote:
> Fix an issue detected by the Smatch tool:
> 
> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
> error: uninitialized symbol 'val_1'.
> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
> error: uninitialized symbol 'val_2'.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

Re: [PATCH v2] sound: fix uninit-value in i2s_dma_isr

Posted by Suraj Sonawane 2 weeks, 5 days ago

On 04/11/24 18:11, Mark Brown wrote:
> On Sat, Nov 02, 2024 at 06:06:30PM +0530, Suraj Sonawane wrote:
>> Fix an issue detected by the Smatch tool:
>>
>> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
>> error: uninitialized symbol 'val_1'.
>> sound/soc/bcm/bcm63xx-pcm-whistler.c:264 i2s_dma_isr()
>> error: uninitialized symbol 'val_2'.
> 
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.
> 
> Please don't send new patches in reply to old patches or serieses, this
> makes it harder for both people and tools to understand what is going
> on - it can bury things in mailboxes and make it difficult to keep track
> of what current patches are, both for the new patches and the old ones.

Thank you for the guidance. I understand the importance of following the 
existing format for subject lines to maintain consistency within the 
subsystem. When I submitted the v2 patch earlier, I had been considering 
this, but I mistakenly thought that switching subject lines between v1 
and v2 wasn’t necessary. Moving forward, I'll make sure my patches align 
with the the style for the subsystem and will submit them as new threads 
to avoid confusion.

Thank you again for your feedback.