[PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Thomas Weißschuh]

Also drop the bounce buffer in cs_dsp_coeff_write_ctrl_raw().

The bounce buffer in cs_dsp_coeff_write_ctrl_raw() could theoretically
also be removed. That would be a functional change as the output may be
modified in error cases.
As I don't know the driver very well I left that part out.

Not tested on real hardware.
This came up while porting kunit to mips64.
Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by
cs_dsp is unnecessary in the first place.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
Thomas Weißschuh (2):
      firmware: cs_dsp: Remove usage of GFP_DMA
      firmware: cs_dsp: Remove bounce buffer in cs_dsp_coeff_write_ctrl_raw()

 drivers/firmware/cirrus/cs_dsp.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250211-cs_dsp-gfp_dma-0581bdd09dd5

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

[PATCH 1/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Thomas Weißschuh]

Usage of GFP_DMA should be avoided in general.
The underlying regmap implementatinon should automatically use its own
bounce buffer if necessary.
On indication is that many other callers of regmap_raw_write() and
regmap_raw_read() in cs_dsp use stack allocated buffers which also do
not obey alignment or location restrictions.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 drivers/firmware/cirrus/cs_dsp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
index 5365e9a4300070c42e1bc998b848e62a4e4c9d53..2a85c1009f10273bee3656e5f7aebe31862036b2 100644
--- a/drivers/firmware/cirrus/cs_dsp.c
+++ b/drivers/firmware/cirrus/cs_dsp.c
@@ -763,7 +763,7 @@ static int cs_dsp_coeff_write_ctrl_raw(struct cs_dsp_coeff_ctl *ctl,
 	if (ret)
 		return ret;
 
-	scratch = kmemdup(buf, len, GFP_KERNEL | GFP_DMA);
+	scratch = kmemdup(buf, len, GFP_KERNEL);
 	if (!scratch)
 		return -ENOMEM;
 
@@ -868,7 +868,7 @@ static int cs_dsp_coeff_read_ctrl_raw(struct cs_dsp_coeff_ctl *ctl,
 	if (ret)
 		return ret;
 
-	scratch = kmalloc(len, GFP_KERNEL | GFP_DMA);
+	scratch = kmalloc(len, GFP_KERNEL);
 	if (!scratch)
 		return -ENOMEM;
 
@@ -1731,7 +1731,7 @@ static void *cs_dsp_read_algs(struct cs_dsp *dsp, size_t n_algs,
 	/* Convert length from DSP words to bytes */
 	len *= sizeof(u32);
 
-	alg = kzalloc(len, GFP_KERNEL | GFP_DMA);
+	alg = kzalloc(len, GFP_KERNEL);
 	if (!alg)
 		return ERR_PTR(-ENOMEM);
 

-- 
2.48.1

[PATCH 2/2] firmware: cs_dsp: Remove bounce buffer in cs_dsp_coeff_write_ctrl_raw()

[Thomas Weißschuh]

regmap_raw_write() is not allowed to modify the input data.
With the removal of GFP_DMA from the bounce buffer it does not serve any
purpose anymore, remove it.

Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 drivers/firmware/cirrus/cs_dsp.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
index 2a85c1009f10273bee3656e5f7aebe31862036b2..5f1b3e3329bc80f80f66b3dbf7c8d6eb015ddb97 100644
--- a/drivers/firmware/cirrus/cs_dsp.c
+++ b/drivers/firmware/cirrus/cs_dsp.c
@@ -755,7 +755,6 @@ static int cs_dsp_coeff_write_ctrl_raw(struct cs_dsp_coeff_ctl *ctl,
 				       unsigned int off, const void *buf, size_t len)
 {
 	struct cs_dsp *dsp = ctl->dsp;
-	void *scratch;
 	int ret;
 	unsigned int reg;
 
@@ -763,22 +762,14 @@ static int cs_dsp_coeff_write_ctrl_raw(struct cs_dsp_coeff_ctl *ctl,
 	if (ret)
 		return ret;
 
-	scratch = kmemdup(buf, len, GFP_KERNEL);
-	if (!scratch)
-		return -ENOMEM;
-
-	ret = regmap_raw_write(dsp->regmap, reg, scratch,
-			       len);
+	ret = regmap_raw_write(dsp->regmap, reg, buf, len);
 	if (ret) {
 		cs_dsp_err(dsp, "Failed to write %zu bytes to %x: %d\n",
 			   len, reg, ret);
-		kfree(scratch);
 		return ret;
 	}
 	cs_dsp_dbg(dsp, "Wrote %zu bytes to %x\n", len, reg);
 
-	kfree(scratch);
-
 	return 0;
 }
 

-- 
2.48.1

Re: [PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Richard Fitzgerald]

On 11/02/2025 5:03 pm, Thomas Weißschuh wrote:
> Also drop the bounce buffer in cs_dsp_coeff_write_ctrl_raw().
> 
> The bounce buffer in cs_dsp_coeff_write_ctrl_raw() could theoretically
> also be removed. That would be a functional change as the output may be
> modified in error cases.
> As I don't know the driver very well I left that part out.
> 
> Not tested on real hardware.
> This came up while porting kunit to mips64.
> Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by
> cs_dsp is unnecessary in the first place.
> 

You're sure that all I2C and SPI bus controllers now handle non-DMA-safe
buffers correctly?

> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
> Thomas Weißschuh (2):
>        firmware: cs_dsp: Remove usage of GFP_DMA
>        firmware: cs_dsp: Remove bounce buffer in cs_dsp_coeff_write_ctrl_raw()
> 
>   drivers/firmware/cirrus/cs_dsp.c | 15 +++------------
>   1 file changed, 3 insertions(+), 12 deletions(-)
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250211-cs_dsp-gfp_dma-0581bdd09dd5
> 
> Best regards,

Re: [PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Thomas Weißschuh]

On Tue, Feb 11, 2025 at 05:21:08PM +0000, Richard Fitzgerald wrote:
> On 11/02/2025 5:03 pm, Thomas Weißschuh wrote:
> > Also drop the bounce buffer in cs_dsp_coeff_write_ctrl_raw().
> > 
> > The bounce buffer in cs_dsp_coeff_write_ctrl_raw() could theoretically
> > also be removed. That would be a functional change as the output may be
> > modified in error cases.
> > As I don't know the driver very well I left that part out.
> > 
> > Not tested on real hardware.
> > This came up while porting kunit to mips64.
> > Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by
> > cs_dsp is unnecessary in the first place.
> > 
> 
> You're sure that all I2C and SPI bus controllers now handle non-DMA-safe
> buffers correctly?

No, but as mentioned in patch 1, many transfers are done from and to
on-stack buffers and these seem to work.
Anyways, I fixed the DMA zone issue on MIPS in general [0],
feel free to disregard the series.

[0] https://lore.kernel.org/r/20250212-kunit-mips-v1-0-eb49c9d76615@linutronix.de
(lore is broken right now, so it will take some time to show up)

> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> > Thomas Weißschuh (2):
> >        firmware: cs_dsp: Remove usage of GFP_DMA
> >        firmware: cs_dsp: Remove bounce buffer in cs_dsp_coeff_write_ctrl_raw()
> > 
> >   drivers/firmware/cirrus/cs_dsp.c | 15 +++------------
> >   1 file changed, 3 insertions(+), 12 deletions(-)
> > ---
> > base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> > change-id: 20250211-cs_dsp-gfp_dma-0581bdd09dd5
> > 
> > Best regards,
> 

Re: [PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Richard Fitzgerald]

On 11/02/2025 5:21 pm, Richard Fitzgerald wrote:
> On 11/02/2025 5:03 pm, Thomas Weißschuh wrote:
>> Also drop the bounce buffer in cs_dsp_coeff_write_ctrl_raw().
>>
>> The bounce buffer in cs_dsp_coeff_write_ctrl_raw() could theoretically
>> also be removed. That would be a functional change as the output may be
>> modified in error cases.
>> As I don't know the driver very well I left that part out.
>>
>> Not tested on real hardware.
>> This came up while porting kunit to mips64.
>> Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by

I would say that is a bug in mips64 that should be fixed in mips64.
It is not reasonable to expect generic drivers to have special cases for
platforms that don't handle GFP_DMA.


>> cs_dsp is unnecessary in the first place.
>>
> 
> You're sure that all I2C and SPI bus controllers now handle non-DMA-safe
> buffers correctly?
> 
I just tested this.

The spi kernel doc says this:

  * struct spi_transfer - a read/write buffer pair
  * @tx_buf: data to be written (DMA-safe memory), or NULL
  * @rx_buf: data to be read (DMA-safe memory), or NULL

On x86_64() a spi_write() fails with -EINVAL if I pass it a non-dma-safe
buffer of data. But it works if I pass it a buffer allocated with
GFP_DMA.

So I think it is a bad idea to remove GFP_DMA to workaround mips64.

>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>> ---
>> Thomas Weißschuh (2):
>>        firmware: cs_dsp: Remove usage of GFP_DMA
>>        firmware: cs_dsp: Remove bounce buffer in 
>> cs_dsp_coeff_write_ctrl_raw()
>>
>>   drivers/firmware/cirrus/cs_dsp.c | 15 +++------------
>>   1 file changed, 3 insertions(+), 12 deletions(-)
>> ---
>> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
>> change-id: 20250211-cs_dsp-gfp_dma-0581bdd09dd5
>>
>> Best regards,
> 

Re: [PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

On Thu, Feb 13, 2025 at 02:28:06PM +0000, Richard Fitzgerald wrote:
> On 11/02/2025 5:21 pm, Richard Fitzgerald wrote:

> > > Not tested on real hardware.
> > > This came up while porting kunit to mips64.
> > > Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by

> I would say that is a bug in mips64 that should be fixed in mips64.
> It is not reasonable to expect generic drivers to have special cases for
> platforms that don't handle GFP_DMA.

What specifically is the issue?  If it's a build time issue I'd
definitely agree that we should just be able to assume that platforms at
least build.  IIRC there is a Kconfig you can depend on for DMA but it
seems more trouble than it's worth to fix all users.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Thomas Weißschuh]

On Thu, Feb 13, 2025 at 03:06:59PM +0000, Mark Brown wrote:
> On Thu, Feb 13, 2025 at 02:28:06PM +0000, Richard Fitzgerald wrote:
> > On 11/02/2025 5:21 pm, Richard Fitzgerald wrote:
> 
> > > > Not tested on real hardware.
> > > > This came up while porting kunit to mips64.
> > > > Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by
> 
> > I would say that is a bug in mips64 that should be fixed in mips64.
> > It is not reasonable to expect generic drivers to have special cases for
> > platforms that don't handle GFP_DMA.

Indeed, I did that, too.

> What specifically is the issue?  If it's a build time issue I'd
> definitely agree that we should just be able to assume that platforms at
> least build.  IIRC there is a Kconfig you can depend on for DMA but it
> seems more trouble than it's worth to fix all users.

More details in [0], It's only a runtime issue.

I'm still wondering how all the on-stack buffers used with regmap_raw_read()
and regmap_raw_write() by cs_dsp are satisfying the DMA requirements.

[0] https://lore.kernel.org/lkml/20250212-kunit-mips-v1-1-eb49c9d76615@linutronix.de/

Re: [PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Richard Fitzgerald]

On 13/02/2025 3:16 pm, Thomas Weißschuh wrote:
> On Thu, Feb 13, 2025 at 03:06:59PM +0000, Mark Brown wrote:
>> On Thu, Feb 13, 2025 at 02:28:06PM +0000, Richard Fitzgerald wrote:
>>> On 11/02/2025 5:21 pm, Richard Fitzgerald wrote:
>>
>>>>> Not tested on real hardware.
>>>>> This came up while porting kunit to mips64.
>>>>> Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by
>>
>>> I would say that is a bug in mips64 that should be fixed in mips64.
>>> It is not reasonable to expect generic drivers to have special cases for
>>> platforms that don't handle GFP_DMA.
> 
> Indeed, I did that, too.
> 
>> What specifically is the issue?  If it's a build time issue I'd
>> definitely agree that we should just be able to assume that platforms at
>> least build.  IIRC there is a Kconfig you can depend on for DMA but it
>> seems more trouble than it's worth to fix all users.
> 
> More details in [0], It's only a runtime issue.
> 
> I'm still wondering how all the on-stack buffers used with regmap_raw_read()
> and regmap_raw_write() by cs_dsp are satisfying the DMA requirements.
> 
There are 3 suspicious regmap_raw_read(). The others are all integers,
which are guaranteed not to cross a page boundary.

However, it looks like it might be safe to remove the GFP_DMA flags
now. regmap_raw_read() calls spi_write_then_read() which specifically
says that the buffers do not need to be DMA-safe and internally uses a
DMA-safe buffer. regmap_raw_write() uses either a temporary physically
contiguous buffer or GFP_DMA buffer (the implementation is terrifyingly
complex so it's difficult to determine exactly what it does).

(Some older systems could only use certain special memory areas for DMA
but we haven't seen any of those used with cs_dsp.)


> [0] https://lore.kernel.org/lkml/20250212-kunit-mips-v1-1-eb49c9d76615@linutronix.de/

Re: [PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Charles Keepax]

On Wed, Feb 19, 2025 at 04:26:55PM +0000, Richard Fitzgerald wrote:
> On 13/02/2025 3:16 pm, Thomas Weißschuh wrote:
> > On Thu, Feb 13, 2025 at 03:06:59PM +0000, Mark Brown wrote:
> > > On Thu, Feb 13, 2025 at 02:28:06PM +0000, Richard Fitzgerald wrote:
> > > > On 11/02/2025 5:21 pm, Richard Fitzgerald wrote:
> > > 
> > > > > > Not tested on real hardware.
> > > > > > This came up while porting kunit to mips64.
> > > > > > Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by
> > > 
> > > > I would say that is a bug in mips64 that should be fixed in mips64.
> > > > It is not reasonable to expect generic drivers to have special cases for
> > > > platforms that don't handle GFP_DMA.
> > 
> > Indeed, I did that, too.
> > 
> > > What specifically is the issue?  If it's a build time issue I'd
> > > definitely agree that we should just be able to assume that platforms at
> > > least build.  IIRC there is a Kconfig you can depend on for DMA but it
> > > seems more trouble than it's worth to fix all users.
> > 
> > More details in [0], It's only a runtime issue.
> > 
> > I'm still wondering how all the on-stack buffers used with regmap_raw_read()
> > and regmap_raw_write() by cs_dsp are satisfying the DMA requirements.
> > 
> There are 3 suspicious regmap_raw_read(). The others are all integers,
> which are guaranteed not to cross a page boundary.
> 
> However, it looks like it might be safe to remove the GFP_DMA flags
> now. regmap_raw_read() calls spi_write_then_read() which specifically
> says that the buffers do not need to be DMA-safe and internally uses a
> DMA-safe buffer. regmap_raw_write() uses either a temporary physically
> contiguous buffer or GFP_DMA buffer (the implementation is terrifyingly
> complex so it's difficult to determine exactly what it does).
> 
> (Some older systems could only use certain special memory areas for DMA
> but we haven't seen any of those used with cs_dsp.)
> 

We also need to consider what the I2C subsystem does, I have a
vague memory of thinking the SPI system will attempt to remap
buffers but I2C will just use them as is. cs_dsp will be used
with both, although SPI is slightly more common for obvious
reasons.

Thanks,
Charles

Re: [PATCH 0/2] firmware: cs_dsp: Remove usage of GFP_DMA

[Richard Fitzgerald]

On 20/02/2025 9:52 am, Charles Keepax wrote:
> On Wed, Feb 19, 2025 at 04:26:55PM +0000, Richard Fitzgerald wrote:
>> On 13/02/2025 3:16 pm, Thomas Weißschuh wrote:
>>> On Thu, Feb 13, 2025 at 03:06:59PM +0000, Mark Brown wrote:
>>>> On Thu, Feb 13, 2025 at 02:28:06PM +0000, Richard Fitzgerald wrote:
>>>>> On 11/02/2025 5:21 pm, Richard Fitzgerald wrote:
>>>>
>>>>>>> Not tested on real hardware.
>>>>>>> This came up while porting kunit to mips64.
>>>>>>> Apparently GFP_DMA does not work there, but IMO the usage of GFP_DMA by
>>>>
>>>>> I would say that is a bug in mips64 that should be fixed in mips64.
>>>>> It is not reasonable to expect generic drivers to have special cases for
>>>>> platforms that don't handle GFP_DMA.
>>>
>>> Indeed, I did that, too.
>>>
>>>> What specifically is the issue?  If it's a build time issue I'd
>>>> definitely agree that we should just be able to assume that platforms at
>>>> least build.  IIRC there is a Kconfig you can depend on for DMA but it
>>>> seems more trouble than it's worth to fix all users.
>>>
>>> More details in [0], It's only a runtime issue.
>>>
>>> I'm still wondering how all the on-stack buffers used with regmap_raw_read()
>>> and regmap_raw_write() by cs_dsp are satisfying the DMA requirements.
>>>
>> There are 3 suspicious regmap_raw_read(). The others are all integers,
>> which are guaranteed not to cross a page boundary.
>>
>> However, it looks like it might be safe to remove the GFP_DMA flags
>> now. regmap_raw_read() calls spi_write_then_read() which specifically
>> says that the buffers do not need to be DMA-safe and internally uses a
>> DMA-safe buffer. regmap_raw_write() uses either a temporary physically
>> contiguous buffer or GFP_DMA buffer (the implementation is terrifyingly
>> complex so it's difficult to determine exactly what it does).
>>
>> (Some older systems could only use certain special memory areas for DMA
>> but we haven't seen any of those used with cs_dsp.)
>>
> 
> We also need to consider what the I2C subsystem does, I have a
> vague memory of thinking the SPI system will attempt to remap
> buffers but I2C will just use them as is. cs_dsp will be used
> with both, although SPI is slightly more common for obvious
> reasons.
> 
> Thanks,
> Charles

For information here is the presentation given by Wolfram Sang
describing the DMA problem. This is the reason we used GFP_DMA buffers
in the cs_dsp code.

https://events19.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf