linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
@ 2018-08-15 21:26 Jon Derrick
  2018-08-15 21:26 ` [PATCH 2/2] Documentation: Document pci=nodpc Jon Derrick
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Jon Derrick @ 2018-08-15 21:26 UTC (permalink / raw)
  To: linux-pci
  Cc: linux-kernel, linux-doc, Bjorn Helgaas, Keith Busch, Sinan Kaya,
	Oza Pawandeep, Dongdong Liu, Jon Derrick

Some users may want to disable downstream port containment (DPC), so
give them this option

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/pci.c      |  2 ++
 drivers/pci/pci.h      |  6 ++++++
 drivers/pci/pcie/dpc.c | 48 ++++++++++++++++++++++++++++++++++++++----------
 include/linux/pci.h    |  6 ++++++
 4 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 80da484..4e629c2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6062,6 +6062,8 @@ static int __init pci_setup(char *str)
 				pcie_ats_disabled = true;
 			} else if (!strcmp(str, "noaer")) {
 				pci_no_aer();
+			} else if (!strcmp(str, "nodpc")) {
+				pci_no_dpc();
 			} else if (!strcmp(str, "earlydump")) {
 				pci_early_dump = true;
 			} else if (!strncmp(str, "realloc=", 8)) {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6e0d152..f73f29e 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -536,4 +536,10 @@ static inline void pci_aer_clear_fatal_status(struct pci_dev *dev) { }
 static inline void pci_aer_clear_device_status(struct pci_dev *dev) { }
 #endif
 
+#ifdef CONFIG_PCIE_DPC
+void pci_no_dpc(void);
+#else
+static inline void pci_no_dpc(void) { }
+#endif
+
 #endif /* DRIVERS_PCI_H */
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index f03279f..068fca0 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -44,6 +44,18 @@ struct dpc_dev {
 	"Memory Request Completion Timeout",		 /* Bit Position 18 */
 };
 
+static int pcie_dpc_disable;
+
+void pci_no_dpc(void)
+{
+	pcie_dpc_disable = 1;
+}
+
+bool pci_dpc_available(void)
+{
+	return !pcie_dpc_disable && pci_aer_available() && pci_msi_enabled();
+}
+
 static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 {
 	unsigned long timeout = jiffies + HZ;
@@ -209,6 +221,17 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	return IRQ_HANDLED;
 }
 
+static void dpc_disable(struct pci_dev *pdev)
+{
+	u16 cap_pos, ctl;
+
+	cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
+	pci_read_config_word(pdev, cap_pos + PCI_EXP_DPC_CTL, &ctl);
+	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_EN_NONFATAL |
+		 PCI_EXP_DPC_CTL_INT_EN);
+	pci_write_config_word(pdev, cap_pos + PCI_EXP_DPC_CTL, ctl);
+}
+
 #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
 static int dpc_probe(struct pcie_device *dev)
 {
@@ -221,9 +244,16 @@ static int dpc_probe(struct pcie_device *dev)
 	if (pcie_aer_get_firmware_first(pdev))
 		return -ENOTSUPP;
 
+	if (!pci_dpc_available()) {
+		status = -ENOTSUPP;
+		goto disable_dpc;
+	}
+
 	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
-	if (!dpc)
-		return -ENOMEM;
+	if (!dpc) {
+		status = -ENOMEM;
+		goto disable_dpc;
+	}
 
 	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
 	dpc->dev = dev;
@@ -235,7 +265,7 @@ static int dpc_probe(struct pcie_device *dev)
 	if (status) {
 		dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
 			 status);
-		return status;
+		goto disable_dpc;
 	}
 
 	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
@@ -260,17 +290,15 @@ static int dpc_probe(struct pcie_device *dev)
 		FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), dpc->rp_log_size,
 		FLAG(cap, PCI_EXP_DPC_CAP_DL_ACTIVE));
 	return status;
+
+disable_dpc:
+	dpc_disable(pdev);
+	return status;
 }
 
 static void dpc_remove(struct pcie_device *dev)
 {
-	struct dpc_dev *dpc = get_service_data(dev);
-	struct pci_dev *pdev = dev->port;
-	u16 ctl;
-
-	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
-	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
-	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
+	dpc_disable(dev->port);
 }
 
 static struct pcie_port_service_driver dpcdriver = {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5454e6b..559b792 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1473,6 +1473,12 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d,
 static inline bool pci_aer_available(void) { return false; }
 #endif
 
+#ifdef CONFIG_PCIE_DPC
+bool pci_dpc_available(void);
+#else
+static inline bool pci_dpc_available(void) { return false; }
+#endif
+
 #ifdef CONFIG_PCIE_ECRC
 void pcie_set_ecrc_checking(struct pci_dev *dev);
 void pcie_ecrc_get_policy(char *str);
-- 
1.8.3.1


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

* [PATCH 2/2] Documentation: Document pci=nodpc
  2018-08-15 21:26 [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter Jon Derrick
@ 2018-08-15 21:26 ` Jon Derrick
  2018-08-15 23:03 ` [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter Keith Busch
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Jon Derrick @ 2018-08-15 21:26 UTC (permalink / raw)
  To: linux-pci
  Cc: linux-kernel, linux-doc, Bjorn Helgaas, Keith Busch, Sinan Kaya,
	Oza Pawandeep, Dongdong Liu, Jon Derrick

Document disabling downstream port containment through pci parameter

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index e372208..1a2fd78 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3041,6 +3041,9 @@
 		noaer		[PCIE] If the PCIEAER kernel config parameter is
 				enabled, this kernel boot option can be used to
 				disable the use of PCIE advanced error reporting.
+		nodpc		[PCIE] If the PCIE_DPC kernel config parameter is
+				enabled, this kernel boot option can be used to
+				disable the use of PCIE downstream port containment
 		nodomains	[PCI] Disable support for multiple PCI
 				root domains (aka PCI segments, in ACPI-speak).
 		nommconf	[X86] Disable use of MMCONFIG for PCI
-- 
1.8.3.1


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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-15 21:26 [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter Jon Derrick
  2018-08-15 21:26 ` [PATCH 2/2] Documentation: Document pci=nodpc Jon Derrick
@ 2018-08-15 23:03 ` Keith Busch
  2018-08-16  9:27 ` poza
  2018-08-16 15:49 ` Matthew Wilcox
  3 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2018-08-15 23:03 UTC (permalink / raw)
  To: Jon Derrick
  Cc: linux-pci, linux-kernel, linux-doc, Bjorn Helgaas, Sinan Kaya,
	Oza Pawandeep, Dongdong Liu

On Wed, Aug 15, 2018 at 03:26:39PM -0600, Jon Derrick wrote:
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -536,4 +536,10 @@ static inline void pci_aer_clear_fatal_status(struct pci_dev *dev) { }
>  static inline void pci_aer_clear_device_status(struct pci_dev *dev) { }
>  #endif
>  
> +#ifdef CONFIG_PCIE_DPC
> +void pci_no_dpc(void);
> +#else
> +static inline void pci_no_dpc(void) { }
> +#endif

<snip>

> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1473,6 +1473,12 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d,
>  static inline bool pci_aer_available(void) { return false; }
>  #endif
>  
> +#ifdef CONFIG_PCIE_DPC
> +bool pci_dpc_available(void);
> +#else
> +static inline bool pci_dpc_available(void) { return false; }
> +#endif

Seems like these two sections belong together. Otherwise, looks fine.

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-15 21:26 [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter Jon Derrick
  2018-08-15 21:26 ` [PATCH 2/2] Documentation: Document pci=nodpc Jon Derrick
  2018-08-15 23:03 ` [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter Keith Busch
@ 2018-08-16  9:27 ` poza
  2018-08-16 15:45   ` Derrick, Jonathan
  2018-08-16 15:49 ` Matthew Wilcox
  3 siblings, 1 reply; 13+ messages in thread
From: poza @ 2018-08-16  9:27 UTC (permalink / raw)
  To: Jon Derrick
  Cc: linux-pci, linux-kernel, linux-doc, Bjorn Helgaas, Keith Busch,
	Sinan Kaya, Dongdong Liu

On 2018-08-16 02:56, Jon Derrick wrote:
> Some users may want to disable downstream port containment (DPC), so
> give them this option
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/pci.c      |  2 ++
>  drivers/pci/pci.h      |  6 ++++++
>  drivers/pci/pcie/dpc.c | 48 
> ++++++++++++++++++++++++++++++++++++++----------
>  include/linux/pci.h    |  6 ++++++
>  4 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 80da484..4e629c2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6062,6 +6062,8 @@ static int __init pci_setup(char *str)
>  				pcie_ats_disabled = true;
>  			} else if (!strcmp(str, "noaer")) {
>  				pci_no_aer();
> +			} else if (!strcmp(str, "nodpc")) {
> +				pci_no_dpc();
>  			} else if (!strcmp(str, "earlydump")) {
>  				pci_early_dump = true;
>  			} else if (!strncmp(str, "realloc=", 8)) {
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6e0d152..f73f29e 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -536,4 +536,10 @@ static inline void
> pci_aer_clear_fatal_status(struct pci_dev *dev) { }
>  static inline void pci_aer_clear_device_status(struct pci_dev *dev) { 
> }
>  #endif
> 
> +#ifdef CONFIG_PCIE_DPC
> +void pci_no_dpc(void);
> +#else
> +static inline void pci_no_dpc(void) { }
> +#endif
> +
>  #endif /* DRIVERS_PCI_H */
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index f03279f..068fca0 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -44,6 +44,18 @@ struct dpc_dev {
>  	"Memory Request Completion Timeout",		 /* Bit Position 18 */
>  };
> 
> +static int pcie_dpc_disable;
> +
> +void pci_no_dpc(void)
> +{
> +	pcie_dpc_disable = 1;
> +}
> +
> +bool pci_dpc_available(void)
> +{
> +	return !pcie_dpc_disable && pci_aer_available() && pci_msi_enabled();

1) why do you check pci_aer_available() ?
2) and pci_aer_available() already internally checks pci_msi_enabled();

> +}
> +
>  static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
>  {
>  	unsigned long timeout = jiffies + HZ;
> @@ -209,6 +221,17 @@ static irqreturn_t dpc_irq(int irq, void *context)
>  	return IRQ_HANDLED;
>  }
> 
> +static void dpc_disable(struct pci_dev *pdev)
> +{
> +	u16 cap_pos, ctl;
> +
> +	cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
> +	pci_read_config_word(pdev, cap_pos + PCI_EXP_DPC_CTL, &ctl);
> +	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_EN_NONFATAL |
> +		 PCI_EXP_DPC_CTL_INT_EN);
> +	pci_write_config_word(pdev, cap_pos + PCI_EXP_DPC_CTL, ctl);
> +}
> +
>  #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
>  static int dpc_probe(struct pcie_device *dev)
>  {
> @@ -221,9 +244,16 @@ static int dpc_probe(struct pcie_device *dev)
>  	if (pcie_aer_get_firmware_first(pdev))
>  		return -ENOTSUPP;
> 
> +	if (!pci_dpc_available()) {
> +		status = -ENOTSUPP;
> +		goto disable_dpc;
> +	}
> +
>  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
> -	if (!dpc)
> -		return -ENOMEM;
> +	if (!dpc) {
> +		status = -ENOMEM;
> +		goto disable_dpc;
> +	}
> 
>  	dpc->cap_pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DPC);
>  	dpc->dev = dev;
> @@ -235,7 +265,7 @@ static int dpc_probe(struct pcie_device *dev)
>  	if (status) {
>  		dev_warn(device, "request IRQ%d failed: %d\n", dev->irq,
>  			 status);
> -		return status;
> +		goto disable_dpc;
>  	}
> 
>  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP, &cap);
> @@ -260,17 +290,15 @@ static int dpc_probe(struct pcie_device *dev)
>  		FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), dpc->rp_log_size,
>  		FLAG(cap, PCI_EXP_DPC_CAP_DL_ACTIVE));
>  	return status;
> +
> +disable_dpc:
> +	dpc_disable(pdev);
> +	return status;
>  }
> 
>  static void dpc_remove(struct pcie_device *dev)
>  {
> -	struct dpc_dev *dpc = get_service_data(dev);
> -	struct pci_dev *pdev = dev->port;
> -	u16 ctl;
> -
> -	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, &ctl);
> -	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN);
> -	pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL, ctl);
> +	dpc_disable(dev->port);
>  }
> 
>  static struct pcie_port_service_driver dpcdriver = {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 5454e6b..559b792 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1473,6 +1473,12 @@ static inline int pci_irqd_intx_xlate(struct
> irq_domain *d,
>  static inline bool pci_aer_available(void) { return false; }
>  #endif
> 
> +#ifdef CONFIG_PCIE_DPC
> +bool pci_dpc_available(void);
> +#else
> +static inline bool pci_dpc_available(void) { return false; }
> +#endif
> +
>  #ifdef CONFIG_PCIE_ECRC
>  void pcie_set_ecrc_checking(struct pci_dev *dev);
>  void pcie_ecrc_get_policy(char *str);

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-16  9:27 ` poza
@ 2018-08-16 15:45   ` Derrick, Jonathan
  0 siblings, 0 replies; 13+ messages in thread
From: Derrick, Jonathan @ 2018-08-16 15:45 UTC (permalink / raw)
  To: poza
  Cc: liudongdong3, linux-kernel, linux-pci, Busch, Keith, okaya,
	linux-doc, helgaas

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

On Thu, 2018-08-16 at 14:57 +0530, poza@codeaurora.org wrote:
> On 2018-08-16 02:56, Jon Derrick wrote:
> > Some users may want to disable downstream port containment (DPC),
> > so
> > give them this option
> > 
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> >  drivers/pci/pci.c      |  2 ++
> >  drivers/pci/pci.h      |  6 ++++++
> >  drivers/pci/pcie/dpc.c | 48 
> > ++++++++++++++++++++++++++++++++++++++----------
> >  include/linux/pci.h    |  6 ++++++
> >  4 files changed, 52 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 80da484..4e629c2 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6062,6 +6062,8 @@ static int __init pci_setup(char *str)
> >  				pcie_ats_disabled = true;
> >  			} else if (!strcmp(str, "noaer")) {
> >  				pci_no_aer();
> > +			} else if (!strcmp(str, "nodpc")) {
> > +				pci_no_dpc();
> >  			} else if (!strcmp(str, "earlydump")) {
> >  				pci_early_dump = true;
> >  			} else if (!strncmp(str, "realloc=", 8)) {
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 6e0d152..f73f29e 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -536,4 +536,10 @@ static inline void
> > pci_aer_clear_fatal_status(struct pci_dev *dev) { }
> >  static inline void pci_aer_clear_device_status(struct pci_dev
> > *dev) { 
> > }
> >  #endif
> > 
> > +#ifdef CONFIG_PCIE_DPC
> > +void pci_no_dpc(void);
> > +#else
> > +static inline void pci_no_dpc(void) { }
> > +#endif
> > +
> >  #endif /* DRIVERS_PCI_H */
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index f03279f..068fca0 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -44,6 +44,18 @@ struct dpc_dev {
> >  	"Memory Request Completion Timeout",		 /*
> > Bit Position 18 */
> >  };
> > 
> > +static int pcie_dpc_disable;
> > +
> > +void pci_no_dpc(void)
> > +{
> > +	pcie_dpc_disable = 1;
> > +}
> > +
> > +bool pci_dpc_available(void)
> > +{
> > +	return !pcie_dpc_disable && pci_aer_available() &&
> > pci_msi_enabled();
> 
> 1) why do you check pci_aer_available() ?
I'd assumed a dependency on aer, but looking at it again I see it calls
into err.c without a requirement on aer.

Then it also seems the pci_aer_available() and AER dependency check in
portdrv_core.c is unneccessary too.

> 2) and pci_aer_available() already internally checks
> pci_msi_enabled();
> 
> > +}
> > +
> >  static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
> >  {
> >  	unsigned long timeout = jiffies + HZ;
> > @@ -209,6 +221,17 @@ static irqreturn_t dpc_irq(int irq, void
> > *context)
> >  	return IRQ_HANDLED;
> >  }
> > 
> > +static void dpc_disable(struct pci_dev *pdev)
> > +{
> > +	u16 cap_pos, ctl;
> > +
> > +	cap_pos = pci_find_ext_capability(pdev,
> > PCI_EXT_CAP_ID_DPC);
> > +	pci_read_config_word(pdev, cap_pos + PCI_EXP_DPC_CTL,
> > &ctl);
> > +	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL |
> > PCI_EXP_DPC_CTL_EN_NONFATAL |
> > +		 PCI_EXP_DPC_CTL_INT_EN);
> > +	pci_write_config_word(pdev, cap_pos + PCI_EXP_DPC_CTL,
> > ctl);
> > +}
> > +
> >  #define FLAG(x, y) (((x) & (y)) ? '+' : '-')
> >  static int dpc_probe(struct pcie_device *dev)
> >  {
> > @@ -221,9 +244,16 @@ static int dpc_probe(struct pcie_device *dev)
> >  	if (pcie_aer_get_firmware_first(pdev))
> >  		return -ENOTSUPP;
> > 
> > +	if (!pci_dpc_available()) {
> > +		status = -ENOTSUPP;
> > +		goto disable_dpc;
> > +	}
> > +
> >  	dpc = devm_kzalloc(device, sizeof(*dpc), GFP_KERNEL);
> > -	if (!dpc)
> > -		return -ENOMEM;
> > +	if (!dpc) {
> > +		status = -ENOMEM;
> > +		goto disable_dpc;
> > +	}
> > 
> >  	dpc->cap_pos = pci_find_ext_capability(pdev,
> > PCI_EXT_CAP_ID_DPC);
> >  	dpc->dev = dev;
> > @@ -235,7 +265,7 @@ static int dpc_probe(struct pcie_device *dev)
> >  	if (status) {
> >  		dev_warn(device, "request IRQ%d failed: %d\n",
> > dev->irq,
> >  			 status);
> > -		return status;
> > +		goto disable_dpc;
> >  	}
> > 
> >  	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CAP,
> > &cap);
> > @@ -260,17 +290,15 @@ static int dpc_probe(struct pcie_device *dev)
> >  		FLAG(cap, PCI_EXP_DPC_CAP_SW_TRIGGER), dpc-
> > >rp_log_size,
> >  		FLAG(cap, PCI_EXP_DPC_CAP_DL_ACTIVE));
> >  	return status;
> > +
> > +disable_dpc:
> > +	dpc_disable(pdev);
> > +	return status;
> >  }
> > 
> >  static void dpc_remove(struct pcie_device *dev)
> >  {
> > -	struct dpc_dev *dpc = get_service_data(dev);
> > -	struct pci_dev *pdev = dev->port;
> > -	u16 ctl;
> > -
> > -	pci_read_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_CTL,
> > &ctl);
> > -	ctl &= ~(PCI_EXP_DPC_CTL_EN_FATAL |
> > PCI_EXP_DPC_CTL_INT_EN);
> > -	pci_write_config_word(pdev, dpc->cap_pos +
> > PCI_EXP_DPC_CTL, ctl);
> > +	dpc_disable(dev->port);
> >  }
> > 
> >  static struct pcie_port_service_driver dpcdriver = {
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 5454e6b..559b792 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1473,6 +1473,12 @@ static inline int pci_irqd_intx_xlate(struct
> > irq_domain *d,
> >  static inline bool pci_aer_available(void) { return false; }
> >  #endif
> > 
> > +#ifdef CONFIG_PCIE_DPC
> > +bool pci_dpc_available(void);
> > +#else
> > +static inline bool pci_dpc_available(void) { return false; }
> > +#endif
> > +
> >  #ifdef CONFIG_PCIE_ECRC
> >  void pcie_set_ecrc_checking(struct pci_dev *dev);
> >  void pcie_ecrc_get_policy(char *str);

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-15 21:26 [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter Jon Derrick
                   ` (2 preceding siblings ...)
  2018-08-16  9:27 ` poza
@ 2018-08-16 15:49 ` Matthew Wilcox
  2018-08-16 15:50   ` Derrick, Jonathan
  3 siblings, 1 reply; 13+ messages in thread
From: Matthew Wilcox @ 2018-08-16 15:49 UTC (permalink / raw)
  To: Jon Derrick
  Cc: linux-pci, linux-kernel, linux-doc, Bjorn Helgaas, Keith Busch,
	Sinan Kaya, Oza Pawandeep, Dongdong Liu

On Wed, Aug 15, 2018 at 03:26:39PM -0600, Jon Derrick wrote:
> Some users may want to disable downstream port containment (DPC), so
> give them this option

Is it possible they might only want to disable DPC on a subset of the
hierarchy rather than globally?

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-16 15:49 ` Matthew Wilcox
@ 2018-08-16 15:50   ` Derrick, Jonathan
  2018-08-16 20:31     ` Bjorn Helgaas
  0 siblings, 1 reply; 13+ messages in thread
From: Derrick, Jonathan @ 2018-08-16 15:50 UTC (permalink / raw)
  To: willy
  Cc: linux-kernel, okaya, liudongdong3, helgaas, poza, linux-pci,
	Busch, Keith, linux-doc

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

On Thu, 2018-08-16 at 08:49 -0700, Matthew Wilcox wrote:
> On Wed, Aug 15, 2018 at 03:26:39PM -0600, Jon Derrick wrote:
> > Some users may want to disable downstream port containment (DPC),
> > so
> > give them this option
> 
> Is it possible they might only want to disable DPC on a subset of the
> hierarchy rather than globally?

Absolutely. I was hoping Logan's pci dev_str would land because I have
a few others I want to convert to that api for granular tuning

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-16 15:50   ` Derrick, Jonathan
@ 2018-08-16 20:31     ` Bjorn Helgaas
  2018-08-16 20:50       ` Derrick, Jonathan
  0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2018-08-16 20:31 UTC (permalink / raw)
  To: Derrick, Jonathan
  Cc: willy, linux-kernel, okaya, liudongdong3, poza, linux-pci, Busch,
	Keith, linux-doc

On Thu, Aug 16, 2018 at 03:50:47PM +0000, Derrick, Jonathan wrote:
> On Thu, 2018-08-16 at 08:49 -0700, Matthew Wilcox wrote:
> > On Wed, Aug 15, 2018 at 03:26:39PM -0600, Jon Derrick wrote:
> > > Some users may want to disable downstream port containment (DPC),
> > > so
> > > give them this option
> > 
> > Is it possible they might only want to disable DPC on a subset of the
> > hierarchy rather than globally?
> 
> Absolutely. I was hoping Logan's pci dev_str would land because I have
> a few others I want to convert to that api for granular tuning

What's the use case here?  I acknowledge there are cases where we need
them, but I'm not a fan of kernel parameters in general because
they're a real hassle for users.

Is there something wrong with DPC?  Is there some way we can make it
smarter so it does the right thing automatically?

I'm more OK with a blanket "nodpc" switch intended for debugging.
If we add the complexity of subsets of the hierarchy it starts
sounding like an administrative thing that makes me more hesitant. 

Could this be done via a sysfs switch instead?  That potentially could
work for hot-added things where a kernel parameter doesn't work so
well.

Please squash the doc patch and the code change so it's easier to keep
them together.

Bjorn

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-16 20:31     ` Bjorn Helgaas
@ 2018-08-16 20:50       ` Derrick, Jonathan
  2018-08-16 21:19         ` Keith Busch
  2018-08-17 14:25         ` Bjorn Helgaas
  0 siblings, 2 replies; 13+ messages in thread
From: Derrick, Jonathan @ 2018-08-16 20:50 UTC (permalink / raw)
  To: helgaas
  Cc: linux-kernel, okaya, willy, liudongdong3, poza, linux-pci, Busch,
	Keith, linux-doc

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

Hi Bjorn,

On Thu, 2018-08-16 at 15:31 -0500, Bjorn Helgaas wrote:
> On Thu, Aug 16, 2018 at 03:50:47PM +0000, Derrick, Jonathan wrote:
> > On Thu, 2018-08-16 at 08:49 -0700, Matthew Wilcox wrote:
> > > On Wed, Aug 15, 2018 at 03:26:39PM -0600, Jon Derrick wrote:
> > > > Some users may want to disable downstream port containment
> > > > (DPC),
> > > > so
> > > > give them this option
> > > 
> > > Is it possible they might only want to disable DPC on a subset of
> > > the
> > > hierarchy rather than globally?
> > 
> > Absolutely. I was hoping Logan's pci dev_str would land because I
> > have
> > a few others I want to convert to that api for granular tuning
> 
> What's the use case here?  I acknowledge there are cases where we
> need
> them, but I'm not a fan of kernel parameters in general because
> they're a real hassle for users.
> 
> Is there something wrong with DPC?  Is there some way we can make it
> smarter so it does the right thing automatically?
I've encountered some BIOS or switches (not sure who) who've appeared
to have enabled DPC by default, prior to kernel setup. Some users may
just not want this, but it was primarily intended for debugging when I
conceived it.

There's also the ongoing thread in linux-pci about err handling in PPC
EEH, who may also desire to disable DPC until those issues are
resolved.


It can also be disabled with setpci, but is that any less of a hassle?
Genuine question to understand your point of view.


> 
> I'm more OK with a blanket "nodpc" switch intended for debugging.
> If we add the complexity of subsets of the hierarchy it starts
> sounding like an administrative thing that makes me more hesitant. 
> 
That's ok with me. I was having trouble cleanly keeping the string
around after early_param anyways.


To again understand your point of view, is there anything wrong with
administrative things in kernel boot parameters? There will be cases
where we may want to deviate from default or global pci=* parameters
for certain hierarchies and they can't necessarily be set after the
fact (ex, hpmemsize).


> Could this be done via a sysfs switch instead?  That potentially
> could
> work for hot-added things where a kernel parameter doesn't work so
> well.
> 
> Please squash the doc patch and the code change so it's easier to
> keep
> them together.
> 
> Bjorn

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-16 20:50       ` Derrick, Jonathan
@ 2018-08-16 21:19         ` Keith Busch
  2018-08-16 21:28           ` Derrick, Jonathan
  2018-08-17 14:25         ` Bjorn Helgaas
  1 sibling, 1 reply; 13+ messages in thread
From: Keith Busch @ 2018-08-16 21:19 UTC (permalink / raw)
  To: Derrick, Jonathan
  Cc: helgaas, linux-kernel, okaya, willy, liudongdong3, poza,
	linux-pci, linux-doc

On Thu, Aug 16, 2018 at 01:50:42PM -0700, Derrick, Jonathan wrote:
> It can also be disabled with setpci, but is that any less of a hassle?
> Genuine question to understand your point of view.

That is not a real solution, IMO. 'setpci' is good to inject things
for testing, but it changes config space without the kernel aware that
you've done that, so it is inherently racey with other kernel threads
touching pci config space. And the kernel or platform may end up undoing
what you had 'setpci' do anyway with no immediate way to be notified it
was changed.

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-16 21:19         ` Keith Busch
@ 2018-08-16 21:28           ` Derrick, Jonathan
  0 siblings, 0 replies; 13+ messages in thread
From: Derrick, Jonathan @ 2018-08-16 21:28 UTC (permalink / raw)
  To: Busch, Keith
  Cc: linux-kernel, okaya, willy, liudongdong3, helgaas, poza,
	linux-pci, linux-doc

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

On Thu, 2018-08-16 at 15:19 -0600, Keith Busch wrote:
> On Thu, Aug 16, 2018 at 01:50:42PM -0700, Derrick, Jonathan wrote:
> > It can also be disabled with setpci, but is that any less of a
> > hassle?
> > Genuine question to understand your point of view.
> 
> That is not a real solution, IMO. 'setpci' is good to inject things
> for testing, but it changes config space without the kernel aware
> that
> you've done that, so it is inherently racey with other kernel threads
> touching pci config space. And the kernel or platform may end up
> undoing
> what you had 'setpci' do anyway with no immediate way to be notified
> it
> was changed.

It sounds like a sysfs toggle could be useful regardless of the boot
parameter.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-16 20:50       ` Derrick, Jonathan
  2018-08-16 21:19         ` Keith Busch
@ 2018-08-17 14:25         ` Bjorn Helgaas
  2018-08-17 14:45           ` Derrick, Jonathan
  1 sibling, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2018-08-17 14:25 UTC (permalink / raw)
  To: Derrick, Jonathan
  Cc: linux-kernel, okaya, willy, liudongdong3, poza, linux-pci, Busch,
	Keith, linux-doc

On Thu, Aug 16, 2018 at 08:50:42PM +0000, Derrick, Jonathan wrote:
> On Thu, 2018-08-16 at 15:31 -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 16, 2018 at 03:50:47PM +0000, Derrick, Jonathan wrote:
> > > On Thu, 2018-08-16 at 08:49 -0700, Matthew Wilcox wrote:
> > > > On Wed, Aug 15, 2018 at 03:26:39PM -0600, Jon Derrick wrote:
> > > > > Some users may want to disable downstream port containment
> > > > > (DPC),
> > > > > so
> > > > > give them this option
> > > > 
> > > > Is it possible they might only want to disable DPC on a subset of
> > > > the
> > > > hierarchy rather than globally?
> > > 
> > > Absolutely. I was hoping Logan's pci dev_str would land because I
> > > have
> > > a few others I want to convert to that api for granular tuning
> > 
> > What's the use case here?  I acknowledge there are cases where we
> > need
> > them, but I'm not a fan of kernel parameters in general because
> > they're a real hassle for users.
> > 
> > Is there something wrong with DPC?  Is there some way we can make it
> > smarter so it does the right thing automatically?
> I've encountered some BIOS or switches (not sure who) who've appeared
> to have enabled DPC by default, prior to kernel setup. Some users may
> just not want this, but it was primarily intended for debugging when I
> conceived it.
> 
> There's also the ongoing thread in linux-pci about err handling in PPC
> EEH, who may also desire to disable DPC until those issues are
> resolved.

I haven't caught up on that thread yet.  If DPC is incompatible with
PPC EEH, there's always the possibility of a switch in the code to
disable DPC automatically on PPC.

> It can also be disabled with setpci, but is that any less of a hassle?
> Genuine question to understand your point of view.

Keith already answered here; setpci is primarily for debugging and
shouldn't be recommended as normal practice.

> > I'm more OK with a blanket "nodpc" switch intended for debugging.
> > If we add the complexity of subsets of the hierarchy it starts
> > sounding like an administrative thing that makes me more hesitant. 
> ...
> To again understand your point of view, is there anything wrong with
> administrative things in kernel boot parameters? There will be cases
> where we may want to deviate from default or global pci=* parameters
> for certain hierarchies and they can't necessarily be set after the
> fact (ex, hpmemsize).

"There will be cases" sounds like we're doing something that *might*
be useful in the future.  It's better if we wait until we actually
discover a need for something.

There's also a tendency among users to trip over a problem, discover a
boot parameter like "pci=nomsi", and conclude that the problem is
"fixed".  In fact, we want the bug report so we can fix the kernel so
no boot parameter is needed.

I agree there are things that can't be set after boot.  Is this one of
them?  This seems like something that could be controlled at run-time.
But even there, I will ask what the requirement for it is, because we
don't want to clutter sysfs with things only needed for debugging.

Boot parameters are a hassle because it's hard to build a user
interface on top of them, and they require a reboot to take effect.

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

* Re: [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter
  2018-08-17 14:25         ` Bjorn Helgaas
@ 2018-08-17 14:45           ` Derrick, Jonathan
  0 siblings, 0 replies; 13+ messages in thread
From: Derrick, Jonathan @ 2018-08-17 14:45 UTC (permalink / raw)
  To: helgaas
  Cc: linux-kernel, okaya, willy, liudongdong3, poza, linux-pci, Busch,
	Keith, linux-doc

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

Hi Bjorn,

Thanks for your pov.

I'm going a lot of different ways on this one, but it seems like the
most reasonable option right now is to drop it until someone actually
presents a non-debugging need to disable DPC from sysfs or globally.

Best regards,
Jon

On Fri, 2018-08-17 at 09:25 -0500, Bjorn Helgaas wrote:
> On Thu, Aug 16, 2018 at 08:50:42PM +0000, Derrick, Jonathan wrote:
> > On Thu, 2018-08-16 at 15:31 -0500, Bjorn Helgaas wrote:
> > > On Thu, Aug 16, 2018 at 03:50:47PM +0000, Derrick, Jonathan
> > > wrote:
> > > > On Thu, 2018-08-16 at 08:49 -0700, Matthew Wilcox wrote:
> > > > > On Wed, Aug 15, 2018 at 03:26:39PM -0600, Jon Derrick wrote:
> > > > > > Some users may want to disable downstream port containment
> > > > > > (DPC),
> > > > > > so
> > > > > > give them this option
> > > > > 
> > > > > Is it possible they might only want to disable DPC on a
> > > > > subset of
> > > > > the
> > > > > hierarchy rather than globally?
> > > > 
> > > > Absolutely. I was hoping Logan's pci dev_str would land because
> > > > I
> > > > have
> > > > a few others I want to convert to that api for granular tuning
> > > 
> > > What's the use case here?  I acknowledge there are cases where we
> > > need
> > > them, but I'm not a fan of kernel parameters in general because
> > > they're a real hassle for users.
> > > 
> > > Is there something wrong with DPC?  Is there some way we can make
> > > it
> > > smarter so it does the right thing automatically?
> > 
> > I've encountered some BIOS or switches (not sure who) who've
> > appeared
> > to have enabled DPC by default, prior to kernel setup. Some users
> > may
> > just not want this, but it was primarily intended for debugging
> > when I
> > conceived it.
> > 
> > There's also the ongoing thread in linux-pci about err handling in
> > PPC
> > EEH, who may also desire to disable DPC until those issues are
> > resolved.
> 
> I haven't caught up on that thread yet.  If DPC is incompatible with
> PPC EEH, there's always the possibility of a switch in the code to
> disable DPC automatically on PPC.
> 
> > It can also be disabled with setpci, but is that any less of a
> > hassle?
> > Genuine question to understand your point of view.
> 
> Keith already answered here; setpci is primarily for debugging and
> shouldn't be recommended as normal practice.
> 
> > > I'm more OK with a blanket "nodpc" switch intended for debugging.
> > > If we add the complexity of subsets of the hierarchy it starts
> > > sounding like an administrative thing that makes me more
> > > hesitant. 
> > 
> > ...
> > To again understand your point of view, is there anything wrong
> > with
> > administrative things in kernel boot parameters? There will be
> > cases
> > where we may want to deviate from default or global pci=*
> > parameters
> > for certain hierarchies and they can't necessarily be set after the
> > fact (ex, hpmemsize).
> 
> "There will be cases" sounds like we're doing something that *might*
> be useful in the future.  It's better if we wait until we actually
> discover a need for something.
> 
> There's also a tendency among users to trip over a problem, discover
> a
> boot parameter like "pci=nomsi", and conclude that the problem is
> "fixed".  In fact, we want the bug report so we can fix the kernel so
> no boot parameter is needed.
> 
> I agree there are things that can't be set after boot.  Is this one
> of
> them?  This seems like something that could be controlled at run-
> time.
> But even there, I will ask what the requirement for it is, because we
> don't want to clutter sysfs with things only needed for debugging.
> 
> Boot parameters are a hassle because it's hard to build a user
> interface on top of them, and they require a reboot to take effect.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

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

end of thread, other threads:[~2018-08-17 14:45 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-15 21:26 [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter Jon Derrick
2018-08-15 21:26 ` [PATCH 2/2] Documentation: Document pci=nodpc Jon Derrick
2018-08-15 23:03 ` [PATCH 1/2] PCI/DPC: Add 'nodpc' parameter Keith Busch
2018-08-16  9:27 ` poza
2018-08-16 15:45   ` Derrick, Jonathan
2018-08-16 15:49 ` Matthew Wilcox
2018-08-16 15:50   ` Derrick, Jonathan
2018-08-16 20:31     ` Bjorn Helgaas
2018-08-16 20:50       ` Derrick, Jonathan
2018-08-16 21:19         ` Keith Busch
2018-08-16 21:28           ` Derrick, Jonathan
2018-08-17 14:25         ` Bjorn Helgaas
2018-08-17 14:45           ` Derrick, Jonathan

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).