| Message ID | 9c1533fd42e9002bd6d2020656fa1dd0e3e3bf3a.1669706952.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PCI/DPC: Add Software Trigger as reset method | expand |
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 >
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
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 */
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.
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
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
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
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 */
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(-)