| Message ID | 20220701131750.240170-1-windhl@126.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | powerpc: kernel: pci_dn: Add missing of_node_put() for of_get_xx API | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
On 7/1/22 06:17, Liang He wrote: > In pci_add_device_node_info(), we should use of_node_put() for the > reference 'parent' returned by of_get_parent() to keep refcount > balance. > > Fixes: cca87d303c85 ("powerpc/pci: Refactor pci_dn") > Co-authored-by: Miaoqian Lin <linmq006@gmail.com> > Signed-off-by: Liang He <windhl@126.com> > --- > arch/powerpc/kernel/pci_dn.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c > index 938ab8838ab5..aa221958007e 100644 > --- a/arch/powerpc/kernel/pci_dn.c > +++ b/arch/powerpc/kernel/pci_dn.c > @@ -330,6 +330,7 @@ struct pci_dn *pci_add_device_node_info(struct pci_controller *hose, > INIT_LIST_HEAD(&pdn->list); > parent = of_get_parent(dn); > pdn->parent = parent ? PCI_DN(parent) : NULL; NACK pdn->parent is now a long term reference so we should not do a put on parent until we pdn->parent is no longer valid. -Tyrel > + of_node_put(parent); > if (pdn->parent) > list_add_tail(&pdn->list, &pdn->parent->child_list); >
At 2022-07-02 03:47:22, "Tyrel Datwyler" <tyreld@linux.ibm.com> wrote: >On 7/1/22 06:17, Liang He wrote: >> In pci_add_device_node_info(), we should use of_node_put() for the >> reference 'parent' returned by of_get_parent() to keep refcount >> balance. >> >> Fixes: cca87d303c85 ("powerpc/pci: Refactor pci_dn") >> Co-authored-by: Miaoqian Lin <linmq006@gmail.com> >> Signed-off-by: Liang He <windhl@126.com> >> --- >> arch/powerpc/kernel/pci_dn.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c >> index 938ab8838ab5..aa221958007e 100644 >> --- a/arch/powerpc/kernel/pci_dn.c >> +++ b/arch/powerpc/kernel/pci_dn.c >> @@ -330,6 +330,7 @@ struct pci_dn *pci_add_device_node_info(struct pci_controller *hose, >> INIT_LIST_HEAD(&pdn->list); >> parent = of_get_parent(dn); >> pdn->parent = parent ? PCI_DN(parent) : NULL; >NACK > >pdn->parent is now a long term reference so we should not do a put on parent >until we pdn->parent is no longer valid. > >-Tyrel Hi, Tyrel Thanks for reviewing this code. But I think there is some confusion about the of_get_parent() and I can confirm my point when I check the 'pci_remove_device_node_info' function, which may be a second bug. In pci_remove_device_node_info(), I notice the comment, 'Drop the parent pci_dn's ref ...'. However, of_get_parent() will increase the refcount of the parent, and then the following of_node_put() will decrease the refcount, so, finally, there is no any change. Back to this case, as the 'pdn->parent' is not the reference of parent device_node, it is device_node's data, so I think it is better to keep the original meaning of refcounting, i.e, add a of_node_put() to keep the refcount balance. If I have some mis-understanding, please correct me. Thanks, Liang > >> + of_node_put(parent); >> if (pdn->parent) >> list_add_tail(&pdn->list, &pdn->parent->child_list); >>
On 7/1/22 12:47, Tyrel Datwyler wrote: > On 7/1/22 06:17, Liang He wrote: >> In pci_add_device_node_info(), we should use of_node_put() for the >> reference 'parent' returned by of_get_parent() to keep refcount >> balance. >> >> Fixes: cca87d303c85 ("powerpc/pci: Refactor pci_dn") >> Co-authored-by: Miaoqian Lin <linmq006@gmail.com> >> Signed-off-by: Liang He <windhl@126.com> >> --- >> arch/powerpc/kernel/pci_dn.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c >> index 938ab8838ab5..aa221958007e 100644 >> --- a/arch/powerpc/kernel/pci_dn.c >> +++ b/arch/powerpc/kernel/pci_dn.c >> @@ -330,6 +330,7 @@ struct pci_dn *pci_add_device_node_info(struct pci_controller *hose, >> INIT_LIST_HEAD(&pdn->list); >> parent = of_get_parent(dn); >> pdn->parent = parent ? PCI_DN(parent) : NULL; > NACK > > pdn->parent is now a long term reference so we should not do a put on parent > until we pdn->parent is no longer valid. I withdraw my NACK. On closer inspection pdn->parent is a reference to the parent struct pci_dn and not a reference to a parent struct device_node. Reviewed-by: Tyrel Datwyler <tyreld@linux.ibm.com> > > -Tyrel > >> + of_node_put(parent); >> if (pdn->parent) >> list_add_tail(&pdn->list, &pdn->parent->child_list); >> >
On Fri, 1 Jul 2022 21:17:50 +0800, Liang He wrote: > In pci_add_device_node_info(), we should use of_node_put() for the > reference 'parent' returned by of_get_parent() to keep refcount > balance. > > Applied to powerpc/next. [1/1] powerpc: kernel: pci_dn: Add missing of_node_put() for of_get_xx API https://git.kernel.org/powerpc/c/110a1fcb6c4d55144d8179983a475f17a1d6f832 cheers
diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c index 938ab8838ab5..aa221958007e 100644 --- a/arch/powerpc/kernel/pci_dn.c +++ b/arch/powerpc/kernel/pci_dn.c @@ -330,6 +330,7 @@ struct pci_dn *pci_add_device_node_info(struct pci_controller *hose, INIT_LIST_HEAD(&pdn->list); parent = of_get_parent(dn); pdn->parent = parent ? PCI_DN(parent) : NULL; + of_node_put(parent); if (pdn->parent) list_add_tail(&pdn->list, &pdn->parent->child_list);
In pci_add_device_node_info(), we should use of_node_put() for the reference 'parent' returned by of_get_parent() to keep refcount balance. Fixes: cca87d303c85 ("powerpc/pci: Refactor pci_dn") Co-authored-by: Miaoqian Lin <linmq006@gmail.com> Signed-off-by: Liang He <windhl@126.com> --- arch/powerpc/kernel/pci_dn.c | 1 + 1 file changed, 1 insertion(+)