From: "Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@linux.intel.com>
To: Ethan Zhao <xerces.zhao@gmail.com>,
Kuppuswamy Sathyanarayanan
<sathyanarayanan.nkuppuswamy@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
Sinan Kaya <okaya@kernel.org>,
linux-pci <linux-pci@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Ashok Raj <ashok.raj@intel.com>
Subject: Re: [PATCH v6 2/2] PCI/ERR: Split the fatal and non-fatal error recovery handling
Date: Wed, 14 Oct 2020 10:06:25 -0700 [thread overview]
Message-ID: <17e142b8-b19a-0ec7-833b-7a4ac2e76d0d@linux.intel.com> (raw)
In-Reply-To: <CAKF3qh3nnLaKUAbBdhdXwzknasTWmLFTjB7gz65vjzpHP4Y46Q@mail.gmail.com>
On 10/14/20 8:07 AM, Ethan Zhao wrote:
> On Wed, Oct 14, 2020 at 5:00 PM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.nkuppuswamy@gmail.com> wrote:
>>
>> Commit bdb5ac85777d ("PCI/ERR: Handle fatal error recovery")
>> merged fatal and non-fatal error recovery paths, and also made
>> recovery code depend on hotplug handler for "remove affected
>> device + rescan" support. But this change also complicated the
>> error recovery path and which in turn led to the following
>> issues.
>>
>> 1. We depend on hotplug handler for removing the affected
>> devices/drivers on DLLSC LINK down event (on DPC event
>> trigger) and DPC handler for handling the error recovery. Since
>> both handlers operate on same set of affected devices, it leads
>> to race condition, which in turn leads to NULL pointer
>> exceptions or error recovery failures.You can find more details
>> about this issue in following link.
>>
>> https://lore.kernel.org/linux-pci/20201007113158.48933-1-haifeng.zhao@intel.com/T/#t
>>
>> 2. For non-hotplug capable devices fatal (DPC) error recovery
>> is currently broken. Current fatal error recovery implementation
>> relies on PCIe hotplug (pciehp) handler for detaching and
>> re-enumerating the affected devices/drivers. So when dealing with
>> non-hotplug capable devices, recovery code does not restore the state
>> of the affected devices correctly. You can find more details about
>> this issue in the following links.
>>
>> https://lore.kernel.org/linux-pci/20200527083130.4137-1-Zhiqiang.Hou@nxp.com/
>> https://lore.kernel.org/linux-pci/12115.1588207324@famine/
>> https://lore.kernel.org/linux-pci/0e6f89cd6b9e4a72293cc90fafe93487d7c2d295.1585000084.git.sathyanarayanan.kuppuswamy@linux.intel.com/
>>
>> In order to fix the above two issues, we should stop relying on hotplug
> Yes, it doesn't rely on hotplug handler to remove and rescan the device,
> but it couldn't prevent hotplug drivers from doing another replicated
> removal/rescanning.
> it doesn't make sense to leave another useless removal/rescanning there.
> Maybe that's why these two paths were merged to one and made it rely on
> hotplug.
No, as per PCIe spec, hotplug and DPC has no functional dependency. Hence
depending on it to handle some of its recovery function is in-correct and
would lead to issues in non-hotplug capable platforms (which is true
currently).
>
>> + else
>> + udev = dev->bus->self;
>> +
>> + parent = udev->subordinate;
>> + pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
>> +
>> + pci_lock_rescan_remove();
> Though here you have lock, but hotplug will do another
> 'pci_stop_and_remove_bus_device()'
> without merging it with the hotplug driver, you have no way to
> remove the replicated actions in
> hotplug handler.
No, the core operation (remove/add device) is syncronzied and done in
only one thread. Please check the following flow. Even in hotplug
handler, before removing the device, it attempts to hold pci_lock_rescan_remove()
lock. So holding the same lock in DPC handler will syncronize the DPC/hotplug
handlers. Also if one of the thread (DPC or hotplug) removes/adds the affected devices,
other thread will not repeat the same action (since the device is already removed/added).
->pciehp_ist()
->pciehp_handle_presence_or_link_change()
->pciehp_disable_slot()
->__pciehp_disable_slot()
->remove_board()
->pciehp_unconfigure_device()
->pci_lock_rescan_remove()
>
>
> Thanks,
> Ethan
>> + pci_dev_get(dev);
>> + list_for_each_entry_safe_reverse(pdev, temp, &parent->devices,
>> + bus_list) {
>> + pci_stop_and_remove_bus_device(pdev);
>> + }
>> +
>> + result = reset_link(udev);
>> +
>> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> + /*
>> + * If the error is reported by a bridge, we think this error
>> + * is related to the downstream link of the bridge, so we
>> + * do error recovery on all subordinates of the bridge instead
>> + * of the bridge and clear the error status of the bridge.
>> + */
>> + pci_aer_clear_fatal_status(dev);
>> + if (pcie_aer_is_native(dev))
>> + pcie_clear_device_status(dev);
>> + }
>> +
>> + if (result == PCI_ERS_RESULT_RECOVERED) {
>> + if (pcie_wait_for_link(udev, true))
> And another pci_rescan_bus() like in the hotplug handler.
As I have mentioned before, holding the same lock should make them synchronized
and not repeat the underlying functionality of pci_rescan_bus() in both threads
at the same time.
>> + pci_rescan_bus(udev->bus);
>> + pci_info(dev, "Device recovery from fatal error successful\n");
>> + } else {
>> + pci_uevent_ers(dev, PCI_ERS_RESULT_DISCONNECT);
>> + pci_info(dev, "Device recovery from fatal error failed\n");
>> --
>> 2.17.1
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
next prev parent reply other threads:[~2020-10-14 17:06 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-14 8:18 [PATCH v6 1/2] PCI/ERR: Call pci_bus_reset() before calling ->slot_reset() callback Kuppuswamy Sathyanarayanan
2020-10-14 8:18 ` [PATCH v6 2/2] PCI/ERR: Split the fatal and non-fatal error recovery handling Kuppuswamy Sathyanarayanan
2020-10-14 15:07 ` Ethan Zhao
2020-10-14 17:06 ` Kuppuswamy, Sathyanarayanan [this message]
2020-10-15 1:58 ` Ethan Zhao
2020-10-15 3:04 ` Kuppuswamy, Sathyanarayanan
2020-10-15 5:05 ` Ethan Zhao
2020-10-15 5:53 ` Kuppuswamy, Sathyanarayanan
2020-10-15 14:03 ` Ethan Zhao
2020-10-15 6:52 ` Christoph Hellwig
2020-10-15 13:55 ` Ethan Zhao
2020-10-15 20:12 ` Kuppuswamy, Sathyanarayanan
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=17e142b8-b19a-0ec7-833b-7a4ac2e76d0d@linux.intel.com \
--to=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=ashok.raj@intel.com \
--cc=bhelgaas@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=okaya@kernel.org \
--cc=sathyanarayanan.nkuppuswamy@gmail.com \
--cc=xerces.zhao@gmail.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).