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
Fixes: cd8d3d321285 ("p54spi: p54spi driver")
Signed-off-by: Christophe JAILLET <[email protected]>
---
v2: reduce diffstat and take advantage on the fact that release_firmware()
checks for NULL
---
drivers/net/wireless/intersil/p54/p54spi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
index f99b7ba69fc3..19152fd449ba 100644
--- a/drivers/net/wireless/intersil/p54/p54spi.c
+++ b/drivers/net/wireless/intersil/p54/p54spi.c
@@ -164,7 +164,7 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev)
ret = p54_parse_firmware(dev, priv->firmware);
if (ret) {
- release_firmware(priv->firmware);
+ /* the firmware is released by the caller */
return ret;
}
@@ -659,6 +659,7 @@ static int p54spi_probe(struct spi_device *spi)
return 0;
err_free_common:
+ release_firmware(priv->firmware);
free_irq(gpio_to_irq(p54spi_gpio_irq), spi);
err_free_gpio_irq:
gpio_free(p54spi_gpio_irq);
--
2.34.1
On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
<[email protected]> 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).
> Fixes: cd8d3d321285 ("p54spi: p54spi driver")
> Signed-off-by: Christophe JAILLET <[email protected]>
Acked-by: Christian Lamparter <[email protected]>
(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.
(I still have hope for the devres-firmware to hit some day).
Cheers
Christian
> ---
> drivers/net/wireless/intersil/p54/p54spi.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
> index f99b7ba69fc3..19152fd449ba 100644
> --- a/drivers/net/wireless/intersil/p54/p54spi.c
> +++ b/drivers/net/wireless/intersil/p54/p54spi.c
> @@ -164,7 +164,7 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev)
>
> ret = p54_parse_firmware(dev, priv->firmware);
> if (ret) {
> - release_firmware(priv->firmware);
> + /* the firmware is released by the caller */
> return ret;
> }
>
> @@ -659,6 +659,7 @@ static int p54spi_probe(struct spi_device *spi)
> return 0;
>
> err_free_common:
> + release_firmware(priv->firmware);
> free_irq(gpio_to_irq(p54spi_gpio_irq), spi);
> err_free_gpio_irq:
> gpio_free(p54spi_gpio_irq);
> --
> 2.34.1
>
Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
> On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
> <[email protected]> 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 <[email protected]>
> Acked-by: Christian Lamparter <[email protected]>
> (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.
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.
CJ
>
> (I still have hope for the devres-firmware to hit some day).
>
> Cheers
> Christian
>
>> ---
>> drivers/net/wireless/intersil/p54/p54spi.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
>> index f99b7ba69fc3..19152fd449ba 100644
>> --- a/drivers/net/wireless/intersil/p54/p54spi.c
>> +++ b/drivers/net/wireless/intersil/p54/p54spi.c
>> @@ -164,7 +164,7 @@ static int p54spi_request_firmware(struct ieee80211_hw *dev)
>>
>> ret = p54_parse_firmware(dev, priv->firmware);
>> if (ret) {
>> - release_firmware(priv->firmware);
>> + /* the firmware is released by the caller */
>> return ret;
>> }
>>
>> @@ -659,6 +659,7 @@ static int p54spi_probe(struct spi_device *spi)
>> return 0;
>>
>> err_free_common:
>> + release_firmware(priv->firmware);
>> free_irq(gpio_to_irq(p54spi_gpio_irq), spi);
>> err_free_gpio_irq:
>> gpio_free(p54spi_gpio_irq);
>> --
>> 2.34.1
>>
>
Christophe JAILLET <[email protected]> writes:
> Le 13/06/2022 à 22:02, Christian Lamparter a écrit :
>> On Sun, Jun 12, 2022 at 11:12 PM Christophe JAILLET
>> <[email protected]> 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_
No worries, I can fix the typo.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Mon, Jun 13, 2022 at 10:57:25PM +0200, Christophe JAILLET wrote:
> > > ---
> > > 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.
>
> 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.
>
I prefered v1 but with s/firmaware/firmware/...
regards,
dan carpenter
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
>> <[email protected]> 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 <[email protected]>
>> Acked-by: Christian Lamparter <[email protected]>
>> (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.
I don't know what's up on the horizon. For my part, I've been devresing
parts of carl9170 and now thinking about it. Because the various
request_firmware*() functions could be a target for devres too.
A driver usually loads the firmware in .probe(). It stays around because
of .suspend()+.resume() and gets freed by .release().
With devresing up request_firmware(), that release_firmware() would be
rendered obsolete in all of p54* cases.
There must be something that I have missed? right?
It's because there's already an extensive list of managed interfaces:
<https://www.kernel.org/doc/html/latest/driver-api/driver-model/devres.html>
But the firmware_class is not on it. Does somebody know the presumably
"very good" reason why not? I can't believe that this hasn't been done yet.
Regards,
Christian
On Wed, 2022-06-15 at 23:03 +0200, Christian Lamparter wrote:
>
> A driver usually loads the firmware in .probe(). It stays around because
> of .suspend()+.resume()
>
Does it, though? the firmware cache thing is a bit odd, it sometimes
seems to me that it only re-requests/loads the firmware when suspending,
and drops the cache when resumed, just in case it's requested inbetween.
johannes
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
> > > <[email protected]> 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 <[email protected]>
> > > Acked-by: Christian Lamparter <[email protected]>
> > > (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, "$"},
regards,
dan carpenter
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
>>>> <[email protected]> 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 <[email protected]>
>>>> Acked-by: Christian Lamparter <[email protected]>
>>>> (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
Le 16/06/2022 à 17:19, Dan Carpenter a écrit :
> On Thu, Jun 16, 2022 at 03:13:26PM +0200, Christian Lamparter wrote:
>> On 16/06/2022 12:36, Dan Carpenter wrote:
>>>>> 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?).
>
> Yep. Generally, static checkers assume that functions clean up after
> themselves on error paths so there would be a warning in
> p54spi_request_firmware(). This is the easiest kind of static analysis
> to implement and it's the way most kernel error handling is written.
>
>> 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().
>
> Presumably Christophe found this bug with static analysis already but
True, I use a coccinelle script that looks at functions called in
.remove() functions that are not called in what looks like an error
handling path in the corresponding probe.
> my guess is that it has a lot of false positives?
This is SOOOO true !
The output is 23k LoC, mostly false positive!
In fact I only checks the diff between the outputs of my coccinelle
script from time to time.
Looking at only the diff, most of the false positives get ignored and I
manage to spot ~5-10 issues of this kind in each dev cycle in new code.
CJ
>
> Eventually the leak in the probe function would be found with static
> analysis as well. The truth is that there are a lot of leaks so I'm
> already a bit overwhelmed fixing the ones that I know about.
>
> It would be fairly simple to make a high quality resource leak checker
> which is specific to probe functions. But the thing is that leaks in
> probe functions are not really exploitable. Also some devices are
> needed for the system to boot so often the devs don't care about about
> cleaning up... My motivation is low.
>
> regards,
> dan carpenter
>
>
Christophe JAILLET <[email protected]> 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 firmware
>
> Fixes: cd8d3d321285 ("p54spi: p54spi driver")
> Signed-off-by: Christophe JAILLET <[email protected]>
> Acked-by: Christian Lamparter <[email protected]>
Patch applied to wireless-next.git, thanks.
83781f0162d0 wifi: p54: Fix an error handling path in p54spi_probe()
--
https://patchwork.kernel.org/project/linux-wireless/patch/297d2547ff2ee627731662abceeab9dbdaf23231.1655068321.git.christophe.jaillet@wanadoo.fr/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches