linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Manikanta Maddireddy <mmaddireddy@nvidia.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	robh+dt@kernel.org, mark.rutland@arm.com, jonathanh@nvidia.com,
	vidyas@nvidia.com, linux-tegra@vger.kernel.org,
	linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
	linux-pm@vger.kernel.org, "Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Johannes Berg <johannes@sipsolutions.net>,
	linux-wireless@vger.kernel.org
Subject: Re: [PATCH V4 22/28] PCI: tegra: Access endpoint config only if PCIe link is up
Date: Tue, 18 Jun 2019 12:49:18 +0200	[thread overview]
Message-ID: <20190618104918.GA28892@ulmo> (raw)
In-Reply-To: <a7e0472d-f4a7-ed63-836a-b5e8b1360645@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 12279 bytes --]

On Tue, Jun 18, 2019 at 11:06:44AM +0530, Manikanta Maddireddy wrote:
> 
> On 18-Jun-19 1:00 AM, Bjorn Helgaas wrote:
> > [+cc Rafael, linux-pm, in case they have insights on how rfkill works]
> >
> > On Mon, Jun 17, 2019 at 01:47:45PM +0200, Thierry Reding wrote:
> >> On Mon, Jun 17, 2019 at 03:31:38PM +0530, Manikanta Maddireddy wrote:
> >>> On 13-Jun-19 9:12 PM, Thierry Reding wrote:
> >>>> On Thu, Jun 13, 2019 at 03:39:46PM +0100, Lorenzo Pieralisi wrote:
> >>>>> On Mon, Jun 10, 2019 at 10:08:16AM +0530, Manikanta Maddireddy wrote:
> >>>>>> 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.
> >>>>>>> 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.
> >>>>>> 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.
> >>>> The problem that Manikanta is trying to solve here occurs in
> >>>> this situation (Manikanta, correct me if I've got this wrong):
> >>>> on some setups, a WiFi module connected over PCI will toggle a
> >>>> power GPIO as part of runtime suspend. This effectively causes
> >>>> the module to disappear from the PCI bus (i.e. it can no longer
> >>>> be accessed until the power GPIO is toggled again).
> >>> GPIO is toggled as part of WiFi on/off, can be triggered from
> >>> network manager UI.
> >>>> This is fine from a kernel point of view because the kernel keeps track
> >>>> of what devices are suspended. However, userspace will occasionally try
> >>>> to read the configuration space access of all devices, and since it
> >>>> doesn't have any knowledge about the suspend state of these devices, it
> >>>> doesn't know which ones to leave alone. I think this happens when the
> >>>> X.Org server is running.
> >>> This is fine from a kernel point of view because PCI client driver
> >>> doesn't initiate any PCIe transaction until network interface
> >>> is up during WiFi on.
> >>>
> >>>> One thing that Manikanta and I had discussed was that perhaps
> >>>> the device should be hot-unplugged when it goes into this
> >>>> low-power state. However, we don't support hotplug on Tegra210
> >>>> where this is needed, so we'd need some sort of software-induced
> >>>> hot-unplug. However, this low power state is entered when the
> >>>> WiFi interface is taken down (i.e. ip link set dev <interface>
> >>>> down). If we were to remove the PCI device in that case, it
> >>>> means that the interface goes away completely, which is
> >>>> completely unexpected from a user's perspective. After all,
> >>>> taking a link down and up may be something that scripts are
> >>>> doing all the time. They'd fall over if after taking the
> >>>> interface down, the interface completely disappears.
> >>>> It's also not entirely clear to me how we get the device back
> >>>> onto the bus again after it is in low power. If we hot-unplug
> >>>> the device, then the driver will be unbound. Presumably the
> >>>> driver is what's controlling the power GPIO, so there won't be
> >>>> any entity that can be used to bring the chip back to life.
> >>>> Unless we deal with that power GPIO elsewhere (rfkill switch
> >>>> perhaps?).
> >>> Correct, rfkill switch should handle the GPIO.
> >>> Sequence will be,
> >>>  - WiFi ON
> >>>    - rfkill switch enables the WiFi GPIO
> >>>    - Tegra PCIe receives hot plug event
> >>>    - Tegra PCIe hot plug driver rescans PCI bus and enumerates the device
> >>>    - PCI client driver is probed, which will create network interface
> >>>  - WiFi OFF
> >>>    - rfkill switch disables the WiFi GPIO
> >>>    - Tegra PCIe receives hot unplug event
> >>>    - Tegra PCIe hot plug driver removes PCI devices under the bus
> >>>    - PCI client driver remove is executed, which will remove
> >>>      network interface
> >>> We don't need current patch in this case because PCI device is not
> >>> present in the PCI hierarchy, so there cannot be EP config access
> >>> with link down.  However Tegra doesn't support hot plug and unplug
> >>> events. I am not sure if we have any software based hot plug event
> >>> trigger.
> >>> I will drop current patch and pursue if above sequence can be
> >>> implemented for Tegra.
> >> I just recalled that we have these messages in the kernel log:
> >>
> >> 	# dmesg | grep tegra-pcie
> >> 	[    1.055761] tegra-pcie 1003000.pcie: 4x1, 1x1 configuration
> >> 	[    2.745764] tegra-pcie 1003000.pcie: 4x1, 1x1 configuration
> >> 	[    2.753073] tegra-pcie 1003000.pcie: probing port 0, using 4 lanes
> >> 	[    2.761334] tegra-pcie 1003000.pcie: Slot present pin change, signature: 00000008
> >> 	[    3.177607] tegra-pcie 1003000.pcie: link 0 down, retrying
> >> 	[    3.585605] tegra-pcie 1003000.pcie: link 0 down, retrying
> >> 	[    3.993606] tegra-pcie 1003000.pcie: link 0 down, retrying
> >> 	[    4.001214] tegra-pcie 1003000.pcie: link 0 down, ignoring
> >> 	[    4.006733] tegra-pcie 1003000.pcie: probing port 1, using 1 lanes
> >> 	[    4.015042] tegra-pcie 1003000.pcie: Slot present pin change, signature: 00000000
> >> 	[    4.031177] tegra-pcie 1003000.pcie: PCI host bridge to bus 0000:00
> >>
> >> These "slot present pin change" message do look a lot like hotplug
> >> related messages. Could we perhaps use those to our advantage for this
> >> case? Do you see these when you run on the platform where WiFi is
> >> enabled/disabled using rfkill?
> >>
> >> Given that rfkill is completely decoupled from PCI, I don't see how we
> >> would trigger any software-based hotplug mechanism. Perhaps one thing
> >> that we could do is the equivalent of this:
> >>
> >> 	# echo 1 > /sys/bus/pci/rescan
> >>
> >> from some script that's perhaps tied to the rfkill somehow. I'm not sure
> >> if that's possible, or generic enough.
> > How does rfkill work?  It sounds like it completely removes power from
> > the wifi device, putting it in D3cold.  Is there any software
> > notification other than the "Slot present pin change" (which looks
> > like a Tegra-specific thing)?
> >
> > If the device is in D3cold, it won't respond to any PCI transactions,
> > and there's no standard PCI mechanism to wake it up.  Probably the
> > cleanest way to handle this is to make it a hot-unplug.
> >
> > If this were an ACPI system, the rfkill might be visible as some sort
> > of ACPI power management event, and there might be a corresponding way
> > for software to bring the device back to D0 temporarily.  That would
> > make lspci and X config reads work.  But I don't think this system has
> > ACPI.
> 
> "Slot present pin change" interrupt is triggered based on the programming
> of PRSNT_MAP bit field in PCIE2_RP_PRIV_MISC controller register and
> they are not triggered when EP is hot plugged/unplugged. Tegra PCIe
> controller doesn't have capability to detect EP hot plug and unplug.
> Consider that WiFi off equivalent to SW aware EP hot unplug event.
> 
> The rfkill subsystem provides a generic interface for disabling any radio
> transmitter in the system. WiFi M.2 form factor cards provide W_DISABLE
> GPIO to control the radio transmitter and I have seen some cards provide
> control to turn off complete chip through this GPIO. Here we are talking
> about second case where device is put in D3cold state. This GPIO can be
> registered to rfkill subsystem and rfkill commands (like "rfkill unblock
> wifi") can be used to turn on radio transmitter during WiFi on.

Perhaps what we need here is some sort of mechanism to make rfkill and
the PCI host controller interoperate? I could imagine for example that
the PCI host controller would get a new "rfkill" property in device
tree that points at the rfkill device via phandle.

The driver could then get a reference to it using something like:

	rfkill = rfkill_get(dev);
	if (IS_ERR(rfkill)) {
		...
	}

and register for notification:

	err = rfkill_subscribe(rfkill, callback);
	if (err < 0) {
		...
	}

rfkill_unsubscribe() and rfkill_put() would then be used upon driver
unload to detach from the rfkill.

I noticed that there's an rfkill-gpio driver (net/rfkill/rfkill-gpio.c)
that already does pretty much everything that we need, except that it
doesn't support DT yet, but I suspect that that's pretty easy to add.

Johannes, any thoughts on this. In a nutshell what we're trying to solve
here is devices that get removed from/added to PCI based on an rfkill-
type of device. The difference to other implementations is that we have
no way of detecting when the device has gone away (PCI hotplug does not
work). So we'd need some software-triggered mechanism to let the PCI
host controller know when the device is presumably going away or being
added back, so that the PCI bus can be rescanned and the PCI device
removed or added at that point).

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2019-06-18 10:49 UTC|newest]

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
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 [this message]
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 publicly 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=20190618104918.GA28892@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=helgaas@kernel.org \
    --cc=johannes@sipsolutions.net \
    --cc=jonathanh@nvidia.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=mmaddireddy@nvidia.com \
    --cc=rjw@rjwysocki.net \
    --cc=robh+dt@kernel.org \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).