Hi, On Fri, Jan 22, 2021 at 03:03:11PM +0800, Mingchuang Qiao wrote: > On Thu, 2021-01-21 at 16:31 -0600, Bjorn Helgaas wrote: > > [+cc Alex and Mingchuang et al from > > https://lore.kernel.org/r/20210112072739.31624-1-mingchuang.qiao@xxxxxxxxxxxx] > > > > On Tue, Jan 19, 2021 at 04:14:10PM +0300, Mika Westerberg wrote: > > > PCIe r5.0, sec 7.5.3.16 says that the downstream ports must reset the > > > LTR enable bit if the link goes down (port goes DL_Down status). Now, if > > > we had LTR previously enabled and the PCIe endpoint gets hot-removed and > > > then hot-added back the ->ltr_path of the downstream port is still set > > > but the port now does not have the LTR enable bit set anymore. > > > > > > For this reason check if the bridge upstream had LTR enabled previously > > > and re-enable it before enabling LTR for the endpoint. > > > > > > Reported-by: Utkarsh H Patel <utkarsh.h.patel@xxxxxxxxx> > > > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx> > > > > I think this and Mingchuang's patch, which is essentially identical, > > are right and solves the problem for hot-remove/hot-add. In that > > scenario we call pci_configure_ltr() on the hot-added device, and > > with this patch, we'll re-enable LTR on the bridge leading to the new > > device before enabling LTR on the new device itself. > > > > But don't we have a similar problem if we simply do a Fundamental > > Reset on a device? I think the reset path will restore the device's > > state, including PCI_EXP_DEVCTL2, but it doesn't do anything with the > > upstream bridge, does it? > > > > Yes. I think the same problem exists under such scenario, and that’s the > issue my patch intends to resolve. > I also prepared a v2 patch for review(update the patch description). > Shall I submit the v2 patch for review? I looked at your patch and indeed it is essentially doing the same as this one. So let's forget this patch and go forward with yours :) Would you like to expand your patch to handle the reset case too that Bjorn desribes below? > > So if a bridge and a device below it both have LTR enabled, can't we > > have the following: > > > > - bridge LTR enabled > > - device LTR enabled > > - reset device, e.g., via Secondary Bus Reset > > - link goes down, bridge disables LTR > > - link comes back up, LTR disabled in both bridge and device > > - restore device state, including LTR enable > > - device sends LTR message > > - bridge reports Unsupported Request
