Re: [PATCH] PCI: mediatek-gen3: Fix refcount leak in mtk_pcie_init_irq_domains

[Miaoqian Lin]

Hi Miles,

On 2022/5/27 16:45, Miles Chen wrote:
> Hi Miaoqian,
>
>> Signed-off-by: Miaoqian Lin <linmq006@xxxxxxxxx>
>> ---
>> drivers/pci/controller/pcie-mediatek-gen3.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
>> index 3e8d70bfabc6..da8e9db0abdf 100644
>> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
>> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
>> @@ -600,6 +600,7 @@ static int mtk_pcie_init_irq_domains(struct mtk_gen3_pcie *pcie)
>> 						  &intx_domain_ops, pcie);
>> 	if (!pcie->intx_domain) {
>> 		dev_err(dev, "failed to create INTx IRQ domain\n");
>> +		of_node_put(intc_node);
>> 		return -ENODEV;
>> 	}
> Thanks for doing this.
>
> I checked mtk_pcie_init_irq_domains() and there are multiple exit paths like
> err_msi_domain and err_msi_bottom_domain and the normal path which also
> need of_node_put(intc_node).

Thanks for your reply,

I didn't add of_node_put() in other paths because I am not sure if the reference passed through irq_domain_add_linear(), since intc_node is passed to irq_domain_add_linear().

__irq_domain_add() keeps &node->fwnode in the irq_domain structure.

and use fwnode_handle_get() to get the reference of fwnode, but I still uncertain.

If the reference don't needed anymore after irq_domain_add_linear(),

your suggestion looks fine, and I will submit v2.

> Maybe we can move the of_node_put(intc_node) to #54 below and cover
> all possible paths?
>
>
> cheers,
> Miles
>
> e.g.,
>
> static int mtk_pcie_init_irq_domains(struct mtk_gen3_pcie *pcie)
> {
> 	struct device *dev = pcie->dev;
> 	struct device_node *intc_node, *node = dev->of_node;
> 	int ret;
>
> 	raw_spin_lock_init(&pcie->irq_lock);
>
> 	/* Setup INTx */
> 	intc_node = of_get_child_by_name(node, "interrupt-controller");
> 	if (!intc_node) {
> 		dev_err(dev, "missing interrupt-controller node\n");
> 		return -ENODEV;
> 	}
>
> 	pcie->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,
> 						  &intx_domain_ops, pcie);
> 	of_node_put(intc_node);
> 	if (!pcie->intx_domain) {
> 		dev_err(dev, "failed to create INTx IRQ domain\n");
> 		return -ENODEV;
> 	}
>
> 	/* Setup MSI */
> 	mutex_init(&pcie->lock);
>
> 	pcie->msi_bottom_domain = irq_domain_add_linear(node, PCIE_MSI_IRQS_NUM,
> 				  &mtk_msi_bottom_domain_ops, pcie);
> 	if (!pcie->msi_bottom_domain) {
> 		dev_err(dev, "failed to create MSI bottom domain\n");
> 		ret = -ENODEV;
> 		goto err_msi_bottom_domain;
> 	}
>
> 	pcie->msi_domain = pci_msi_create_irq_domain(dev->fwnode,
> 						     &mtk_msi_domain_info,
> 						     pcie->msi_bottom_domain);
> 	if (!pcie->msi_domain) {
> 		dev_err(dev, "failed to create MSI domain\n");
> 		ret = -ENODEV;
> 		goto err_msi_domain;
> 	}
>
> 	return 0;
>
> err_msi_domain:
> 	irq_domain_remove(pcie->msi_bottom_domain);
> err_msi_bottom_domain:
> 	irq_domain_remove(pcie->intx_domain);
>
> 	return ret;
> }
>> -- 
>> 2.25.1
>