linux-kernel - Re: [PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250912232348.GA1653056@bhelgaas>
Date: Fri, 12 Sep 2025 18:23:48 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: manivannan.sadhasivam@....qualcomm.com
Cc: Manivannan Sadhasivam <mani@...nel.org>,
	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>,
	Saravana Kannan <saravanak@...gle.com>, linux-pci@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org,
	Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>,
	Brian Norris <briannorris@...omium.org>
Subject: Re: [PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset'
 pointers to struct qcom_pcie_port

On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> 
> DT binding allows specifying 'phy' and 'reset' properties in both host
> bridge and Root Port nodes, though specifying in the host bridge node is
> marked as deprecated. Still, the pcie-qcom driver should support both
> combinations for maintaining the DT backwards compatibility. For this
> purpose, the driver is holding the relevant pointers of these properties in
> two structs: struct qcom_pcie_port and struct qcom_pcie.
> 
> However, this causes confusion and increases the driver complexity. Hence,
> move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> result, even if these properties are specified in the host bridge node,
> the pointers will be stored in struct qcom_pcie_port as if the properties
> are specified in a single Root Port node. This logic simplifies the driver
> a lot.

> @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
>  	struct qcom_pcie_port *port;
>  	int val = assert ? 1 : 0;
>  
> -	if (list_empty(&pcie->ports))
> -		gpiod_set_value_cansleep(pcie->reset, val);
> -	else
> -		list_for_each_entry(port, &pcie->ports, list)
> -			gpiod_set_value_cansleep(port->reset, val);
> +	list_for_each_entry(port, &pcie->ports, list)
> +		gpiod_set_value_cansleep(port->reset, val);

This is so much nicer, thanks for doing this!

>  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
>  {
>  	struct device *dev = pcie->pci->dev;
> +	struct qcom_pcie_port *port;
> +	struct gpio_desc *reset;
> +	struct phy *phy;
>  	int ret;
>  
> -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> -	if (IS_ERR(pcie->phy))
> -		return PTR_ERR(pcie->phy);
> +	phy = devm_phy_optional_get(dev, "pciephy");
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);

Seems like it would be easier to integrate this fallback into
qcom_pcie_parse_port() instead if separating it into
qcom_pcie_parse_legacy_binding().

What if you did something like this in qcom_pcie_parse_port():

  qcom_pcie_parse_port
  {
      reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
				    "reset",  GPIOD_OUT_HIGH, "PERST#");
      if (IS_ERR(reset)) {
	  reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
	  if (IS_ERR(reset))
	      return PTR_ERR(reset);
      }
      ...

Then you could share all the port kzalloc and port list management.

Could do the same with the PHY stuff.

> -	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> -	if (IS_ERR(pcie->reset))
> -		return PTR_ERR(pcie->reset);
> +	reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return PTR_ERR(reset);
>  
> -	ret = phy_init(pcie->phy);
> +	ret = phy_init(phy);
>  	if (ret)
>  		return ret;
>  
> +	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->reset = reset;
> +	port->phy = phy;
> +	INIT_LIST_HEAD(&port->list);
> +	list_add_tail(&port->list, &pcie->ports);
> +
>  	return 0;
>  }