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

[Sherry Sun]

> 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;
> 		}

Ok, will do.

> 
> >         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.

Get your point, so seems I should just add the err_cleanup handle path like this, right?

    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)
            goto err_cleanup;
    }

    if (ret)
        return ret;

    return devm_add_action_or_reset(dev, pci_host_common_delete_ports,
                    &bridge->ports);

err_cleanup:
    pci_host_common_delete_ports(&bridge->ports);
    return ret;

Best Regards
Sherry