The conversion of this file to use the agnostic GPIO API has introduced
a new early return where the refcounts of two device nodes (parent and
child) are not decremented.
Given that the device nodes are not required outside the loops where
they are used, and to avoid potential bugs every time a new error path
is introduced to the loop, the _scoped() versions of the macros have
been applied. The bug was introduced recently, and the fix is not
relevant for old stable kernels that might not support the scoped()
variant.
Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API")
Signed-off-by: Javier Carrasco <[email protected]>
---
This bug was found while analyzing the code and I don't have hardware to
validate it beyond compilation and static analysis. Any test with real
hardware to make sure there are no regressions is always welcome.
The dev_err() messages have not been converted into dev_err_probe() to
keep the current format, but I am open to convert them if preferred.
---
drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index d1f54f188e71..0a29136491b8 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
struct device_node *node)
{
struct device *dev = &pdev->dev;
- struct device_node *parent, *child;
int ret, slot, i;
- for_each_available_child_of_node(node, parent) {
- for_each_available_child_of_node(parent, child) {
+ for_each_available_child_of_node_scoped(node, parent) {
+ for_each_available_child_of_node_scoped(parent, child) {
i = pcie->num_slots;
pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev,
@@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
pcie->num_slots++;
if (pcie->num_slots > MAX_PCI_SLOTS) {
dev_err(dev, "Too many PCI slots!\n");
- ret = -EINVAL;
- goto put_node;
+ return -EINVAL;
}
ret = of_pci_get_devfn(child);
if (ret < 0) {
dev_err(dev, "failed to parse devfn: %d\n", ret);
- goto put_node;
+ return ret;
}
slot = PCI_SLOT(ret);
@@ -439,10 +437,8 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
pcie->reset_names[i] = devm_kasprintf(dev, GFP_KERNEL,
"pcie_perst_%d",
slot);
- if (!pcie->reset_names[i]) {
- ret = -ENOMEM;
- goto put_node;
- }
+ if (!pcie->reset_names[i])
+ return -ENOMEM;
gpiod_set_consumer_name(pcie->id_reset_gpio[i],
pcie->reset_names[i]);
@@ -450,11 +446,6 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
}
return 0;
-
-put_node:
- of_node_put(child);
- of_node_put(parent);
- return ret;
}
static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
---
base-commit: d35b2284e966c0bef3e2182a5c5ea02177dd32e4
change-id: 20240609-pcie-kirin-memleak-18c83a31d111
Best regards,
--
Javier Carrasco <[email protected]>
On Sun, Jun 09, 2024 at 12:56:14PM +0200, Javier Carrasco wrote:
> The conversion of this file to use the agnostic GPIO API has introduced
> a new early return where the refcounts of two device nodes (parent and
> child) are not decremented.
>
> Given that the device nodes are not required outside the loops where
> they are used, and to avoid potential bugs every time a new error path
> is introduced to the loop, the _scoped() versions of the macros have
> been applied. The bug was introduced recently, and the fix is not
> relevant for old stable kernels that might not support the scoped()
> variant.
Looks reasonable to me,
Reviewed-by: Andy Shevchenko <[email protected]>
Thanks!
--
With Best Regards,
Andy Shevchenko
On Sun, Jun 09, 2024 at 12:56:14PM +0200, Javier Carrasco wrote:
> The conversion of this file to use the agnostic GPIO API has introduced
> a new early return where the refcounts of two device nodes (parent and
> child) are not decremented.
>
> Given that the device nodes are not required outside the loops where
> they are used, and to avoid potential bugs every time a new error path
> is introduced to the loop, the _scoped() versions of the macros have
> been applied. The bug was introduced recently, and the fix is not
> relevant for old stable kernels that might not support the scoped()
> variant.
>
> Fixes: 1d38f9d89f85 ("PCI: kirin: Convert to use agnostic GPIO API")
> Signed-off-by: Javier Carrasco <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
> ---
> This bug was found while analyzing the code and I don't have hardware to
> validate it beyond compilation and static analysis. Any test with real
> hardware to make sure there are no regressions is always welcome.
>
> The dev_err() messages have not been converted into dev_err_probe() to
> keep the current format, but I am open to convert them if preferred.
Sure, please do it in a separate patch.
- Mani
> ---
> drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index d1f54f188e71..0a29136491b8 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -403,11 +403,10 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
> struct device_node *node)
> {
> struct device *dev = &pdev->dev;
> - struct device_node *parent, *child;
> int ret, slot, i;
>
> - for_each_available_child_of_node(node, parent) {
> - for_each_available_child_of_node(parent, child) {
> + for_each_available_child_of_node_scoped(node, parent) {
> + for_each_available_child_of_node_scoped(parent, child) {
> i = pcie->num_slots;
>
> pcie->id_reset_gpio[i] = devm_fwnode_gpiod_get_index(dev,
> @@ -424,14 +423,13 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
> pcie->num_slots++;
> if (pcie->num_slots > MAX_PCI_SLOTS) {
> dev_err(dev, "Too many PCI slots!\n");
> - ret = -EINVAL;
> - goto put_node;
> + return -EINVAL;
> }
>
> ret = of_pci_get_devfn(child);
> if (ret < 0) {
> dev_err(dev, "failed to parse devfn: %d\n", ret);
> - goto put_node;
> + return ret;
> }
>
> slot = PCI_SLOT(ret);
> @@ -439,10 +437,8 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
> pcie->reset_names[i] = devm_kasprintf(dev, GFP_KERNEL,
> "pcie_perst_%d",
> slot);
> - if (!pcie->reset_names[i]) {
> - ret = -ENOMEM;
> - goto put_node;
> - }
> + if (!pcie->reset_names[i])
> + return -ENOMEM;
>
> gpiod_set_consumer_name(pcie->id_reset_gpio[i],
> pcie->reset_names[i]);
> @@ -450,11 +446,6 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
> }
>
> return 0;
> -
> -put_node:
> - of_node_put(child);
> - of_node_put(parent);
> - return ret;
> }
>
> static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
>
> ---
> base-commit: d35b2284e966c0bef3e2182a5c5ea02177dd32e4
> change-id: 20240609-pcie-kirin-memleak-18c83a31d111
>
> Best regards,
> --
> Javier Carrasco <[email protected]>
>
--
மணிவண்ணன் சதாசிவம்