From: Denis Kirjanov <kirjanov@gmail.com>
To: Kulikov Vasiliy <segooon@gmail.com>
Cc: Kernel Janitors <kernel-janitors@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
John Linn <john.linn@xilinx.com>,
Grant Likely <grant.likely@secretlab.ca>,
Jiri Pirko <jpirko@redhat.com>,
Brian Hill <brian.hill@xilinx.com>,
netdev@vger.kernel.org
Subject: Re: [PATCH 3/4] ll_temac: free everything on error path
Date: Thu, 8 Jul 2010 17:56:09 +0400 [thread overview]
Message-ID: <AANLkTilMaITOMsn_9MmZ7-zHwpvTd2ZYjj4UUWOloYsn@mail.gmail.com> (raw)
In-Reply-To: <20100708134651.GA27196@shinshilla>
On Thu, Jul 8, 2010 at 5:46 PM, Kulikov Vasiliy <segooon@gmail.com> wrote:
> On Thu, Jul 08, 2010 at 17:16 +0400, Denis Kirjanov wrote:
>> On Thu, Jul 8, 2010 at 4:10 PM, Kulikov Vasiliy <segooon@gmail.com> wrote:
>> > temac_dma_bd_init() must free all allocated resources: memory, dma, skbs.
>> >
>> > Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
>> > ---
>> > drivers/net/ll_temac_main.c | 30 +++++++++++++++++++++++++-----
>> > 1 files changed, 25 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
>> > index a2da3d7..38d658a 100644
>> > --- a/drivers/net/ll_temac_main.c
>> > +++ b/drivers/net/ll_temac_main.c
>> > @@ -200,6 +200,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> > struct temac_local *lp = netdev_priv(ndev);
>> > struct sk_buff *skb;
>> > int i;
>> > + int tx_bd_v_size, rx_bd_v_size;
>> >
>> > lp->rx_skb = kzalloc(sizeof(*lp->rx_skb) * RX_BD_NUM, GFP_KERNEL);
>> > if (!lp->rx_skb) {
>> > @@ -209,21 +210,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> > }
>> > /* allocate the tx and rx ring buffer descriptors. */
>> > /* returns a virtual addres and a physical address. */
>> > + tx_bd_v_size = sizeof(*lp->tx_bd_v) * TX_BD_NUM;
>> > lp->tx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> ^^^^^^^^^^^
> 1st
>> > - sizeof(*lp->tx_bd_v) * TX_BD_NUM,
>> > + tx_bd_v_size,
>> > &lp->tx_bd_p, GFP_KERNEL);
>> > if (!lp->tx_bd_v) {
>> > dev_err(&ndev->dev,
>> > "unable to allocate DMA TX buffer descriptors");
>> > - goto out;
>> > + goto err_rx_skb;
>> > }
>> > + rx_bd_v_size = sizeof(*lp->rx_bd_v) * RX_BD_NUM;
>> > lp->rx_bd_v = dma_alloc_coherent(ndev->dev.parent,
> ^^^^^^^^^^^
> 2nd
>> > - sizeof(*lp->rx_bd_v) * RX_BD_NUM,
>> > + rx_bd_v_size,
>> > &lp->rx_bd_p, GFP_KERNEL);
>> > if (!lp->rx_bd_v) {
>> > dev_err(&ndev->dev,
>> > "unable to allocate DMA RX buffer descriptors");
>> > - goto out;
>> > + goto err_tx_bd;
>> > }
>> >
>> > memset(lp->tx_bd_v, 0, sizeof(*lp->tx_bd_v) * TX_BD_NUM);
>> > @@ -242,7 +245,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >
>> > if (skb == 0) {
>> > dev_err(&ndev->dev, "alloc_skb error %d\n", i);
>> > - goto out;
>> > + goto err_dma_single;
>> > }
>> > lp->rx_skb[i] = skb;
>> > /* returns physical address of skb->data */
>> > @@ -274,6 +277,23 @@ static int temac_dma_bd_init(struct net_device *ndev)
>> >
>> > return 0;
>> >
>> > +err_dma_single:
>> > + for (i = 0; i < RX_BD_NUM; i++) {
>> > + if (lp->rx_skb[i] == NULL)
>> > + break;
>> > +
>> > + kfree_skb(lp->rx_skb[i]);
>> > + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
>> > + XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
>> > + }
>> > +
>> > + dma_free_coherent(ndev->dev.parent, rx_bd_v_size,
>> > + lp->rx_bd_v, lp->rx_bd_p);
> ^^^^^^^^^^^
> 2nd
>> > +err_tx_bd:
>> > + dma_free_coherent(ndev->dev.parent, tx_bd_v_size,
>> > + lp->tx_bd_v, lp->tx_bd_p);
> ^^^^^^^^^^^
> 1st
>> > +err_rx_skb:
>> > + kfree(lp->rx_skb);
>> > out:
>> > return -ENOMEM;
>> > }
>> > --
>> > 1.7.0.4
>> This is not enough. DMA resources also must be released on exit.
> Could you point to it? I see only two variables with allocated DMA (see above).
> --
I just sent a patch that fixes this.
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Regards,
Denis
next prev parent reply other threads:[~2010-07-08 13:56 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-07-08 12:10 [PATCH 3/4] ll_temac: free everything on error path Kulikov Vasiliy
2010-07-08 13:16 ` Denis Kirjanov
2010-07-08 13:46 ` Kulikov Vasiliy
2010-07-08 13:56 ` Denis Kirjanov [this message]
2010-07-08 14:24 ` Kulikov Vasiliy
2010-07-10 7:30 ` [PATCH 3/4 v3] ll_temac: fix DMA and memory leaks Kulikov Vasiliy
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=AANLkTilMaITOMsn_9MmZ7-zHwpvTd2ZYjj4UUWOloYsn@mail.gmail.com \
--to=kirjanov@gmail.com \
--cc=brian.hill@xilinx.com \
--cc=davem@davemloft.net \
--cc=dkirjanov@kernel.org \
--cc=grant.likely@secretlab.ca \
--cc=john.linn@xilinx.com \
--cc=jpirko@redhat.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=segooon@gmail.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).