Re: [PATCH V11 02/12] PCI: host-generic: Add common helpers for parsing Root Port properties

[Manivannan Sadhasivam]

On Wed, Apr 08, 2026 at 06:34:02AM +0000, Sherry Sun wrote:

[...]

> > > +/**
> > > + * pci_host_common_parse_port - Parse a single Root Port node
> > > + * @dev: Device pointer
> > > + * @bridge: PCI host bridge
> > > + * @node: Device tree node of the Root Port
> > > + *
> > > + * Returns: 0 on success, negative error code on failure  */ static
> > > +int pci_host_common_parse_port(struct device *dev,
> > > +				      struct pci_host_bridge *bridge,
> > > +				      struct device_node *node)
> > > +{
> > > +	struct pci_host_port *port;
> > > +	struct gpio_desc *reset;
> > > +
> > > +	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> > > +				      "reset", GPIOD_ASIS, "PERST#");
> > 
> > Sorry, I missed this earlier.
> > 
> > Since PERST# is optional, you cannot reliably detect whether the Root Port
> > binding intentionally skipped the PERST# GPIO or legacy binding is used, just
> > by checking for PERST# in Root Port node.
> > 
> > So this helper should do 3 things:
> > 
> > 1. If PERST# is found in Root Port node, use it.
> > 2. If not, check the RC node and if present, return -ENOENT to fallback to the
> > legacy binding.
> > 3. If not found in both nodes, assume that the PERST# is not present in the
> > design, and proceed with parsing Root Port binding further.
> 
> Hi Mani, understand, does the following code looks ok for above three cases?
> 
>     /* Check if PERST# is present in Root Port node */
>     reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
>                       "reset", GPIOD_ASIS, "PERST#");
>     if (IS_ERR(reset)) {
>         /* If error is not -ENOENT, it's a real error */
>         if (PTR_ERR(reset) != -ENOENT)
>             return PTR_ERR(reset);
> 
>         /* PERST# not found in Root Port node, check RC node */
>         rc_has_reset = of_property_read_bool(dev->of_node, "reset-gpios") ||
>                    of_property_read_bool(dev->of_node, "reset-gpio");

Just:
		if (of_property_read_bool(dev->of_node, "reset-gpios") ||
		    of_property_read_bool(dev->of_node, "reset-gpio")) {
			return -ENOENT;
		} 

>         if (rc_has_reset)
>             return -ENOENT;
> 
>         /* No PERST# in either node, assume not present in design */
>         reset = NULL;
>     }
> 
>     port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>     if (!port)
>         return -ENOMEM;
> ...
> 
> > 
> > But there is one more important limitation here. Right now, this API only
> > handles PERST#. But if another vendor tries to use it and if they need other
> > properties such as PHY, clocks etc... those resources should be fetched
> > optionally only by this helper. But if the controller has a hard dependency on
> > those resources, the driver will fail to operate.
> > 
> > I don't think we can fix this limitation though and those platforms should
> > ensure that the resource dependency is correctly modeled in DT binding and
> > the DTS is validated properly. It'd be good to mention this in the kernel doc of
> > this API.
> 
> Ok, I will add a NOTE for this in this API description.
> 
> > 
> > > +	if (IS_ERR(reset))
> > > +		return PTR_ERR(reset);
> > > +
> > > +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > > +	if (!port)
> > > +		return -ENOMEM;
> > > +
> > > +	port->reset = reset;
> > > +	INIT_LIST_HEAD(&port->list);
> > > +	list_add_tail(&port->list, &bridge->ports);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * pci_host_common_parse_ports - Parse Root Port nodes from device
> > > +tree
> > > + * @dev: Device pointer
> > > + * @bridge: PCI host bridge
> > > + *
> > > + * This function iterates through child nodes of the host bridge and
> > > +parses
> > > + * Root Port properties (currently only reset GPIO).
> > > + *
> > > + * Returns: 0 on success, -ENOENT if no ports found, other negative
> > > +error codes
> > > + * on failure
> > > + */
> > > +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;
> > 
> > As Sashiko flagged, you need to make sure that devm_add_action_or_reset()
> > is added even during the error path:
> 
> Yes, it needs to be fixed. We can handle it with the following two methods, I am not sure which method is better or more preferable?
> 
> #1: register cleanup action after first successful port parse and use cleanup_registered flag to avoid duplicate register.
>     int ret = -ENOENT;    
>     bool cleanup_registered = false;
> 
>     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;
> 
>         /* Register cleanup action after first successful port parse */
>         if (!cleanup_registered) {
>             ret = devm_add_action_or_reset(dev,
>                                pci_host_common_delete_ports,
>                                &bridge->ports);

Even if you register devm_add_action_or_reset(), it won't be called when
pci_host_common_parse_port() fails since the legacy fallback will be used.

So you need to manually call pci_host_common_delete_ports() in the error path.

- Mani

-- 
மணிவண்ணன் சதாசிவம்