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@xxxxxxx/ > > (@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 -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
