| Message ID | 20230615062559.1268404-1-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v1] PCI: pciehp: Make sure DPC trigger status is reset in PDC handler | expand |
Hi Kuppuswamy,
kernel test robot noticed the following build warnings:
[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.4-rc6 next-20230615]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kuppuswamy-Sathyanarayanan/PCI-pciehp-Make-sure-DPC-trigger-status-is-reset-in-PDC-handler/20230615-142706
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20230615062559.1268404-1-sathyanarayanan.kuppuswamy%40linux.intel.com
patch subject: [PATCH v1] PCI: pciehp: Make sure DPC trigger status is reset in PDC handler
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230615/202306151528.9ZEmHZMt-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git remote add pci https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git
git fetch pci next
git checkout pci/next
b4 shazam https://lore.kernel.org/r/20230615062559.1268404-1-sathyanarayanan.kuppuswamy@linux.intel.com
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/pci/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306151528.9ZEmHZMt-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/pci/pcie/dpc.c:123: warning: expecting prototype for pci_reset_trigger(). Prototype was for pci_dpc_reset_trigger() instead
vim +123 drivers/pci/pcie/dpc.c
114
115 /**
116 * pci_reset_trigger - Clear DPC trigger status
117 * @pdev: PCI device
118 *
119 * It is called from the PCIe hotplug driver to clean the DPC
120 * trigger status in the PDC interrupt handler.
121 */
122 void pci_dpc_reset_trigger(struct pci_dev *pdev)
> 123 {
124 if (!pdev->dpc_cap)
125 return;
126
127 pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS,
128 PCI_EXP_DPC_STATUS_TRIGGER);
129 }
130
On Wed, Jun 14, 2023 at 11:25:59PM -0700, Kuppuswamy Sathyanarayanan wrote: > During the EDR-based DPC recovery process, for devices with persistent > issues, the firmware may choose not to handle the DPC error and leave > the port in DPC triggered state. In such scenarios, if the user > replaces the faulty device with a new one, the OS is expected to clear > the DPC trigger status in the hotplug error handler to enable the new > device enumeration. You're clearing the DPC trigger status upon a PDC event, yet are saying here the purpose is to reset port state for a future hotplugged device. A PDC event may be synthesized, e.g. to trigger slot bringup via sysfs, so using a PDC event to clear DPC trigger status feels wrong. pciehp_unconfigure_device() seems like a more appropriate place to me. > More details about this issue can be found in PCIe > firmware specification, r3.3, sec titled "DPC Event Handling" > Implementation note. That Implementation Note contains a lot of text and a fairly complex flow chart. If you could point to specific paragraphs or numbers in the Implementation Note that would make life easier for a reviewer to make the connection between your code and the spec. > Similar issue might also happen if the DPC or EDR recovery handler > exits before clearing the trigger status. To fix this issue, clear the > DPC trigger status in PDC interrupt handler. I was about to ask why the code is added to dpc.c, not edr.c, and why it's not constrained to CONFIG_PCIE_EDR, but I assume that's the reason? Because it "might" happen for OS-native DPC as well? > +/** > + * pci_reset_trigger - Clear DPC trigger status > + * @pdev: PCI device > + * > + * It is called from the PCIe hotplug driver to clean the DPC > + * trigger status in the PDC interrupt handler. > + */ > +void pci_dpc_reset_trigger(struct pci_dev *pdev) > +{ > + if (!pdev->dpc_cap) > + return; > + > + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS, > + PCI_EXP_DPC_STATUS_TRIGGER); > +} This may run concurrently to dpc_reset_link(), so I'd expect that you need some kind of serialization. What happens if pciehp clears trigger status behind the DPC driver's back while it is handling an error? Thanks, Lukas
Hi Lukas, Thanks for the review. On 6/15/23 11:35 AM, Lukas Wunner wrote: > On Wed, Jun 14, 2023 at 11:25:59PM -0700, Kuppuswamy Sathyanarayanan wrote: >> During the EDR-based DPC recovery process, for devices with persistent >> issues, the firmware may choose not to handle the DPC error and leave >> the port in DPC triggered state. In such scenarios, if the user >> replaces the faulty device with a new one, the OS is expected to clear >> the DPC trigger status in the hotplug error handler to enable the new >> device enumeration. > > You're clearing the DPC trigger status upon a PDC event, yet are saying > here the purpose is to reset port state for a future hotplugged device. Sorry, it is a typo. I meant "hotplug interrupt handler". The goal is to ensure that when a new device presence is detected, the old DPC trigger status is cleared. > > A PDC event may be synthesized, e.g. to trigger slot bringup via > sysfs, so using a PDC event to clear DPC trigger status feels wrong. IMO, it is harmless. We just want to make sure the previous DPC trigger status is cleared before enumerating a new device. > pciehp_unconfigure_device() seems like a more appropriate place to me. > I initially thought to add it there. Spec also recommends clearing it when removing the device. But I wasn't sure if pciehp_unconfigure_device() would be called only during device removal. Let me test this path and get back to you. > >> More details about this issue can be found in PCIe >> firmware specification, r3.3, sec titled "DPC Event Handling" >> Implementation note. > > That Implementation Note contains a lot of text and a fairly complex > flow chart. If you could point to specific paragraphs or numbers in > the Implementation Note that would make life easier for a reviewer > to make the connection between your code and the spec. It is the text at the end of the flowchart. Copied it here for reference. For devices with persistent errors, a port may be kept in the DPC triggered state (disabled) to keep those devices from continuing to generate errors. For hot-plug slots, the errant device may be removed and replaced with a new device. If the DPC trigger state is not cleared, then the port above the newly inserted device will still be disabled and will be non-operational. Therefore, operating systems may need to modify their hot-plug interrupt handling code to clear DPC Trigger Status when a device is removed so that a subsequent insertion will succeed. > > >> Similar issue might also happen if the DPC or EDR recovery handler >> exits before clearing the trigger status. To fix this issue, clear the >> DPC trigger status in PDC interrupt handler. > > I was about to ask why the code is added to dpc.c, not edr.c, > and why it's not constrained to CONFIG_PCIE_EDR, but I assume > that's the reason? Because it "might" happen for OS-native DPC > as well? Yes. There are code paths in the DPC driver where error recover handler can exit before clearing the DPC trigger status. So I think this fix is applicable for native code as well. > > >> +/** >> + * pci_reset_trigger - Clear DPC trigger status >> + * @pdev: PCI device >> + * >> + * It is called from the PCIe hotplug driver to clean the DPC >> + * trigger status in the PDC interrupt handler. >> + */ >> +void pci_dpc_reset_trigger(struct pci_dev *pdev) >> +{ >> + if (!pdev->dpc_cap) >> + return; >> + >> + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS, >> + PCI_EXP_DPC_STATUS_TRIGGER); >> +} > > This may run concurrently to dpc_reset_link(), so I'd expect that > you need some kind of serialization. What happens if pciehp clears > trigger status behind the DPC driver's back while it is handling an > error? Currently, we only call pci_dpc_reset_trigger() in PDC interrupt handler. Do you think there would be a race between error handler and PDC handler? > > Thanks, > > Lukas
[cc += Smita] On Thu, Jun 15, 2023 at 04:03:54PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 6/15/23 11:35 AM, Lukas Wunner wrote: > > On Wed, Jun 14, 2023 at 11:25:59PM -0700, Kuppuswamy Sathyanarayanan wrote: > > > During the EDR-based DPC recovery process, for devices with persistent > > > issues, the firmware may choose not to handle the DPC error and leave > > > the port in DPC triggered state. In such scenarios, if the user > > > replaces the faulty device with a new one, the OS is expected to clear > > > the DPC trigger status in the hotplug error handler to enable the new > > > device enumeration. [...] > > > > pciehp_unconfigure_device() seems like a more appropriate place to me. > > > > I initially thought to add it there. Spec also recommends clearing it > when removing the device. But I wasn't sure if pciehp_unconfigure_device() > would be called only during device removal. It is. > > > More details about this issue can be found in PCIe > > > firmware specification, r3.3, sec titled "DPC Event Handling" > > > Implementation note. > > > > That Implementation Note contains a lot of text and a fairly complex > > flow chart. If you could point to specific paragraphs or numbers in > > the Implementation Note that would make life easier for a reviewer > > to make the connection between your code and the spec. > > It is the text at the end of the flowchart. Copied it here for reference. > > For devices with persistent errors, a port may be kept in the DPC triggered > state (disabled) to keep those devices from continuing to generate errors. > For hot-plug slots, the errant device may be removed and replaced with a new > device. > If the DPC trigger state is not cleared, then the port above the newly > inserted device will still be disabled and will be non-operational. > Therefore, operating systems may need to modify their hot-plug interrupt > handling code to clear DPC Trigger Status when a device is removed so that > a subsequent insertion will succeed. Please add that excerpt to the commit message. > > This may run concurrently to dpc_reset_link(), so I'd expect that > > you need some kind of serialization. What happens if pciehp clears > > trigger status behind the DPC driver's back while it is handling an > > error? > > Currently, we only call pci_dpc_reset_trigger() in PDC interrupt handler. > > Do you think there would be a race between error handler and PDC handler? Yes I think so. We need to differentiate between two cases: (1) DPC handled by firmware, hotplug handled by OS: In this case clearing DPC trigger status from pciehp device removal code path seems reasonable. But it must be constrained to !host_bridge->native_dpc. (2) DPC handled by OS: In this case clearing DPC trigger status from pciehp could race with the dpc interrupt handler so must not be done. Instead, I recommend clearing trigger status from the dpc interrupt handler. You should see a Surprise Down error handled by the dpc interrupt handler. Make sure DPC trigger status is *always* cleared in that case. Note that Smita Koralahalli is currently working on something similar: https://lore.kernel.org/linux-pci/20230418210526.36514-2-Smita.KoralahalliChannabasappa@amd.com/ (@Smita sorry for the delay, I'll get to your patches ASAP.) I recommend splitting the two cases above into two commits, one for firmware-handled DPC and one for OS-native DPC. IIUC, you only need the former to address Dell's finding. Thanks, Lukas
Hi Lukas, On 6/16/23 2:06 AM, Lukas Wunner wrote: > [cc += Smita] > > On Thu, Jun 15, 2023 at 04:03:54PM -0700, Sathyanarayanan Kuppuswamy wrote: >> On 6/15/23 11:35 AM, Lukas Wunner wrote: >>> On Wed, Jun 14, 2023 at 11:25:59PM -0700, Kuppuswamy Sathyanarayanan wrote: >>>> During the EDR-based DPC recovery process, for devices with persistent >>>> issues, the firmware may choose not to handle the DPC error and leave >>>> the port in DPC triggered state. In such scenarios, if the user >>>> replaces the faulty device with a new one, the OS is expected to clear >>>> the DPC trigger status in the hotplug error handler to enable the new >>>> device enumeration. > [...] >>> >>> pciehp_unconfigure_device() seems like a more appropriate place to me. >>> >> >> I initially thought to add it there. Spec also recommends clearing it >> when removing the device. But I wasn't sure if pciehp_unconfigure_device() >> would be called only during device removal. > > It is. Do you know how pciehp_unconfigure_device() will be called when the device is removed? Is it due to a DLLSC event or a PDC state change? If it is DLLSC, pciehp_unconfigure_device() may not be called because we ignore the DLLSC event if there is an active DPC trigger. > > >>>> More details about this issue can be found in PCIe >>>> firmware specification, r3.3, sec titled "DPC Event Handling" >>>> Implementation note. >>> >>> That Implementation Note contains a lot of text and a fairly complex >>> flow chart. If you could point to specific paragraphs or numbers in >>> the Implementation Note that would make life easier for a reviewer >>> to make the connection between your code and the spec. >> >> It is the text at the end of the flowchart. Copied it here for reference. >> >> For devices with persistent errors, a port may be kept in the DPC triggered >> state (disabled) to keep those devices from continuing to generate errors. >> For hot-plug slots, the errant device may be removed and replaced with a new >> device. >> If the DPC trigger state is not cleared, then the port above the newly >> inserted device will still be disabled and will be non-operational. >> Therefore, operating systems may need to modify their hot-plug interrupt >> handling code to clear DPC Trigger Status when a device is removed so that >> a subsequent insertion will succeed. > > Please add that excerpt to the commit message. Ok. I will add it. > > >>> This may run concurrently to dpc_reset_link(), so I'd expect that >>> you need some kind of serialization. What happens if pciehp clears >>> trigger status behind the DPC driver's back while it is handling an >>> error? >> >> Currently, we only call pci_dpc_reset_trigger() in PDC interrupt handler. >> >> Do you think there would be a race between error handler and PDC handler? > > Yes I think so. > > We need to differentiate between two cases: > > (1) DPC handled by firmware, hotplug handled by OS: > > In this case clearing DPC trigger status from pciehp device removal > code path seems reasonable. But it must be constrained to > !host_bridge->native_dpc. Agree. > > (2) DPC handled by OS: > > In this case clearing DPC trigger status from pciehp could race with > the dpc interrupt handler so must not be done. Instead, I recommend If we clear the DPC trigger status in the DLLSC state change handler, I agree there could be a race. However, if we clear the DPC trigger in the PDC state change handler, I believe it will not race because the device has already been removed. Is my understanding correct? > clearing trigger status from the dpc interrupt handler. You should > see a Surprise Down error handled by the dpc interrupt handler. > Make sure DPC trigger status is *always* cleared in that case. > > Note that Smita Koralahalli is currently working on something similar: > > https://lore.kernel.org/linux-pci/20230418210526.36514-2-Smita.KoralahalliChannabasappa@amd.com/ > > (@Smita sorry for the delay, I'll get to your patches ASAP.) > > I recommend splitting the two cases above into two commits, one for > firmware-handled DPC and one for OS-native DPC. IIUC, you only need > the former to address Dell's finding. > > Thanks, > > Lukas
On Fri, Jun 16, 2023 at 04:27:33PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 6/16/23 2:06 AM, Lukas Wunner wrote: > > On Thu, Jun 15, 2023 at 04:03:54PM -0700, Sathyanarayanan Kuppuswamy wrote: > >> On 6/15/23 11:35 AM, Lukas Wunner wrote: > >>> On Wed, Jun 14, 2023 at 11:25:59PM -0700, Kuppuswamy Sathyanarayanan wrote: > >>>> During the EDR-based DPC recovery process, for devices with persistent > >>>> issues, the firmware may choose not to handle the DPC error and leave > >>>> the port in DPC triggered state. In such scenarios, if the user > >>>> replaces the faulty device with a new one, the OS is expected to clear > >>>> the DPC trigger status in the hotplug error handler to enable the new > >>>> device enumeration. > > [...] > >>> pciehp_unconfigure_device() seems like a more appropriate place to me. > >> > >> I initially thought to add it there. Spec also recommends clearing it > >> when removing the device. But I wasn't sure if pciehp_unconfigure_device() > >> would be called only during device removal. > > > > It is. > > Do you know how pciehp_unconfigure_device() will be called when the device is > removed? Is it due to a DLLSC event or a PDC state change? If it is DLLSC, > pciehp_unconfigure_device() may not be called because we ignore the DLLSC > event if there is an active DPC trigger. A DLLSC event is only ignored by pciehp if successful recovery from DPC was possible. In that case, the device in the slot is still the *same*. And you're saying in the commit message that you seek to fix the case when "the user replaces the faulty device with a *new* one". So for your goal, ignored DLLSC events shouldn't be relevant. As to your question, pciehp_ist() doesn't really differentiate between PDC and DLLSC events. If either of them occurs, the slot is brought down through pciehp_handle_presence_or_link_change(). And that will end up in pciehp_unconfigure_device(). I think right at the end of that function, after pci_unlock_rescan_remove() would be a good place to perform cleanup of the hotplug port before a newly hotplugged device can be handled. Smita's clearing of ARI Forwarding Enable, AtomicOp Requester Enable and 10-Bit Tag Requester Enable could be done as well at the end of pciehp_unconfigure_device(). > > We need to differentiate between two cases: > > > > (1) DPC handled by firmware, hotplug handled by OS: > > > > In this case clearing DPC trigger status from pciehp device removal > > code path seems reasonable. But it must be constrained to > > !host_bridge->native_dpc. > > Agree. The crucial thing here is that the dpc driver isn't bound to the port if DPC is handled by firmware. Hence there's no chance of racing with dpc_reset_link() and it's safe to clear DPC trigger status from pciehp. > > (2) DPC handled by OS: > > > > In this case clearing DPC trigger status from pciehp could race with > > the dpc interrupt handler so must not be done. Instead, I recommend > > If we clear the DPC trigger status in the DLLSC state change handler, I > agree there could be a race. However, if we clear the DPC trigger in the > PDC state change handler, I believe it will not race because the device > has already been removed. Is my understanding correct? Unfortunately it's not guaranteed that dpc_reset_link() is finished when pciehp brings down the slot. So clearing DPC trigger status can only be done safely from the dpc driver itself (if DPC is handled natively by the OS). Thanks, Lukas
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index f8c70115b691..03f8f18a6cf5 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -735,6 +735,12 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id) PCI_EXP_SLTCTL_ATTN_IND_ON); } + /* Clear DPC trigger status */ + if ((events & PCI_EXP_SLTSTA_PDC) && pci_dpc_is_triggered(pdev)) { + ctrl_dbg(ctrl, "Slot(%s): Clear DPC trigger\n", slot_name(ctrl)); + pci_dpc_reset_trigger(pdev); + } + /* * Ignore Link Down/Up events caused by Downstream Port Containment * if recovery from the error succeeded. diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 2475098f6518..bbe70e9ab747 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -426,11 +426,15 @@ void pci_dpc_init(struct pci_dev *pdev); void dpc_process_error(struct pci_dev *pdev); pci_ers_result_t dpc_reset_link(struct pci_dev *pdev); bool pci_dpc_recovered(struct pci_dev *pdev); +bool pci_dpc_is_triggered(struct pci_dev *pdev); +void pci_dpc_reset_trigger(struct pci_dev *pdev); #else static inline void pci_save_dpc_state(struct pci_dev *dev) {} static inline void pci_restore_dpc_state(struct pci_dev *dev) {} static inline void pci_dpc_init(struct pci_dev *pdev) {} static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; } +static inline bool pci_dpc_is_triggered(struct pci_dev *pdev) { return false; } +static inline void pci_dpc_reset_trigger(struct pci_dev *pdev) {} #endif #ifdef CONFIG_PCIEPORTBUS diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c index 3ceed8e3de41..ccdc90d37f6d 100644 --- a/drivers/pci/pcie/dpc.c +++ b/drivers/pci/pcie/dpc.c @@ -88,6 +88,46 @@ static bool dpc_completed(struct pci_dev *pdev) return true; } +/** + * pci_dpc_is_triggered - Check for DPC trigger status + * @pdev: PCI device + * + * It is called from the PCIe hotplug driver to check for DPC + * trigger status. + * + * Return True if DPC is triggered or false for other cases. + */ +bool pci_dpc_is_triggered(struct pci_dev *pdev) +{ + u16 status; + + if (!pdev->dpc_cap) + return false; + + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS, &status); + + if ((!PCI_POSSIBLE_ERROR(status)) && (status & PCI_EXP_DPC_STATUS_TRIGGER)) + return true; + + return false; +} + +/** + * pci_reset_trigger - Clear DPC trigger status + * @pdev: PCI device + * + * It is called from the PCIe hotplug driver to clean the DPC + * trigger status in the PDC interrupt handler. + */ +void pci_dpc_reset_trigger(struct pci_dev *pdev) +{ + if (!pdev->dpc_cap) + return; + + pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS, + PCI_EXP_DPC_STATUS_TRIGGER); +} + /** * pci_dpc_recovered - whether DPC triggered and has recovered successfully * @pdev: PCI device
During the EDR-based DPC recovery process, for devices with persistent issues, the firmware may choose not to handle the DPC error and leave the port in DPC triggered state. In such scenarios, if the user replaces the faulty device with a new one, the OS is expected to clear the DPC trigger status in the hotplug error handler to enable the new device enumeration. More details about this issue can be found in PCIe firmware specification, r3.3, sec titled "DPC Event Handling" Implementation note. Similar issue might also happen if the DPC or EDR recovery handler exits before clearing the trigger status. To fix this issue, clear the DPC trigger status in PDC interrupt handler. Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> Reported-by: Thatchanamurthy Satish <Satish.Thatchanamurt@Dell.com> --- drivers/pci/hotplug/pciehp_hpc.c | 6 +++++ drivers/pci/pci.h | 4 ++++ drivers/pci/pcie/dpc.c | 40 ++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+)