| Message ID | 20220704062608.273440-1-windhl@126.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | pci: controller: mobiveil: Hold reference returned by of_parse_phandle() | expand |
On Mon, Jul 04, 2022 at 02:26:08PM +0800, Liang He wrote: > In ls_g4_pcie_probe(), we should hold the reference returned by > of_parse_phandle() and use it to call of_node_put() for refcount > balance. > > Fixes: d29ad70a813b ("PCI: mobiveil: Add PCIe Gen4 RC driver for Layerscape SoCs") > Co-authored-by: Miaoqian Lin <linmq006@gmail.com> > Signed-off-by: Liang He <windhl@126.com> > --- > drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c b/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c > index d7b7350f02dd..075aa487f92e 100644 > --- a/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c > +++ b/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c > @@ -204,13 +204,15 @@ static int __init ls_g4_pcie_probe(struct platform_device *pdev) > struct pci_host_bridge *bridge; > struct mobiveil_pcie *mv_pci; > struct ls_g4_pcie *pcie; > - struct device_node *np = dev->of_node; > + struct device_node *np = dev->of_node, *parse_np; > int ret; > > - if (!of_parse_phandle(np, "msi-parent", 0)) { > + parse_np = of_parse_phandle(np, "msi-parent", 0); I don't understand what's going on here. Where is "msi-parent" actually used? If we just need to know whether "msi-parent" exists, can we use of_property_read_bool() instead? Or can we call of_parse_phandle() closer to where it is used? > + if (!parse_np) { > dev_err(dev, "Failed to find msi-parent\n"); > return -EINVAL; > } > + of_node_put(parse_np); > > bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie)); > if (!bridge) > -- > 2.25.1 >
At 2022-07-15 00:37:51, "Bjorn Helgaas" <helgaas@kernel.org> wrote: >On Mon, Jul 04, 2022 at 02:26:08PM +0800, Liang He wrote: >> In ls_g4_pcie_probe(), we should hold the reference returned by >> of_parse_phandle() and use it to call of_node_put() for refcount >> balance. >> >> Fixes: d29ad70a813b ("PCI: mobiveil: Add PCIe Gen4 RC driver for Layerscape SoCs") >> Co-authored-by: Miaoqian Lin <linmq006@gmail.com> >> Signed-off-by: Liang He <windhl@126.com> >> --- >> drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c b/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c >> index d7b7350f02dd..075aa487f92e 100644 >> --- a/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c >> +++ b/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c >> @@ -204,13 +204,15 @@ static int __init ls_g4_pcie_probe(struct platform_device *pdev) >> struct pci_host_bridge *bridge; >> struct mobiveil_pcie *mv_pci; >> struct ls_g4_pcie *pcie; >> - struct device_node *np = dev->of_node; >> + struct device_node *np = dev->of_node, *parse_np; >> int ret; >> >> - if (!of_parse_phandle(np, "msi-parent", 0)) { >> + parse_np = of_parse_phandle(np, "msi-parent", 0); > >I don't understand what's going on here. Where is "msi-parent" >actually used? If we just need to know whether "msi-parent" exists, >can we use of_property_read_bool() instead? Or can we call >of_parse_phandle() closer to where it is used? > Hi, Bjorn, I think this code is used to check the existence of the device_node whose property is "msi-parent". of_property_read_bool() can only be used to check current device_node 'np'. If this is right, I think we can use a helper to make thing simple like this: https://lore.kernel.org/all/20220712061417.363145-1-windhl@126.com/ If it is ok, I will prepare the helper in new version. Thanks again, Liang >> + if (!parse_np) { >> dev_err(dev, "Failed to find msi-parent\n"); >> return -EINVAL; >> } >> + of_node_put(parse_np); >> >> bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie)); >> if (!bridge) >> -- >> 2.25.1 >>
On Fri, Jul 15, 2022 at 08:06:46AM +0800, Liang He wrote: > At 2022-07-15 00:37:51, "Bjorn Helgaas" <helgaas@kernel.org> wrote: > >On Mon, Jul 04, 2022 at 02:26:08PM +0800, Liang He wrote: > >> In ls_g4_pcie_probe(), we should hold the reference returned by > >> of_parse_phandle() and use it to call of_node_put() for refcount > >> balance. > >> > >> Fixes: d29ad70a813b ("PCI: mobiveil: Add PCIe Gen4 RC driver for Layerscape SoCs") > >> Co-authored-by: Miaoqian Lin <linmq006@gmail.com> > >> Signed-off-by: Liang He <windhl@126.com> > >> --- > >> drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c b/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c > >> index d7b7350f02dd..075aa487f92e 100644 > >> --- a/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c > >> +++ b/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c > >> @@ -204,13 +204,15 @@ static int __init ls_g4_pcie_probe(struct platform_device *pdev) > >> struct pci_host_bridge *bridge; > >> struct mobiveil_pcie *mv_pci; > >> struct ls_g4_pcie *pcie; > >> - struct device_node *np = dev->of_node; > >> + struct device_node *np = dev->of_node, *parse_np; > >> int ret; > >> > >> - if (!of_parse_phandle(np, "msi-parent", 0)) { > >> + parse_np = of_parse_phandle(np, "msi-parent", 0); > > > >I don't understand what's going on here. Where is "msi-parent" > >actually used? If we just need to know whether "msi-parent" exists, > >can we use of_property_read_bool() instead? Or can we call > >of_parse_phandle() closer to where it is used? > > I think this code is used to check the existence of the device_node > whose property is "msi-parent". of_property_read_bool() can only be > used to check current device_node 'np'. > > If this is right, I think we can use a helper to make thing simple > like this: > > https://lore.kernel.org/all/20220712061417.363145-1-windhl@126.com/ > > If it is ok, I will prepare the helper in new version. I dunno. The helper in that patch has nothing ftgmac100-specific in it, so if the helper is correct and useful, it seems like it should be somewhere generic so we don't end up with a zillion copies of it. Maybe Rob will chime in. I just want drivers/pci/controller/* to deal with "msi-parent" consistently. dw_pcie_host_init() uses of_property_read_bool(). ls_g4_pcie_probe(), brcm_pcie_probe(), and iproc_pcie_msi_enable() use of_parse_phandle(). It's not clear to me that these uses are fundamentally different and need to use different interfaces. > >> + if (!parse_np) { > >> dev_err(dev, "Failed to find msi-parent\n"); > >> return -EINVAL; > >> } > >> + of_node_put(parse_np); > >> > >> bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie)); > >> if (!bridge) > >> -- > >> 2.25.1 > >>
diff --git a/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c b/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c index d7b7350f02dd..075aa487f92e 100644 --- a/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c +++ b/drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c @@ -204,13 +204,15 @@ static int __init ls_g4_pcie_probe(struct platform_device *pdev) struct pci_host_bridge *bridge; struct mobiveil_pcie *mv_pci; struct ls_g4_pcie *pcie; - struct device_node *np = dev->of_node; + struct device_node *np = dev->of_node, *parse_np; int ret; - if (!of_parse_phandle(np, "msi-parent", 0)) { + parse_np = of_parse_phandle(np, "msi-parent", 0); + if (!parse_np) { dev_err(dev, "Failed to find msi-parent\n"); return -EINVAL; } + of_node_put(parse_np); bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie)); if (!bridge)
In ls_g4_pcie_probe(), we should hold the reference returned by of_parse_phandle() and use it to call of_node_put() for refcount balance. Fixes: d29ad70a813b ("PCI: mobiveil: Add PCIe Gen4 RC driver for Layerscape SoCs") Co-authored-by: Miaoqian Lin <linmq006@gmail.com> Signed-off-by: Liang He <windhl@126.com> --- drivers/pci/controller/mobiveil/pcie-layerscape-gen4.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)