| Message ID | 20150804215435.9189.94410.stgit@bhelgaas-glaptop2.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[+cc Ben, Rob] On Tue, Aug 04, 2015 at 04:54:35PM -0500, Bjorn Helgaas wrote: > of_parse_phandle() returns a device_node pointer with the refcount > incremented. We should dispose of this reference when we're finished. > > Drop the reference acquired by of_parse_phandle(). I cc'd everybody who wrote or signed off on 0d5a6db3aa46 ("of: pci: add registry of MSI chips"), which added of_pci_find_msi_chip_by_node(). The existing code (before this patch) looks wrong to me, but I'm not an OF person. > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/host/pci-mvebu.c | 1 + > drivers/pci/host/pci-xgene.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > index 70aa095..67ec5e1 100644 > --- a/drivers/pci/host/pci-mvebu.c > +++ b/drivers/pci/host/pci-mvebu.c > @@ -879,6 +879,7 @@ static void mvebu_pcie_msi_enable(struct mvebu_pcie *pcie) > return; > > pcie->msi = of_pci_find_msi_chip_by_node(msi_node); > + of_node_put(msi_node); > > if (pcie->msi) > pcie->msi->dev = &pcie->pdev->dev; > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c > index a9dfb70..4c2fb1f 100644 > --- a/drivers/pci/host/pci-xgene.c > +++ b/drivers/pci/host/pci-xgene.c > @@ -514,6 +514,7 @@ static int xgene_pcie_msi_enable(struct pci_bus *bus) > if (!bus->msi) > return -ENODEV; > > + of_node_put(msi_node); > bus->msi->dev = &bus->dev; > return 0; > } >
On Mon, Aug 10, 2015 at 4:39 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > [+cc Ben, Rob] > > On Tue, Aug 04, 2015 at 04:54:35PM -0500, Bjorn Helgaas wrote: >> of_parse_phandle() returns a device_node pointer with the refcount >> incremented. We should dispose of this reference when we're finished. >> >> Drop the reference acquired by of_parse_phandle(). > > I cc'd everybody who wrote or signed off on 0d5a6db3aa46 ("of: pci: add > registry of MSI chips"), which added of_pci_find_msi_chip_by_node(). > > The existing code (before this patch) looks wrong to me, but I'm not an OF > person. In general, how DT reference counting works is broken and difficult to get right. It doesn't really matter for most cases anyway (as nodes don't get removed). I cannot tell you whether this patch is right or wrong just looking at the patch. It seems correct based on your description. Rob > >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> --- >> drivers/pci/host/pci-mvebu.c | 1 + >> drivers/pci/host/pci-xgene.c | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c >> index 70aa095..67ec5e1 100644 >> --- a/drivers/pci/host/pci-mvebu.c >> +++ b/drivers/pci/host/pci-mvebu.c >> @@ -879,6 +879,7 @@ static void mvebu_pcie_msi_enable(struct mvebu_pcie *pcie) >> return; >> >> pcie->msi = of_pci_find_msi_chip_by_node(msi_node); >> + of_node_put(msi_node); >> >> if (pcie->msi) >> pcie->msi->dev = &pcie->pdev->dev; >> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c >> index a9dfb70..4c2fb1f 100644 >> --- a/drivers/pci/host/pci-xgene.c >> +++ b/drivers/pci/host/pci-xgene.c >> @@ -514,6 +514,7 @@ static int xgene_pcie_msi_enable(struct pci_bus *bus) >> if (!bus->msi) >> return -ENODEV; >> >> + of_node_put(msi_node); >> bus->msi->dev = &bus->dev; >> return 0; >> } >>
On Tue, Aug 04, 2015 at 10:54:35PM +0100, Bjorn Helgaas wrote: > of_parse_phandle() returns a device_node pointer with the refcount > incremented. We should dispose of this reference when we're finished. > > Drop the reference acquired by of_parse_phandle(). > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/host/pci-mvebu.c | 1 + > drivers/pci/host/pci-xgene.c | 1 + > 2 files changed, 2 insertions(+) It looks fine to me: Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > index 70aa095..67ec5e1 100644 > --- a/drivers/pci/host/pci-mvebu.c > +++ b/drivers/pci/host/pci-mvebu.c > @@ -879,6 +879,7 @@ static void mvebu_pcie_msi_enable(struct mvebu_pcie *pcie) > return; > > pcie->msi = of_pci_find_msi_chip_by_node(msi_node); > + of_node_put(msi_node); > > if (pcie->msi) > pcie->msi->dev = &pcie->pdev->dev; > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c > index a9dfb70..4c2fb1f 100644 > --- a/drivers/pci/host/pci-xgene.c > +++ b/drivers/pci/host/pci-xgene.c > @@ -514,6 +514,7 @@ static int xgene_pcie_msi_enable(struct pci_bus *bus) > if (!bus->msi) > return -ENODEV; > > + of_node_put(msi_node); > bus->msi->dev = &bus->dev; > return 0; > } >
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index 70aa095..67ec5e1 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -879,6 +879,7 @@ static void mvebu_pcie_msi_enable(struct mvebu_pcie *pcie) return; pcie->msi = of_pci_find_msi_chip_by_node(msi_node); + of_node_put(msi_node); if (pcie->msi) pcie->msi->dev = &pcie->pdev->dev; diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c index a9dfb70..4c2fb1f 100644 --- a/drivers/pci/host/pci-xgene.c +++ b/drivers/pci/host/pci-xgene.c @@ -514,6 +514,7 @@ static int xgene_pcie_msi_enable(struct pci_bus *bus) if (!bus->msi) return -ENODEV; + of_node_put(msi_node); bus->msi->dev = &bus->dev; return 0; }
of_parse_phandle() returns a device_node pointer with the refcount incremented. We should dispose of this reference when we're finished. Drop the reference acquired by of_parse_phandle(). Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/host/pci-mvebu.c | 1 + drivers/pci/host/pci-xgene.c | 1 + 2 files changed, 2 insertions(+)