| Message ID | 20210709123813.8700-8-ameynarkhede03@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Expose and manage PCI device reset | expand |
On Fri, 9 Jul 2021 18:08:12 +0530 Amey Narkhede <ameynarkhede03@gmail.com> wrote: > From: Shanker Donthineni <sdonthineni@nvidia.com> > > The _RST is a standard method specified in the ACPI specification. It > provides a function level reset when it is described in the acpi_device > context associated with PCI-device. Implement a new reset function > pci_dev_acpi_reset() for probing RST method and execute if it is defined > in the firmware. > > The default priority of the ACPI reset is set to below device-specific > and above hardware resets. > > Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com> > Suggested-by: Alex Williamson <alex.williamson@redhat.com> > Reviewed-by: Sinan Kaya <okaya@kernel.org> > --- > drivers/pci/pci-acpi.c | 23 +++++++++++++++++++++++ > drivers/pci/pci.c | 1 + > drivers/pci/pci.h | 6 ++++++ > include/linux/pci.h | 2 +- > 4 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c > index dae021322..b6de71d15 100644 > --- a/drivers/pci/pci-acpi.c > +++ b/drivers/pci/pci-acpi.c > @@ -941,6 +941,29 @@ void pci_set_acpi_fwnode(struct pci_dev *dev) > acpi_pci_find_companion(&dev->dev)); > } > > +/** > + * pci_dev_acpi_reset - do a function level reset using _RST method > + * @dev: device to reset > + * @probe: check if _RST method is included in the acpi_device context. > + */ > +int pci_dev_acpi_reset(struct pci_dev *dev, int probe) > +{ > + acpi_handle handle = ACPI_HANDLE(&dev->dev); > + > + if (!handle || !acpi_has_method(handle, "_RST")) > + return -ENOTTY; > + > + if (probe) > + return 0; > + > + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) { > + pci_warn(dev, "ACPI _RST failed\n"); > + return -EINVAL; Should we return -ENOTTY here instead to give a possible secondary reset method a chance? Thanks, Alex > + } > + > + return 0; > +} > + > static bool acpi_pci_bridge_d3(struct pci_dev *dev) > { > const struct fwnode_handle *fwnode; > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index d5c16492c..1e64dbd3e 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -5115,6 +5115,7 @@ static void pci_dev_restore(struct pci_dev *dev) > const struct pci_reset_fn_method pci_reset_fn_methods[] = { > { }, > { &pci_dev_specific_reset, .name = "device_specific" }, > + { &pci_dev_acpi_reset, .name = "acpi" }, > { &pcie_reset_flr, .name = "flr" }, > { &pci_af_flr, .name = "af_flr" }, > { &pci_pm_reset, .name = "pm" }, > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 990b73e90..2c12017ed 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -705,7 +705,13 @@ static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL > int pci_acpi_program_hp_params(struct pci_dev *dev); > extern const struct attribute_group pci_dev_acpi_attr_group; > void pci_set_acpi_fwnode(struct pci_dev *dev); > +int pci_dev_acpi_reset(struct pci_dev *dev, int probe); > #else > +static inline int pci_dev_acpi_reset(struct pci_dev *dev, int probe) > +{ > + return -ENOTTY; > +} > + > static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {} > static inline int pci_acpi_program_hp_params(struct pci_dev *dev) > { > diff --git a/include/linux/pci.h b/include/linux/pci.h > index f34563831..c3b0d771c 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 6 > +#define PCI_NUM_RESET_METHODS 7 > > /* > * The PCI interface treats multi-function devices as independent
Hi Alex, On 7/12/21 6:09 PM, Alex Williamson wrote: >> +/** >> + * pci_dev_acpi_reset - do a function level reset using _RST method >> + * @dev: device to reset >> + * @probe: check if _RST method is included in the acpi_device context. >> + */ >> +int pci_dev_acpi_reset(struct pci_dev *dev, int probe) >> +{ >> + acpi_handle handle = ACPI_HANDLE(&dev->dev); >> + >> + if (!handle || !acpi_has_method(handle, "_RST")) >> + return -ENOTTY; >> + >> + if (probe) >> + return 0; >> + >> + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) { >> + pci_warn(dev, "ACPI _RST failed\n"); >> + return -EINVAL; > Should we return -ENOTTY here instead to give a possible secondary > reset method a chance? Thanks, Thanks for reviewing patches. ACPI/AML _RST method type is VOID. The only possibility of failure would be either system is running out of memory or bugs in ACPICA. There is no strong reason not to return -NOTTY. I'll fix in the next version. Is there opportunity to include reset feature in v5.14-rc2? Can I add your reviewed-by since no other comments to this patch? -Shanker
On Mon, 12 Jul 2021 19:51:41 -0500 Shanker R Donthineni <sdonthineni@nvidia.com> wrote: > Hi Alex, > > On 7/12/21 6:09 PM, Alex Williamson wrote: > >> +/** > >> + * pci_dev_acpi_reset - do a function level reset using _RST method > >> + * @dev: device to reset > >> + * @probe: check if _RST method is included in the acpi_device context. > >> + */ > >> +int pci_dev_acpi_reset(struct pci_dev *dev, int probe) > >> +{ > >> + acpi_handle handle = ACPI_HANDLE(&dev->dev); > >> + > >> + if (!handle || !acpi_has_method(handle, "_RST")) > >> + return -ENOTTY; > >> + > >> + if (probe) > >> + return 0; > >> + > >> + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) { > >> + pci_warn(dev, "ACPI _RST failed\n"); > >> + return -EINVAL; > > Should we return -ENOTTY here instead to give a possible secondary > > reset method a chance? Thanks, > Thanks for reviewing patches. > > ACPI/AML _RST method type is VOID. The only possibility of failure would be > either system is running out of memory or bugs in ACPICA. There is no strong > reason not to return -NOTTY. > > I'll fix in the next version. Is there opportunity to include reset feature in v5.14-rc2? Sounds good, it's a corner case but since we've got a series of methods we can try and part of the point of Amey's series is giving the user control of the order and methods to try, we might as well make use of it. I think there's also some precedence in the quirks that they can fail and fall through to standard resets. I'll leave any upstream timing questions to Bjorn, but we've passed the v5.14 merge window when new functionality is generally accepted. > Can I add your reviewed-by since no other comments to this patch? Yeah, s/-EINVAL/-ENOTTY/ Reviewed-by: Alex Williamson <alex.williamson@redhat.com> Thanks!
diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c index dae021322..b6de71d15 100644 --- a/drivers/pci/pci-acpi.c +++ b/drivers/pci/pci-acpi.c @@ -941,6 +941,29 @@ void pci_set_acpi_fwnode(struct pci_dev *dev) acpi_pci_find_companion(&dev->dev)); } +/** + * pci_dev_acpi_reset - do a function level reset using _RST method + * @dev: device to reset + * @probe: check if _RST method is included in the acpi_device context. + */ +int pci_dev_acpi_reset(struct pci_dev *dev, int probe) +{ + acpi_handle handle = ACPI_HANDLE(&dev->dev); + + if (!handle || !acpi_has_method(handle, "_RST")) + return -ENOTTY; + + if (probe) + return 0; + + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) { + pci_warn(dev, "ACPI _RST failed\n"); + return -EINVAL; + } + + return 0; +} + static bool acpi_pci_bridge_d3(struct pci_dev *dev) { const struct fwnode_handle *fwnode; diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index d5c16492c..1e64dbd3e 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5115,6 +5115,7 @@ static void pci_dev_restore(struct pci_dev *dev) const struct pci_reset_fn_method pci_reset_fn_methods[] = { { }, { &pci_dev_specific_reset, .name = "device_specific" }, + { &pci_dev_acpi_reset, .name = "acpi" }, { &pcie_reset_flr, .name = "flr" }, { &pci_af_flr, .name = "af_flr" }, { &pci_pm_reset, .name = "pm" }, diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 990b73e90..2c12017ed 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -705,7 +705,13 @@ static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL int pci_acpi_program_hp_params(struct pci_dev *dev); extern const struct attribute_group pci_dev_acpi_attr_group; void pci_set_acpi_fwnode(struct pci_dev *dev); +int pci_dev_acpi_reset(struct pci_dev *dev, int probe); #else +static inline int pci_dev_acpi_reset(struct pci_dev *dev, int probe) +{ + return -ENOTTY; +} + static inline void pci_set_acpi_fwnode(struct pci_dev *dev) {} static inline int pci_acpi_program_hp_params(struct pci_dev *dev) { diff --git a/include/linux/pci.h b/include/linux/pci.h index f34563831..c3b0d771c 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 6 +#define PCI_NUM_RESET_METHODS 7 /* * The PCI interface treats multi-function devices as independent