Re: WC vs UC mappings in snd_dma_sg_alloc()

[Takashi Iwai]

On Tue, 19 Oct 2021 17:03:08 +0200,
sanekf@xxxxxx wrote:
> 
> Hi
> 
> I've stumbled across this code in sound/core/sgbuf.c:
> 
> 66 static void *snd_dma_sg_alloc(struct snd_dma_buffer *dmab, size_t size)
> 67 {
> < ... >
> 80 	if (dmab->dev.type == SNDRV_DMA_TYPE_DEV_WC_SG) {
> 81 		type = SNDRV_DMA_TYPE_DEV_WC;
> 82 #ifdef pgprot_noncached
> 83 		prot = pgprot_noncached(PAGE_KERNEL);
> 84 #endif
> 85 	}
> < ... >
> 131 	area = vmap(sgbuf->page_table, sgbuf->pages, VM_MAP, prot);
> 
> Does not this violate x86 rules about using the same memory mapping type for all mappings? It seems that the following patch should fix it (only compile tested - my x86 PCs are either without Linux or without sound, and probably in practice this might not trigger any problems since both WC and UC-minus are incoherent types):

Actually this has been already fixed in the latest code for 5.16
together with the rewriting of the SG buffer code.  There, sgbuf.c is
gone completely, and this inconsistency was resolved, too.


thanks,

Takashi

> 
> -----------------------------
> 
> ALSA: memalloc: duly use pgprot_writecombine() for WC mapping
> 
> x86 has strict rules about not having memory type aliasing (Documentation/x86/pat.rst). snd_dma_sg_alloc() violates them by mapping first as WC (with set_memory_wc()) and then as UC- (with pgprot_noncached() + vmap()). Switching to pgprot_writecombine() should fix this.
> 
> Signed-off-by: Aleksandr Fedorov <halcien@xxxxxxxxx>
> diff --git a/sound/core/sgbuf.c b/sound/core/sgbuf.c
> index 8352a5cdb19f..670b30c3b6e5 100644
> --- a/sound/core/sgbuf.c
> +++ b/sound/core/sgbuf.c
> @@ -79,9 +79,7 @@ static void *snd_dma_sg_alloc(struct snd_dma_buffer *dmab, size_t size)
>  		return NULL;
>  	if (dmab->dev.type == SNDRV_DMA_TYPE_DEV_WC_SG) {
>  		type = SNDRV_DMA_TYPE_DEV_WC;
> -#ifdef pgprot_noncached
> -		prot = pgprot_noncached(PAGE_KERNEL);
> -#endif
> +		prot = pgprot_writecombine(PAGE_KERNEL);
>  	}
>  	sgbuf->dev = dmab->dev.dev;
>  	pages = snd_sgbuf_aligned_pages(size);
>