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

Sherry Sun
2026-04-09T02:58:35Z

> 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