| Message ID | 20241030170829.36161-1-surajsonawane0215@gmail.com (mailing list archive) |
|---|---|
| State | New |
| 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?
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(-)