Re: [PATCH v2] brcmfmac: firmware: Fix firmware loading

[Linus Walleij <linus.walleij@linaro.org>]

On Thu, Aug 5, 2021 at 3:44 AM Dmitry Osipenko <digetx@gmail.com> wrote:

> 04.08.2021 18:34, Linus Walleij пишет:
> > +             fwctx->tested_board_variant = false;
>
> This shouldn't be really needed, isn't it?

It is true in the sense that fwctx is allocated with kzalloc.

But it is done in a different function brcmf_fw_alloc_request()
and it is not entirely clear that the allocated struct is not reused,
and if someone refactors the code they could reuse it, so I add
this just to be 100% sure that this gets set to false.

Usually in the kernel when we have functions named *alloc*
it is allocating objects that can later be reused several times
(see the block layer for example), so I try to follow that pattern
here and assume that fwctx can be reused.

The only time I really rely on fields being zero is when the
allocation is in the same base block, e.g. inside probe() or
so.

> > +     } else {
> > +             fwctx->tested_board_variant = true;
> >               ret = request_firmware_nowait(THIS_MODULE, true, first->path,
> >                                             fwctx->dev, GFP_KERNEL, fwctx,
> > -                                           brcmf_fw_request_done);
> > +                                           brcmf_fw_request_done_first);
> >       }
> >       if (ret < 0)
> > -             brcmf_fw_request_done(NULL, fwctx);
> > +             brcmf_fw_request_done_first(NULL, fwctx);
>
> This "else" can be replaced with:
>
> if (!alt_path || ret < 0)
>         brcmf_fw_request_done(NULL, fwctx);

Sorry I don't quite get this... both branches of the if/else clause will
assign ret also if alt_path is set request_firmware_nowait() can return
nonzero and then brcmf_fw_request_done() needs to get
called?

Yours,
Linus Walleij