* [bug report] ALSA: hda: Interrupt servicing and BDL setup for compress streams
@ 2022-12-07 14:28 Dan Carpenter
2022-12-07 14:35 ` Cezary Rojewski
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2022-12-07 14:28 UTC (permalink / raw)
To: cezary.rojewski; +Cc: alsa-devel
Hello Cezary Rojewski,
The patch 3e9582267e3a: "ALSA: hda: Interrupt servicing and BDL setup
for compress streams" from Dec 2, 2022, leads to the following Smatch
static checker warning:
sound/hda/hdac_stream.c:544 snd_hdac_stream_setup_periods()
error: uninitialized symbol 'dmab'.
sound/hda/hdac_stream.c
487 int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
488 {
489 struct hdac_bus *bus = azx_dev->bus;
490 struct snd_pcm_substream *substream = azx_dev->substream;
491 struct snd_compr_stream *cstream = azx_dev->cstream;
492 struct snd_pcm_runtime *runtime = NULL;
493 struct snd_dma_buffer *dmab;
494 __le32 *bdl;
495 int i, ofs, periods, period_bytes;
496 int pos_adj, pos_align;
497
498 if (substream) {
499 runtime = substream->runtime;
500 dmab = snd_pcm_get_dma_buf(substream);
501 } else if (cstream) {
502 dmab = snd_pcm_get_dma_buf(cstream);
503 }
dmab is not initialized on else path.
504
505 /* reset BDL address */
506 snd_hdac_stream_writel(azx_dev, SD_BDLPL, 0);
507 snd_hdac_stream_writel(azx_dev, SD_BDLPU, 0);
508
509 period_bytes = azx_dev->period_bytes;
510 periods = azx_dev->bufsize / period_bytes;
511
512 /* program the initial BDL entries */
513 bdl = (__le32 *)azx_dev->bdl.area;
514 ofs = 0;
515 azx_dev->frags = 0;
516
517 pos_adj = bus->bdl_pos_adj;
518 if (runtime && !azx_dev->no_period_wakeup && pos_adj > 0) {
519 pos_align = pos_adj;
520 pos_adj = DIV_ROUND_UP(pos_adj * runtime->rate, 48000);
521 if (!pos_adj)
522 pos_adj = pos_align;
523 else
524 pos_adj = roundup(pos_adj, pos_align);
525 pos_adj = frames_to_bytes(runtime, pos_adj);
526 if (pos_adj >= period_bytes) {
527 dev_warn(bus->dev, "Too big adjustment %d\n",
528 pos_adj);
529 pos_adj = 0;
530 } else {
531 ofs = setup_bdle(bus, dmab, azx_dev,
532 &bdl, ofs, pos_adj, true);
This is okay because runtime is set.
533 if (ofs < 0)
534 goto error;
535 }
536 } else
537 pos_adj = 0;
538
539 for (i = 0; i < periods; i++) {
540 if (i == periods - 1 && pos_adj)
541 ofs = setup_bdle(bus, dmab, azx_dev,
This is okay because pos_adj is checked.
542 &bdl, ofs, period_bytes - pos_adj, 0);
543 else
544 ofs = setup_bdle(bus, dmab, azx_dev,
^^^^
Potentially uninitialized?
545 &bdl, ofs, period_bytes,
546 !azx_dev->no_period_wakeup);
547 if (ofs < 0)
548 goto error;
549 }
550 return 0;
551
552 error:
553 dev_err(bus->dev, "Too many BDL entries: buffer=%d, period=%d\n",
554 azx_dev->bufsize, period_bytes);
555 return -EINVAL;
556 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] ALSA: hda: Interrupt servicing and BDL setup for compress streams
2022-12-07 14:28 [bug report] ALSA: hda: Interrupt servicing and BDL setup for compress streams Dan Carpenter
@ 2022-12-07 14:35 ` Cezary Rojewski
2022-12-07 16:55 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Cezary Rojewski @ 2022-12-07 14:35 UTC (permalink / raw)
To: Dan Carpenter, Takashi Iwai; +Cc: alsa-devel
On 2022-12-07 3:28 PM, Dan Carpenter wrote:
> Hello Cezary Rojewski,
>
> The patch 3e9582267e3a: "ALSA: hda: Interrupt servicing and BDL setup
> for compress streams" from Dec 2, 2022, leads to the following Smatch
> static checker warning:
>
> sound/hda/hdac_stream.c:544 snd_hdac_stream_setup_periods()
> error: uninitialized symbol 'dmab'.
>
> sound/hda/hdac_stream.c
> 487 int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
> 488 {
> 489 struct hdac_bus *bus = azx_dev->bus;
> 490 struct snd_pcm_substream *substream = azx_dev->substream;
> 491 struct snd_compr_stream *cstream = azx_dev->cstream;
> 492 struct snd_pcm_runtime *runtime = NULL;
> 493 struct snd_dma_buffer *dmab;
> 494 __le32 *bdl;
> 495 int i, ofs, periods, period_bytes;
> 496 int pos_adj, pos_align;
> 497
> 498 if (substream) {
> 499 runtime = substream->runtime;
> 500 dmab = snd_pcm_get_dma_buf(substream);
> 501 } else if (cstream) {
> 502 dmab = snd_pcm_get_dma_buf(cstream);
> 503 }
>
> dmab is not initialized on else path.
Hello,
Thanks for the report. Perhaps I should just do: s/else if/else/ as the
situation with ->substream AND ->cstream being uninitialized is invalid.
Before support for compress stream was added in this part of the code,
it was always assumed ->substream is valid.
So, either s/else if/else or append:
} else {
return -EINVAL;
}
Takashi, what do you think?
Regards,
Czarek
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] ALSA: hda: Interrupt servicing and BDL setup for compress streams
2022-12-07 14:35 ` Cezary Rojewski
@ 2022-12-07 16:55 ` Takashi Iwai
0 siblings, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2022-12-07 16:55 UTC (permalink / raw)
To: Cezary Rojewski; +Cc: alsa-devel, Dan Carpenter
On Wed, 07 Dec 2022 15:35:12 +0100,
Cezary Rojewski wrote:
>
> On 2022-12-07 3:28 PM, Dan Carpenter wrote:
> > Hello Cezary Rojewski,
> >
> > The patch 3e9582267e3a: "ALSA: hda: Interrupt servicing and BDL setup
> > for compress streams" from Dec 2, 2022, leads to the following Smatch
> > static checker warning:
> >
> > sound/hda/hdac_stream.c:544 snd_hdac_stream_setup_periods()
> > error: uninitialized symbol 'dmab'.
> >
> > sound/hda/hdac_stream.c
> > 487 int snd_hdac_stream_setup_periods(struct hdac_stream *azx_dev)
> > 488 {
> > 489 struct hdac_bus *bus = azx_dev->bus;
> > 490 struct snd_pcm_substream *substream = azx_dev->substream;
> > 491 struct snd_compr_stream *cstream = azx_dev->cstream;
> > 492 struct snd_pcm_runtime *runtime = NULL;
> > 493 struct snd_dma_buffer *dmab;
> > 494 __le32 *bdl;
> > 495 int i, ofs, periods, period_bytes;
> > 496 int pos_adj, pos_align;
> > 497
> > 498 if (substream) {
> > 499 runtime = substream->runtime;
> > 500 dmab = snd_pcm_get_dma_buf(substream);
> > 501 } else if (cstream) {
> > 502 dmab = snd_pcm_get_dma_buf(cstream);
> > 503 }
> >
> > dmab is not initialized on else path.
>
> Hello,
>
> Thanks for the report. Perhaps I should just do: s/else if/else/ as
> the situation with ->substream AND ->cstream being uninitialized is
> invalid.
>
> Before support for compress stream was added in this part of the code,
> it was always assumed ->substream is valid.
>
> So, either s/else if/else or append:
> } else {
> return -EINVAL;
> }
>
> Takashi, what do you think?
The return -EINVAL would be OK. As this must not happen, it can be
even with WARN_ON().
thanks,
Takashi
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-12-07 16:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-07 14:28 [bug report] ALSA: hda: Interrupt servicing and BDL setup for compress streams Dan Carpenter
2022-12-07 14:35 ` Cezary Rojewski
2022-12-07 16:55 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.