| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ibvk4it7th4bi6djoxshjqjh7zusbulzpndac5jtqkqovvgcei@5sycben7pqkk>
Date: Sat, 27 Dec 2025 10:12:29 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
Bartosz Golaszewski <brgl@...ev.pl>, Bartosz Golaszewski <brgl@...nel.org>, linux-pci@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, Chen-Yu Tsai <wens@...nel.org>,
Brian Norris <briannorris@...omium.org>, Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>,
Niklas Cassel <cassel@...nel.org>, Alex Elder <elder@...cstar.com>,
Chen-Yu Tsai <wenst@...omium.org>
Subject: Re: [PATCH v2 1/5] PCI: qcom: Parse PERST# from all PCIe bridge nodes
On Fri, Dec 26, 2025 at 05:24:58PM -0600, Bjorn Helgaas wrote:
> On Tue, Dec 16, 2025 at 06:21:43PM +0530, Manivannan Sadhasivam wrote:
> > Devicetree schema allows the PERST# GPIO to be present in all PCIe bridge
> > nodes, not just in Root Port node. But the current logic parses PERST# only
> > from the Root Port nodes. Though it is not causing any issue on the current
> > platforms, the upcoming platforms will have PERST# in PCIe switch
> > downstream ports also. So this requires parsing all the PCIe bridge nodes
> > for the PERST# GPIO.
> >
> > Hence, rework the parsing logic to extend to all PCIe bridge nodes starting
> > from the Root Port node. If the 'reset-gpios' property is found for a PCI
> > bridge node, the GPIO descriptor will be stored in qcom_pcie_perst::desc
> > and added to the qcom_pcie_port::perst list.
>
> > static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> > {
> > + struct qcom_pcie_perst *perst;
> > struct qcom_pcie_port *port;
> > int val = assert ? 1 : 0;
> >
> > - list_for_each_entry(port, &pcie->ports, list)
> > - gpiod_set_value_cansleep(port->reset, val);
> > + list_for_each_entry(port, &pcie->ports, list) {
> > + list_for_each_entry(perst, &port->perst, list)
> > + gpiod_set_value_cansleep(perst->desc, val);
> > + }
> >
> > usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> > }
> > @@ -1702,18 +1710,58 @@ static const struct pci_ecam_ops pci_qcom_ecam_ops = {
> > }
> > };
> >
> > -static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node)
> > +/* Parse PERST# from all nodes in depth first manner starting from @np */
> > +static int qcom_pcie_parse_perst(struct qcom_pcie *pcie,
> > + struct qcom_pcie_port *port,
> > + struct device_node *np)
> > {
> > struct device *dev = pcie->pci->dev;
> > - struct qcom_pcie_port *port;
> > + struct qcom_pcie_perst *perst;
> > struct gpio_desc *reset;
> > - struct phy *phy;
> > int ret;
> >
> > - reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> > - "reset", GPIOD_OUT_HIGH, "PERST#");
> > - if (IS_ERR(reset))
> > + if (!of_find_property(np, "reset-gpios", NULL))
> > + goto parse_child_node;
> > +
> > + reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(np), "reset",
> > + GPIOD_OUT_HIGH, "PERST#");
> > + if (IS_ERR(reset)) {
> > + /*
> > + * FIXME: GPIOLIB currently supports exclusive GPIO access only.
> > + * Non exclusive access is broken. But shared PERST# requires
> > + * non-exclusive access. So once GPIOLIB properly supports it,
> > + * implement it here.
> > + */
> > + if (PTR_ERR(reset) == -EBUSY)
> > + dev_err(dev, "Shared PERST# is not supported\n");
> > +
> > return PTR_ERR(reset);
> > + }
> > +
> > + perst = devm_kzalloc(dev, sizeof(*perst), GFP_KERNEL);
> > + if (!perst)
> > + return -ENOMEM;
> > +
> > + INIT_LIST_HEAD(&perst->list);
> > + perst->desc = reset;
> > + list_add_tail(&perst->list, &port->perst);
> > +
> > +parse_child_node:
> > + for_each_available_child_of_node_scoped(np, child) {
> > + ret = qcom_pcie_parse_perst(pcie, port, child);
>
> It looks like the perst->list will be ordered by distance from the
> root, i.e., a Root Port first, followed by downstream devices?
>
Yes.
> And qcom_perst_assert() will assert/deassert PERST# in that same
> order? Intuitively I would have expected that if there are multiple
> PERST# signals, we would assert them bottom-up, and deassert them
> top-down. Does the order matter?
>
I did't give much importance to the PERST# ordering since it doesn't matter,
atleast per base/electromechanical specs.
> I suppose maybe you plan to enhance pwrctrl so it can assert/deassert
> individual PERST# in the hierarchy?
>
No, that plan has been dropped for good. For now, PERST# will be handled
entirely by the controller drivers. Sharing the PERST# handling with pwrctrl
proved to be a pain and it looks more clean (after the API introduction) to
handle PERST# in controller drivers.
> I'm confused about qcom_perst_assert() because it's only called from
> qcom_ep_reset_assert() and qcom_ep_reset_deassert(), which are only
> called from qcom_pcie_assert_perst(). Seems like a mix of host and
> endpoint situation. I assumed pwrctrl would be used on the host.
> Maybe the "_ep_" names are not quite right? Or more likely I'm just
> misunderstanding the plan.
>
Yeah, it is a bit confusing now due to the introduction of the
'dw_pcie_ops::assert_perst' callback. But it will go away at the end of the
pwrctrl rework series and I'll fix those names by then.
> I notice you'd only applied this patch (1/5) so far on
> pci/controller/dwc-qcom. Is this patch useful by itself?
>
Yes, ofc. This patch allows the controller driver to parse and assert/deassert
PERST# from all PCI nodes, not just from the Root Port node.
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists