| Message ID | 20191017193256.3636-4-stuart.w.hayes@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: pciehp: Do not turn off slot if presence comes up after link | expand |
On Thu, Oct 17, 2019 at 03:32:56PM -0400, Stuart Hayes wrote: > Some systems have in-band presence detection disabled for hot-plug PCI > slots, but do not report this in the slot capabilities 2 (SLTCAP2) register. > On these systems, presence detect can become active well after the link is > reported to be active, which can cause the slots to be disabled after a > device is connected. > > Add a dmi table to flag these systems as having in-band presence disabled. > > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> > --- > drivers/pci/hotplug/pciehp_hpc.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 02eb811a014f..4d377a2a62ce 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -14,6 +14,7 @@ > > #define dev_fmt(fmt) "pciehp: " fmt > > +#include <linux/dmi.h> > #include <linux/kernel.h> > #include <linux/types.h> > #include <linux/jiffies.h> > @@ -26,6 +27,16 @@ > #include "../pci.h" > #include "pciehp.h" > > +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = { > + { > + .ident = "Dell System", > + .matches = { > + DMI_MATCH(DMI_OEM_STRING, "Dell System"), Sorry if this has been discussed previously already but isn't this going to apply on all Dell systems, not just the affected ones? Is this the intention?
On 10/21/19 8:47 AM, Mika Westerberg wrote: > On Thu, Oct 17, 2019 at 03:32:56PM -0400, Stuart Hayes wrote: >> Some systems have in-band presence detection disabled for hot-plug PCI >> slots, but do not report this in the slot capabilities 2 (SLTCAP2) register. >> On these systems, presence detect can become active well after the link is >> reported to be active, which can cause the slots to be disabled after a >> device is connected. >> >> Add a dmi table to flag these systems as having in-band presence disabled. >> >> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> >> --- >> drivers/pci/hotplug/pciehp_hpc.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c >> index 02eb811a014f..4d377a2a62ce 100644 >> --- a/drivers/pci/hotplug/pciehp_hpc.c >> +++ b/drivers/pci/hotplug/pciehp_hpc.c >> @@ -14,6 +14,7 @@ >> >> #define dev_fmt(fmt) "pciehp: " fmt >> >> +#include <linux/dmi.h> >> #include <linux/kernel.h> >> #include <linux/types.h> >> #include <linux/jiffies.h> >> @@ -26,6 +27,16 @@ >> #include "../pci.h" >> #include "pciehp.h" >> >> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = { >> + { >> + .ident = "Dell System", >> + .matches = { >> + DMI_MATCH(DMI_OEM_STRING, "Dell System"), > > Sorry if this has been discussed previously already but isn't this going > to apply on all Dell systems, not just the affected ones? Is this the > intention? > Yes, that is the intention. Applying this just makes the hotplug code wait for the presence detect bit to be set before proceeding, which ideally wouldn't hurt anything--for devices that don't have inband presence detect disabled, presence detect should already be up when the code in patch 2/3 starts to wait for it. The only issue should be with broken hotplug implementations that don't ever bring presence detect active (these apparently exist)--but even those would still work, they would just take an extra second to come up. On the other hand, a number of Dell systems have (and will have) NVMe implementations that have inband presence detect disabled (but they won't have the new bit implemented to report that), and they don't work correctly without this.
On 10/21/19 1:19 PM, Stuart Hayes wrote: > > > On 10/21/19 8:47 AM, Mika Westerberg wrote: >> On Thu, Oct 17, 2019 at 03:32:56PM -0400, Stuart Hayes wrote: >>> Some systems have in-band presence detection disabled for hot-plug PCI >>> slots, but do not report this in the slot capabilities 2 (SLTCAP2) register. >>> On these systems, presence detect can become active well after the link is >>> reported to be active, which can cause the slots to be disabled after a >>> device is connected. >>> >>> Add a dmi table to flag these systems as having in-band presence disabled. >>> >>> Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> >>> --- >>> drivers/pci/hotplug/pciehp_hpc.c | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c >>> index 02eb811a014f..4d377a2a62ce 100644 >>> --- a/drivers/pci/hotplug/pciehp_hpc.c >>> +++ b/drivers/pci/hotplug/pciehp_hpc.c >>> @@ -14,6 +14,7 @@ >>> >>> #define dev_fmt(fmt) "pciehp: " fmt >>> >>> +#include <linux/dmi.h> >>> #include <linux/kernel.h> >>> #include <linux/types.h> >>> #include <linux/jiffies.h> >>> @@ -26,6 +27,16 @@ >>> #include "../pci.h" >>> #include "pciehp.h" >>> >>> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = { >>> + { >>> + .ident = "Dell System", >>> + .matches = { >>> + DMI_MATCH(DMI_OEM_STRING, "Dell System"), >> >> Sorry if this has been discussed previously already but isn't this going >> to apply on all Dell systems, not just the affected ones? Is this the >> intention? >> > > Yes, that is the intention. Applying this just makes the hotplug code wait for > the presence detect bit to be set before proceeding, which ideally wouldn't hurt > anything--for devices that don't have inband presence detect disabled, presence > detect should already be up when the code in patch 2/3 starts to wait for it. > > The only issue should be with broken hotplug implementations that don't ever > bring presence detect active (these apparently exist)--but even those would still > work, they would just take an extra second to come up. > > On the other hand, a number of Dell systems have (and will have) NVMe > implementations that have inband presence detect disabled (but they won't have > the new bit implemented to report that), and they don't work correctly without > this. I think it's clearer if this is explained in a comment. That it doesn't break anything, and we're okay this applies to all hotplug ports, even those that are not in front of an NVMe backplane. Alex
On Mon, Oct 21, 2019 at 07:13:32PM -0500, Alex G. wrote: > I think it's clearer if this is explained in a comment. That it doesn't > break anything, and we're okay this applies to all hotplug ports, even those > that are not in front of an NVMe backplane. I agree.
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 02eb811a014f..4d377a2a62ce 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -14,6 +14,7 @@ #define dev_fmt(fmt) "pciehp: " fmt +#include <linux/dmi.h> #include <linux/kernel.h> #include <linux/types.h> #include <linux/jiffies.h> @@ -26,6 +27,16 @@ #include "../pci.h" #include "pciehp.h" +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = { + { + .ident = "Dell System", + .matches = { + DMI_MATCH(DMI_OEM_STRING, "Dell System"), + }, + }, + {} +}; + static inline struct pci_dev *ctrl_dev(struct controller *ctrl) { return ctrl->pcie->port; @@ -895,6 +906,9 @@ struct controller *pcie_init(struct pcie_device *dev) ctrl->inband_presence_disabled = 1; } + if (dmi_first_match(inband_presence_disabled_dmi_table)) + ctrl->inband_presence_disabled = 1; + /* * If empty slot's power status is on, turn power off. The IRQ isn't * requested yet, so avoid triggering a notification with this command.
Some systems have in-band presence detection disabled for hot-plug PCI slots, but do not report this in the slot capabilities 2 (SLTCAP2) register. On these systems, presence detect can become active well after the link is reported to be active, which can cause the slots to be disabled after a device is connected. Add a dmi table to flag these systems as having in-band presence disabled. Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> --- drivers/pci/hotplug/pciehp_hpc.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)