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

[Richard Fitzgerald]

On 25/11/25 18:55, david laight wrote:
> On Mon, 24 Nov 2025 17:15:35 +0000
> Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   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?