linux-kernel - Re: [PATCH v4 2/2] PCI: xilinx: Support reset GPIO for PERST#

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: <ec74b712-4fe8-46dc-b446-6cb244a94198@topic.nl>
Date: Wed, 11 Jun 2025 08:45:32 +0200
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Manivannan Sadhasivam <mani@...nel.org>, Michal Simek
 <michal.simek@....com>, Rob Herring <robh@...nel.org>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] PCI: xilinx: Support reset GPIO for PERST#

On 10-06-2025 21:07, Bjorn Helgaas wrote:
> On Tue, Jun 10, 2025 at 04:39:04PM +0200, Mike Looijmans wrote:
>> Support providing the PERST# reset signal through a devicetree binding.
>> Thus the system no longer relies on external components to perform the
>> bus reset.
>> @@ -576,11 +577,17 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>>   	struct device *dev = &pdev->dev;
>>   	struct xilinx_pcie *pcie;
>>   	struct pci_host_bridge *bridge;
>> +	struct gpio_desc *perst_gpio;
>>   	int err;
>>   
>>   	if (!dev->of_node)
>>   		return -ENODEV;
>>   
>> +	perst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(perst_gpio))
>> +		return dev_err_probe(dev, PTR_ERR(perst_gpio),
>> +				     "Failed to request reset GPIO\n");
>> +
>>   	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*pcie));
>>   	if (!bridge)
>>   		return -ENODEV;
>> @@ -595,6 +602,13 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>>   		return err;
>>   	}
>>   
>> +	if (perst_gpio) {
>> +		msleep(PCIE_T_PVPERL_MS); /* Minimum assertion time */
>> +		gpiod_set_value_cansleep(perst_gpio, 0);
> Are we assured that PERST# was already asserted when we entered
> xilinx_pcie_probe()?

Yes, because of the GPIOD_OUT_HIGH a few lines up, the reset GPIO is asserted 
when we arrive here.


>> +		/* Initial delay to provide endpoint time to initialize */
>> +		msleep(PCIE_T_RRS_READY_MS);
> I don't think this is the right spot for PCIE_T_RRS_READY_MS, details
> in https://lore.kernel.org/r/20250610185734.GA819344@bhelgaas
>
> I guess the spec assumes that for ports that don't support speeds
> greater than 5.0 GT/s, 100ms is enough for the link to come up *and*
> the endpoint to initialize.  But since you're going to wait for the
> link to come up immediately *after* this PCIE_T_RRS_READY_MS sleep, I
> would think you could extend the timeout in xilinx_pci_wait_link_up()
> and then do the PCIE_T_RRS_READY_MS sleep.

That would change the behavior of the original driver though, which never did 
any sleep during init. But it appears to match the spec better. Note that the 
hardware is limited to 5GT/s.

M.