| Message ID | 20190716054047.2671-1-nishkadg.linux@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Commit | 9e38e690ace3e7a22a81fc02652fc101efb340cf |
| Headers | show |
| Series | pci: controller: pci-tegra: Add of_node_put() before return | expand |
Thanks for the fix! Please pay attention to the subject line convention and make yours match, e.g., $ git log --oneline drivers/pci/controller/pci-tegra.c | head -5 7be142caabc4 PCI: tegra: Enable Relaxed Ordering only for Tegra20 & Tegra30 4b16a8227907 PCI: tegra: Change link retry log level to debug dbdcc22c845b PCI: tegra: Add support for GPIO based PERST# 2d8c7361585f PCI: tegra: Put PEX CLK & BIAS pads in DPD mode adb2653b3d2e PCI: tegra: Add AFI_PEX2_CTRL reg offset as part of SoC struct I think this actually fixes a *reference* leak (not a memory leak as you say below). The subject line should mention that. We can tell by looking at the patch that it adds of_node_put(); the subject and commit log should tell us *why*. On Tue, Jul 16, 2019 at 11:10:47AM +0530, Nishka Dasgupta wrote: > Each iteration of for_each_child_of_node puts the previous node, but in > the case of a return from the middle of the loop, there is no put, thus > causing a memory leak. > Hence store these mid-loop return values in variable err and add a new > label err_node_put which puts the previous node and returns err. Change > six mid-loop return statements to point to this new label instead. This sort of looks like two paragraphs and sort of looks like one. Please either rewrap it so it's clearly one paragraph, or add a blank line so it's clearly two paragraphs. s/for_each_child_of_node/for_each_child_of_node()/ (as you already did for of_node_put() in the subject, thanks for that) > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > --- > drivers/pci/controller/pci-tegra.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c > index 9a917b2456f6..673a1725ef38 100644 > --- a/drivers/pci/controller/pci-tegra.c > +++ b/drivers/pci/controller/pci-tegra.c > @@ -2237,14 +2237,15 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > err = of_pci_get_devfn(port); > if (err < 0) { > dev_err(dev, "failed to parse address: %d\n", err); > - return err; > + goto err_node_put; > } > > index = PCI_SLOT(err); > > if (index < 1 || index > soc->num_ports) { > dev_err(dev, "invalid port number: %d\n", index); > - return -EINVAL; > + err = -EINVAL; > + goto err_node_put; > } > > index--; > @@ -2253,12 +2254,13 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > if (err < 0) { > dev_err(dev, "failed to parse # of lanes: %d\n", > err); > - return err; > + goto err_node_put; > } > > if (value > 16) { > dev_err(dev, "invalid # of lanes: %u\n", value); > - return -EINVAL; > + err = -EINVAL; > + goto err_node_put; > } > > lanes |= value << (index << 3); > @@ -2272,13 +2274,15 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > lane += value; > > rp = devm_kzalloc(dev, sizeof(*rp), GFP_KERNEL); > - if (!rp) > - return -ENOMEM; > + if (!rp) { > + err = -ENOMEM; > + goto err_node_put; > + } > > err = of_address_to_resource(port, 0, &rp->regs); > if (err < 0) { > dev_err(dev, "failed to parse address: %d\n", err); > - return err; > + goto err_node_put; > } > > INIT_LIST_HEAD(&rp->list); > @@ -2330,6 +2334,10 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > return err; > > return 0; > + > +err_node_put: > + of_node_put(port); > + return err; > } > > /* > -- > 2.19.1 >
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c index 9a917b2456f6..673a1725ef38 100644 --- a/drivers/pci/controller/pci-tegra.c +++ b/drivers/pci/controller/pci-tegra.c @@ -2237,14 +2237,15 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) err = of_pci_get_devfn(port); if (err < 0) { dev_err(dev, "failed to parse address: %d\n", err); - return err; + goto err_node_put; } index = PCI_SLOT(err); if (index < 1 || index > soc->num_ports) { dev_err(dev, "invalid port number: %d\n", index); - return -EINVAL; + err = -EINVAL; + goto err_node_put; } index--; @@ -2253,12 +2254,13 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) if (err < 0) { dev_err(dev, "failed to parse # of lanes: %d\n", err); - return err; + goto err_node_put; } if (value > 16) { dev_err(dev, "invalid # of lanes: %u\n", value); - return -EINVAL; + err = -EINVAL; + goto err_node_put; } lanes |= value << (index << 3); @@ -2272,13 +2274,15 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) lane += value; rp = devm_kzalloc(dev, sizeof(*rp), GFP_KERNEL); - if (!rp) - return -ENOMEM; + if (!rp) { + err = -ENOMEM; + goto err_node_put; + } err = of_address_to_resource(port, 0, &rp->regs); if (err < 0) { dev_err(dev, "failed to parse address: %d\n", err); - return err; + goto err_node_put; } INIT_LIST_HEAD(&rp->list); @@ -2330,6 +2334,10 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) return err; return 0; + +err_node_put: + of_node_put(port); + return err; } /*
Each iteration of for_each_child_of_node puts the previous node, but in the case of a return from the middle of the loop, there is no put, thus causing a memory leak. Hence store these mid-loop return values in variable err and add a new label err_node_put which puts the previous node and returns err. Change six mid-loop return statements to point to this new label instead. Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> --- drivers/pci/controller/pci-tegra.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)