From: Mike Looijmans <mike.looijmans@topic.nl>
To: Alexey Khoroshilov <khoroshilov@ispras.ru>
Cc: <linux-wireless@vger.kernel.org>, <kvalo@codeaurora.org>,
<fariyaf@gmail.com>
Subject: Re: Commit "rsi: fix memory leak in rsi_load_ta_instructions()" breaks things
Date: Fri, 24 Jul 2015 13:02:15 +0200 [thread overview]
Message-ID: <55B21B37.4080509@topic.nl> (raw)
In-Reply-To: <55B1F9CC.4060507@ispras.ru>
On 24-07-15 10:39, Alexey Khoroshilov wrote:
> Dear Mike,
>
> On 24.07.2015 14:01, Mike Looijmans wrote:
>> Regarding this commit:
>>
>> https://lkml.org/lkml/2014/12/12/709
>>
>> rsi: fix memory leak in rsi_load_ta_instructions()
>>
>> Memory allocated by kmemdup() in rsi_load_ta_instructions() is leaked.
>> But duplication of firmware data here is useless,
>> so the patch removes kmemdup() at all.
>>
>> Found by Linux Driver Verification project (linuxtesting.org).
>>
>> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>
>> We use this driver for the Redpine Wifi chip on our "florida" board, and
>> after this commit it stopped working. Symptom was that the "wlan0"
>> device was not created at all. Reverting the commit makes it work again.
>>
>> Apparently, the kmemdup action is needed for something. I suspect the
>> DMA controller is still copying the firmware data before the method
>> returned.
>
> To test your hypothesis, could you please check if it is still broken
> with kfree(fw); added just after release_firmware(fw_entry); in
> rsi_load_ta_instructions().
Tried, and appears to work if i just kfree() the firmware copy. It does leave
a bad taste though. I'd expect fw_entry->data to point to a kmalloc'd area as
well. So it might work now just because it happens to be that the memory if
"far enough away" and isn't being touched by anything else until the transfer
is done. And on some other setup, it may suddenly fail unexpectedly.
I thought to move the kfree to a point where the driver unregisters, but
apparently it doesn't have any internal hook for that (sdio_done or so).
I'd really like to see some comment from the Redpine folks on this, but since
there hasn't been any event in the past year or so, I don't expect much.
Kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
next prev parent reply other threads:[~2015-07-24 11:31 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-24 7:01 Commit "rsi: fix memory leak in rsi_load_ta_instructions()" breaks things Mike Looijmans
2015-07-24 8:02 ` Kalle Valo
2015-07-24 8:39 ` Alexey Khoroshilov
2015-07-24 11:02 ` Mike Looijmans [this message]
2015-07-24 11:35 ` Alexey Khoroshilov
2015-07-24 13:42 ` Mike Looijmans
2015-07-24 14:12 ` Kalle Valo
2015-07-24 16:26 ` Alexey Khoroshilov
2015-07-24 16:59 ` Kalle Valo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55B21B37.4080509@topic.nl \
--to=mike.looijmans@topic.nl \
--cc=fariyaf@gmail.com \
--cc=khoroshilov@ispras.ru \
--cc=kvalo@codeaurora.org \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.