On 11-06-17 02:18, Peter Housel wrote:
>
>> On Jun 10, 2017, at 12:27 PM, Arend van Spriel
>> <arend.vanspr...@broadcom.com> wrote:
>>
>> On 03-06-17 17:36, Andy Shevchenko wrote:
>>> On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel <hou...@acm.org> wrote:
>>>> An earlier change to this function (3bdae810721b) fixed a leak in the
>>>> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
>>>> glom_skb buffer, used for emulating a scattering read, is never used
>>>> or referenced after its contents are copied into the destination
>>>> buffers, and therefore always needs to be freed by the end of the
>>>> function.
>>
>> [snip]
>>
>>>> + skb_queue_walk(pktq, skb) {
>>>> + memcpy(skb->data, glom_skb->data,
>>>> skb->len);
>>>> + skb_pull(glom_skb, skb->len);
>>>> + }
>>>> }
>>>
>>>> + brcmu_pkt_buf_free_skb(glom_skb);
>>>
>>> Can we just add this one line instead or I'm missing something?
>>
>> I guess. We don't want to walk the packet queue if glom_skb is not
>> carrying data due to brcmf_sdiod_buffrw() failure.
>>
>> So I would go with the patch below as brcmu_pkt_buf_free_skb() simply
>> ignores null pointer.
>
> I’m fine with this, or indeed most of the other proposed solutions. The
> important thing is that the leak is fixed; in the driver's current state I
> was able to run our wearable device out of memory in just over 20 seconds
> running iperf.
Sure. The reason behind the suggestion from Franky was to get rid of the
label inside branch and I agree with that. To address Andy's comment I
think my proposal should tackle that.
Just out of curiosity, we added the broken-sg-support thing for OMAP
platform. So what platform/mmc-host are you using. I try to keep an
overview where this workaround is needed.
Regards,
Arend
