Re: [PATCH] firmware: cs_dsp: Fix fragmentation regression in firmware download

[Takashi Iwai <tiwai@suse.de>]

On Wed, 04 Mar 2026 16:35:24 +0100,
Richard Fitzgerald wrote:
> 
> On 04/03/2026 3:17 pm, Takashi Iwai wrote:
> > On Wed, 04 Mar 2026 15:12:50 +0100,
> > Richard Fitzgerald wrote:
> >> 
> >> Use vmalloc() instead of kmalloc(..., GFP_DMA) to alloc the temporary
> >> buffer for firmware download blobs. This avoids the problem that a
> >> heavily fragmented system cannot allocate enough physically-contiguous
> >> memory for a large blob.
> >> 
> >> The redundant alloc buffer mechanism was removed in commit 900baa6e7bb0
> >> ("firmware: cs_dsp: Remove redundant download buffer allocator").
> >> While doing that I was overly focused on the possibility of the
> >> underlying bus requiring DMA-safe memory. So I used GFP_DMA kmalloc()s.
> >> I failed to notice that the code I was removing used vmalloc().
> >> This creates a regression.
> >> 
> >> Way back in 2014 the problem of fragmentation with kmalloc()s was fixed
> >> by commit cdcd7f728753 ("ASoC: wm_adsp: Use vmalloc to allocate firmware
> >> download buffer").
> >> 
> >> Although we don't need physically-contiguous memory, we don't know if the
> >> bus needs some particular alignment of the buffers. Since the change in
> >> 2014, the firmware download has always used whatever alignment vmalloc()
> >> returns. To avoid introducing a new problem, the temporary buffer is still
> >> used, to keep the same alignment of pointers passed to regmap_raw_write().
> >> 
> >> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> >> Fixes: 900baa6e7bb0 ("firmware: cs_dsp: Remove redundant download buffer allocator")
> > 
> > FYI, if the data isn't always large, kvmalloc() could be a better
> > alternative, which is a hybrid for both speed and size, too.
> > 
> > 
> > Takashi
> 
> I originally did that, but as this is a bugfix for backporting I decided
> not to risk introducing a change inside a bugfix. The original code was
> vmalloc() so I have corrected back to what the original code did.
> 
> vmalloc() appears to allocate whole pages, on PAGE_SIZE boundary but
> kvmalloc() could allocate on a smaller boundary. I know that kmalloc()
> memory is claimed to be be DMA-safe. But I don't want to risk fixing one
> regression and introducing a new regression into stable kernels.
> 
> It's on my to-do list to have a look at using kvmalloc(), and possibly
> skipping the buffer if the source data is already aligned on
> ARCH_KMALLOC_MINALIGN. But that's for future kernel releases.

OK, then the change sounds reasonable.  Thanks for explanation!


Takashi