Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Manikanta Maddireddy <mmaddireddy@nvidia.com>
To: Thierry Reding <thierry.reding@gmail.com>, <bhelgaas@google.com>
Cc: <robh+dt@kernel.org>, <mark.rutland@arm.com>,
	<jonathanh@nvidia.com>, <lorenzo.pieralisi@arm.com>,
	<vidyas@nvidia.com>, <linux-tegra@vger.kernel.org>,
	<linux-pci@vger.kernel.org>, <devicetree@vger.kernel.org>
Subject: Re: [PATCH V4 22/28] PCI: tegra: Access endpoint config only if PCIe link is up
Date: Mon, 10 Jun 2019 10:08:16 +0530
Message-ID: <ca34eb24-8696-576f-26bc-8d6141f81a41@nvidia.com> (raw)
In-Reply-To: <09bcc121-eaca-3866-d0ef-7806503e883f@nvidia.com>



On 04-Jun-19 7:40 PM, Manikanta Maddireddy wrote:
>
> On 04-Jun-19 6:44 PM, Thierry Reding wrote:
>> On Thu, May 16, 2019 at 11:23:01AM +0530, Manikanta Maddireddy wrote:
>>> Few endpoints like Wi-Fi supports power on/off and to leverage that
>>> root port must support hot-plug and hot-unplug. Tegra PCIe doesn't
>>> support hot-plug and hot-unplug, however it supports endpoint power
>>> on/off feature as follows,
>>>  - Power off sequence:
>>>    - Transition of PCIe link to L2
>>>    - Power off endpoint
>>>    - Leave root port in power up state with the link in L2
>>>  - Power on sequence:
>>>    - Power on endpoint
>>>    - Apply hot reset to get PCIe link up
>>>
>>> PCIe client driver stops accessing PCIe endpoint config and BAR registers
>>> after endpoint is powered off. However, software applications like x11
>>> server or lspci can access endpoint config registers in which case
>>> host controller raises "response decoding" errors. To avoid this scenario,
>>> add PCIe link up check in config read and write callback functions before
>>> accessing endpoint config registers.
>>>
>>> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
>>> ---
>>> V4: No change
>>>
>>> V3: Update the commit log with explanation for the need of this patch
>>>
>>> V2: Change tegra_pcie_link_status() to tegra_pcie_link_up()
>>>
>>>  drivers/pci/controller/pci-tegra.c | 38 ++++++++++++++++++++++++++++++
>>>  1 file changed, 38 insertions(+)
>> This still doesn't look right to me conceptually. If somebody wants to
>> access the PCI devices after the kernel has powered them off, why can't
>> we just power the devices back on so that we allow userspace to properly
>> access the devices?
> 1. WiFi devices provides power-off feature for power saving in mobiles.
> When WiFi is turned off we shouldn't power on the HW back without user
> turning it back on.
> 2. When ever user process tries to access config space, it'll end up
> in these functions. We cannot have is_powered_on check in config read/write
> callbacks.
> 3. WiFi power on/off is device specific feature, we shouldn't handle it
> in PCI subsystem or host controller driver.
>
>> Or if that's not what we want, shouldn't we add something to the core
>> PCI infrastructure to let us deal with this? It seems like this is some
>> general problem that would apply to every PCI device and host bridge
>> driver. Having each driver implement this logic separately doesn't seem
>> like a good idea to me.
>>
>> Thierry
> This should be handled by hotplug feature, whenever endpoint is powered-off/
> removed from the slot, hot unplug event should take care of it. Unfortunately
> Tegra PCIe doesn't support hotplug feature.
>
> Manikanta

Hi Bjorn,

I thought about your comment in https://patchwork.ozlabs.org/patch/1084204/ again.
What if I add link up check in tegra_pcie_isr() and make "response decoding error"
as debug print? EP Config access will happen when link is down, but "Response
decoding error" print comes only if debug log is enabled. This way we can avoid
race issue in config accessors and we get prints when debug logs are enabled.

Thierry,
Please share your inputs as well.

Manikanta
 

>>> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
>>> index d20c88a79e00..33f4dfab9e35 100644
>>> --- a/drivers/pci/controller/pci-tegra.c
>>> +++ b/drivers/pci/controller/pci-tegra.c
>>> @@ -428,6 +428,14 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset)
>>>  	return readl(pcie->pads + offset);
>>>  }
>>>  
>>> +static bool tegra_pcie_link_up(struct tegra_pcie_port *port)
>>> +{
>>> +	u32 value;
>>> +
>>> +	value = readl(port->base + RP_LINK_CONTROL_STATUS);
>>> +	return !!(value & RP_LINK_CONTROL_STATUS_DL_LINK_ACTIVE);
>>> +}
>>> +
>>>  /*
>>>   * The configuration space mapping on Tegra is somewhat similar to the ECAM
>>>   * defined by PCIe. However it deviates a bit in how the 4 bits for extended
>>> @@ -493,20 +501,50 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
>>>  static int tegra_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
>>>  				  int where, int size, u32 *value)
>>>  {
>>> +	struct tegra_pcie *pcie = bus->sysdata;
>>> +	struct pci_dev *bridge;
>>> +	struct tegra_pcie_port *port;
>>> +
>>>  	if (bus->number == 0)
>>>  		return pci_generic_config_read32(bus, devfn, where, size,
>>>  						 value);
>>>  
>>> +	bridge = pcie_find_root_port(bus->self);
>>> +
>>> +	list_for_each_entry(port, &pcie->ports, list)
>>> +		if (port->index + 1 == PCI_SLOT(bridge->devfn))
>>> +			break;
>>> +
>>> +	/* If there is no link, then there is no device */
>>> +	if (!tegra_pcie_link_up(port)) {
>>> +		*value = 0xffffffff;
>>> +		return PCIBIOS_DEVICE_NOT_FOUND;
>>> +	}
>>> +
>>>  	return pci_generic_config_read(bus, devfn, where, size, value);
>>>  }
>>>  
>>>  static int tegra_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
>>>  				   int where, int size, u32 value)
>>>  {
>>> +	struct tegra_pcie *pcie = bus->sysdata;
>>> +	struct tegra_pcie_port *port;
>>> +	struct pci_dev *bridge;
>>> +
>>>  	if (bus->number == 0)
>>>  		return pci_generic_config_write32(bus, devfn, where, size,
>>>  						  value);
>>>  
>>> +	bridge = pcie_find_root_port(bus->self);
>>> +
>>> +	list_for_each_entry(port, &pcie->ports, list)
>>> +		if (port->index + 1 == PCI_SLOT(bridge->devfn))
>>> +			break;
>>> +
>>> +	/* If there is no link, then there is no device */
>>> +	if (!tegra_pcie_link_up(port))
>>> +		return PCIBIOS_DEVICE_NOT_FOUND;
>>> +
>>>  	return pci_generic_config_write(bus, devfn, where, size, value);
>>>  }
>>>  
>>> -- 
>>> 2.17.1
>>>


  reply index

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-16  5:52 [PATCH V4 00/28] Enable Tegra PCIe root port features Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 01/28] soc/tegra: pmc: Export tegra_powergate_power_on() Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 02/28] PCI: tegra: Handle failure cases in tegra_pcie_power_on() Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 03/28] PCI: tegra: Rearrange Tegra PCIe driver functions Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 04/28] PCI: tegra: Mask AFI_INTR in runtime suspend Manikanta Maddireddy
2019-06-04 13:08   ` Thierry Reding
2019-05-16  5:52 ` [PATCH V4 05/28] PCI: tegra: Fix PCIe host power up sequence Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 06/28] PCI: tegra: Add PCIe Gen2 link speed support Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 07/28] PCI: tegra: Advertise PCIe Advanced Error Reporting (AER) capability Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 08/28] PCI: tegra: Program UPHY electrical settings for Tegra210 Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 09/28] PCI: tegra: Enable opportunistic UpdateFC and ACK Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 10/28] PCI: tegra: Disable AFI dynamic clock gating Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 11/28] PCI: tegra: Process pending DLL transactions before entering L1 or L2 Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 12/28] PCI: tegra: Enable PCIe xclk clock clamping Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 13/28] PCI: tegra: Increase the deskew retry time Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 14/28] PCI: tegra: Add SW fixup for RAW violations Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 15/28] PCI: tegra: Update flow control timer frequency in Tegra210 Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 16/28] PCI: tegra: Set target speed as Gen1 before starting LTSSM Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 17/28] PCI: tegra: Fix PLLE power down issue due to CLKREQ# signal Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 18/28] PCI: tegra: Program AFI_CACHE* registers only for Tegra20 Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 19/28] PCI: tegra: Change PRSNT_SENSE IRQ log to debug Manikanta Maddireddy
2019-05-16  5:52 ` [PATCH V4 20/28] PCI: tegra: Use legacy IRQ for port service drivers Manikanta Maddireddy
2019-05-20 20:37   ` Bjorn Helgaas
2019-05-21  9:07     ` Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 21/28] PCI: tegra: Add AFI_PEX2_CTRL reg offset as part of soc struct Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 22/28] PCI: tegra: Access endpoint config only if PCIe link is up Manikanta Maddireddy
2019-06-04 13:14   ` Thierry Reding
2019-06-04 14:10     ` Manikanta Maddireddy
2019-06-10  4:38       ` Manikanta Maddireddy [this message]
2019-06-13 14:39         ` Lorenzo Pieralisi
2019-06-13 15:42           ` Thierry Reding
2019-06-17 10:01             ` Manikanta Maddireddy
2019-06-17 11:47               ` Thierry Reding
2019-06-17 19:30                 ` Bjorn Helgaas
2019-06-18  5:36                   ` Manikanta Maddireddy
2019-06-18 10:49                     ` Thierry Reding
2019-06-18 12:32                       ` Johannes Berg
2019-06-18 13:40                         ` Thierry Reding
2019-06-18 14:48                           ` Johannes Berg
2019-06-19 13:38                         ` Bjorn Helgaas
2019-06-19 13:40                           ` Johannes Berg
2019-05-16  5:53 ` [PATCH V4 23/28] dt-bindings: pci: tegra: Document PCIe DPD pinctrl optional prop Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 24/28] arm64: tegra: Add PEX DPD states as pinctrl properties Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 25/28] PCI: tegra: Put PEX CLK & BIAS pads in DPD mode Manikanta Maddireddy
2019-05-16  5:53 ` [PATCH V4 26/28] PCI: Add DT binding for "reset-gpios" property Manikanta Maddireddy
2019-06-17 11:30   ` Thierry Reding
2019-06-17 11:38     ` Manikanta Maddireddy
2019-06-17 11:48       ` Thierry Reding
2019-05-16  5:53 ` [PATCH V4 27/28] PCI: tegra: Add support for GPIO based PERST# Manikanta Maddireddy
2019-06-04 13:22   ` Thierry Reding
2019-06-13 15:24     ` Lorenzo Pieralisi
2019-06-14 10:37       ` Manikanta Maddireddy
2019-06-14 14:32         ` Lorenzo Pieralisi
2019-06-14 14:38           ` Manikanta Maddireddy
2019-06-14 14:50             ` Lorenzo Pieralisi
2019-06-14 14:56               ` Manikanta Maddireddy
2019-06-14 15:23               ` Thierry Reding
2019-06-14 15:59                 ` Lorenzo Pieralisi
2019-06-14 16:30                   ` Manikanta Maddireddy
2019-06-14 16:53                     ` Lorenzo Pieralisi
2019-06-14 17:23                       ` Manikanta Maddireddy
2019-06-17  9:48                         ` Lorenzo Pieralisi
2019-06-17 10:27                           ` Manikanta Maddireddy
2019-06-17 10:39                             ` Lorenzo Pieralisi
2019-06-17 11:29                         ` Thierry Reding
2019-06-17 11:26                       ` Thierry Reding
2019-05-16  5:53 ` [PATCH V4 28/28] PCI: tegra: Change link retry log level to debug Manikanta Maddireddy
2019-06-04 13:22   ` Thierry Reding
2019-05-16 13:12 ` [PATCH V4 00/28] Enable Tegra PCIe root port features Bjorn Helgaas
2019-05-17  8:38   ` Manikanta Maddireddy
2019-06-10  4:45 ` Manikanta Maddireddy
2019-06-10 17:33   ` Lorenzo Pieralisi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ca34eb24-8696-576f-26bc-8d6141f81a41@nvidia.com \
    --to=mmaddireddy@nvidia.com \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jonathanh@nvidia.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=vidyas@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox