| Message ID | 801f5209-f5a0-7414-f8f4-1500178a680b@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 11-06-17 02:18, Peter Housel wrote: > >> On Jun 10, 2017, at 12:27 PM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: >> >> On 03-06-17 17:36, Andy Shevchenko wrote: >>> On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel <housel@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
On Sat, Jun 10, 2017 at 10:27 PM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > On 03-06-17 17:36, Andy Shevchenko wrote: >> On Sat, Jun 3, 2017 at 1:29 AM, Peter S. Housel <housel@acm.org> wrote: The following looks good to me. Feel free to add Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -705,7 +705,7 @@ int brcmf_sdiod_recv_pkt(struct brcmf_sdio_dev > *sdiodev, struct sk_buff *pkt) > int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev, > struct sk_buff_head *pktq, uint totlen) > { > - struct sk_buff *glom_skb; > + struct sk_buff *glom_skb = NULL; > struct sk_buff *skb; > u32 addr = sdiodev->sbwad; > int err = 0; > @@ -726,10 +726,8 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev > *sdiodev, > return -ENOMEM; > err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr, > glom_skb); > - if (err) { > - brcmu_pkt_buf_free_skb(glom_skb); > + if (err) > goto done; > - } > > skb_queue_walk(pktq, skb) { > memcpy(skb->data, glom_skb->data, skb->len); > @@ -740,6 +738,7 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev > *sdiodev, > pktq); > > done: > + brcmu_pkt_buf_free_skb(glom_skb); > return err; > } >
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 5bc2ba2..3722f23 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -705,7 +705,7 @@ int brcmf_sdiod_recv_pkt(struct brcmf_sdio_dev *sdiodev, struct sk_buff *pkt) int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev, struct sk_buff_head *pktq, uint totlen) { - struct sk_buff *glom_skb; + struct sk_buff *glom_skb = NULL; struct sk_buff *skb; u32 addr = sdiodev->sbwad;