Re: Commit "rsi: fix memory leak in rsi_load_ta_instructions()" breaks things

[Kalle Valo <kvalo@codeaurora.org>]

Alexey Khoroshilov <khoroshilov@ispras.ru> writes:

> On 24.07.2015 21:12, Kalle Valo wrote:
>> Mike Looijmans <mike.looijmans@topic.nl> writes:
>> 

>>> Just read some documentation:
>>> https://kernel.org/doc/Documentation/firmware_class/README
>>> It states "kernel: grows a buffer in PAGE_SIZE increments to hold the
>>> image as it comes in". Probably the firmware buffer is fragmented in
>>> memory as a result, and that wouldn't be very DMA friendly indeed. The
>>> copy would be contiguous again.
>
> Interesting idea. Even fw_get_filesystem_firmware() uses vmalloc(), so
> fw_entry->data may be physically noncontiguous in case of reading
> firmware from file system as well.
>
> The only my doubt is whether contiguous memory is required here. As far
> as I can see, rsi_copy_to_card() writes data by blocks of size
> dev->tx_blk_size that is 256 for rsi_91x_sdio. So, it is not clear where
> problems can appear.

You cannot DMA from memory allocated with vmalloc(). So kmalloc() & co
has to be used when doing DMA.

>>> I can submit a patch to add the "kfree()" call. Don't know about the
>>> revert though, can I just submit the revert as a patch and then the
>>> "kfree" as a second patch in the same set?
>> 
>> Better to do the revert and kfree() addition in one patch, they are
>> simple enough. Just document in the commit log that it reverts the
>> broken commit.
>> 
>> And remember to add CC stable to the commit log so that the fix goes to
>> stable releases.
>
> Agree with Kalle with a couple of notes.
>
> To document a fix of a broken commit the preferable way is to use Fixes:
> tag.
> https://kernel.org/doc/Documentation/SubmittingPatches
>
> Also it would be useful to add a comment before kmemdup() with
> information why it is needed there.

Good points.

-- 
Kalle Valo