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

Manivannan Sadhasivam
2026-04-08T14:10:38Z

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

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