Re: [PATCH] firmware: cs_dsp: avoid large local variables

[Richard Fitzgerald]

On 16/12/2024 10:38 am, Richard Fitzgerald wrote:
> On 16/12/2024 8:33 am, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@xxxxxxxx>
> >
> > Having 1280 bytes of local variables on the stack exceeds the limit
> > on 32-bit architectures:
> >
> > drivers/firmware/cirrus/test/cs_dsp_test_bin.c: In function 
> > 'bin_patch_mixed_packed_unpacked_random':
> > drivers/firmware/cirrus/test/cs_dsp_test_bin.c:2097:1: error: the 
> > frame size of 1784 bytes is larger than 1024 bytes [-Werror=frame- 
> > larger-than=]
> >
> > Use dynamic allocation for the largest two here.
> >
> > Fixes: dd0b6b1f29b9 ("firmware: cs_dsp: Add KUnit testing of bin file 
> > download")
> > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> > ---
> >   .../firmware/cirrus/test/cs_dsp_test_bin.c    | 36 +++++++++++--------
> >   1 file changed, 22 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/firmware/cirrus/test/cs_dsp_test_bin.c b/drivers/ 
> > firmware/cirrus/test/cs_dsp_test_bin.c
> > index 689190453307..f1955813919c 100644
> > --- a/drivers/firmware/cirrus/test/cs_dsp_test_bin.c
> > +++ b/drivers/firmware/cirrus/test/cs_dsp_test_bin.c
> > @@ -1978,8 +1978,10 @@ static void 
> > bin_patch_mixed_packed_unpacked_random(struct kunit *test)
> >            4, 51, 76, 72, 16,  6, 39, 62, 15, 41, 28, 73, 53, 40, 45, 54,
> >           14, 55, 46, 66, 64, 59, 23,  9, 67, 47, 19, 71, 35, 18, 42,  1,
> >       };
> > -    u32 packed_payload[80][3];
> > -    u32 unpacked_payload[80];
> > +    struct {
> > +        u32 packed[80][3];
> > +        u32 unpacked[80];
> > +    } *payload;
> >       u32 readback[3];
> >       unsigned int alg_base_words, patch_pos_words;
> >       unsigned int alg_base_in_packed_regs, patch_pos_in_packed_regs;
> > @@ -1988,8 +1990,12 @@ static void 
> > bin_patch_mixed_packed_unpacked_random(struct kunit *test)
> >       struct firmware *fw;
> >       int i;
> > -    get_random_bytes(packed_payload, sizeof(packed_payload));
> > -    get_random_bytes(unpacked_payload, sizeof(unpacked_payload));
> > +    payload = kmalloc(sizeof(*payload), GFP_KERNEL);
>
> Should be kunit_kmalloc() so the kunit framework frees it automatically
> if any test case fails and the test terminates early.
>
> Apart from that it looks ok to me.

Also the check for NULL should use KUNIT_ASSERT_NOT_NULL() so the test
case will error-out.