* [PATCHv2 0/2] VMD PCIe specific LED control @ 2016-08-26 17:23 Keith Busch 2016-08-26 17:23 ` [PATCHv2 1/2] pciehp: Let user control LED status Keith Busch 2016-08-26 17:23 ` [PATCHv2 2/2] x86/vmd: Add PCI domain specific LED option Keith Busch 0 siblings, 2 replies; 4+ messages in thread From: Keith Busch @ 2016-08-26 17:23 UTC (permalink / raw) To: linux-pci, Bjorn Helgaas, Thomas Gleixner; +Cc: LKML, x86, Keith Busch Here is the second version for handling non-standard LED status in VMD domains. There are significant differences this time around: First, after discussing the original proposal, we decided that we can't support allowing user space have direct write access to config space. This potentially breaks the PCIe Slot Control command sequence, so patch 1/2 provides an alternate sysfs attention setter/getter in addition to ignoring the indicators for standard usage. Second, I've since learned that there is no such VID/DID list I can use in order to flag this quirk. The exact same devices with the quirk behave differently when used outside a VMD domain. Since this quirk behavior is specific to the VMD PCIe domain, the new approach is to set options specific to domains as pci_dev's are being added. Since patch 2/2 requires changes to x86 common code, I've added the x86 maintainers in addition to PCI, and I'll re-summarize what this feature/quirk is about: This came from wanting a simple SGPIO-like LED management solution for PCIe SSDs. The Intel group who made this hardware, not considering the more broad impact on standarization, chose to reuse the hot plug serial SMBus in the Intel CPUs (aka VPP) that already carried the Slot Control register bits out of the CPU. This hardware implementation therefore re-purposes the Slot Control's Attention Indicator Control and Power Indicator Control of the PCI Express Capabilities structure. Rather than using the PCIe standard interpretation, this hardware uses IBPI (International Blinking Pattern Interpretation). One side affect is that the Attention and Power indicator presence bits in the Slot Capabilities structures must remain enabled due to how the hardware is wired. This would confuse the pciehp driver since it would incorrectly (albeit understandably) assume how to control these capabilities. So this patch series has to tell pciehp to keep away from controlling these indicators and provide a way for the user to set them instead. This quirky behavior consideration is only required for the current generation of this hardware. Future generations will use standards compliance as being pursued within the PCI-SIG and NVMe-MI standards bodies. Keith Busch (2): pciehp: Let user control LED status x86/vmd: Add PCI domain specific LED option arch/x86/include/asm/pci.h | 14 ++++++++++++++ arch/x86/pci/common.c | 7 +++++++ arch/x86/pci/vmd.c | 1 + drivers/pci/hotplug/pciehp.h | 1 + drivers/pci/hotplug/pciehp_core.c | 26 ++++++++++++++++++++++++++ drivers/pci/hotplug/pciehp_hpc.c | 6 +++++- include/linux/pci.h | 1 + 7 files changed, 55 insertions(+), 1 deletion(-) -- 2.7.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCHv2 1/2] pciehp: Let user control LED status 2016-08-26 17:23 [PATCHv2 0/2] VMD PCIe specific LED control Keith Busch @ 2016-08-26 17:23 ` Keith Busch 2016-09-07 20:04 ` Bjorn Helgaas 2016-08-26 17:23 ` [PATCHv2 2/2] x86/vmd: Add PCI domain specific LED option Keith Busch 1 sibling, 1 reply; 4+ messages in thread From: Keith Busch @ 2016-08-26 17:23 UTC (permalink / raw) To: linux-pci, Bjorn Helgaas, Thomas Gleixner; +Cc: LKML, x86, Keith Busch This patch adds a new flag to the pci_dev structure instructing pciehp to ignore PCIe slot LED indicators. The pciehp driver will instead provide exclusive attention control to the user by setting the slot control exactly as the user requested with out interpreting the input. This is preparing for domain devices that repurpose these for non-standard use. Signed-off-by: Keith Busch <keith.busch@intel.com> --- drivers/pci/hotplug/pciehp.h | 1 + drivers/pci/hotplug/pciehp_core.c | 26 ++++++++++++++++++++++++++ drivers/pci/hotplug/pciehp_hpc.c | 6 +++++- include/linux/pci.h | 1 + 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h index e764918..ed62645 100644 --- a/drivers/pci/hotplug/pciehp.h +++ b/drivers/pci/hotplug/pciehp.h @@ -151,6 +151,7 @@ int pciehp_check_link_status(struct controller *ctrl); bool pciehp_check_link_active(struct controller *ctrl); void pciehp_release_ctrl(struct controller *ctrl); int pciehp_reset_slot(struct slot *slot, int probe); +void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask); static inline const char *slot_name(struct slot *slot) { diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index ac531e6..ad19b33 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -70,6 +70,8 @@ static int get_attention_status(struct hotplug_slot *slot, u8 *value); static int get_latch_status(struct hotplug_slot *slot, u8 *value); static int get_adapter_status(struct hotplug_slot *slot, u8 *value); static int reset_slot(struct hotplug_slot *slot, int probe); +static int set_user_led_status(struct hotplug_slot *slot, u8 value); +static int get_user_led_status(struct hotplug_slot *slot, u8 *value); /** * release_slot - free up the memory used by a slot @@ -114,6 +116,9 @@ static int init_slot(struct controller *ctrl) if (ATTN_LED(ctrl)) { ops->get_attention_status = get_attention_status; ops->set_attention_status = set_attention_status; + } else if (ctrl->pcie->port->user_leds) { + ops->get_attention_status = get_user_led_status; + ops->set_attention_status = set_user_led_status; } /* register this slot with the hotplug pci core */ @@ -142,6 +147,27 @@ static void cleanup_slot(struct controller *ctrl) pci_hp_deregister(ctrl->slot->hotplug_slot); } +static int get_user_led_status(struct hotplug_slot *hotplug_slot, u8 *value) +{ + u16 slot_ctrl; + struct slot *slot = hotplug_slot->private; + struct pci_dev *pdev = slot->ctrl->pcie->port; + + pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl); + *value = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6; + + return 0; +} + +static int set_user_led_status(struct hotplug_slot *hotplug_slot, u8 status) +{ + struct slot *slot = hotplug_slot->private; + + pcie_write_cmd_nowait(slot->ctrl, status << 6, + PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC); + return 0; +} + /* * set_attention_status - Turns the Amber LED for a slot on, off or blink */ diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 08e84d6..51cfc96 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -228,7 +228,7 @@ static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask) } /* Same as above without waiting for the hardware to latch */ -static void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask) +void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask) { pcie_do_write_cmd(ctrl, cmd, mask, false); } @@ -804,6 +804,10 @@ struct controller *pcie_init(struct pcie_device *dev) } ctrl->pcie = dev; pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &slot_cap); + + if (pdev->user_leds) + slot_cap &= ~(PCI_EXP_SLTCAP_AIP | PCI_EXP_SLTCAP_PIP); + ctrl->slot_cap = slot_cap; mutex_init(&ctrl->ctrl_lock); init_waitqueue_head(&ctrl->queue); diff --git a/include/linux/pci.h b/include/linux/pci.h index bbca681..a8ba7d7 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -309,6 +309,7 @@ struct pci_dev { powered on/off by the corresponding bridge */ unsigned int ignore_hotplug:1; /* Ignore hotplug events */ + unsigned int user_leds:1; /* User excluse LED SlotCtl */ unsigned int d3_delay; /* D3->D0 transition time in ms */ unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */ -- 2.7.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCHv2 1/2] pciehp: Let user control LED status 2016-08-26 17:23 ` [PATCHv2 1/2] pciehp: Let user control LED status Keith Busch @ 2016-09-07 20:04 ` Bjorn Helgaas 0 siblings, 0 replies; 4+ messages in thread From: Bjorn Helgaas @ 2016-09-07 20:04 UTC (permalink / raw) To: Keith Busch; +Cc: linux-pci, Bjorn Helgaas, Thomas Gleixner, LKML, x86 Hi Keith, I like this. I think this fits into pciehp pretty well. Minor comments below. On Fri, Aug 26, 2016 at 11:23:01AM -0600, Keith Busch wrote: > This patch adds a new flag to the pci_dev structure instructing pciehp to > ignore PCIe slot LED indicators. The pciehp driver will instead provide > exclusive attention control to the user by setting the slot control > exactly as the user requested with out interpreting the input. This is > preparing for domain devices that repurpose these for non-standard use. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > --- > drivers/pci/hotplug/pciehp.h | 1 + > drivers/pci/hotplug/pciehp_core.c | 26 ++++++++++++++++++++++++++ > drivers/pci/hotplug/pciehp_hpc.c | 6 +++++- > include/linux/pci.h | 1 + > 4 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h > index e764918..ed62645 100644 > --- a/drivers/pci/hotplug/pciehp.h > +++ b/drivers/pci/hotplug/pciehp.h > @@ -151,6 +151,7 @@ int pciehp_check_link_status(struct controller *ctrl); > bool pciehp_check_link_active(struct controller *ctrl); > void pciehp_release_ctrl(struct controller *ctrl); > int pciehp_reset_slot(struct slot *slot, int probe); > +void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask); > > static inline const char *slot_name(struct slot *slot) > { > diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c > index ac531e6..ad19b33 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -70,6 +70,8 @@ static int get_attention_status(struct hotplug_slot *slot, u8 *value); > static int get_latch_status(struct hotplug_slot *slot, u8 *value); > static int get_adapter_status(struct hotplug_slot *slot, u8 *value); > static int reset_slot(struct hotplug_slot *slot, int probe); > +static int set_user_led_status(struct hotplug_slot *slot, u8 value); > +static int get_user_led_status(struct hotplug_slot *slot, u8 *value); > > /** > * release_slot - free up the memory used by a slot > @@ -114,6 +116,9 @@ static int init_slot(struct controller *ctrl) > if (ATTN_LED(ctrl)) { > ops->get_attention_status = get_attention_status; > ops->set_attention_status = set_attention_status; > + } else if (ctrl->pcie->port->user_leds) { > + ops->get_attention_status = get_user_led_status; > + ops->set_attention_status = set_user_led_status; > } > > /* register this slot with the hotplug pci core */ > @@ -142,6 +147,27 @@ static void cleanup_slot(struct controller *ctrl) > pci_hp_deregister(ctrl->slot->hotplug_slot); > } > > +static int get_user_led_status(struct hotplug_slot *hotplug_slot, u8 *value) > +{ > + u16 slot_ctrl; > + struct slot *slot = hotplug_slot->private; > + struct pci_dev *pdev = slot->ctrl->pcie->port; > + > + pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl); > + *value = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6; > + > + return 0; > +} > + > +static int set_user_led_status(struct hotplug_slot *hotplug_slot, u8 status) > +{ > + struct slot *slot = hotplug_slot->private; > + > + pcie_write_cmd_nowait(slot->ctrl, status << 6, > + PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC); > + return 0; > +} I want these to correspond as much as possible with the regular interfaces in name and structure. Can you rename these to pciehp_get_raw_attention_status() pciehp_set_raw_attention_status() and move them to drivers/pci/hotplug/pciehp_hpc.c alongside pciehp_get_attention_status() and pciehp_set_attention_status()? I know the interface doesn't exactly match: void pciehp_get_attention_status(struct slot *slot, u8 *status) int pciehp_get_raw_attention_status(struct hotplug_slot *hotplug_slot, u8 *value) But I think the get_attention_status() and set_attention_status() wrapper functions are pointless and should be removed. If you lead the way by omitting them, we can simplify the existing ones later. This also will remove the need to make pcie_write_cmd_nowait() non-static. > + > /* > * set_attention_status - Turns the Amber LED for a slot on, off or blink > */ > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 08e84d6..51cfc96 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -228,7 +228,7 @@ static void pcie_write_cmd(struct controller *ctrl, u16 cmd, u16 mask) > } > > /* Same as above without waiting for the hardware to latch */ > -static void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask) > +void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask) > { > pcie_do_write_cmd(ctrl, cmd, mask, false); > } > @@ -804,6 +804,10 @@ struct controller *pcie_init(struct pcie_device *dev) > } > ctrl->pcie = dev; > pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &slot_cap); > + > + if (pdev->user_leds) > + slot_cap &= ~(PCI_EXP_SLTCAP_AIP | PCI_EXP_SLTCAP_PIP); > + > ctrl->slot_cap = slot_cap; > mutex_init(&ctrl->ctrl_lock); > init_waitqueue_head(&ctrl->queue); > diff --git a/include/linux/pci.h b/include/linux/pci.h > index bbca681..a8ba7d7 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -309,6 +309,7 @@ struct pci_dev { > powered on/off by the > corresponding bridge */ > unsigned int ignore_hotplug:1; /* Ignore hotplug events */ > + unsigned int user_leds:1; /* User excluse LED SlotCtl */ > unsigned int d3_delay; /* D3->D0 transition time in ms */ > unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */ > > -- > 2.7.2 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCHv2 2/2] x86/vmd: Add PCI domain specific LED option 2016-08-26 17:23 [PATCHv2 0/2] VMD PCIe specific LED control Keith Busch 2016-08-26 17:23 ` [PATCHv2 1/2] pciehp: Let user control LED status Keith Busch @ 2016-08-26 17:23 ` Keith Busch 1 sibling, 0 replies; 4+ messages in thread From: Keith Busch @ 2016-08-26 17:23 UTC (permalink / raw) To: linux-pci, Bjorn Helgaas, Thomas Gleixner; +Cc: LKML, x86, Keith Busch This patch adds a new function to set PCI domain specific options as devices are added. The usage included in this patch is for LED indicator control in VMD domains, but may be extended in the future as new domain specific options are required. PCIe LED Slot Control in a VMD domain is repurposed to a non-standard implementation. As such, all devices in a VMD domain will be flagged so that pciehp does not attempt to use LED indicators. This user_led flag has pciehp provide a different sysfs entry for user exclusive control over the domain's slot indicators. In order to determine if a bus is within a PCI domain, the patch appends a bool to the pci_sysdata structure that the VMD driver sets during initialization. Requested-by: Kapil Karkra <kapil.karkra@intel.com> Tested-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com> Signed-off-by: Keith Busch <keith.busch@intel.com> --- arch/x86/include/asm/pci.h | 14 ++++++++++++++ arch/x86/pci/common.c | 7 +++++++ arch/x86/pci/vmd.c | 1 + 3 files changed, 22 insertions(+) diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h index 9ab7507..1411dbe 100644 --- a/arch/x86/include/asm/pci.h +++ b/arch/x86/include/asm/pci.h @@ -23,6 +23,9 @@ struct pci_sysdata { #ifdef CONFIG_PCI_MSI_IRQ_DOMAIN void *fwnode; /* IRQ domain for MSI assignment */ #endif +#if IS_ENABLED(CONFIG_VMD) + bool vmd_domain; /* True if in Intel VMD domain */ +#endif }; extern int pci_routeirq; @@ -56,6 +59,17 @@ static inline void *_pci_root_bus_fwnode(struct pci_bus *bus) #define pci_root_bus_fwnode _pci_root_bus_fwnode #endif +static inline bool is_vmd(struct pci_bus *bus) +{ +#if IS_ENABLED(CONFIG_VMD) + struct pci_sysdata *sd = bus->sysdata; + + return sd->vmd_domain; +#else + return false; +#endif +} + /* Can be used to override the logic in pci_scan_bus for skipping already-configured bus numbers - to be used for buggy BIOSes or architectures with incomplete PCI setup by the loader */ diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c index 7b6a9d1..ccf696c 100644 --- a/arch/x86/pci/common.c +++ b/arch/x86/pci/common.c @@ -677,6 +677,12 @@ static void set_dma_domain_ops(struct pci_dev *pdev) static void set_dma_domain_ops(struct pci_dev *pdev) {} #endif +static void set_dev_domain_options(struct pci_dev *pdev) +{ + if (is_vmd(pdev->bus)) + pdev->user_leds = 1; +} + int pcibios_add_device(struct pci_dev *dev) { struct setup_data *data; @@ -707,6 +713,7 @@ int pcibios_add_device(struct pci_dev *dev) iounmap(data); } set_dma_domain_ops(dev); + set_dev_domain_options(dev); return 0; } diff --git a/arch/x86/pci/vmd.c b/arch/x86/pci/vmd.c index b73da50..3014f8e 100644 --- a/arch/x86/pci/vmd.c +++ b/arch/x86/pci/vmd.c @@ -604,6 +604,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd) .parent = res, }; + sd->vmd_domain = true; sd->domain = vmd_find_free_domain(); if (sd->domain < 0) return sd->domain; -- 2.7.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-07 20:05 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-26 17:23 [PATCHv2 0/2] VMD PCIe specific LED control Keith Busch 2016-08-26 17:23 ` [PATCHv2 1/2] pciehp: Let user control LED status Keith Busch 2016-09-07 20:04 ` Bjorn Helgaas 2016-08-26 17:23 ` [PATCHv2 2/2] x86/vmd: Add PCI domain specific LED option Keith Busch
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).