1. Patchew
  2. linux
  3. View series

[PATCH] firmware: cs_dsp: Store control length as 32-bit

Richard Fitzgerald posted 1 patch 1 week ago 
drivers/firmware/cirrus/cs_dsp.c       | 2 +-
include/linux/firmware/cirrus/cs_dsp.h | 2 +-
sound/soc/codecs/wm_adsp.c             | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
[PATCH] firmware: cs_dsp: Store control length as 32-bit
Posted by Richard Fitzgerald 1 week ago 
The architectures supported by this driver have a maximum of 32-bits
of address, so we don't need more than 32-bits to store the length of
control data. Change the length in struct cs_dsp_coeff_ctl to an
unsigned int instead of a size_t. Also make a corresponding trivial
change to wm_adsp.c to prevent a compiler warning.

Tested on x86_64 builds this saves at least 4 bytes per control
(another 4 bytes might be saved if the compiler was inserting padding
to align the size_t).

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 drivers/firmware/cirrus/cs_dsp.c       | 2 +-
 include/linux/firmware/cirrus/cs_dsp.h | 2 +-
 sound/soc/codecs/wm_adsp.c             | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
index 9acdcd75928a..36a5aefa16e7 100644
--- a/drivers/firmware/cirrus/cs_dsp.c
+++ b/drivers/firmware/cirrus/cs_dsp.c
@@ -477,7 +477,7 @@ static int cs_dsp_debugfs_read_controls_show(struct seq_file *s, void *ignored)
 
 	list_for_each_entry(ctl, &dsp->ctl_list, list) {
 		cs_dsp_coeff_base_reg(ctl, &reg, 0);
-		seq_printf(s, "%22.*s: %#8zx %s:%08x %#8x %s %#8x %#4x %c%c%c%c %s %s\n",
+		seq_printf(s, "%22.*s: %#8x %s:%08x %#8x %s %#8x %#4x %c%c%c%c %s %s\n",
 			   ctl->subname_len, ctl->subname, ctl->len,
 			   cs_dsp_mem_region_name(ctl->alg_region.type),
 			   ctl->offset, reg, ctl->fw_name, ctl->alg_region.alg, ctl->type,
diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h
index 69959032f8f5..0ec1cdc5585d 100644
--- a/include/linux/firmware/cirrus/cs_dsp.h
+++ b/include/linux/firmware/cirrus/cs_dsp.h
@@ -102,7 +102,7 @@ struct cs_dsp_coeff_ctl {
 	const char *subname;
 	unsigned int subname_len;
 	unsigned int offset;
-	size_t len;
+	unsigned int len;
 	unsigned int type;
 	unsigned int flags;
 	unsigned int set:1;
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 172dcdd7dbca..17cec79245d4 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -1561,7 +1561,7 @@ static int wm_adsp_buffer_parse_coeff(struct cs_dsp_coeff_ctl *cs_ctl)
 
 	for (i = 0; i < 5; ++i) {
 		ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, &coeff_v1,
-					     min(cs_ctl->len, sizeof(coeff_v1)));
+					     min((size_t)cs_ctl->len, sizeof(coeff_v1)));
 		if (ret < 0)
 			return ret;
 
-- 
2.47.3
Re: [PATCH] firmware: cs_dsp: Store control length as 32-bit
Posted by david laight 6 days, 5 hours ago 
On Mon, 24 Nov 2025 17:15:35 +0000
Richard Fitzgerald <rf@opensource.cirrus.com> wrote:

> The architectures supported by this driver have a maximum of 32-bits
> of address, so we don't need more than 32-bits to store the length of
> control data. Change the length in struct cs_dsp_coeff_ctl to an
> unsigned int instead of a size_t. Also make a corresponding trivial
> change to wm_adsp.c to prevent a compiler warning.
> 
> Tested on x86_64 builds this saves at least 4 bytes per control
> (another 4 bytes might be saved if the compiler was inserting padding
> to align the size_t).
> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  drivers/firmware/cirrus/cs_dsp.c       | 2 +-
>  include/linux/firmware/cirrus/cs_dsp.h | 2 +-
>  sound/soc/codecs/wm_adsp.c             | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
> index 9acdcd75928a..36a5aefa16e7 100644
> --- a/drivers/firmware/cirrus/cs_dsp.c
> +++ b/drivers/firmware/cirrus/cs_dsp.c
> @@ -477,7 +477,7 @@ static int cs_dsp_debugfs_read_controls_show(struct seq_file *s, void *ignored)
>  
>  	list_for_each_entry(ctl, &dsp->ctl_list, list) {
>  		cs_dsp_coeff_base_reg(ctl, &reg, 0);
> -		seq_printf(s, "%22.*s: %#8zx %s:%08x %#8x %s %#8x %#4x %c%c%c%c %s %s\n",
> +		seq_printf(s, "%22.*s: %#8x %s:%08x %#8x %s %#8x %#4x %c%c%c%c %s %s\n",
>  			   ctl->subname_len, ctl->subname, ctl->len,
>  			   cs_dsp_mem_region_name(ctl->alg_region.type),
>  			   ctl->offset, reg, ctl->fw_name, ctl->alg_region.alg, ctl->type,
> diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h
> index 69959032f8f5..0ec1cdc5585d 100644
> --- a/include/linux/firmware/cirrus/cs_dsp.h
> +++ b/include/linux/firmware/cirrus/cs_dsp.h
> @@ -102,7 +102,7 @@ struct cs_dsp_coeff_ctl {
>  	const char *subname;
>  	unsigned int subname_len;
>  	unsigned int offset;
> -	size_t len;
> +	unsigned int len;
>  	unsigned int type;
>  	unsigned int flags;
>  	unsigned int set:1;
> diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
> index 172dcdd7dbca..17cec79245d4 100644
> --- a/sound/soc/codecs/wm_adsp.c
> +++ b/sound/soc/codecs/wm_adsp.c
> @@ -1561,7 +1561,7 @@ static int wm_adsp_buffer_parse_coeff(struct cs_dsp_coeff_ctl *cs_ctl)
>  
>  	for (i = 0; i < 5; ++i) {
>  		ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, &coeff_v1,
> -					     min(cs_ctl->len, sizeof(coeff_v1)));
> +					     min((size_t)cs_ctl->len, sizeof(coeff_v1)));

You don't (shouldn't) need that cast.
min() won't object because both arguments are unsigned types.
The compiler will then 'promote' cs_ctl->len for the compares, but might use
the 'as if' rule and do 32bit maths anyway.

Given that the called function starts:
int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl,
			   unsigned int off, void *buf, size_t len)
{
...
	if (len + off * sizeof(u32) > ctl->len)
		return -EINVAL;

Maybe you should be changing that 'len' to u32 as well?

	David

>  		if (ret < 0)
>  			return ret;
>
Re: [PATCH] firmware: cs_dsp: Store control length as 32-bit
Posted by Richard Fitzgerald 5 days, 14 hours ago 
On 25/11/25 18:55, david laight wrote:
> On Mon, 24 Nov 2025 17:15:35 +0000
> Richard Fitzgerald <rf@opensource.cirrus.com> wrote:
> 
>> The architectures supported by this driver have a maximum of 32-bits
>> of address, so we don't need more than 32-bits to store the length of
>> control data. Change the length in struct cs_dsp_coeff_ctl to an
>> unsigned int instead of a size_t. Also make a corresponding trivial
>> change to wm_adsp.c to prevent a compiler warning.
>>
>> Tested on x86_64 builds this saves at least 4 bytes per control
>> (another 4 bytes might be saved if the compiler was inserting padding
>> to align the size_t).
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>>   drivers/firmware/cirrus/cs_dsp.c       | 2 +-
>>   include/linux/firmware/cirrus/cs_dsp.h | 2 +-
>>   sound/soc/codecs/wm_adsp.c             | 2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c
>> index 9acdcd75928a..36a5aefa16e7 100644
>> --- a/drivers/firmware/cirrus/cs_dsp.c
>> +++ b/drivers/firmware/cirrus/cs_dsp.c
>> @@ -477,7 +477,7 @@ static int cs_dsp_debugfs_read_controls_show(struct seq_file *s, void *ignored)
>>   
>>   	list_for_each_entry(ctl, &dsp->ctl_list, list) {
>>   		cs_dsp_coeff_base_reg(ctl, &reg, 0);
>> -		seq_printf(s, "%22.*s: %#8zx %s:%08x %#8x %s %#8x %#4x %c%c%c%c %s %s\n",
>> +		seq_printf(s, "%22.*s: %#8x %s:%08x %#8x %s %#8x %#4x %c%c%c%c %s %s\n",
>>   			   ctl->subname_len, ctl->subname, ctl->len,
>>   			   cs_dsp_mem_region_name(ctl->alg_region.type),
>>   			   ctl->offset, reg, ctl->fw_name, ctl->alg_region.alg, ctl->type,
>> diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h
>> index 69959032f8f5..0ec1cdc5585d 100644
>> --- a/include/linux/firmware/cirrus/cs_dsp.h
>> +++ b/include/linux/firmware/cirrus/cs_dsp.h
>> @@ -102,7 +102,7 @@ struct cs_dsp_coeff_ctl {
>>   	const char *subname;
>>   	unsigned int subname_len;
>>   	unsigned int offset;
>> -	size_t len;
>> +	unsigned int len;
>>   	unsigned int type;
>>   	unsigned int flags;
>>   	unsigned int set:1;
>> diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
>> index 172dcdd7dbca..17cec79245d4 100644
>> --- a/sound/soc/codecs/wm_adsp.c
>> +++ b/sound/soc/codecs/wm_adsp.c
>> @@ -1561,7 +1561,7 @@ static int wm_adsp_buffer_parse_coeff(struct cs_dsp_coeff_ctl *cs_ctl)
>>   
>>   	for (i = 0; i < 5; ++i) {
>>   		ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, &coeff_v1,
>> -					     min(cs_ctl->len, sizeof(coeff_v1)));
>> +					     min((size_t)cs_ctl->len, sizeof(coeff_v1)));
> 
> You don't (shouldn't) need that cast.
> min() won't object because both arguments are unsigned types.

When this code was written one of gcc/clang/sparse didn't agree with
you.
I built for multiple architectures so maybe some architecture version
of the compiler complains.

> The compiler will then 'promote' cs_ctl->len for the compares, but might use
> the 'as if' rule and do 32bit maths anyway.
> 
> Given that the called function starts:
> int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl,
> 			   unsigned int off, void *buf, size_t len)
> {
> ...
> 	if (len + off * sizeof(u32) > ctl->len)
> 		return -EINVAL;
> 
> Maybe you should be changing that 'len' to u32 as well?
> 
Why? What's wrong with taking a size_t for length arguments?
Re: [PATCH] firmware: cs_dsp: Store control length as 32-bit
Posted by Mark Brown 6 days, 8 hours ago 
On Mon, 24 Nov 2025 17:15:35 +0000, Richard Fitzgerald wrote:
> The architectures supported by this driver have a maximum of 32-bits
> of address, so we don't need more than 32-bits to store the length of
> control data. Change the length in struct cs_dsp_coeff_ctl to an
> unsigned int instead of a size_t. Also make a corresponding trivial
> change to wm_adsp.c to prevent a compiler warning.
> 
> Tested on x86_64 builds this saves at least 4 bytes per control
> (another 4 bytes might be saved if the compiler was inserting padding
> to align the size_t).
> 
> [...]

Applied to

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

Thanks!

[1/1] firmware: cs_dsp: Store control length as 32-bit
      commit: 7584edf15892e29190b2145294cc1680aa142586

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.