| Message ID | 20241030170829.36161-1-surajsonawane0215@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sound: fix uninit-value in i2s_dma_isr | expand |
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?
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.
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?
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
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.
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
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;
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(-)