| Message ID | 20250204053758.6025-1-feng.tang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/2] PCI/portdrv: Add necessary delay for disabling hotplug events | expand |
On Tue, Feb 04, 2025 at 01:37:57PM +0800, Feng Tang wrote: > According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at > least 1 second for the command-complete event, before resending the cmd > or sending a new cmd. > > Currently get_port_device_capability() sends slot control cmd to disable > PCIe hotplug interrupts without waiting for its completion and there was > real problem reported for the lack of waiting. > > Add the necessary wait to comply with PCIe spec. The waiting logic refers > existing pcie_poll_cmd(). [...] > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -230,8 +260,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); > } The language of the code comment is a bit confusing in that it says the hot-plug driver may not be "loaded". This sounds like it could be modular. But it can't. It's always built-in. So I think what is really meant here is that the driver may be *disabled* in the config, i.e. CONFIG_HOTPLUG_PCI_PCIE=n. Now if CONFIG_HOTPLUG_PCI_PCIE=n, you don't need to observe the Command Completed delay because the hotplug driver won't touch the Slot Control register afterwards. It's not compiled in. On the other hand if CONFIG_HOTPLUG_PCI_PCIE=y, you don't need to disable the CCIE and HPIE interrupt because the hotplug driver will handle them. So I think the proper solution here is to make the write to the Slot Control register conditional on CONFIG_HOTPLUG_PCI_PCIE, like this: if (dev->is_hotplug_bridge && (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || - pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) && - (pcie_ports_native || host->native_pcie_hotplug)) { - services |= PCIE_PORT_SERVICE_HP; + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)) { + if (pcie_ports_native || host->native_pcie_hotplug) + services |= PCIE_PORT_SERVICE_HP; /* * 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); + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)) + pcie_capability_clear_word(dev, PCI_EXP_SLTCTL, + PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE); } The above patch also makes sure the interrupts are quiesced if the platform didn't grant hotplug control to OSPM. Thanks, Lukas
Hi Lukas, Thanks for your review and suggestions! On Tue, Feb 04, 2025 at 10:07:04AM +0100, Lukas Wunner wrote: > On Tue, Feb 04, 2025 at 01:37:57PM +0800, Feng Tang wrote: > > According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at > > least 1 second for the command-complete event, before resending the cmd > > or sending a new cmd. > > > > Currently get_port_device_capability() sends slot control cmd to disable > > PCIe hotplug interrupts without waiting for its completion and there was > > real problem reported for the lack of waiting. > > > > Add the necessary wait to comply with PCIe spec. The waiting logic refers > > existing pcie_poll_cmd(). > [...] > > --- a/drivers/pci/pcie/portdrv.c > > +++ b/drivers/pci/pcie/portdrv.c > > @@ -230,8 +260,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); > > } > > The language of the code comment is a bit confusing in that it > says the hot-plug driver may not be "loaded". This sounds like > it could be modular. But it can't. It's always built-in. 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. > So I think what is really meant here is that the driver may be > *disabled* in the config, i.e. CONFIG_HOTPLUG_PCI_PCIE=n. > > Now if CONFIG_HOTPLUG_PCI_PCIE=n, you don't need to observe the > Command Completed delay because the hotplug driver won't touch > the Slot Control register afterwards. It's not compiled in. > > On the other hand if CONFIG_HOTPLUG_PCI_PCIE=y, you don't need > to disable the CCIE and HPIE interrupt because the hotplug driver > will handle them. > > So I think the proper solution here is to make the write to the > Slot Control register conditional on CONFIG_HOTPLUG_PCI_PCIE, > like this: > > if (dev->is_hotplug_bridge && > (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT || > - pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) && > - (pcie_ports_native || host->native_pcie_hotplug)) { > - services |= PCIE_PORT_SERVICE_HP; > + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)) { > + if (pcie_ports_native || host->native_pcie_hotplug) > + services |= PCIE_PORT_SERVICE_HP; > > /* > * 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); > + if (!IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)) > + pcie_capability_clear_word(dev, PCI_EXP_SLTCTL, > + PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE); Yes, this could sovlve the original issue. The problem I heard from BIOS developer is, they didn't expect to receive 2 link control commands at almost the same time, which doesn't comply to pcie spec, and normaly the handling of one command will take some time, though not as long as 1 second. Thanks, Feng > The above patch also makes sure the interrupts are quiesced if the > platform didn't grant hotplug control to OSPM. > > Thanks, > > Lukas
… > Add the necessary wait to comply with PCIe spec. The waiting logic refers > existing pcie_poll_cmd(). * How do you think about to add any tags (like “Fixes” and “Cc”) accordingly? * Will cover letters be usually helpful for such patch series? * You would probably like to avoid a typo in an email address. Regards, Markus
On 2/3/25 9:37 PM, Feng Tang wrote: > According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at > least 1 second for the command-complete event, before resending the cmd > or sending a new cmd. > > Currently get_port_device_capability() sends slot control cmd to disable > PCIe hotplug interrupts without waiting for its completion and there was > real problem reported for the lack of waiting. Can you include the error log associated with this issue? What is the actual issue you are seeing and in which hardware? > > Add the necessary wait to comply with PCIe spec. The waiting logic refers > existing pcie_poll_cmd(). > > Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com> > --- > drivers/pci/pci.h | 2 ++ > drivers/pci/pcie/portdrv.c | 33 +++++++++++++++++++++++++++++++-- > 2 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 01e51db8d285..c1e234d1b81d 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -759,12 +759,14 @@ static inline void pcie_ecrc_get_policy(char *str) { } > #ifdef CONFIG_PCIEPORTBUS > void pcie_reset_lbms_count(struct pci_dev *port); > int pcie_lbms_count(struct pci_dev *port, unsigned long *val); > +void pcie_disable_hp_interrupts_early(struct pci_dev *dev); > #else > static inline void pcie_reset_lbms_count(struct pci_dev *port) {} > static inline int pcie_lbms_count(struct pci_dev *port, unsigned long *val) > { > return -EOPNOTSUPP; > } > +static inline void pcie_disable_hp_interrupts_early(struct pci_dev *dev) {} > #endif > > struct pci_dev_reset_methods { > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > index 02e73099bad0..16010973bfe2 100644 > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -18,6 +18,7 @@ > #include <linux/string.h> > #include <linux/slab.h> > #include <linux/aer.h> > +#include <linux/delay.h> > > #include "../pci.h" > #include "portdrv.h" > @@ -205,6 +206,35 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask) > return 0; > } > > +static int pcie_wait_sltctl_cmd_raw(struct pci_dev *pdev) > +{ > + u16 slot_status; > + /* 1000 ms, according toPCIe spec 6.1, section 6.7.3.2 */ > + int timeout = 1000; > + > + do { > + pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); > + if (slot_status & PCI_EXP_SLTSTA_CC) { > + pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, > + PCI_EXP_SLTSTA_CC); > + return 0; > + } > + msleep(10); > + timeout -= 10; > + } while (timeout); > + > + /* Timeout */ > + return -1; > +} May be this logic can be simplified using readl_poll_timeout()? > + > +void pcie_disable_hp_interrupts_early(struct pci_dev *dev) > +{ > + pcie_capability_clear_word(dev, PCI_EXP_SLTCTL, > + PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE); > + if (pcie_wait_sltctl_cmd_raw(dev)) > + pci_info(dev, "Timeout on disabling hot-plug interrupts\n"); > +} > + > /** > * get_port_device_capability - discover capabilities of a PCI Express port > * @dev: PCI Express port to examine > @@ -230,8 +260,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); > } > > #ifdef CONFIG_PCIEAER
Hi Markus, Thanks for the review and reminding! On Wed, Feb 05, 2025 at 06:48:35PM +0100, Markus Elfring wrote: > … > > Add the necessary wait to comply with PCIe spec. The waiting logic refers > > existing pcie_poll_cmd(). > > * How do you think about to add any tags (like “Fixes” and “Cc”) accordingly? The code went through some refactor, and the related commit should be: commit 2bd50dd800b52245294cfceb56be62020cdc7515 Author: Rafael J. Wysocki <rjw@rjwysocki.net> Date: Sat Aug 21 01:57:39 2010 +0200 PCI: PCIe: Disable PCIe port services during port initialization In principle PCIe port services may be enabled by the BIOS, so it's better to disable them during port initialization to avoid spurious events from being generated. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> Reviewed-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> Add Rafael to cc. > * Will cover letters be usually helpful for such patch series? Initially I planned to send the 2 patches seprately as they handle different issues, but as 2/2 will reuse the helper function added by 1/2, I put them together. > * You would probably like to avoid a typo in an email address. Could you be more specific? I got the mail addresses from get_maintainers.pl. thanks! - Feng
Hi Sathyanarayanan, On Wed, Feb 05, 2025 at 10:26:59AM -0800, Sathyanarayanan Kuppuswamy wrote: > > On 2/3/25 9:37 PM, Feng Tang wrote: > > According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at > > least 1 second for the command-complete event, before resending the cmd > > or sending a new cmd. > > > > Currently get_port_device_capability() sends slot control cmd to disable > > PCIe hotplug interrupts without waiting for its completion and there was > > real problem reported for the lack of waiting. > > Can you include the error log associated with this issue? What is the > actual issue you are seeing and in which hardware? For this one, we don't have specific log, as it was raised by firmware developer, as in https://lore.kernel.org/lkml/Z6LRAozZm1UfgjqT@U-2FWC9VHC-2323.local/ When handling PCI hotplug problem, they hit issue and found their state machine corrupted , and back traced to OS. They didn't expect to receive 2 link control commands at almost the same time, which doesn't comply to pcie spec, and normally the handling of one command will take some time in BIOS, though not as long as 1 second. The HW is an ARM server. I will try to add these info to commit log in next version. > > > > > Add the necessary wait to comply with PCIe spec. The waiting logic refers > > existing pcie_poll_cmd(). > > > > Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com> > > --- > > drivers/pci/pci.h | 2 ++ > > drivers/pci/pcie/portdrv.c | 33 +++++++++++++++++++++++++++++++-- > > 2 files changed, 33 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > > index 01e51db8d285..c1e234d1b81d 100644 > > --- a/drivers/pci/pci.h > > +++ b/drivers/pci/pci.h > > @@ -759,12 +759,14 @@ static inline void pcie_ecrc_get_policy(char *str) { } > > #ifdef CONFIG_PCIEPORTBUS > > void pcie_reset_lbms_count(struct pci_dev *port); > > int pcie_lbms_count(struct pci_dev *port, unsigned long *val); > > +void pcie_disable_hp_interrupts_early(struct pci_dev *dev); > > #else > > static inline void pcie_reset_lbms_count(struct pci_dev *port) {} > > static inline int pcie_lbms_count(struct pci_dev *port, unsigned long *val) > > { > > return -EOPNOTSUPP; > > } > > +static inline void pcie_disable_hp_interrupts_early(struct pci_dev *dev) {} > > #endif > > struct pci_dev_reset_methods { > > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > > index 02e73099bad0..16010973bfe2 100644 > > --- a/drivers/pci/pcie/portdrv.c > > +++ b/drivers/pci/pcie/portdrv.c > > @@ -18,6 +18,7 @@ > > #include <linux/string.h> > > #include <linux/slab.h> > > #include <linux/aer.h> > > +#include <linux/delay.h> > > #include "../pci.h" > > #include "portdrv.h" > > @@ -205,6 +206,35 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask) > > return 0; > > } > > +static int pcie_wait_sltctl_cmd_raw(struct pci_dev *pdev) > > +{ > > + u16 slot_status; > > + /* 1000 ms, according toPCIe spec 6.1, section 6.7.3.2 */ > > + int timeout = 1000; > > + > > + do { > > + pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); > > + if (slot_status & PCI_EXP_SLTSTA_CC) { > > + pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, > > + PCI_EXP_SLTSTA_CC); > > + return 0; > > + } > > + msleep(10); > > + timeout -= 10; > > + } while (timeout); > > + > > + /* Timeout */ > > + return -1; > > +} > > May be this logic can be simplified using readl_poll_timeout()? Seems this is what exactly I needed :) Many thanks for the suggestion! - Feng
> Could you be more specific? I got the mail addresses from get_maintainers.pl.
Would you like to take another look at information also according to Jonathan Cameron
(for example in your patch recipient list)?
Regards,
Markus
On Thu, Feb 06, 2025 at 12:40:44PM +0100, Markus Elfring wrote: > > Could you be more specific? I got the mail addresses from get_maintainers.pl. > Would you like to take another look at information also according to Jonathan Cameron > (for example in your patch recipient list)? I see, thanks! - Feng > Regards, > Markus
On 2/5/25 7:18 PM, Feng Tang wrote: > Hi Sathyanarayanan, > > On Wed, Feb 05, 2025 at 10:26:59AM -0800, Sathyanarayanan Kuppuswamy wrote: >> On 2/3/25 9:37 PM, Feng Tang wrote: >>> According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at >>> least 1 second for the command-complete event, before resending the cmd >>> or sending a new cmd. >>> >>> Currently get_port_device_capability() sends slot control cmd to disable >>> PCIe hotplug interrupts without waiting for its completion and there was >>> real problem reported for the lack of waiting. >> Can you include the error log associated with this issue? What is the >> actual issue you are seeing and in which hardware? > For this one, we don't have specific log, as it was raised by firmware > developer, as in https://lore.kernel.org/lkml/Z6LRAozZm1UfgjqT@U-2FWC9VHC-2323.local/ > > When handling PCI hotplug problem, they hit issue and found their state > machine corrupted , and back traced to OS. They didn't expect to receive > 2 link control commands at almost the same time, which doesn't comply to Which 2 commands from OS? Did you identify both commands? > pcie spec, and normally the handling of one command will take some time > in BIOS, though not as long as 1 second. The HW is an ARM server. > > I will try to add these info to commit log in next version. Ok. Please include it. > >>> Add the necessary wait to comply with PCIe spec. The waiting logic refers >>> existing pcie_poll_cmd(). >>> >>> Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com> >>> --- >>> drivers/pci/pci.h | 2 ++ >>> drivers/pci/pcie/portdrv.c | 33 +++++++++++++++++++++++++++++++-- >>> 2 files changed, 33 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >>> index 01e51db8d285..c1e234d1b81d 100644 >>> --- a/drivers/pci/pci.h >>> +++ b/drivers/pci/pci.h >>> @@ -759,12 +759,14 @@ static inline void pcie_ecrc_get_policy(char *str) { } >>> #ifdef CONFIG_PCIEPORTBUS >>> void pcie_reset_lbms_count(struct pci_dev *port); >>> int pcie_lbms_count(struct pci_dev *port, unsigned long *val); >>> +void pcie_disable_hp_interrupts_early(struct pci_dev *dev); >>> #else >>> static inline void pcie_reset_lbms_count(struct pci_dev *port) {} >>> static inline int pcie_lbms_count(struct pci_dev *port, unsigned long *val) >>> { >>> return -EOPNOTSUPP; >>> } >>> +static inline void pcie_disable_hp_interrupts_early(struct pci_dev *dev) {} >>> #endif >>> struct pci_dev_reset_methods { >>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c >>> index 02e73099bad0..16010973bfe2 100644 >>> --- a/drivers/pci/pcie/portdrv.c >>> +++ b/drivers/pci/pcie/portdrv.c >>> @@ -18,6 +18,7 @@ >>> #include <linux/string.h> >>> #include <linux/slab.h> >>> #include <linux/aer.h> >>> +#include <linux/delay.h> >>> #include "../pci.h" >>> #include "portdrv.h" >>> @@ -205,6 +206,35 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask) >>> return 0; >>> } >>> +static int pcie_wait_sltctl_cmd_raw(struct pci_dev *pdev) >>> +{ >>> + u16 slot_status; >>> + /* 1000 ms, according toPCIe spec 6.1, section 6.7.3.2 */ >>> + int timeout = 1000; >>> + >>> + do { >>> + pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); >>> + if (slot_status & PCI_EXP_SLTSTA_CC) { >>> + pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, >>> + PCI_EXP_SLTSTA_CC); >>> + return 0; >>> + } >>> + msleep(10); >>> + timeout -= 10; >>> + } while (timeout); >>> + >>> + /* Timeout */ >>> + return -1; >>> +} >> May be this logic can be simplified using readl_poll_timeout()? > Seems this is what exactly I needed :) Many thanks for the suggestion! > > - Feng
On Thu, Feb 06, 2025 at 08:26:13PM -0800, Sathyanarayanan Kuppuswamy wrote: > > On 2/5/25 7:18 PM, Feng Tang wrote: > > Hi Sathyanarayanan, > > > > On Wed, Feb 05, 2025 at 10:26:59AM -0800, Sathyanarayanan Kuppuswamy wrote: > > > On 2/3/25 9:37 PM, Feng Tang wrote: > > > > According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at > > > > least 1 second for the command-complete event, before resending the cmd > > > > or sending a new cmd. > > > > > > > > Currently get_port_device_capability() sends slot control cmd to disable > > > > PCIe hotplug interrupts without waiting for its completion and there was > > > > real problem reported for the lack of waiting. > > > Can you include the error log associated with this issue? What is the > > > actual issue you are seeing and in which hardware? > > For this one, we don't have specific log, as it was raised by firmware > > developer, as in https://lore.kernel.org/lkml/Z6LRAozZm1UfgjqT@U-2FWC9VHC-2323.local/ > > > > When handling PCI hotplug problem, they hit issue and found their state > > machine corrupted , and back traced to OS. They didn't expect to receive > > 2 link control commands at almost the same time, which doesn't comply to > > Which 2 commands from OS? Did you identify both commands? Firmware developer saw 2 programming to PCIE Slot Control register of one root port, first is writing 0x5ca, the second is writing 0x15f8. IIUC, the first one is to disable CCIE/HPIE here from get_port_device_capability() Thanks, Feng
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 01e51db8d285..c1e234d1b81d 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -759,12 +759,14 @@ static inline void pcie_ecrc_get_policy(char *str) { } #ifdef CONFIG_PCIEPORTBUS void pcie_reset_lbms_count(struct pci_dev *port); int pcie_lbms_count(struct pci_dev *port, unsigned long *val); +void pcie_disable_hp_interrupts_early(struct pci_dev *dev); #else static inline void pcie_reset_lbms_count(struct pci_dev *port) {} static inline int pcie_lbms_count(struct pci_dev *port, unsigned long *val) { return -EOPNOTSUPP; } +static inline void pcie_disable_hp_interrupts_early(struct pci_dev *dev) {} #endif struct pci_dev_reset_methods { diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c index 02e73099bad0..16010973bfe2 100644 --- a/drivers/pci/pcie/portdrv.c +++ b/drivers/pci/pcie/portdrv.c @@ -18,6 +18,7 @@ #include <linux/string.h> #include <linux/slab.h> #include <linux/aer.h> +#include <linux/delay.h> #include "../pci.h" #include "portdrv.h" @@ -205,6 +206,35 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask) return 0; } +static int pcie_wait_sltctl_cmd_raw(struct pci_dev *pdev) +{ + u16 slot_status; + /* 1000 ms, according toPCIe spec 6.1, section 6.7.3.2 */ + int timeout = 1000; + + do { + pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); + if (slot_status & PCI_EXP_SLTSTA_CC) { + pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, + PCI_EXP_SLTSTA_CC); + return 0; + } + msleep(10); + timeout -= 10; + } while (timeout); + + /* Timeout */ + return -1; +} + +void pcie_disable_hp_interrupts_early(struct pci_dev *dev) +{ + pcie_capability_clear_word(dev, PCI_EXP_SLTCTL, + PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE); + if (pcie_wait_sltctl_cmd_raw(dev)) + pci_info(dev, "Timeout on disabling hot-plug interrupts\n"); +} + /** * get_port_device_capability - discover capabilities of a PCI Express port * @dev: PCI Express port to examine @@ -230,8 +260,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); } #ifdef CONFIG_PCIEAER
According to PCIe 6.1 spec, section 6.7.3.2, software need to wait at least 1 second for the command-complete event, before resending the cmd or sending a new cmd. Currently get_port_device_capability() sends slot control cmd to disable PCIe hotplug interrupts without waiting for its completion and there was real problem reported for the lack of waiting. Add the necessary wait to comply with PCIe spec. The waiting logic refers existing pcie_poll_cmd(). Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com> --- drivers/pci/pci.h | 2 ++ drivers/pci/pcie/portdrv.c | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-)