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

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

[Richard Fitzgerald]

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")
---
 drivers/firmware/cirrus/cs_dsp.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
index f9d8a883900d..42a50a4f8f58 100644
--- a/drivers/firmware/cirrus/cs_dsp.c
+++ b/drivers/firmware/cirrus/cs_dsp.c
@@ -1649,11 +1649,17 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware,
 			   region_name);
 
 		if (reg) {
+			/*
+			 * Although we expect the underlying bus does not require
+			 * physically-contiguous buffers, we pessimistically use
+			 * a temporary buffer instead of trusting that the
+			 * alignment of region->data is ok.
+			 */
 			region_len = le32_to_cpu(region->len);
 			if (region_len > buf_len) {
 				buf_len = round_up(region_len, PAGE_SIZE);
-				kfree(buf);
-				buf = kmalloc(buf_len, GFP_KERNEL | GFP_DMA);
+				vfree(buf);
+				buf = vmalloc(buf_len);
 				if (!buf) {
 					ret = -ENOMEM;
 					goto out_fw;
@@ -1682,7 +1688,7 @@ static int cs_dsp_load(struct cs_dsp *dsp, const struct firmware *firmware,
 
 	ret = 0;
 out_fw:
-	kfree(buf);
+	vfree(buf);
 
 	if (ret == -EOVERFLOW)
 		cs_dsp_err(dsp, "%s: file content overflows file data\n", file);
@@ -2370,11 +2376,17 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware
 		}
 
 		if (reg) {
+			/*
+			 * Although we expect the underlying bus does not require
+			 * physically-contiguous buffers, we pessimistically use
+			 * a temporary buffer instead of trusting that the
+			 * alignment of blk->data is ok.
+			 */
 			region_len = le32_to_cpu(blk->len);
 			if (region_len > buf_len) {
 				buf_len = round_up(region_len, PAGE_SIZE);
-				kfree(buf);
-				buf = kmalloc(buf_len, GFP_KERNEL | GFP_DMA);
+				vfree(buf);
+				buf = vmalloc(buf_len);
 				if (!buf) {
 					ret = -ENOMEM;
 					goto out_fw;
@@ -2405,7 +2417,7 @@ static int cs_dsp_load_coeff(struct cs_dsp *dsp, const struct firmware *firmware
 
 	ret = 0;
 out_fw:
-	kfree(buf);
+	vfree(buf);
 
 	if (ret == -EOVERFLOW)
 		cs_dsp_err(dsp, "%s: file content overflows file data\n", file);
-- 
2.47.3

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

[Takashi Iwai]

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

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

[Richard Fitzgerald]

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.

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

[Takashi Iwai]

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

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

[Mark Brown]

On Wed, 04 Mar 2026 14:12:50 +0000, 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.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] firmware: cs_dsp: Fix fragmentation regression in firmware download
      commit: facfdef64d11c08e6f1e69d02a0b87cb74cee0f5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark