| Message ID | 1431762709-20740-7-git-send-email-selvin.xavier@avagotech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, 2015-05-16 at 13:21 +0530, Selvin Xavier wrote: > @@ -1468,10 +1471,8 @@ static int ocrdma_mbx_alloc_pd_range(struct ocrdma_dev *dev) > > cmd->pd_count = dev->attr.max_pd - dev->attr.max_dpp_pds; > status = ocrdma_mbx_cmd(dev, (struct ocrdma_mqe *)cmd); > - if (status) > - goto mbx_err; > rsp = (struct ocrdma_alloc_pd_range_rsp *)cmd; > - if (rsp->pd_count) { > + if (!status && rsp->pd_count) { > dev->pd_mgr->pd_norm_start = rsp->dpp_page_pdid & > OCRDMA_ALLOC_PD_RNG_RSP_START_PDID_MASK; > dev->pd_mgr->max_normal_pd = rsp->pd_count; > @@ -1486,7 +1487,6 @@ static int ocrdma_mbx_alloc_pd_range(struct ocrdma_dev *dev) > } else { > return -ENOMEM; > } > -mbx_err: > kfree(cmd); > return status; > } I didn't go into the file to make sure, but this looks like a memory leak (in fact, it looks like a leak that always existed, specifically if rsp->pd_count == 0 then the else causes you to leave before the kfree(cmd) and therefore you leak). It's now been made worse because it would now happen both when status != 0 and when pd_count == 0.
Thanks Doug for the review. We will fix this memory leak in V2. Thanks, Selvin Xavier > -----Original Message----- > From: Doug Ledford [mailto:dledford@redhat.com] > Sent: Friday, May 15, 2015 9:13 PM > To: Selvin Xavier > Cc: linux-rdma@vger.kernel.org > Subject: Re: [PATCH for-4.1 06/11] RDMA/ocrdma: Prevent allocation of DPP > PDs if FW doesnt support it > > On Sat, 2015-05-16 at 13:21 +0530, Selvin Xavier wrote: > > > @@ -1468,10 +1471,8 @@ static int ocrdma_mbx_alloc_pd_range(struct > > ocrdma_dev *dev) > > > > cmd->pd_count = dev->attr.max_pd - dev->attr.max_dpp_pds; > > status = ocrdma_mbx_cmd(dev, (struct ocrdma_mqe *)cmd); > > - if (status) > > - goto mbx_err; > > rsp = (struct ocrdma_alloc_pd_range_rsp *)cmd; > > - if (rsp->pd_count) { > > + if (!status && rsp->pd_count) { > > dev->pd_mgr->pd_norm_start = rsp->dpp_page_pdid & > > > OCRDMA_ALLOC_PD_RNG_RSP_START_PDID_MASK; > > dev->pd_mgr->max_normal_pd = rsp->pd_count; @@ - > 1486,7 +1487,6 @@ > > static int ocrdma_mbx_alloc_pd_range(struct ocrdma_dev *dev) > > } else { > > return -ENOMEM; > > } > > -mbx_err: > > kfree(cmd); > > return status; > > } > > I didn't go into the file to make sure, but this looks like a memory leak > (in fact, > it looks like a leak that always existed, specifically if > rsp->pd_count == 0 then the else causes you to leave before the > kfree(cmd) and therefore you leak). It's now been made worse because it > would now happen both when status != 0 and when pd_count == 0. > > > -- > Doug Ledford <dledford@redhat.com> > GPG KeyID: 0E572FDD -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c index 5636eb6..4b77f34 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c @@ -1440,27 +1440,30 @@ static int ocrdma_mbx_alloc_pd_range(struct ocrdma_dev *dev) struct ocrdma_alloc_pd_range_rsp *rsp; /* Pre allocate the DPP PDs */ - cmd = ocrdma_init_emb_mqe(OCRDMA_CMD_ALLOC_PD_RANGE, sizeof(*cmd)); - if (!cmd) - return -ENOMEM; - cmd->pd_count = dev->attr.max_dpp_pds; - cmd->enable_dpp_rsvd |= OCRDMA_ALLOC_PD_ENABLE_DPP; - status = ocrdma_mbx_cmd(dev, (struct ocrdma_mqe *)cmd); - if (status) - goto mbx_err; - rsp = (struct ocrdma_alloc_pd_range_rsp *)cmd; - - if ((rsp->dpp_page_pdid & OCRDMA_ALLOC_PD_RSP_DPP) && rsp->pd_count) { - dev->pd_mgr->dpp_page_index = rsp->dpp_page_pdid >> - OCRDMA_ALLOC_PD_RSP_DPP_PAGE_SHIFT; - dev->pd_mgr->pd_dpp_start = rsp->dpp_page_pdid & - OCRDMA_ALLOC_PD_RNG_RSP_START_PDID_MASK; - dev->pd_mgr->max_dpp_pd = rsp->pd_count; - pd_bitmap_size = BITS_TO_LONGS(rsp->pd_count) * sizeof(long); - dev->pd_mgr->pd_dpp_bitmap = kzalloc(pd_bitmap_size, - GFP_KERNEL); + if (dev->attr.max_dpp_pds) { + cmd = ocrdma_init_emb_mqe(OCRDMA_CMD_ALLOC_PD_RANGE, + sizeof(*cmd)); + if (!cmd) + return -ENOMEM; + cmd->pd_count = dev->attr.max_dpp_pds; + cmd->enable_dpp_rsvd |= OCRDMA_ALLOC_PD_ENABLE_DPP; + status = ocrdma_mbx_cmd(dev, (struct ocrdma_mqe *)cmd); + rsp = (struct ocrdma_alloc_pd_range_rsp *)cmd; + + if (!status && (rsp->dpp_page_pdid & OCRDMA_ALLOC_PD_RSP_DPP) && + rsp->pd_count) { + dev->pd_mgr->dpp_page_index = rsp->dpp_page_pdid >> + OCRDMA_ALLOC_PD_RSP_DPP_PAGE_SHIFT; + dev->pd_mgr->pd_dpp_start = rsp->dpp_page_pdid & + OCRDMA_ALLOC_PD_RNG_RSP_START_PDID_MASK; + dev->pd_mgr->max_dpp_pd = rsp->pd_count; + pd_bitmap_size = + BITS_TO_LONGS(rsp->pd_count) * sizeof(long); + dev->pd_mgr->pd_dpp_bitmap = kzalloc(pd_bitmap_size, + GFP_KERNEL); + } + kfree(cmd); } - kfree(cmd); cmd = ocrdma_init_emb_mqe(OCRDMA_CMD_ALLOC_PD_RANGE, sizeof(*cmd)); if (!cmd) @@ -1468,10 +1471,8 @@ static int ocrdma_mbx_alloc_pd_range(struct ocrdma_dev *dev) cmd->pd_count = dev->attr.max_pd - dev->attr.max_dpp_pds; status = ocrdma_mbx_cmd(dev, (struct ocrdma_mqe *)cmd); - if (status) - goto mbx_err; rsp = (struct ocrdma_alloc_pd_range_rsp *)cmd; - if (rsp->pd_count) { + if (!status && rsp->pd_count) { dev->pd_mgr->pd_norm_start = rsp->dpp_page_pdid & OCRDMA_ALLOC_PD_RNG_RSP_START_PDID_MASK; dev->pd_mgr->max_normal_pd = rsp->pd_count; @@ -1486,7 +1487,6 @@ static int ocrdma_mbx_alloc_pd_range(struct ocrdma_dev *dev) } else { return -ENOMEM; } -mbx_err: kfree(cmd); return status; } diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c index 06e8ab7..9dcb660 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c @@ -365,7 +365,7 @@ static struct ocrdma_pd *_ocrdma_alloc_pd(struct ocrdma_dev *dev, if (!pd) return ERR_PTR(-ENOMEM); - if (udata && uctx) { + if (udata && uctx && dev->attr.max_dpp_pds) { pd->dpp_enabled = ocrdma_get_asic_type(dev) == OCRDMA_ASIC_GEN_SKH_R; pd->num_dpp_qp =