Re: [PATCH v2] p54: Fix an error handling path in p54spi_probe()

[Christian Lamparter]

On 16/06/2022 12:36, Dan Carpenter wrote:
> On Wed, Jun 15, 2022 at 11:03:34PM +0200, Christian Lamparter wrote:
> > On 13/06/2022 22:57, Christophe JAILLET wrote:
> > > Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
> > > > On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
> > > > <christophe.jaillet@xxxxxxxxxx> wrote:
> > > > > If an error occurs after a successful call to p54spi_request_firmware(), it
> > > > > must be undone by a corresponding release_firmware() as already done in
> > > > > the error handling path of p54spi_request_firmware() and in the .remove()
> > > > > function.
> > > > >
> > > > > Add the missing call in the error handling path and remove it from
> > > > > p54spi_request_firmware() now that it is the responsibility of the caller
> > > > > to release the firmawre
> > > >
> > > > that last word hast a typo:  firmware. (maybe Kalle can fix this in post).
> > >
> > > More or less the same typo twice in a row... _Embarrassed_
> > >
> > > > > Fixes: cd8d3d321285 ("p54spi: p54spi driver")
> > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> > > > Acked-by: Christian Lamparter <chunkeey@xxxxxxxxx>
> > > > (Though, v1 was fine too.)
> > > > > ---
> > > > > v2: reduce diffstat and take advantage on the fact that release_firmware()
> > > > > checks for NULL
> > > >
> > > > Heh, ok ;) . Now that I see it,  the "ret = p54_parse_firmware(...); ... "
> > > > could have been replaced with "return p54_parse_firmware(dev, priv->firmware);"
> > > > so the p54spi.c could shrink another 5-6 lines.
> > > >
> > > > I think leaving p54spi_request_firmware() callee to deal with
> > > > releasing the firmware
> > > > in the error case as well is nicer because it gets rid of a "but in
> > > > this case" complexity.
> > >
> > >
> > > Take the one you consider being the best one.
> >
> > well said!
> >
> > > If it deserves a v3 to axe some lines of code, I can do it but, as said
> > > previously,
> > > v1 is for me the cleaner and more future proof.
> >
> > Gee, that last sentence about "future proof" is daring.
>
> The future is vast and unknowable but one thing which is pretty likely
> is that Christophe's patch will introduce a static checker warning.  We
> really would have expected a to find a release_firmware() in the place
> where it was in the original code.  There is a comment there now so no
> one is going to re-add the release_firmware() but that's been an issue
> in the past.
>
> I'm sort of surprised that it wasn't a static checker warning already.
> Anyway, I'll add this to Smatch check_unwind.c
>
> +         { "request_firmware", ALLOC, 0, "*$", &int_zero, &int_zero},
> +         { "release_firmware", RELEASE, 0, "$"},

hmm? I don't follow you there. Why should there be a warning "now"?
(I assume you mean with v2 but not with v1?). If it's because the static
checker can't look beyond the function scope then this would be bad news
since on the "success" path the firmware will stick around until
p54spi_remove().

The p54* drivers would need to be updated (they are ooold) to make good
use of the firmware_cache. There must have been a good reason why it
was designed that way. Especially because the firmware_class uses devm
for the cache already internally. (thanks for Johannes for pointing this
out).

Cheers,
Christian