RE: [PATCH V7 02/13] PCI: host-generic: Add common helpers for parsing Root Port properties

[Sherry Sun]

> Subject: Re: [PATCH V7 02/13] PCI: host-generic: Add common helpers for
> parsing Root Port properties
> 
> On Wed, Mar 11, 2026 at 10:03:01AM +0000, Sherry Sun wrote:
> > > From: Frank Li (AI-BOT) <frank.li@xxxxxxx>
> > >
> > > > +int pci_host_common_parse_ports(struct device *dev, struct
> > > > +pci_host_bridge *bridge) {
> > > > +	int ret = -ENOENT;
> > > > +
> > > > +	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > > > +		if (!of_node_is_type(of_port, "pci"))
> > > > +			continue;
> > > > +		ret = pci_host_common_parse_port(dev, bridge, of_port);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > >
> > > AI: Returning -ENOENT when no ports are found may be unexpected by
> > > callers.
> > > Consider whether this should return 0 (success, zero ports parsed)
> > > or if callers must explicitly handle -ENOENT. Document the semantics
> clearly.
> > >
> >
> > Returning -ENOENT when no ports are found is intentional and aligns
> > with the design requirements for this use case. This pattern is common
> > in the kernel where parsing functions return -ENOENT when expected
> nodes/properties are missing.
> >
> > 1. -ENOENT semantically means  "the expected entity was not found", which
> accurately
> >     reflects the situation when no Root Port nodes exist in the device tree.
> > 2. The callers of this function need to distinguish between:
> >    - Success with ports found (return 0)
> >    - No ports found in DT (-ENOENT) - this may be a DT configuration issue,
> for controllers
> >      where ports are optional, they can explicitly check for -ENOENT and treat
> it as non-fatal.
> >    - Actual parsing errors (other negative error codes like -ENOMEM,
> > -EPROBE_DEFER)
> 
> I think no ports under bridge should treat as 0. otherwise it may broken other
> platform, which dt have not added child node yet.
> 

Hi Frank, no, it will not break the old platforms, the caller will explicitly check for
-ENOENT and treat it as non-fatal for such optional properties. You can check my patch#3.
This is a common behavior in linux, for example, we also do the same check when
calling devm_regulator_get_optional() or devm_pm_opp_of_add_table().

@@ -581,6 +582,13 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)

        pp->bridge = bridge;

+       /* Parse Root Port nodes if present */
+       ret = pci_host_common_parse_ports(dev, bridge);
+       if (ret && ret != -ENOENT) {
+               dev_err(dev, "Failed to parse Root Port nodes: %d\n", ret);
+               return ret;
+       }

Best Regards
Sherry

> Frank
> 
> >
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index
> > > > bccc7a4bdd79..9cc3f4ebb370 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -647,6 +647,7 @@ static void pci_release_host_bridge_dev(struct
> > > > device *dev)
> > > >
> > > >  	pci_free_resource_list(&bridge->windows);
> > > >  	pci_free_resource_list(&bridge->dma_ranges);
> > > > +	pci_free_resource_list(&bridge->ports);
> > >
> > > AI: pci_free_resource_list() expects struct resource_entry nodes.
> > > The ports list contains struct pci_host_port nodes. This will
> > > corrupt memory or crash. Use
> > > list_for_each_entry_safe() with proper cleanup, or define a
> > > dedicated
> > > pci_free_host_ports() function.
> >
> > Yes, thanks for catching this, pci_free_resource_list() expects struct
> > resource_entry nodes and cannot be used for struct pci_host_port nodes.
> > I will add back the cleanup function pci_host_common_delete_ports() in
> > earlier version patchset to properly handles the ports list.
> >
> > Best regards,
> > Sherry