All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv3 1/2] pciehp: Let user control LED status
@ 2016-09-13 15:05 Keith Busch
  2016-09-13 15:05 ` [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option Keith Busch
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Keith Busch @ 2016-09-13 15:05 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas, Thomas Gleixner; +Cc: LKML, Keith Busch

This patch adds a new flag to the pci_dev structure instructing pciehp
to not interpret PCIe slot LED indicators. The pciehp driver will instead
allow all LED control from the user by setting the slot control indicators
as the user requested through sysfs. This is preparing for domain devices
that repurpose these control bits for non-standard use.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
v2 -> v3:

  Moved the slot op's attention status callback to pciehp_hpc.c

 drivers/pci/hotplug/pciehp.h      |  5 +++++
 drivers/pci/hotplug/pciehp_core.c |  3 +++
 drivers/pci/hotplug/pciehp_hpc.c  | 27 +++++++++++++++++++++++++++
 include/linux/pci.h               |  1 +
 4 files changed, 36 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index e764918..7f71ac1 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -152,6 +152,11 @@ bool pciehp_check_link_active(struct controller *ctrl);
 void pciehp_release_ctrl(struct controller *ctrl);
 int pciehp_reset_slot(struct slot *slot, int probe);
 
+int pciehp_set_raw_attention_status(struct hotplug_slot *hotplug_slot,
+								u8 status);
+int pciehp_get_raw_attention_status(struct hotplug_slot *hotplug_slot,
+								u8 *value);
+
 static inline const char *slot_name(struct slot *slot)
 {
 	return hotplug_slot_name(slot->hotplug_slot);
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index fb0f863..68b7aeb 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -113,6 +113,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 = pciehp_get_raw_attention_status;
+		ops->set_attention_status = pciehp_set_raw_attention_status;
 	}
 
 	/* register this slot with the hotplug pci core */
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 08e84d6..fccb63d 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -355,6 +355,18 @@ static int pciehp_link_enable(struct controller *ctrl)
 	return __pciehp_link_set(ctrl, true);
 }
 
+int pciehp_get_raw_attention_status(struct hotplug_slot *hotplug_slot,
+								u8 *value)
+{
+	struct slot *slot = hotplug_slot->private;
+	struct pci_dev *pdev = ctrl_dev(slot->ctrl);
+	u16 slot_ctrl;
+
+	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
+	*status = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
+	return 0;
+}
+
 void pciehp_get_attention_status(struct slot *slot, u8 *status)
 {
 	struct controller *ctrl = slot->ctrl;
@@ -431,6 +443,17 @@ int pciehp_query_power_fault(struct slot *slot)
 	return !!(slot_status & PCI_EXP_SLTSTA_PFD);
 }
 
+int pciehp_set_raw_attention_status(struct hotplug_slot *hotplug_slot,
+								u8 status)
+{
+	struct slot *slot = hotplug_slot->private;
+	struct controller *ctrl = slot->ctrl;
+
+	pcie_write_cmd_nowait(ctrl, value << 6,
+			      PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC);
+	return 0;
+}
+
 void pciehp_set_attention_status(struct slot *slot, u8 value)
 {
 	struct controller *ctrl = slot->ctrl;
@@ -804,6 +827,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 7256f33..f41bbca 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -308,6 +308,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] 8+ messages in thread

* [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option
  2016-09-13 15:05 [PATCHv3 1/2] pciehp: Let user control LED status Keith Busch
@ 2016-09-13 15:05 ` Keith Busch
  2016-09-23 14:34   ` Bjorn Helgaas
  2016-09-13 15:28 ` [PATCHv3 1/2] pciehp: Let user control LED status kbuild test robot
  2016-09-13 16:36 ` Keith Busch
  2 siblings, 1 reply; 8+ messages in thread
From: Keith Busch @ 2016-09-13 15:05 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas, Thomas Gleixner; +Cc: LKML, 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
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>
---

No change from previous version of this patch; just part of the series.

 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 b814ca6..a021b7b 100644
--- a/arch/x86/pci/vmd.c
+++ b/arch/x86/pci/vmd.c
@@ -596,6 +596,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] 8+ messages in thread

* Re: [PATCHv3 1/2] pciehp: Let user control LED status
  2016-09-13 15:05 [PATCHv3 1/2] pciehp: Let user control LED status Keith Busch
  2016-09-13 15:05 ` [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option Keith Busch
@ 2016-09-13 15:28 ` kbuild test robot
  2016-09-13 16:36 ` Keith Busch
  2 siblings, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2016-09-13 15:28 UTC (permalink / raw)
  To: Keith Busch
  Cc: kbuild-all, linux-pci, Bjorn Helgaas, Thomas Gleixner, LKML, Keith Busch

[-- Attachment #1: Type: text/plain, Size: 4881 bytes --]

Hi Keith,

[auto build test ERROR on pci/next]
[also build test ERROR on v4.8-rc6 next-20160913]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Keith-Busch/pciehp-Let-user-control-LED-status/20160913-231123
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-x010-201637 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/pci/hotplug/pciehp_hpc.c: In function 'pciehp_get_raw_attention_status':
>> drivers/pci/hotplug/pciehp_hpc.c:366:3: error: 'status' undeclared (first use in this function)
     *status = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
      ^~~~~~
   drivers/pci/hotplug/pciehp_hpc.c:366:3: note: each undeclared identifier is reported only once for each function it appears in
   drivers/pci/hotplug/pciehp_hpc.c: In function 'pciehp_set_raw_attention_status':
>> drivers/pci/hotplug/pciehp_hpc.c:452:30: error: 'value' undeclared (first use in this function)
     pcie_write_cmd_nowait(ctrl, value << 6,
                                 ^~~~~

vim +/status +366 drivers/pci/hotplug/pciehp_hpc.c

   360	{
   361		struct slot *slot = hotplug_slot->private;
   362		struct pci_dev *pdev = ctrl_dev(slot->ctrl);
   363		u16 slot_ctrl;
   364	
   365		pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
 > 366		*status = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;
   367		return 0;
   368	}
   369	
   370	void pciehp_get_attention_status(struct slot *slot, u8 *status)
   371	{
   372		struct controller *ctrl = slot->ctrl;
   373		struct pci_dev *pdev = ctrl_dev(ctrl);
   374		u16 slot_ctrl;
   375	
   376		pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
   377		ctrl_dbg(ctrl, "%s: SLOTCTRL %x, value read %x\n", __func__,
   378			 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
   379	
   380		switch (slot_ctrl & PCI_EXP_SLTCTL_AIC) {
   381		case PCI_EXP_SLTCTL_ATTN_IND_ON:
   382			*status = 1;	/* On */
   383			break;
   384		case PCI_EXP_SLTCTL_ATTN_IND_BLINK:
   385			*status = 2;	/* Blink */
   386			break;
   387		case PCI_EXP_SLTCTL_ATTN_IND_OFF:
   388			*status = 0;	/* Off */
   389			break;
   390		default:
   391			*status = 0xFF;
   392			break;
   393		}
   394	}
   395	
   396	void pciehp_get_power_status(struct slot *slot, u8 *status)
   397	{
   398		struct controller *ctrl = slot->ctrl;
   399		struct pci_dev *pdev = ctrl_dev(ctrl);
   400		u16 slot_ctrl;
   401	
   402		pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
   403		ctrl_dbg(ctrl, "%s: SLOTCTRL %x value read %x\n", __func__,
   404			 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, slot_ctrl);
   405	
   406		switch (slot_ctrl & PCI_EXP_SLTCTL_PCC) {
   407		case PCI_EXP_SLTCTL_PWR_ON:
   408			*status = 1;	/* On */
   409			break;
   410		case PCI_EXP_SLTCTL_PWR_OFF:
   411			*status = 0;	/* Off */
   412			break;
   413		default:
   414			*status = 0xFF;
   415			break;
   416		}
   417	}
   418	
   419	void pciehp_get_latch_status(struct slot *slot, u8 *status)
   420	{
   421		struct pci_dev *pdev = ctrl_dev(slot->ctrl);
   422		u16 slot_status;
   423	
   424		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
   425		*status = !!(slot_status & PCI_EXP_SLTSTA_MRLSS);
   426	}
   427	
   428	void pciehp_get_adapter_status(struct slot *slot, u8 *status)
   429	{
   430		struct pci_dev *pdev = ctrl_dev(slot->ctrl);
   431		u16 slot_status;
   432	
   433		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
   434		*status = !!(slot_status & PCI_EXP_SLTSTA_PDS);
   435	}
   436	
   437	int pciehp_query_power_fault(struct slot *slot)
   438	{
   439		struct pci_dev *pdev = ctrl_dev(slot->ctrl);
   440		u16 slot_status;
   441	
   442		pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
   443		return !!(slot_status & PCI_EXP_SLTSTA_PFD);
   444	}
   445	
   446	int pciehp_set_raw_attention_status(struct hotplug_slot *hotplug_slot,
   447									u8 status)
   448	{
   449		struct slot *slot = hotplug_slot->private;
   450		struct controller *ctrl = slot->ctrl;
   451	
 > 452		pcie_write_cmd_nowait(ctrl, value << 6,
   453				      PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC);
   454		return 0;
   455	}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25485 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv3 1/2] pciehp: Let user control LED status
  2016-09-13 15:05 [PATCHv3 1/2] pciehp: Let user control LED status Keith Busch
  2016-09-13 15:05 ` [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option Keith Busch
  2016-09-13 15:28 ` [PATCHv3 1/2] pciehp: Let user control LED status kbuild test robot
@ 2016-09-13 16:36 ` Keith Busch
  2 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2016-09-13 16:36 UTC (permalink / raw)
  To: linux-pci, Bjorn Helgaas, Thomas Gleixner; +Cc: LKML

On Tue, Sep 13, 2016 at 09:05:39AM -0600, Keith Busch wrote:
> +int pciehp_get_raw_attention_status(struct hotplug_slot *hotplug_slot,
> +								u8 *value)
> +{
> +	struct slot *slot = hotplug_slot->private;
> +	struct pci_dev *pdev = ctrl_dev(slot->ctrl);
> +	u16 slot_ctrl;
> +
> +	pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl);
> +	*status = (slot_ctrl & (PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC)) >> 6;

Sorry, I generated this patch from a wrong branch. Won't compile, will
send the correct series. Apologies for the churn.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option
  2016-09-13 15:05 ` [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option Keith Busch
@ 2016-09-23 14:34   ` Bjorn Helgaas
  2016-09-23 16:57     ` Keith Busch
  0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2016-09-23 14:34 UTC (permalink / raw)
  To: Keith Busch; +Cc: linux-pci, Bjorn Helgaas, Thomas Gleixner, LKML

On Tue, Sep 13, 2016 at 09:05:40AM -0600, Keith Busch wrote:
> 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
> 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>

Applied on pci/hotplug for v4.9, thanks!

I made the necessary changes to match the renaming I did in the first
patch, and I also used plain old "#ifdef" instead of "#if IS_ENABLED"
since the rest of the file uses the former style.  If there's a reason
to switch, we should change the whole file in a separate patch so we
can explain the rationale.

Please check it out and make sure everything you need made it in.

> ---
> 
> No change from previous version of this patch; just part of the series.
> 
>  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 b814ca6..a021b7b 100644
> --- a/arch/x86/pci/vmd.c
> +++ b/arch/x86/pci/vmd.c
> @@ -596,6 +596,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option
  2016-09-23 14:34   ` Bjorn Helgaas
@ 2016-09-23 16:57     ` Keith Busch
  2016-09-23 19:12       ` Bjorn Helgaas
  0 siblings, 1 reply; 8+ messages in thread
From: Keith Busch @ 2016-09-23 16:57 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, Bjorn Helgaas, Thomas Gleixner, LKML

On Fri, Sep 23, 2016 at 09:34:41AM -0500, Bjorn Helgaas wrote:
> I made the necessary changes to match the renaming I did in the first
> patch, and I also used plain old "#ifdef" instead of "#if IS_ENABLED"
> since the rest of the file uses the former style.  If there's a reason
> to switch, we should change the whole file in a separate patch so we
> can explain the rationale.

The check was "IS_ENABLED" because VMD can be a loadable module, which
fails the ifdef check. I see Fengguang 0'dayed it using the module
configuration option. I can send you a fix based on your pci/hotplug
branch, or revert and apply the original patch if you prefer.

BTW, you had asked me not to split a series when incremental fixes
touched a single patch. I didn't resend the whole series here, and while
you got the right patches, I apologize for making it more difficult to find.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option
  2016-09-23 16:57     ` Keith Busch
@ 2016-09-23 19:12       ` Bjorn Helgaas
  2016-09-23 22:14         ` Keith Busch
  0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2016-09-23 19:12 UTC (permalink / raw)
  To: Keith Busch; +Cc: linux-pci, Bjorn Helgaas, Thomas Gleixner, LKML

On Fri, Sep 23, 2016 at 12:57:02PM -0400, Keith Busch wrote:
> On Fri, Sep 23, 2016 at 09:34:41AM -0500, Bjorn Helgaas wrote:
> > I made the necessary changes to match the renaming I did in the first
> > patch, and I also used plain old "#ifdef" instead of "#if IS_ENABLED"
> > since the rest of the file uses the former style.  If there's a reason
> > to switch, we should change the whole file in a separate patch so we
> > can explain the rationale.
> 
> The check was "IS_ENABLED" because VMD can be a loadable module, which
> fails the ifdef check. I see Fengguang 0'dayed it using the module
> configuration option. I can send you a fix based on your pci/hotplug
> branch, or revert and apply the original patch if you prefer.

I didn't realize VMD could be a loadable module, and I didn't realize
that would make a difference for the config symbol.

BTW, the "Volume Management Device Driver" config item appears by
itself in the top-level menuconfig menu.  That seems a little ...
presumptuous; is it what you intended?

It took me a while, but I did eventually figure out why #ifdef doesn't
work -- we generate a different include/generated/autoconf.h symbol
for modules:

                 built-in                 loadable module
                 ---------------------    ---------------------------
  .config        CONFIG_VMD=y             CONFIG_VMD=m
  autoconf.h     #define CONFIG_VMD 1     #define CONFIG_VMD_MODULE 1

Anyway, I fixed it by using IS_ENABLED() again.

I might propose a comment update to help anybody else who stumbles
over this.  It was kind of annoying to puzzle this out.

> BTW, you had asked me not to split a series when incremental fixes
> touched a single patch. I didn't resend the whole series here, and while
> you got the right patches, I apologize for making it more difficult to find.

No problem, I was just paying more attention this time :)
Except for IS_ENABLED(), anyway.

Bjorn

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option
  2016-09-23 19:12       ` Bjorn Helgaas
@ 2016-09-23 22:14         ` Keith Busch
  0 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2016-09-23 22:14 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, Bjorn Helgaas, Thomas Gleixner, LKML

On Fri, Sep 23, 2016 at 02:12:23PM -0500, Bjorn Helgaas wrote:
> BTW, the "Volume Management Device Driver" config item appears by
> itself in the top-level menuconfig menu.  That seems a little ...
> presumptuous; is it what you intended?

Not really intended, but I didn't really know any better at the time. :).

I think drivers/pci/host/ is a more appropriate home for this driver. I'll
send a patch to relocate it, along with necessary Kconfig updates.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-09-23 22:03 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-13 15:05 [PATCHv3 1/2] pciehp: Let user control LED status Keith Busch
2016-09-13 15:05 ` [PATCHv3 2/2] x86/vmd: Add PCI domain specific LED option Keith Busch
2016-09-23 14:34   ` Bjorn Helgaas
2016-09-23 16:57     ` Keith Busch
2016-09-23 19:12       ` Bjorn Helgaas
2016-09-23 22:14         ` Keith Busch
2016-09-13 15:28 ` [PATCHv3 1/2] pciehp: Let user control LED status kbuild test robot
2016-09-13 16:36 ` Keith Busch

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.