[bug report] ALSA: memalloc: Add fallback SG-buffer allocations for x86

[bug report] ALSA: memalloc: Add fallback SG-buffer allocations for x86

[Dan Carpenter]

Hello Takashi Iwai,

The patch 925ca893b4a6: "ALSA: memalloc: Add fallback SG-buffer
allocations for x86" from Apr 13, 2022, leads to the following Smatch
static checker warning:

	sound/core/memalloc.c:732 snd_dma_sg_fallback_alloc()
	error: 'p' came from dma_alloc_coherent() so we can't do virt_to_phys()

sound/core/memalloc.c
    708 static void *snd_dma_sg_fallback_alloc(struct snd_dma_buffer *dmab, size_t size)
    709 {
    710         struct snd_dma_sg_fallback *sgbuf;
    711         struct page **pages;
    712         size_t i, count;
    713         void *p;
    714 
    715         sgbuf = kzalloc(sizeof(*sgbuf), GFP_KERNEL);
    716         if (!sgbuf)
    717                 return NULL;
    718         count = PAGE_ALIGN(size) >> PAGE_SHIFT;
    719         pages = kvcalloc(count, sizeof(*pages), GFP_KERNEL);
    720         if (!pages)
    721                 goto error;
    722         sgbuf->pages = pages;
    723         sgbuf->addrs = kvcalloc(count, sizeof(*sgbuf->addrs), GFP_KERNEL);
    724         if (!sgbuf->addrs)
    725                 goto error;
    726 
    727         for (i = 0; i < count; sgbuf->count++, i++) {
    728                 p = dma_alloc_coherent(dmab->dev.dev, PAGE_SIZE,
    729                                        &sgbuf->addrs[i], DEFAULT_GFP);
    730                 if (!p)
    731                         goto error;
--> 732                 sgbuf->pages[i] = virt_to_page(p);

The warning is a bit useless.  It's complaining about __phys_addr()
and not virt_to_phys().  I don't really understand the rules here, it
might be legal in certain contexts.

    733         }
    734 
    735         if (dmab->dev.type == SNDRV_DMA_TYPE_DEV_WC_SG_FALLBACK)
    736                 set_pages_array_wc(pages, count);
    737         p = vmap(pages, count, VM_MAP, PAGE_KERNEL);
    738         if (!p)
    739                 goto error;
    740         dmab->private_data = sgbuf;
    741         return p;
    742 

regards,
dan carpenter

Re: [bug report] ALSA: memalloc: Add fallback SG-buffer allocations for x86

[Takashi Iwai]

On Wed, 20 Apr 2022 09:44:58 +0200,
Dan Carpenter wrote:
> 
> Hello Takashi Iwai,
> 
> The patch 925ca893b4a6: "ALSA: memalloc: Add fallback SG-buffer
> allocations for x86" from Apr 13, 2022, leads to the following Smatch
> static checker warning:
> 
> 	sound/core/memalloc.c:732 snd_dma_sg_fallback_alloc()
> 	error: 'p' came from dma_alloc_coherent() so we can't do virt_to_phys()
> 
> sound/core/memalloc.c
>     708 static void *snd_dma_sg_fallback_alloc(struct snd_dma_buffer *dmab, size_t size)
>     709 {
>     710         struct snd_dma_sg_fallback *sgbuf;
>     711         struct page **pages;
>     712         size_t i, count;
>     713         void *p;
>     714 
>     715         sgbuf = kzalloc(sizeof(*sgbuf), GFP_KERNEL);
>     716         if (!sgbuf)
>     717                 return NULL;
>     718         count = PAGE_ALIGN(size) >> PAGE_SHIFT;
>     719         pages = kvcalloc(count, sizeof(*pages), GFP_KERNEL);
>     720         if (!pages)
>     721                 goto error;
>     722         sgbuf->pages = pages;
>     723         sgbuf->addrs = kvcalloc(count, sizeof(*sgbuf->addrs), GFP_KERNEL);
>     724         if (!sgbuf->addrs)
>     725                 goto error;
>     726 
>     727         for (i = 0; i < count; sgbuf->count++, i++) {
>     728                 p = dma_alloc_coherent(dmab->dev.dev, PAGE_SIZE,
>     729                                        &sgbuf->addrs[i], DEFAULT_GFP);
>     730                 if (!p)
>     731                         goto error;
> --> 732                 sgbuf->pages[i] = virt_to_page(p);
> 
> The warning is a bit useless.  It's complaining about __phys_addr()
> and not virt_to_phys().  I don't really understand the rules here, it
> might be legal in certain contexts.

In general it's not good to perform virt_to_page() for the
dma_alloc_coherent(), but here the code is special only for x86 and
certain circumstances, so this must work -- it's the very same stuff
we had over a decade, after all.  The code was resurrected in a
simplified form as a fallback now.


thanks,

Takashi