[PATCH] PCI/DPC: Add Software Trigger as reset method

[PATCH] PCI/DPC: Add Software Trigger as reset method

[Lukas Wunner]

Add DPC Software Trigger as a reset method to be used for silicon
validation among other things:

  # echo dpc_sw_trigger > reset_method
  # echo 1 > reset

After validating DPC, the default reset_method(s) may be reinstated:

  # echo default > reset_method

Writing the DPC Control Register requires that control was granted by
firmware, so expose the reset_method only if DPC is native.  (And AER,
which must always be granted or denied in unison per PCI Firmware Spec
r3.3 table 4-5.)

The reset attribute in sysfs is meant to reset a single PCI Function,
but DPC resets the entire hierarchy below the parent.  So only expose
the reset method on PCI Functions without siblings or children.
Checking for that may happen both *before* the PCI Function has been
added to the bus list (via pci_device_add() -> pci_init_capabilities())
and *after* (via reset_method_store()), hence differentiate between
those two cases on reset probing.

It would be useful for silicon validation to have a separate sysfs
attribute for PCI bridges to reset their downstream hierarchy.  Prepare
for introduction of such an attribute by adding separate functions
pci_dpc_sw_trigger() (to reset the hierarchy below a bridge) and
pci_dpc_sw_trigger_parent() (to reset a single PCI Function), where the
latter calls the former to trigger DPC on the parent bridge.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci.c             |  1 +
 drivers/pci/pci.h             |  2 ++
 drivers/pci/pcie/dpc.c        | 57 +++++++++++++++++++++++++++++++++++
 include/linux/pci.h           |  2 +-
 include/uapi/linux/pci_regs.h |  1 +
 5 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index fba95486caaf..f561f84a8bca 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5225,6 +5225,7 @@ static const struct pci_reset_fn_method pci_reset_fn_methods[] = {
 	{ pci_af_flr, .name = "af_flr" },
 	{ pci_pm_reset, .name = "pm" },
 	{ pci_reset_bus_function, .name = "bus" },
+	{ pci_dpc_sw_trigger_parent, .name = "dpc_sw_trigger" },
 };
 
 static ssize_t reset_method_show(struct device *dev,
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9ed3b5550043..da2a3af4c46c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -425,11 +425,13 @@ void pci_dpc_init(struct pci_dev *pdev);
 void dpc_process_error(struct pci_dev *pdev);
 pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 bool pci_dpc_recovered(struct pci_dev *pdev);
+int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe);
 #else
 static inline void pci_save_dpc_state(struct pci_dev *dev) {}
 static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
 static inline void pci_dpc_init(struct pci_dev *pdev) {}
 static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; }
+static inline int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe) { return -ENOTTY; }
 #endif
 
 #ifdef CONFIG_PCIEPORTBUS
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index f5ffea17c7f8..47fd69d0a9c2 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -322,6 +322,63 @@ static irqreturn_t dpc_irq(int irq, void *context)
 	return IRQ_HANDLED;
 }
 
+static int pci_dpc_sw_trigger(struct pci_dev *pdev, bool probe)
+{
+	struct pci_host_bridge *host;
+	u16 cap, ctl;
+
+	if (probe) {
+		if (!pdev->dpc_cap)
+			return -ENOTTY;
+
+		host = pci_find_host_bridge(pdev->bus);
+		if (!host->native_dpc && !pcie_ports_dpc_native)
+			return -ENOTTY;
+
+		if (!pcie_aer_is_native(pdev))
+			return -ENOTTY;
+
+		pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP,
+				     &cap);
+		if (!(cap & PCI_EXP_DPC_CAP_SW_TRIGGER))
+			return -ENOTTY;
+
+		return 0;
+	}
+
+	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
+	ctl |= PCI_EXP_DPC_CTL_SW_TRIGGER;
+	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
+	return 0;
+}
+
+int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe)
+{
+	if (probe) {
+		/*
+		 * Reset must only affect @pdev, so bail out if it has siblings
+		 * or descendants.  Need to differentiate whether @pdev has
+		 * already been added to the bus list or not:
+		 */
+		if (list_empty(&pdev->bus_list) &&
+		    !list_empty(&pdev->bus->devices))
+			return -ENOTTY;
+
+		if (!list_empty(&pdev->bus_list) &&
+		    !list_is_singular(&pdev->bus->devices))
+			return -ENOTTY;
+
+		if (pdev->subordinate &&
+		    !list_empty(&pdev->subordinate->devices))
+			return -ENOTTY;
+
+		if (!pdev->bus->self)
+			return -ENOTTY;
+	}
+
+	return pci_dpc_sw_trigger(pdev->bus->self, probe);
+}
+
 void pci_dpc_init(struct pci_dev *pdev)
 {
 	u16 cap;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 28af4414f789..7890cd4eb97d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -50,7 +50,7 @@
 			       PCI_STATUS_PARITY)
 
 /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
-#define PCI_NUM_RESET_METHODS 7
+#define PCI_NUM_RESET_METHODS 8
 
 #define PCI_RESET_PROBE		true
 #define PCI_RESET_DO_RESET	false
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 1c3591c8e09e..73b3ccdffb3a 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1035,6 +1035,7 @@
 #define PCI_EXP_DPC_CTL			0x06	/* DPC control */
 #define  PCI_EXP_DPC_CTL_EN_FATAL	0x0001	/* Enable trigger on ERR_FATAL message */
 #define  PCI_EXP_DPC_CTL_EN_NONFATAL	0x0002	/* Enable trigger on ERR_NONFATAL message */
+#define  PCI_EXP_DPC_CTL_SW_TRIGGER	0x0040	/* Software Trigger */
 #define  PCI_EXP_DPC_CTL_INT_EN		0x0008	/* DPC Interrupt Enable */
 
 #define PCI_EXP_DPC_STATUS		0x08	/* DPC Status */
-- 
2.36.1

Re: [PATCH] PCI/DPC: Add Software Trigger as reset method

[Ashok Raj]

On Tue, Nov 29, 2022 at 08:35:55AM +0100, Lukas Wunner wrote:
> Add DPC Software Trigger as a reset method to be used for silicon
> validation among other things:
> 
>   # echo dpc_sw_trigger > reset_method
>   # echo 1 > reset
> 
> After validating DPC, the default reset_method(s) may be reinstated:
> 
>   # echo default > reset_method
> 
> Writing the DPC Control Register requires that control was granted by
> firmware, so expose the reset_method only if DPC is native.  (And AER,
> which must always be granted or denied in unison per PCI Firmware Spec
> r3.3 table 4-5.)
> 
> The reset attribute in sysfs is meant to reset a single PCI Function,
> but DPC resets the entire hierarchy below the parent.  So only expose
> the reset method on PCI Functions without siblings or children.
> Checking for that may happen both *before* the PCI Function has been
> added to the bus list (via pci_device_add() -> pci_init_capabilities())
> and *after* (via reset_method_store()), hence differentiate between
> those two cases on reset probing.
> 
> It would be useful for silicon validation to have a separate sysfs
> attribute for PCI bridges to reset their downstream hierarchy.  Prepare
> for introduction of such an attribute by adding separate functions
> pci_dpc_sw_trigger() (to reset the hierarchy below a bridge) and
> pci_dpc_sw_trigger_parent() (to reset a single PCI Function), where the
> latter calls the former to trigger DPC on the parent bridge.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Minor nit: Do you think debugfs might be a better place to stash some of
these non-production use cases?

Reviewed-by: Ashok Raj <ashok.raj@intel.com>
> ---
>  drivers/pci/pci.c             |  1 +
>  drivers/pci/pci.h             |  2 ++
>  drivers/pci/pcie/dpc.c        | 57 +++++++++++++++++++++++++++++++++++
>  include/linux/pci.h           |  2 +-
>  include/uapi/linux/pci_regs.h |  1 +
>  5 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index fba95486caaf..f561f84a8bca 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5225,6 +5225,7 @@ static const struct pci_reset_fn_method pci_reset_fn_methods[] = {
>  	{ pci_af_flr, .name = "af_flr" },
>  	{ pci_pm_reset, .name = "pm" },
>  	{ pci_reset_bus_function, .name = "bus" },
> +	{ pci_dpc_sw_trigger_parent, .name = "dpc_sw_trigger" },
>  };
>  
>  static ssize_t reset_method_show(struct device *dev,
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9ed3b5550043..da2a3af4c46c 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -425,11 +425,13 @@ void pci_dpc_init(struct pci_dev *pdev);
>  void dpc_process_error(struct pci_dev *pdev);
>  pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
>  bool pci_dpc_recovered(struct pci_dev *pdev);
> +int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe);
>  #else
>  static inline void pci_save_dpc_state(struct pci_dev *dev) {}
>  static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
>  static inline void pci_dpc_init(struct pci_dev *pdev) {}
>  static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; }
> +static inline int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe) { return -ENOTTY; }
>  #endif
>  
>  #ifdef CONFIG_PCIEPORTBUS
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index f5ffea17c7f8..47fd69d0a9c2 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -322,6 +322,63 @@ static irqreturn_t dpc_irq(int irq, void *context)
>  	return IRQ_HANDLED;
>  }
>  
> +static int pci_dpc_sw_trigger(struct pci_dev *pdev, bool probe)
> +{
> +	struct pci_host_bridge *host;
> +	u16 cap, ctl;
> +
> +	if (probe) {
> +		if (!pdev->dpc_cap)
> +			return -ENOTTY;
> +
> +		host = pci_find_host_bridge(pdev->bus);
> +		if (!host->native_dpc && !pcie_ports_dpc_native)
> +			return -ENOTTY;
> +
> +		if (!pcie_aer_is_native(pdev))
> +			return -ENOTTY;
> +
> +		pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP,
> +				     &cap);
> +		if (!(cap & PCI_EXP_DPC_CAP_SW_TRIGGER))
> +			return -ENOTTY;
> +
> +		return 0;
> +	}
> +
> +	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
> +	ctl |= PCI_EXP_DPC_CTL_SW_TRIGGER;
> +	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
> +	return 0;
> +}
> +
> +int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe)
> +{
> +	if (probe) {
> +		/*
> +		 * Reset must only affect @pdev, so bail out if it has siblings
> +		 * or descendants.  Need to differentiate whether @pdev has
> +		 * already been added to the bus list or not:
> +		 */
> +		if (list_empty(&pdev->bus_list) &&
> +		    !list_empty(&pdev->bus->devices))
> +			return -ENOTTY;
> +
> +		if (!list_empty(&pdev->bus_list) &&
> +		    !list_is_singular(&pdev->bus->devices))
> +			return -ENOTTY;
> +
> +		if (pdev->subordinate &&
> +		    !list_empty(&pdev->subordinate->devices))
> +			return -ENOTTY;
> +
> +		if (!pdev->bus->self)
> +			return -ENOTTY;
> +	}
> +
> +	return pci_dpc_sw_trigger(pdev->bus->self, probe);
> +}
> +
>  void pci_dpc_init(struct pci_dev *pdev)
>  {
>  	u16 cap;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 28af4414f789..7890cd4eb97d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -50,7 +50,7 @@
>  			       PCI_STATUS_PARITY)
>  
>  /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 7
> +#define PCI_NUM_RESET_METHODS 8
>  
>  #define PCI_RESET_PROBE		true
>  #define PCI_RESET_DO_RESET	false
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 1c3591c8e09e..73b3ccdffb3a 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1035,6 +1035,7 @@
>  #define PCI_EXP_DPC_CTL			0x06	/* DPC control */
>  #define  PCI_EXP_DPC_CTL_EN_FATAL	0x0001	/* Enable trigger on ERR_FATAL message */
>  #define  PCI_EXP_DPC_CTL_EN_NONFATAL	0x0002	/* Enable trigger on ERR_NONFATAL message */
> +#define  PCI_EXP_DPC_CTL_SW_TRIGGER	0x0040	/* Software Trigger */
>  #define  PCI_EXP_DPC_CTL_INT_EN		0x0008	/* DPC Interrupt Enable */
>  
>  #define PCI_EXP_DPC_STATUS		0x08	/* DPC Status */
> -- 
> 2.36.1
> 

Re: [PATCH] PCI/DPC: Add Software Trigger as reset method

[Mika Westerberg]

Hi,

On Tue, Nov 29, 2022 at 08:35:55AM +0100, Lukas Wunner wrote:
> Add DPC Software Trigger as a reset method to be used for silicon
> validation among other things:
> 
>   # echo dpc_sw_trigger > reset_method
>   # echo 1 > reset
> 
> After validating DPC, the default reset_method(s) may be reinstated:
> 
>   # echo default > reset_method
> 
> Writing the DPC Control Register requires that control was granted by
> firmware, so expose the reset_method only if DPC is native.  (And AER,
> which must always be granted or denied in unison per PCI Firmware Spec
> r3.3 table 4-5.)
> 
> The reset attribute in sysfs is meant to reset a single PCI Function,
> but DPC resets the entire hierarchy below the parent.  So only expose
> the reset method on PCI Functions without siblings or children.
> Checking for that may happen both *before* the PCI Function has been
> added to the bus list (via pci_device_add() -> pci_init_capabilities())
> and *after* (via reset_method_store()), hence differentiate between
> those two cases on reset probing.
> 
> It would be useful for silicon validation to have a separate sysfs
> attribute for PCI bridges to reset their downstream hierarchy.  Prepare
> for introduction of such an attribute by adding separate functions
> pci_dpc_sw_trigger() (to reset the hierarchy below a bridge) and
> pci_dpc_sw_trigger_parent() (to reset a single PCI Function), where the
> latter calls the former to trigger DPC on the parent bridge.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Looks good to me. There are few minor comments below. Feel free to
ignore them.

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> ---
>  drivers/pci/pci.c             |  1 +
>  drivers/pci/pci.h             |  2 ++
>  drivers/pci/pcie/dpc.c        | 57 +++++++++++++++++++++++++++++++++++
>  include/linux/pci.h           |  2 +-
>  include/uapi/linux/pci_regs.h |  1 +
>  5 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index fba95486caaf..f561f84a8bca 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5225,6 +5225,7 @@ static const struct pci_reset_fn_method pci_reset_fn_methods[] = {
>  	{ pci_af_flr, .name = "af_flr" },
>  	{ pci_pm_reset, .name = "pm" },
>  	{ pci_reset_bus_function, .name = "bus" },
> +	{ pci_dpc_sw_trigger_parent, .name = "dpc_sw_trigger" },
>  };
>  
>  static ssize_t reset_method_show(struct device *dev,
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9ed3b5550043..da2a3af4c46c 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -425,11 +425,13 @@ void pci_dpc_init(struct pci_dev *pdev);
>  void dpc_process_error(struct pci_dev *pdev);
>  pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
>  bool pci_dpc_recovered(struct pci_dev *pdev);
> +int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe);
>  #else
>  static inline void pci_save_dpc_state(struct pci_dev *dev) {}
>  static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
>  static inline void pci_dpc_init(struct pci_dev *pdev) {}
>  static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; }
> +static inline int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe) { return -ENOTTY; }
>  #endif
>  
>  #ifdef CONFIG_PCIEPORTBUS
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index f5ffea17c7f8..47fd69d0a9c2 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -322,6 +322,63 @@ static irqreturn_t dpc_irq(int irq, void *context)
>  	return IRQ_HANDLED;
>  }
>  
> +static int pci_dpc_sw_trigger(struct pci_dev *pdev, bool probe)
> +{
> +	struct pci_host_bridge *host;
> +	u16 cap, ctl;
> +
> +	if (probe) {
> +		if (!pdev->dpc_cap)
> +			return -ENOTTY;

-ENODEV?

> +
> +		host = pci_find_host_bridge(pdev->bus);
> +		if (!host->native_dpc && !pcie_ports_dpc_native)
> +			return -ENOTTY;
> +
> +		if (!pcie_aer_is_native(pdev))
> +			return -ENOTTY;
> +
> +		pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP,
> +				     &cap);
> +		if (!(cap & PCI_EXP_DPC_CAP_SW_TRIGGER))
> +			return -ENOTTY;
> +
> +		return 0;
> +	}
> +
> +	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
> +	ctl |= PCI_EXP_DPC_CTL_SW_TRIGGER;
> +	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);

Empty line here.

> +	return 0;
> +}
> +

Kernel-doc would make it easier for the caller to figure out what this
does and what the @probe attribute is used for.

> +int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe)
> +{
> +	if (probe) {
> +		/*
> +		 * Reset must only affect @pdev, so bail out if it has siblings
> +		 * or descendants.  Need to differentiate whether @pdev has
> +		 * already been added to the bus list or not:
> +		 */
> +		if (list_empty(&pdev->bus_list) &&
> +		    !list_empty(&pdev->bus->devices))
> +			return -ENOTTY;
> +
> +		if (!list_empty(&pdev->bus_list) &&
> +		    !list_is_singular(&pdev->bus->devices))
> +			return -ENOTTY;
> +
> +		if (pdev->subordinate &&
> +		    !list_empty(&pdev->subordinate->devices))
> +			return -ENOTTY;
> +
> +		if (!pdev->bus->self)
> +			return -ENOTTY;
> +	}
> +
> +	return pci_dpc_sw_trigger(pdev->bus->self, probe);
> +}
> +
>  void pci_dpc_init(struct pci_dev *pdev)
>  {
>  	u16 cap;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 28af4414f789..7890cd4eb97d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -50,7 +50,7 @@
>  			       PCI_STATUS_PARITY)
>  
>  /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 7
> +#define PCI_NUM_RESET_METHODS 8

I guess we cannot use ARRAY_SIZE() here?

>  
>  #define PCI_RESET_PROBE		true
>  #define PCI_RESET_DO_RESET	false
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 1c3591c8e09e..73b3ccdffb3a 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1035,6 +1035,7 @@
>  #define PCI_EXP_DPC_CTL			0x06	/* DPC control */
>  #define  PCI_EXP_DPC_CTL_EN_FATAL	0x0001	/* Enable trigger on ERR_FATAL message */
>  #define  PCI_EXP_DPC_CTL_EN_NONFATAL	0x0002	/* Enable trigger on ERR_NONFATAL message */
> +#define  PCI_EXP_DPC_CTL_SW_TRIGGER	0x0040	/* Software Trigger */
>  #define  PCI_EXP_DPC_CTL_INT_EN		0x0008	/* DPC Interrupt Enable */
>  
>  #define PCI_EXP_DPC_STATUS		0x08	/* DPC Status */
> -- 
> 2.36.1

Re: [PATCH] PCI/DPC: Add Software Trigger as reset method

[Sathyanarayanan Kuppuswamy]

Hi Lukas,

On 11/28/22 11:35 PM, Lukas Wunner wrote:
> Add DPC Software Trigger as a reset method to be used for silicon
> validation among other things:
> 
>   # echo dpc_sw_trigger > reset_method
>   # echo 1 > reset
> 
> After validating DPC, the default reset_method(s) may be reinstated:
> 
>   # echo default > reset_method
> 
> Writing the DPC Control Register requires that control was granted by
> firmware, so expose the reset_method only if DPC is native.  (And AER,
> which must always be granted or denied in unison per PCI Firmware Spec
> r3.3 table 4-5.)
> 
> The reset attribute in sysfs is meant to reset a single PCI Function,
> but DPC resets the entire hierarchy below the parent.  So only expose
> the reset method on PCI Functions without siblings or children.
> Checking for that may happen both *before* the PCI Function has been
> added to the bus list (via pci_device_add() -> pci_init_capabilities())
> and *after* (via reset_method_store()), hence differentiate between
> those two cases on reset probing.

Does this mean you want to only allow this reset method for DPC capable
ports without any devices attached to it? If yes, why not use other
reset methods available?

> 
> It would be useful for silicon validation to have a separate sysfs
> attribute for PCI bridges to reset their downstream hierarchy.  Prepare
> for introduction of such an attribute by adding separate functions
> pci_dpc_sw_trigger() (to reset the hierarchy below a bridge) and
> pci_dpc_sw_trigger_parent() (to reset a single PCI Function), where the
> latter calls the former to trigger DPC on the parent bridge.

To reset downstream hierarchy, DPC SW trigger bit can be modified using
setpci aswell right. Any reason to add special sysfs for it?

> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci.c             |  1 +
>  drivers/pci/pci.h             |  2 ++
>  drivers/pci/pcie/dpc.c        | 57 +++++++++++++++++++++++++++++++++++
>  include/linux/pci.h           |  2 +-
>  include/uapi/linux/pci_regs.h |  1 +
>  5 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index fba95486caaf..f561f84a8bca 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5225,6 +5225,7 @@ static const struct pci_reset_fn_method pci_reset_fn_methods[] = {
>  	{ pci_af_flr, .name = "af_flr" },
>  	{ pci_pm_reset, .name = "pm" },
>  	{ pci_reset_bus_function, .name = "bus" },
> +	{ pci_dpc_sw_trigger_parent, .name = "dpc_sw_trigger" },
>  };
>  
>  static ssize_t reset_method_show(struct device *dev,
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 9ed3b5550043..da2a3af4c46c 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -425,11 +425,13 @@ void pci_dpc_init(struct pci_dev *pdev);
>  void dpc_process_error(struct pci_dev *pdev);
>  pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
>  bool pci_dpc_recovered(struct pci_dev *pdev);
> +int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe);
>  #else
>  static inline void pci_save_dpc_state(struct pci_dev *dev) {}
>  static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
>  static inline void pci_dpc_init(struct pci_dev *pdev) {}
>  static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; }
> +static inline int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe) { return -ENOTTY; }
>  #endif
>  
>  #ifdef CONFIG_PCIEPORTBUS
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index f5ffea17c7f8..47fd69d0a9c2 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -322,6 +322,63 @@ static irqreturn_t dpc_irq(int irq, void *context)
>  	return IRQ_HANDLED;
>  }
>  
> +static int pci_dpc_sw_trigger(struct pci_dev *pdev, bool probe)
> +{
> +	struct pci_host_bridge *host;
> +	u16 cap, ctl;
> +
> +	if (probe) {
> +		if (!pdev->dpc_cap)
> +			return -ENOTTY;
> +
> +		host = pci_find_host_bridge(pdev->bus);
> +		if (!host->native_dpc && !pcie_ports_dpc_native)
> +			return -ENOTTY;
> +
> +		if (!pcie_aer_is_native(pdev))
> +			return -ENOTTY;
> +
> +		pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP,
> +				     &cap);
> +		if (!(cap & PCI_EXP_DPC_CAP_SW_TRIGGER))
> +			return -ENOTTY;
> +
> +		return 0;
> +	}
> +
> +	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
> +	ctl |= PCI_EXP_DPC_CTL_SW_TRIGGER;
> +	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
> +	return 0;
> +}
> +
> +int pci_dpc_sw_trigger_parent(struct pci_dev *pdev, bool probe)
> +{
> +	if (probe) {
> +		/*
> +		 * Reset must only affect @pdev, so bail out if it has siblings
> +		 * or descendants.  Need to differentiate whether @pdev has
> +		 * already been added to the bus list or not:
> +		 */
> +		if (list_empty(&pdev->bus_list) &&
> +		    !list_empty(&pdev->bus->devices))
> +			return -ENOTTY;
> +
> +		if (!list_empty(&pdev->bus_list) &&
> +		    !list_is_singular(&pdev->bus->devices))
> +			return -ENOTTY;
> +
> +		if (pdev->subordinate &&
> +		    !list_empty(&pdev->subordinate->devices))
> +			return -ENOTTY;
> +
> +		if (!pdev->bus->self)
> +			return -ENOTTY;
> +	}
> +
> +	return pci_dpc_sw_trigger(pdev->bus->self, probe);
> +}
> +
>  void pci_dpc_init(struct pci_dev *pdev)
>  {
>  	u16 cap;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 28af4414f789..7890cd4eb97d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -50,7 +50,7 @@
>  			       PCI_STATUS_PARITY)
>  
>  /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 7
> +#define PCI_NUM_RESET_METHODS 8
>  
>  #define PCI_RESET_PROBE		true
>  #define PCI_RESET_DO_RESET	false
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 1c3591c8e09e..73b3ccdffb3a 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1035,6 +1035,7 @@
>  #define PCI_EXP_DPC_CTL			0x06	/* DPC control */
>  #define  PCI_EXP_DPC_CTL_EN_FATAL	0x0001	/* Enable trigger on ERR_FATAL message */
>  #define  PCI_EXP_DPC_CTL_EN_NONFATAL	0x0002	/* Enable trigger on ERR_NONFATAL message */
> +#define  PCI_EXP_DPC_CTL_SW_TRIGGER	0x0040	/* Software Trigger */
>  #define  PCI_EXP_DPC_CTL_INT_EN		0x0008	/* DPC Interrupt Enable */
>  
>  #define PCI_EXP_DPC_STATUS		0x08	/* DPC Status */

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH] PCI/DPC: Add Software Trigger as reset method

[Keith Busch]

On Tue, Nov 29, 2022 at 08:35:55AM +0100, Lukas Wunner wrote:
> Add DPC Software Trigger as a reset method to be used for silicon
> validation among other things:

Do you really need a kernel helper to do this? You can test these with

  # setpci -s <dsp's b:d.f> ECAP_DPC+6.w=40:40

And since the kernel is a not aware you are synthesizing the DPC event,
that more naturally tests how the kernel would react to a hardware dpc
trigger.

Re: [PATCH] PCI/DPC: Add Software Trigger as reset method

[Lukas Wunner]

On Tue, Nov 29, 2022 at 09:36:33AM -0700, Keith Busch wrote:
> On Tue, Nov 29, 2022 at 08:35:55AM +0100, Lukas Wunner wrote:
> > Add DPC Software Trigger as a reset method to be used for silicon
> > validation among other things:
> 
> Do you really need a kernel helper to do this? You can test these with
> 
>   # setpci -s <dsp's b:d.f> ECAP_DPC+6.w=40:40
> 
> And since the kernel is a not aware you are synthesizing the DPC event,
> that more naturally tests how the kernel would react to a hardware dpc
> trigger.

I've seen people write a value directly instead of using the "data:mask"
syntax above and that's dangerous because the register is under the
control of the kernel.

By exposing software trigger as a reset method I hope to make its
invocation error-proof and more comfortable.

Thanks,

Lukas

Re: [PATCH] PCI/DPC: Add Software Trigger as reset method

[Lukas Wunner]

On Tue, Nov 29, 2022 at 08:27:43AM -0800, Sathyanarayanan Kuppuswamy wrote:
> On 11/28/22 11:35 PM, Lukas Wunner wrote:
> > Add DPC Software Trigger as a reset method to be used for silicon
> > validation among other things:
> > 
> >   # echo dpc_sw_trigger > reset_method
> >   # echo 1 > reset
> > 
> > After validating DPC, the default reset_method(s) may be reinstated:
> > 
> >   # echo default > reset_method
> > 
> > Writing the DPC Control Register requires that control was granted by
> > firmware, so expose the reset_method only if DPC is native.  (And AER,
> > which must always be granted or denied in unison per PCI Firmware Spec
> > r3.3 table 4-5.)
> > 
> > The reset attribute in sysfs is meant to reset a single PCI Function,
> > but DPC resets the entire hierarchy below the parent.  So only expose
> > the reset method on PCI Functions without siblings or children.
> > Checking for that may happen both *before* the PCI Function has been
> > added to the bus list (via pci_device_add() -> pci_init_capabilities())
> > and *after* (via reset_method_store()), hence differentiate between
> > those two cases on reset probing.
> 
> Does this mean you want to only allow this reset method for DPC capable
> ports without any devices attached to it? If yes, why not use other
> reset methods available?

This reset method is allowed if the DPC-capable port has a single child
device and that child has no siblings or descendants.  

And the reset is performed by echoing 1 to the single child's reset
attribute in sysfs.

Those are the same semantics as the Secondary Bus Reset method.

Thanks,

Lukas

Re: [PATCH] PCI/DPC: Add Software Trigger as reset method

[Bjorn Helgaas]

Hi Lukas,

On Tue, Nov 29, 2022 at 08:35:55AM +0100, Lukas Wunner wrote:
> Add DPC Software Trigger as a reset method to be used for silicon
> validation among other things:
> 
>   # echo dpc_sw_trigger > reset_method
>   # echo 1 > reset
> 
> After validating DPC, the default reset_method(s) may be reinstated:
> 
>   # echo default > reset_method
> 
> Writing the DPC Control Register requires that control was granted by
> firmware, so expose the reset_method only if DPC is native.  (And AER,
> which must always be granted or denied in unison per PCI Firmware Spec
> r3.3 table 4-5.)
> 
> The reset attribute in sysfs is meant to reset a single PCI Function,
> but DPC resets the entire hierarchy below the parent.  So only expose
> the reset method on PCI Functions without siblings or children.
> Checking for that may happen both *before* the PCI Function has been
> added to the bus list (via pci_device_add() -> pci_init_capabilities())
> and *after* (via reset_method_store()), hence differentiate between
> those two cases on reset probing.
> 
> It would be useful for silicon validation to have a separate sysfs
> attribute for PCI bridges to reset their downstream hierarchy.  Prepare
> for introduction of such an attribute by adding separate functions
> pci_dpc_sw_trigger() (to reset the hierarchy below a bridge) and
> pci_dpc_sw_trigger_parent() (to reset a single PCI Function), where the
> latter calls the former to trigger DPC on the parent bridge.

Does this add functionality above what the "bus" method (Secondary Bus
Reset) provides?  If it's effectively the same as SBR, I'm not sure
it's worth complicating the user interface just to support silicon
validation.

Bjorn