| Message ID | 20191025190047.38130-4-stuart.w.hayes@gmail.com |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: pciehp: Do not turn off slot if presence comes up after link | expand |
On Fri, Oct 25, 2019 at 10:01 PM Stuart Hayes <stuart.w.hayes@gmail.com> 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. > FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> > --- > v4 > add comment to dmi table > > drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 02d95ab27a12..9541735bd0aa 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,24 @@ > #include "../pci.h" > #include "pciehp.h" > > +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = { > + /* > + * Match all Dell systems, as some Dell systems have inband > + * presence disabled on NVMe slots (but don't support the bit to > + * report it). Setting inband presence disabled should have no > + * negative effect, except on broken hotplug slots that never > + * assert presence detect--and those will still work, they will > + * just have a bit of extra delay before being probed. > + */ > + { > + .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 +914,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. > -- > 2.18.1 >
On Fri, Oct 25, 2019 at 03:00:47PM -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. This doesn't seem quite accurate to me. PCI_EXP_SLTCAP2_IBPD does not actually tell us whether in-band presence detection is disabled. It only tells us whether it *can* be disabled. I think I know what you mean, but this text suggests that PCI_EXP_SLTCAP2_IBPD not being set is the defect, and I don't think it is. IIUC, even if PCI_EXP_SLTCAP2_IBPD were set, PCI_EXP_SLTCTL_IBPD_DISABLE would have no effect because in-band presence detect just isn't supported at all regardless of how we set PCI_EXP_SLTCTL_IBPD_DISABLE. > 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> > --- > v4 > add comment to dmi table > > drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 02d95ab27a12..9541735bd0aa 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,24 @@ > #include "../pci.h" > #include "pciehp.h" > > +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = { > + /* > + * Match all Dell systems, as some Dell systems have inband > + * presence disabled on NVMe slots (but don't support the bit to Is there something that restricts these slots to being used only for NVMe? If not, I'd rather simply say that some Root Ports don't support in-band presence detect. You say it's "disabled", which suggests that it could be *enabled*. But I have the impression that it's actually just not supported at all (or maybe it's disabled by the BIOS via some non-architected mechanism). > + * report it). Setting inband presence disabled should have no > + * negative effect, except on broken hotplug slots that never > + * assert presence detect--and those will still work, they will > + * just have a bit of extra delay before being probed. > + */ > + { > + .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 +914,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; This doesn't seem quite right: the DMI table should only apply to built-in ports, not to ports on plugin cards. If we plug in a switch with hotplug-capable downstream ports, and those ports do not advertise PCI_EXP_SLTCAP2_IBPD, I think this code sets "inband_presence_disabled" for those ports even though it is not disabled. IIUC, that will make this plugin card behave differently in a Dell system than it will in other systems, and that doesn't seem right to me. > /* > * 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. > -- > 2.18.1 >
On 2/20/20 11:21 PM, Bjorn Helgaas wrote: > On Fri, Oct 25, 2019 at 03:00:47PM -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. > > This doesn't seem quite accurate to me. PCI_EXP_SLTCAP2_IBPD does not > actually tell us whether in-band presence detection is disabled. It > only tells us whether it *can* be disabled. > > I think I know what you mean, but this text suggests that > PCI_EXP_SLTCAP2_IBPD not being set is the defect, and I don't think it > is. IIUC, even if PCI_EXP_SLTCAP2_IBPD were set, > PCI_EXP_SLTCTL_IBPD_DISABLE would have no effect because in-band > presence detect just isn't supported at all regardless of how we set > PCI_EXP_SLTCTL_IBPD_DISABLE. > I agree, the patch notes aren't quite correct. Many Dell systems use out-of-band presence detect, and there is no way to enable in-band presence detection. So I guess the notes would be more accurate if they said something like this: Some systems use out-of-band presence detection exclusively, and do not support the SLTCAP2_IBPD and SLTCTL_IBPD_DISABLE bits, but they still need to have "inband_presence_disabled" set. Add a dmi table to flag these systems as having in-band presence disabled. >> 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> >> --- >> v4 >> add comment to dmi table >> >> drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c >> index 02d95ab27a12..9541735bd0aa 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,24 @@ >> #include "../pci.h" >> #include "pciehp.h" >> >> +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = { >> + /* >> + * Match all Dell systems, as some Dell systems have inband >> + * presence disabled on NVMe slots (but don't support the bit to > > Is there something that restricts these slots to being used only for > NVMe? If not, I'd rather simply say that some Root Ports don't > support in-band presence detect. You say it's "disabled", which > suggests that it could be *enabled*. But I have the impression that > it's actually just not supported at all (or maybe it's disabled by the > BIOS via some non-architected mechanism). > These slots are in the backplane that is meant for hard drives, so the I don't know that there would be any other devices that would fit in them. But, if some other device did fit physically and had the right connector, I believe it would work. I'm not sure if in-band presence detection is disabled by firmware, or hardwired that way in the circuitry somehow, but you are correct, in-band presence detect can't be enabled on these systems. >> + * report it). Setting inband presence disabled should have no >> + * negative effect, except on broken hotplug slots that never >> + * assert presence detect--and those will still work, they will >> + * just have a bit of extra delay before being probed. >> + */ >> + { >> + .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 +914,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; > > This doesn't seem quite right: the DMI table should only apply to > built-in ports, not to ports on plugin cards. > > If we plug in a switch with hotplug-capable downstream ports, and > those ports do not advertise PCI_EXP_SLTCAP2_IBPD, I think this code > sets "inband_presence_disabled" for those ports even though it is not > disabled. > > IIUC, that will make this plugin card behave differently in a Dell > system than it will in other systems, and that doesn't seem right to > me. > Setting "inband_presence_disabled" will only make a hotplug-capable slot behave differently in two situations: (1) The slot is using out-of-band presence detection instead of in-band, in which case the hotplug code will correctly wait for the presence detect bit to be active (after the link goes active) before proceeding to add the device. (2) The slot is broken and the presence detect bit never goes high in spite of the PCI spec saying "This bit must be implemented on all Downstream Ports that implement slots" (pcie base spec 5.0r1.0, section 7.5.3.11, slot status register). In this case, the hotplug slot will still work when "inband_presence_disabled" is set, but it will take it a second or so longer for the device to be added in linux because it will timeout waiting for the presence detect bit to go active, instead of adding the device as soon as the link goes active. Since Dell doesn't support any plugin cards that provide hot plug slots that are broken in that way, setting "inband_presence_disabled" shouldn't make any difference. But even if someone did use a card that was broken in this way, it would still function. If you think this is important, I could try to find a way to set "inband_presence_disabled" just for built-in ports. I'm not sure it would be of any practical benefit, though. >> /* >> * 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. >> -- >> 2.18.1 >>
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 02d95ab27a12..9541735bd0aa 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,24 @@ #include "../pci.h" #include "pciehp.h" +static const struct dmi_system_id inband_presence_disabled_dmi_table[] = { + /* + * Match all Dell systems, as some Dell systems have inband + * presence disabled on NVMe slots (but don't support the bit to + * report it). Setting inband presence disabled should have no + * negative effect, except on broken hotplug slots that never + * assert presence detect--and those will still work, they will + * just have a bit of extra delay before being probed. + */ + { + .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 +914,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> --- v4 add comment to dmi table drivers/pci/hotplug/pciehp_hpc.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)