Re: [PATCH v11 2/3] of: Factor arguments passed to of_map_id() into a struct

[Bjorn Helgaas]

[cc->to: Richard, Lucas for pci-imx6.c question]

On Wed, Mar 25, 2026 at 04:38:23PM +0530, Vijayanand Jitta wrote:
> From: Charan Teja Kalla <charan.kalla@xxxxxxxxxxxxxxxx>
> 
> Change of_map_id() to take a pointer to struct of_phandle_args
> instead of passing target device node and translated IDs separately.
> Update all callers accordingly.
> 
> Add an explicit filter_np parameter to of_map_id() and of_map_msi_id()
> to separate the filter input from the output. Previously, the target
> parameter served dual purpose: as an input filter (if non-NULL, only
> match entries targeting that node) and as an output (receiving the
> matched node with a reference held). Now filter_np is the explicit
> input filter and arg->np is the pure output.
> 
> Previously, of_map_id() would call of_node_put() on the matched node
> when a filter was provided, making reference ownership inconsistent.
> Remove this internal of_node_put() call so that of_map_id() now always
> transfers ownership of the matched node reference to the caller via
> arg->np. Callers are now consistently responsible for releasing this
> reference with of_node_put(arg->np) when done.
> ...

Not actually part of *this* patch, and AFAICS this patch is correct
as-is, but is it necessary to have different logic around
of_node_put() for imx_pcie_add_lut_by_rid() and
apple_pcie_enable_device()?

> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1137,6 +1137,8 @@ static void imx_pcie_remove_lut(struct imx_pcie *imx_pcie, u16 rid)
>  
>  static int imx_pcie_add_lut_by_rid(struct imx_pcie *imx_pcie, u32 rid)
>  {
> +	struct of_phandle_args iommu_spec = {};
> +	struct of_phandle_args msi_spec = {};
>  	struct device *dev = imx_pcie->pci->dev;
>  	struct device_node *target;
>  	u32 sid_i, sid_m;
> @@ -1144,7 +1146,12 @@ static int imx_pcie_add_lut_by_rid(struct imx_pcie *imx_pcie, u32 rid)
>  	u32 sid = 0;
>  
>  	target = NULL;
> -	err_i = of_map_iommu_id(dev->of_node, rid, &target, &sid_i);
> +	err_i = of_map_iommu_id(dev->of_node, rid, &iommu_spec);
> +	if (!err_i) {
> +		target = iommu_spec.np;
> +		sid_i = iommu_spec.args[0];
> +	}
> +
>  	if (target) {
>  		of_node_put(target);

Here it's conditional on "target" even though of_node_put() checks
internally for non-NULL, so it would be safe without the conditional
here.

>  	} else {
> @@ -1156,8 +1163,11 @@ static int imx_pcie_add_lut_by_rid(struct imx_pcie *imx_pcie, u32 rid)
>  		err_i = -EINVAL;
>  	}
>  
> -	target = NULL;
> -	err_m = of_map_msi_id(dev->of_node, rid, &target, &sid_m);
> +	err_m = of_map_msi_id(dev->of_node, rid, NULL, &msi_spec);
> +	if (!err_m) {
> +		target = msi_spec.np;
> +		sid_m = msi_spec.args[0];
> +	}
>  
>  	/*
>  	 *   err_m      target

And here (outside the diff context) we also call of_node_put()
conditionally:

  ...
  else if (target)
    of_node_put(target);

> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index a0937b7b3c4d..c2cffc0659f4 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -755,6 +755,7 @@ static int apple_pcie_enable_device(struct pci_host_bridge *bridge, struct pci_d
>  {
>  	u32 sid, rid = pci_dev_id(pdev);
>  	struct apple_pcie_port *port;
> +	struct of_phandle_args iommu_spec = {};
>  	int idx, err;
>  
>  	port = apple_pcie_get_port(pdev);
> @@ -764,10 +765,12 @@ static int apple_pcie_enable_device(struct pci_host_bridge *bridge, struct pci_d
>  	dev_dbg(&pdev->dev, "added to bus %s, index %d\n",
>  		pci_name(pdev->bus->self), port->idx);
>  
> -	err = of_map_iommu_id(port->pcie->dev->of_node, rid, NULL, &sid);
> +	err = of_map_iommu_id(port->pcie->dev->of_node, rid, &iommu_spec);
>  	if (err)
>  		return err;
>  
> +	of_node_put(iommu_spec.np);

Here we call of_node_put() unconditionally.

I think it would be much nicer if imx_pcie_add_lut_by_rid() used the
same style as apple_pcie_enable_device() and did the of_node_put()
unconditionally.  That would untangle the function a bit and make it
easier to analyze.

> +	sid = iommu_spec.args[0];
>  	mutex_lock(&port->pcie->lock);
>  
>  	idx = bitmap_find_free_region(port->sid_map, port->sid_map_sz, 0);