Hi Lukas, On Mon, Feb 24, 2025 at 07:12:11PM +0100, Lukas Wunner wrote: > On Mon, Feb 24, 2025 at 11:44:58AM +0800, Feng Tang wrote: > > @@ -255,8 +271,7 @@ static int get_port_device_capability(struct pci_dev *dev) > > * Disable hot-plug interrupts in case they have been enabled > > * by the BIOS and the hot-plug service driver is not loaded. > > */ > > - pcie_capability_clear_word(dev, PCI_EXP_SLTCTL, > > - PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE); > > + pcie_disable_hp_interrupts_early(dev); > > } > > Moving the Slot Control code from pciehp to portdrv (as is done in > patch 1 of this series) is hackish. It should be avoided if at all > possible. I tried to remove the code duplication of 2 waiting function, according to Bjorn's comment in https://lore.kernel.org/lkml/20250218223354.GA196886@bhelgaas/. Maybe I didn't git it right. Any suggestion? > > As I've already said before... > > https://lore.kernel.org/all/Z6HYuBDP6uvE1Sf4@xxxxxxxxx/ > > ...it seems clearing those interrupts is only necessary > in the CONFIG_HOTPLUG_PCI_PCIE=n case. And in that case, > there is no second Slot Control write, so there is no delay > to be observed. > > Hence the proper solution is to make the clearing of the > interrupts conditional on: !IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE) > > You keep sending new versions of these patches that do not > incorporate the feedback I provided in the above-linked e-mail. > > Please re-read that e-mail and verify if the solution that > I proposed solves the problem. That solution does not > require moving the Slot Control code from pciehp to portdrv. There might be some misunderstaning here :), I responded in https://lore.kernel.org/lkml/Z6LRAozZm1UfgjqT@U-2FWC9VHC-2323.local/ that your suggestion could solve our issue. And the reason I didn't take it is I was afraid that it might hurt the problem what commit 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port services during port initialization") tried to solve. As you mentioned, the comment for 2bd50dd800b5 is "a bit confusing", and I tried to guess in my previous reply: " I'm not sure what problem this piece of code try to avoid, maybe something simliar to the irq storm isseu as mentioned in the 2/2 patch? The code comments could be about the small time window between this point and the loading of pciehp driver, which will request_irq and enable hotplug interrupt again. " The code comment from 2bd50dd800b5 is: /* * Disable hot-plug interrupts in case they have been * enabled by the BIOS and the hot-plug service driver * is not loaded. */ The "is not loaded" has 2 possible meanings: 1. the pciehp driver is not loaded yet at this point inside get_port_device_capability(), and will be loaded later 2. the pciehp will never be loaded, i.e. CONFIG_HOTPLUG_PCI_PCIE=n If it's case 2, your suggestion can solve it nicely, but for case 1, we may have to keep the interrupt disabling. Thanks, Feng
