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

[Manivannan Sadhasivam]

On Thu, Apr 09, 2026 at 02:58:21AM +0000, Sherry Sun wrote:
> > 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;
> 

Yes!

- Mani

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