| Message ID | 20210608054857.18963-2-ameynarkhede03@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Expose and manage PCI device reset | expand |
On 6/8/21 12:48 AM, Amey Narkhede wrote: > Currently there is separate function pcie_has_flr() to probe if pcie flr is > supported by the device which does not match the calling convention > followed by reset methods which use second function argument to decide > whether to probe or not. Add new function pcie_reset_flr() that follows > the calling convention of reset methods. > > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > Co-developed-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> Tested-by: Shanker Donthineni <sdonthineni@nvidia.com>
[+cc Christoph, since he added pcie_flr()] On Tue, Jun 08, 2021 at 11:18:50AM +0530, Amey Narkhede wrote: > Currently there is separate function pcie_has_flr() to probe if pcie flr is > supported by the device which does not match the calling convention > followed by reset methods which use second function argument to decide > whether to probe or not. Add new function pcie_reset_flr() that follows > the calling convention of reset methods. I don't like the fact that we handle FLR differently from other types of reset, so I do like the fact that this makes them more consistent. > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > Co-developed-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> > --- > drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +- > drivers/pci/pci.c | 62 ++++++++++++---------- > drivers/pci/pcie/aer.c | 12 ++--- > drivers/pci/quirks.c | 9 ++-- > include/linux/pci.h | 2 +- > 5 files changed, 43 insertions(+), 46 deletions(-) > > diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c > index facc8e6bc..15d6c8452 100644 > --- a/drivers/crypto/cavium/nitrox/nitrox_main.c > +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c > @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev) > return -ENOMEM; > } > > - /* check flr support */ > - if (pcie_has_flr(pdev)) > - pcie_flr(pdev); > + pcie_reset_flr(pdev, 0); > > pci_restore_state(pdev); > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 452351025..3bf36924c 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4611,32 +4611,12 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev) > } > EXPORT_SYMBOL(pci_wait_for_pending_transaction); > > -/** > - * pcie_has_flr - check if a device supports function level resets > - * @dev: device to check > - * > - * Returns true if the device advertises support for PCIe function level > - * resets. > - */ > -bool pcie_has_flr(struct pci_dev *dev) > -{ > - u32 cap; > - > - if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > - return false; > - > - pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > - return cap & PCI_EXP_DEVCAP_FLR; > -} > -EXPORT_SYMBOL_GPL(pcie_has_flr); > - > /** > * pcie_flr - initiate a PCIe function level reset > * @dev: device to reset > * > - * Initiate a function level reset on @dev. The caller should ensure the > - * device supports FLR before calling this function, e.g. by using the > - * pcie_has_flr() helper. > + * Initiate a function level reset unconditionally on @dev without > + * checking any flags and DEVCAP > */ > int pcie_flr(struct pci_dev *dev) > { > @@ -4659,6 +4639,31 @@ int pcie_flr(struct pci_dev *dev) > } > EXPORT_SYMBOL_GPL(pcie_flr); > > +/** > + * pcie_reset_flr - initiate a PCIe function level reset > + * @dev: device to reset > + * @probe: If set, only check if the device can be reset this way. > + * > + * Initiate a function level reset on @dev. > + */ > +int pcie_reset_flr(struct pci_dev *dev, int probe) > +{ > + u32 cap; > + > + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > + return -ENOTTY; > + > + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > + if (!(cap & PCI_EXP_DEVCAP_FLR)) > + return -ENOTTY; > + > + if (probe) > + return 0; > + > + return pcie_flr(dev); Christoph added pcie_flr() with a60a2b73ba69 ("PCI: Export pcie_flr()"), where the commit log says he split out the probing because "non-core callers already know their hardware." It *is* reasonable to expect that drivers know whether their device supports FLR so they don't need to probe. But we don't expose the "probe" argument outside the PCI core for any other reset methods, and I would like to avoid that here. It seems excessive to have to read PCI_EXP_DEVCAP every time. PCI_EXP_DEVCAP_FLR is a read-only bit, and we should only need to look at it once. What I would really like here is a single bit in the pci_dev that we could set at enumeration-time, e.g., something like this: struct pci_dev { ... unsigned int has_flr:1; }; void set_pcie_port_type(...) # during enumeration { pci_read_config_word(dev, pos + PCI_EXP_DEVCAP, ®16); if (reg16 & PCI_EXP_DEVCAP_FLR) dev->has_flr = 1; } static void quirk_no_flr(...) { dev->has_flr = 0; # get rid of PCI_DEV_FLAGS_NO_FLR_RESET } int pcie_flr(...) { if (!dev->has_flr) return -ENOTTY; if (!pci_wait_for_pending_transaction(dev)) ... } I think this should be enough that we could get rid of pcie_has_flr() without having to expose the "probe" argument outside drivers/pci/. Procedural note: if we *do* have to expose the "probe" argument, can you arrange it to have the correct type before touching the drivers, so we only have to touch the drivers once? > +} > +EXPORT_SYMBOL_GPL(pcie_reset_flr); > + > static int pci_af_flr(struct pci_dev *dev, int probe) > { > int pos; > @@ -5139,11 +5144,9 @@ int __pci_reset_function_locked(struct pci_dev *dev) > rc = pci_dev_specific_reset(dev, 0); > if (rc != -ENOTTY) > return rc; > - if (pcie_has_flr(dev)) { > - rc = pcie_flr(dev); > - if (rc != -ENOTTY) > - return rc; > - } > + rc = pcie_reset_flr(dev, 0); > + if (rc != -ENOTTY) > + return rc; > rc = pci_af_flr(dev, 0); > if (rc != -ENOTTY) > return rc; > @@ -5174,8 +5177,9 @@ int pci_probe_reset_function(struct pci_dev *dev) > rc = pci_dev_specific_reset(dev, 1); > if (rc != -ENOTTY) > return rc; > - if (pcie_has_flr(dev)) > - return 0; > + rc = pcie_reset_flr(dev, 1); > + if (rc != -ENOTTY) > + return rc; > rc = pci_af_flr(dev, 1); > if (rc != -ENOTTY) > return rc; > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index ec943cee5..98077595a 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -1405,13 +1405,11 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev) > } > > if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) { > - if (pcie_has_flr(dev)) { > - rc = pcie_flr(dev); > - pci_info(dev, "has been reset (%d)\n", rc); > - } else { > - pci_info(dev, "not reset (no FLR support)\n"); > - rc = -ENOTTY; > - } > + rc = pcie_reset_flr(dev, 0); > + if (!rc) > + pci_info(dev, "has been reset\n"); > + else > + pci_info(dev, "not reset (no FLR support: %d)\n", rc); > } else { > rc = pci_bus_error_reset(dev); > pci_info(dev, "%s Port link has been reset (%d)\n", > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index d85914afe..f977ba79a 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -3819,7 +3819,7 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe) > u32 cfg; > > if (dev->class != PCI_CLASS_STORAGE_EXPRESS || > - !pcie_has_flr(dev) || !pci_resource_start(dev, 0)) > + pcie_reset_flr(dev, 1) || !pci_resource_start(dev, 0)) > return -ENOTTY; > > if (probe) > @@ -3888,13 +3888,10 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe) > */ > static int delay_250ms_after_flr(struct pci_dev *dev, int probe) > { > - if (!pcie_has_flr(dev)) > - return -ENOTTY; > + int ret = pcie_reset_flr(dev, probe); > > if (probe) > - return 0; > - > - pcie_flr(dev); > + return ret; > > msleep(250); > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index c20211e59..20b90c205 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1225,7 +1225,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev, > enum pci_bus_speed *speed, > enum pcie_link_width *width); > void pcie_print_link_status(struct pci_dev *dev); > -bool pcie_has_flr(struct pci_dev *dev); > +int pcie_reset_flr(struct pci_dev *dev, int probe); > int pcie_flr(struct pci_dev *dev); > int __pci_reset_function_locked(struct pci_dev *dev); > int pci_reset_function(struct pci_dev *dev); > -- > 2.31.1 >
On Thu, 17 Jun 2021 16:57:34 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Christoph, since he added pcie_flr()] > > On Tue, Jun 08, 2021 at 11:18:50AM +0530, Amey Narkhede wrote: > > Currently there is separate function pcie_has_flr() to probe if pcie flr is > > supported by the device which does not match the calling convention > > followed by reset methods which use second function argument to decide > > whether to probe or not. Add new function pcie_reset_flr() that follows > > the calling convention of reset methods. > > I don't like the fact that we handle FLR differently from other types > of reset, so I do like the fact that this makes them more consistent. > > > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > > Co-developed-by: Alex Williamson <alex.williamson@redhat.com> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> > > --- > > drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +- > > drivers/pci/pci.c | 62 ++++++++++++---------- > > drivers/pci/pcie/aer.c | 12 ++--- > > drivers/pci/quirks.c | 9 ++-- > > include/linux/pci.h | 2 +- > > 5 files changed, 43 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c > > index facc8e6bc..15d6c8452 100644 > > --- a/drivers/crypto/cavium/nitrox/nitrox_main.c > > +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c > > @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev) > > return -ENOMEM; > > } > > > > - /* check flr support */ > > - if (pcie_has_flr(pdev)) > > - pcie_flr(pdev); > > + pcie_reset_flr(pdev, 0); > > > > pci_restore_state(pdev); > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 452351025..3bf36924c 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -4611,32 +4611,12 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev) > > } > > EXPORT_SYMBOL(pci_wait_for_pending_transaction); > > > > -/** > > - * pcie_has_flr - check if a device supports function level resets > > - * @dev: device to check > > - * > > - * Returns true if the device advertises support for PCIe function level > > - * resets. > > - */ > > -bool pcie_has_flr(struct pci_dev *dev) > > -{ > > - u32 cap; > > - > > - if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > > - return false; > > - > > - pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > > - return cap & PCI_EXP_DEVCAP_FLR; > > -} > > -EXPORT_SYMBOL_GPL(pcie_has_flr); > > - > > /** > > * pcie_flr - initiate a PCIe function level reset > > * @dev: device to reset > > * > > - * Initiate a function level reset on @dev. The caller should ensure the > > - * device supports FLR before calling this function, e.g. by using the > > - * pcie_has_flr() helper. > > + * Initiate a function level reset unconditionally on @dev without > > + * checking any flags and DEVCAP > > */ > > int pcie_flr(struct pci_dev *dev) > > { > > @@ -4659,6 +4639,31 @@ int pcie_flr(struct pci_dev *dev) > > } > > EXPORT_SYMBOL_GPL(pcie_flr); > > > > +/** > > + * pcie_reset_flr - initiate a PCIe function level reset > > + * @dev: device to reset > > + * @probe: If set, only check if the device can be reset this way. > > + * > > + * Initiate a function level reset on @dev. > > + */ > > +int pcie_reset_flr(struct pci_dev *dev, int probe) > > +{ > > + u32 cap; > > + > > + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > > + return -ENOTTY; > > + > > + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > > + if (!(cap & PCI_EXP_DEVCAP_FLR)) > > + return -ENOTTY; > > + > > + if (probe) > > + return 0; > > + > > + return pcie_flr(dev); > > Christoph added pcie_flr() with a60a2b73ba69 ("PCI: Export > pcie_flr()"), where the commit log says he split out the probing > because "non-core callers already know their hardware." > > It *is* reasonable to expect that drivers know whether their device > supports FLR so they don't need to probe. I don't think it changes your suggestion below, but this statement is a little troublesome when we look at devices running in VMs where we've been known to hide various capabilities, or simply quirks where some combination of a known device feature might be otherwise avoided. A more robust driver should try to make fewer assumptions for these cases, if not simply the inability to predict future changes to the hardware. FLR should be a relatively rare event, but caching for driver API purposes seems reasonable. Thanks, Alex > But we don't expose the "probe" argument outside the PCI core for any > other reset methods, and I would like to avoid that here. > > It seems excessive to have to read PCI_EXP_DEVCAP every time. > PCI_EXP_DEVCAP_FLR is a read-only bit, and we should only need to look > at it once. > > What I would really like here is a single bit in the pci_dev that we > could set at enumeration-time, e.g., something like this: > > struct pci_dev { > ... > unsigned int has_flr:1; > }; > > void set_pcie_port_type(...) # during enumeration > { > pci_read_config_word(dev, pos + PCI_EXP_DEVCAP, ®16); > if (reg16 & PCI_EXP_DEVCAP_FLR) > dev->has_flr = 1; > } > > static void quirk_no_flr(...) > { > dev->has_flr = 0; # get rid of PCI_DEV_FLAGS_NO_FLR_RESET > } > > int pcie_flr(...) > { > if (!dev->has_flr) > return -ENOTTY; > > if (!pci_wait_for_pending_transaction(dev)) > ... > } > > I think this should be enough that we could get rid of pcie_has_flr() > without having to expose the "probe" argument outside drivers/pci/. > > Procedural note: if we *do* have to expose the "probe" argument, can > you arrange it to have the correct type before touching the drivers, so > we only have to touch the drivers once? > > > +} > > +EXPORT_SYMBOL_GPL(pcie_reset_flr); > > + > > static int pci_af_flr(struct pci_dev *dev, int probe) > > { > > int pos; > > @@ -5139,11 +5144,9 @@ int __pci_reset_function_locked(struct pci_dev *dev) > > rc = pci_dev_specific_reset(dev, 0); > > if (rc != -ENOTTY) > > return rc; > > - if (pcie_has_flr(dev)) { > > - rc = pcie_flr(dev); > > - if (rc != -ENOTTY) > > - return rc; > > - } > > + rc = pcie_reset_flr(dev, 0); > > + if (rc != -ENOTTY) > > + return rc; > > rc = pci_af_flr(dev, 0); > > if (rc != -ENOTTY) > > return rc; > > @@ -5174,8 +5177,9 @@ int pci_probe_reset_function(struct pci_dev *dev) > > rc = pci_dev_specific_reset(dev, 1); > > if (rc != -ENOTTY) > > return rc; > > - if (pcie_has_flr(dev)) > > - return 0; > > + rc = pcie_reset_flr(dev, 1); > > + if (rc != -ENOTTY) > > + return rc; > > rc = pci_af_flr(dev, 1); > > if (rc != -ENOTTY) > > return rc; > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index ec943cee5..98077595a 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -1405,13 +1405,11 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev) > > } > > > > if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) { > > - if (pcie_has_flr(dev)) { > > - rc = pcie_flr(dev); > > - pci_info(dev, "has been reset (%d)\n", rc); > > - } else { > > - pci_info(dev, "not reset (no FLR support)\n"); > > - rc = -ENOTTY; > > - } > > + rc = pcie_reset_flr(dev, 0); > > + if (!rc) > > + pci_info(dev, "has been reset\n"); > > + else > > + pci_info(dev, "not reset (no FLR support: %d)\n", rc); > > } else { > > rc = pci_bus_error_reset(dev); > > pci_info(dev, "%s Port link has been reset (%d)\n", > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index d85914afe..f977ba79a 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -3819,7 +3819,7 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe) > > u32 cfg; > > > > if (dev->class != PCI_CLASS_STORAGE_EXPRESS || > > - !pcie_has_flr(dev) || !pci_resource_start(dev, 0)) > > + pcie_reset_flr(dev, 1) || !pci_resource_start(dev, 0)) > > return -ENOTTY; > > > > if (probe) > > @@ -3888,13 +3888,10 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe) > > */ > > static int delay_250ms_after_flr(struct pci_dev *dev, int probe) > > { > > - if (!pcie_has_flr(dev)) > > - return -ENOTTY; > > + int ret = pcie_reset_flr(dev, probe); > > > > if (probe) > > - return 0; > > - > > - pcie_flr(dev); > > + return ret; > > > > msleep(250); > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index c20211e59..20b90c205 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -1225,7 +1225,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev, > > enum pci_bus_speed *speed, > > enum pcie_link_width *width); > > void pcie_print_link_status(struct pci_dev *dev); > > -bool pcie_has_flr(struct pci_dev *dev); > > +int pcie_reset_flr(struct pci_dev *dev, int probe); > > int pcie_flr(struct pci_dev *dev); > > int __pci_reset_function_locked(struct pci_dev *dev); > > int pci_reset_function(struct pci_dev *dev); > > -- > > 2.31.1 > > >
On 21/06/17 04:57PM, Bjorn Helgaas wrote: > [+cc Christoph, since he added pcie_flr()] > > On Tue, Jun 08, 2021 at 11:18:50AM +0530, Amey Narkhede wrote: > > Currently there is separate function pcie_has_flr() to probe if pcie flr is > > supported by the device which does not match the calling convention > > followed by reset methods which use second function argument to decide > > whether to probe or not. Add new function pcie_reset_flr() that follows > > the calling convention of reset methods. > > I don't like the fact that we handle FLR differently from other types > of reset, so I do like the fact that this makes them more consistent. > > > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > > Co-developed-by: Alex Williamson <alex.williamson@redhat.com> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com> > > --- > > drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +- > > drivers/pci/pci.c | 62 ++++++++++++---------- > > drivers/pci/pcie/aer.c | 12 ++--- > > drivers/pci/quirks.c | 9 ++-- > > include/linux/pci.h | 2 +- > > 5 files changed, 43 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c > > index facc8e6bc..15d6c8452 100644 > > --- a/drivers/crypto/cavium/nitrox/nitrox_main.c > > +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c > > @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev) > > return -ENOMEM; > > } > > > > - /* check flr support */ > > - if (pcie_has_flr(pdev)) > > - pcie_flr(pdev); > > + pcie_reset_flr(pdev, 0); > > > > pci_restore_state(pdev); > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 452351025..3bf36924c 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -4611,32 +4611,12 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev) > > } > > EXPORT_SYMBOL(pci_wait_for_pending_transaction); > > > > -/** > > - * pcie_has_flr - check if a device supports function level resets > > - * @dev: device to check > > - * > > - * Returns true if the device advertises support for PCIe function level > > - * resets. > > - */ > > -bool pcie_has_flr(struct pci_dev *dev) > > -{ > > - u32 cap; > > - > > - if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > > - return false; > > - > > - pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > > - return cap & PCI_EXP_DEVCAP_FLR; > > -} > > -EXPORT_SYMBOL_GPL(pcie_has_flr); > > - > > /** > > * pcie_flr - initiate a PCIe function level reset > > * @dev: device to reset > > * > > - * Initiate a function level reset on @dev. The caller should ensure the > > - * device supports FLR before calling this function, e.g. by using the > > - * pcie_has_flr() helper. > > + * Initiate a function level reset unconditionally on @dev without > > + * checking any flags and DEVCAP > > */ > > int pcie_flr(struct pci_dev *dev) > > { > > @@ -4659,6 +4639,31 @@ int pcie_flr(struct pci_dev *dev) > > } > > EXPORT_SYMBOL_GPL(pcie_flr); > > > > +/** > > + * pcie_reset_flr - initiate a PCIe function level reset > > + * @dev: device to reset > > + * @probe: If set, only check if the device can be reset this way. > > + * > > + * Initiate a function level reset on @dev. > > + */ > > +int pcie_reset_flr(struct pci_dev *dev, int probe) > > +{ > > + u32 cap; > > + > > + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > > + return -ENOTTY; > > + > > + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > > + if (!(cap & PCI_EXP_DEVCAP_FLR)) > > + return -ENOTTY; > > + > > + if (probe) > > + return 0; > > + > > + return pcie_flr(dev); > > Christoph added pcie_flr() with a60a2b73ba69 ("PCI: Export > pcie_flr()"), where the commit log says he split out the probing > because "non-core callers already know their hardware." > > It *is* reasonable to expect that drivers know whether their device > supports FLR so they don't need to probe. > > But we don't expose the "probe" argument outside the PCI core for any > other reset methods, and I would like to avoid that here. > > It seems excessive to have to read PCI_EXP_DEVCAP every time. > PCI_EXP_DEVCAP_FLR is a read-only bit, and we should only need to look > at it once. > > What I would really like here is a single bit in the pci_dev that we > could set at enumeration-time, e.g., something like this: > > struct pci_dev { > ... > unsigned int has_flr:1; > }; > > void set_pcie_port_type(...) # during enumeration > { > pci_read_config_word(dev, pos + PCI_EXP_DEVCAP, ®16); > if (reg16 & PCI_EXP_DEVCAP_FLR) > dev->has_flr = 1; > } > > static void quirk_no_flr(...) > { > dev->has_flr = 0; # get rid of PCI_DEV_FLAGS_NO_FLR_RESET > } > > int pcie_flr(...) > { > if (!dev->has_flr) > return -ENOTTY; > > if (!pci_wait_for_pending_transaction(dev)) > ... > } > > I think this should be enough that we could get rid of pcie_has_flr() > without having to expose the "probe" argument outside drivers/pci/. > > Procedural note: if we *do* have to expose the "probe" argument, can > you arrange it to have the correct type before touching the drivers, so > we only have to touch the drivers once? > Thanks for the details. I'll add dev->has_flr check in pcie_reset_flr. [...] Thanks, Amey
On Tue, Jun 08, 2021 at 11:18:50AM +0530, Amey Narkhede wrote: > Currently there is separate function pcie_has_flr() to probe if pcie flr is > supported by the device which does not match the calling convention > followed by reset methods which use second function argument to decide > whether to probe or not. Add new function pcie_reset_flr() that follows > the calling convention of reset methods. > +/** > + * pcie_reset_flr - initiate a PCIe function level reset > + * @dev: device to reset > + * @probe: If set, only check if the device can be reset this way. > + * > + * Initiate a function level reset on @dev. > + */ > +int pcie_reset_flr(struct pci_dev *dev, int probe) > +{ > + u32 cap; > + > + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > + return -ENOTTY; > + > + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > + if (!(cap & PCI_EXP_DEVCAP_FLR)) > + return -ENOTTY; > + > + if (probe) > + return 0; > + > + return pcie_flr(dev); > +} Tangent: I've been told before, but I can't remember why we need the "probe" interface. Since we're looking at this area again, can we add a comment to clarify this? Every time I read this, I wonder why we can't just get rid of the probe and attempt a reset. If it fails because it's not supported, we could just try the next one in the list.
On 21/06/24 07:23AM, Bjorn Helgaas wrote: > On Tue, Jun 08, 2021 at 11:18:50AM +0530, Amey Narkhede wrote: > > Currently there is separate function pcie_has_flr() to probe if pcie flr is > > supported by the device which does not match the calling convention > > followed by reset methods which use second function argument to decide > > whether to probe or not. Add new function pcie_reset_flr() that follows > > the calling convention of reset methods. > > > +/** > > + * pcie_reset_flr - initiate a PCIe function level reset > > + * @dev: device to reset > > + * @probe: If set, only check if the device can be reset this way. > > + * > > + * Initiate a function level reset on @dev. > > + */ > > +int pcie_reset_flr(struct pci_dev *dev, int probe) > > +{ > > + u32 cap; > > + > > + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > > + return -ENOTTY; > > + > > + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > > + if (!(cap & PCI_EXP_DEVCAP_FLR)) > > + return -ENOTTY; > > + > > + if (probe) > > + return 0; > > + > > + return pcie_flr(dev); > > +} > > Tangent: I've been told before, but I can't remember why we need the > "probe" interface. Since we're looking at this area again, can we add > a comment to clarify this? > > Every time I read this, I wonder why we can't just get rid of the > probe and attempt a reset. If it fails because it's not supported, we > could just try the next one in the list. Part of the reason is to have same calling convention as other reset methods and other reason is devices that run in VMs where various capabilities can be hidden or have quirks for avoiding known troublesome combination of device features as Alex explained here https://lore.kernel.org/linux-pci/20210624151242.ybew2z5rseuusj7v@archlinux/T/#mb67c09a2ce08ce4787652e4c0e7b9e5adf1df57a On the side note as you suggested earlier to cache flr capability earlier the PCI_EXP_DEVCAP reading code won't be there in next version so its just trivial check(dev->has_flr). Thanks, Amey
[+to Alex] On Thu, Jun 24, 2021 at 08:58:09PM +0530, Amey Narkhede wrote: > On 21/06/24 07:23AM, Bjorn Helgaas wrote: > > On Tue, Jun 08, 2021 at 11:18:50AM +0530, Amey Narkhede wrote: > > > Currently there is separate function pcie_has_flr() to probe if pcie flr is > > > supported by the device which does not match the calling convention > > > followed by reset methods which use second function argument to decide > > > whether to probe or not. Add new function pcie_reset_flr() that follows > > > the calling convention of reset methods. > > > > > +/** > > > + * pcie_reset_flr - initiate a PCIe function level reset > > > + * @dev: device to reset > > > + * @probe: If set, only check if the device can be reset this way. > > > + * > > > + * Initiate a function level reset on @dev. > > > + */ > > > +int pcie_reset_flr(struct pci_dev *dev, int probe) > > > +{ > > > + u32 cap; > > > + > > > + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > > > + return -ENOTTY; > > > + > > > + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > > > + if (!(cap & PCI_EXP_DEVCAP_FLR)) > > > + return -ENOTTY; > > > + > > > + if (probe) > > > + return 0; > > > + > > > + return pcie_flr(dev); > > > +} > > > > Tangent: I've been told before, but I can't remember why we need the > > "probe" interface. Since we're looking at this area again, can we add > > a comment to clarify this? > > > > Every time I read this, I wonder why we can't just get rid of the > > probe and attempt a reset. If it fails because it's not supported, we > > could just try the next one in the list. > > Part of the reason is to have same calling convention as other reset > methods and other reason is devices that run in VMs where various > capabilities can be hidden or have quirks for avoiding known troublesome > combination of device features as Alex explained here > https://lore.kernel.org/linux-pci/20210624151242.ybew2z5rseuusj7v@archlinux/T/#mb67c09a2ce08ce4787652e4c0e7b9e5adf1df57a > > On the side note as you suggested earlier to cache flr capability > earlier the PCI_EXP_DEVCAP reading code won't be there in next version > so its just trivial check(dev->has_flr). Sorry, I didn't make my question clear. I'm not asking why we're adding a "probe" argument to pcie_reset_flr() to make it consistent with pci_af_flr(), pci_pm_reset(), pci_parent_bus_reset(), etc. I like making the interfaces consistent. What I'm asking here is why the "probe" argument exists for *any* of these interfaces and why pci_probe_reset_function() exists. This is really more a question for Alex since it's a historical question, not anything directly related to your series. I'm not proposing *removing* the "probe" argument; I know it exists for a reason because I've asked about it before. But I forgot the answer, which makes me think a hint in the code would be useful. Bjorn
On Thu, 24 Jun 2021 11:15:59 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > [+to Alex] > > On Thu, Jun 24, 2021 at 08:58:09PM +0530, Amey Narkhede wrote: > > On 21/06/24 07:23AM, Bjorn Helgaas wrote: > > > On Tue, Jun 08, 2021 at 11:18:50AM +0530, Amey Narkhede wrote: > > > > Currently there is separate function pcie_has_flr() to probe if pcie flr is > > > > supported by the device which does not match the calling convention > > > > followed by reset methods which use second function argument to decide > > > > whether to probe or not. Add new function pcie_reset_flr() that follows > > > > the calling convention of reset methods. > > > > > > > +/** > > > > + * pcie_reset_flr - initiate a PCIe function level reset > > > > + * @dev: device to reset > > > > + * @probe: If set, only check if the device can be reset this way. > > > > + * > > > > + * Initiate a function level reset on @dev. > > > > + */ > > > > +int pcie_reset_flr(struct pci_dev *dev, int probe) > > > > +{ > > > > + u32 cap; > > > > + > > > > + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) > > > > + return -ENOTTY; > > > > + > > > > + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > > > > + if (!(cap & PCI_EXP_DEVCAP_FLR)) > > > > + return -ENOTTY; > > > > + > > > > + if (probe) > > > > + return 0; > > > > + > > > > + return pcie_flr(dev); > > > > +} > > > > > > Tangent: I've been told before, but I can't remember why we need the > > > "probe" interface. Since we're looking at this area again, can we add > > > a comment to clarify this? > > > > > > Every time I read this, I wonder why we can't just get rid of the > > > probe and attempt a reset. If it fails because it's not supported, we > > > could just try the next one in the list. > > > > Part of the reason is to have same calling convention as other reset > > methods and other reason is devices that run in VMs where various > > capabilities can be hidden or have quirks for avoiding known troublesome > > combination of device features as Alex explained here > > https://lore.kernel.org/linux-pci/20210624151242.ybew2z5rseuusj7v@archlinux/T/#mb67c09a2ce08ce4787652e4c0e7b9e5adf1df57a > > > > On the side note as you suggested earlier to cache flr capability > > earlier the PCI_EXP_DEVCAP reading code won't be there in next version > > so its just trivial check(dev->has_flr). > > Sorry, I didn't make my question clear. I'm not asking why we're > adding a "probe" argument to pcie_reset_flr() to make it consistent > with pci_af_flr(), pci_pm_reset(), pci_parent_bus_reset(), etc. I > like making the interfaces consistent. > > What I'm asking here is why the "probe" argument exists for *any* of > these interfaces and why pci_probe_reset_function() exists. > > This is really more a question for Alex since it's a historical > question, not anything directly related to your series. I'm not > proposing *removing* the "probe" argument; I know it exists for a > reason because I've asked about it before. But I forgot the answer, > which makes me think a hint in the code would be useful. Heh [1] That might be what you're recalling, but in that case I was adding exported symbols that allowed probing bus vs slot reset because the scope of affected devices is different. My use case is testing whether the user owns all the affected devices, so it's really not a test-by-doing opportunity. For these single-function scoped resets, as in the reply to [1] pci_probe_reset_function() isn't exported and the only caller is internal PCI code to determine whether to create the 'reset' sysfs attribute. Sure, as it exists today we could reset the device and test whether it worked to get that value, that's what vfio-pci does now before we give the device to the user, but the critical difference is that in the vfio case we always want to flush any state that might be leaked to the user and at device init time, doing so only invites issues. This series obviously expands the scope of probing, we don't just want to know that there's at least one method available to us, but precisely which ones. It's rather impractical to try to reset a function a half dozen different ways on boot for the possibility that the admin might want to manipulate the reset order later. And oh gosh, if we don't cache the methods supported and re-test-by-doing when the attribute is written, let's just not go there. Thanks, Alex [1]https://lore.kernel.org/linux-pci/CAErSpo625CTnxZvy-gmy8VzxT4favF4s=_giU6nGey_N=VwK5A@mail.gmail.com/
diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c index facc8e6bc..15d6c8452 100644 --- a/drivers/crypto/cavium/nitrox/nitrox_main.c +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev) return -ENOMEM; } - /* check flr support */ - if (pcie_has_flr(pdev)) - pcie_flr(pdev); + pcie_reset_flr(pdev, 0); pci_restore_state(pdev); diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 452351025..3bf36924c 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4611,32 +4611,12 @@ int pci_wait_for_pending_transaction(struct pci_dev *dev) } EXPORT_SYMBOL(pci_wait_for_pending_transaction); -/** - * pcie_has_flr - check if a device supports function level resets - * @dev: device to check - * - * Returns true if the device advertises support for PCIe function level - * resets. - */ -bool pcie_has_flr(struct pci_dev *dev) -{ - u32 cap; - - if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) - return false; - - pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); - return cap & PCI_EXP_DEVCAP_FLR; -} -EXPORT_SYMBOL_GPL(pcie_has_flr); - /** * pcie_flr - initiate a PCIe function level reset * @dev: device to reset * - * Initiate a function level reset on @dev. The caller should ensure the - * device supports FLR before calling this function, e.g. by using the - * pcie_has_flr() helper. + * Initiate a function level reset unconditionally on @dev without + * checking any flags and DEVCAP */ int pcie_flr(struct pci_dev *dev) { @@ -4659,6 +4639,31 @@ int pcie_flr(struct pci_dev *dev) } EXPORT_SYMBOL_GPL(pcie_flr); +/** + * pcie_reset_flr - initiate a PCIe function level reset + * @dev: device to reset + * @probe: If set, only check if the device can be reset this way. + * + * Initiate a function level reset on @dev. + */ +int pcie_reset_flr(struct pci_dev *dev, int probe) +{ + u32 cap; + + if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET) + return -ENOTTY; + + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); + if (!(cap & PCI_EXP_DEVCAP_FLR)) + return -ENOTTY; + + if (probe) + return 0; + + return pcie_flr(dev); +} +EXPORT_SYMBOL_GPL(pcie_reset_flr); + static int pci_af_flr(struct pci_dev *dev, int probe) { int pos; @@ -5139,11 +5144,9 @@ int __pci_reset_function_locked(struct pci_dev *dev) rc = pci_dev_specific_reset(dev, 0); if (rc != -ENOTTY) return rc; - if (pcie_has_flr(dev)) { - rc = pcie_flr(dev); - if (rc != -ENOTTY) - return rc; - } + rc = pcie_reset_flr(dev, 0); + if (rc != -ENOTTY) + return rc; rc = pci_af_flr(dev, 0); if (rc != -ENOTTY) return rc; @@ -5174,8 +5177,9 @@ int pci_probe_reset_function(struct pci_dev *dev) rc = pci_dev_specific_reset(dev, 1); if (rc != -ENOTTY) return rc; - if (pcie_has_flr(dev)) - return 0; + rc = pcie_reset_flr(dev, 1); + if (rc != -ENOTTY) + return rc; rc = pci_af_flr(dev, 1); if (rc != -ENOTTY) return rc; diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index ec943cee5..98077595a 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -1405,13 +1405,11 @@ static pci_ers_result_t aer_root_reset(struct pci_dev *dev) } if (type == PCI_EXP_TYPE_RC_EC || type == PCI_EXP_TYPE_RC_END) { - if (pcie_has_flr(dev)) { - rc = pcie_flr(dev); - pci_info(dev, "has been reset (%d)\n", rc); - } else { - pci_info(dev, "not reset (no FLR support)\n"); - rc = -ENOTTY; - } + rc = pcie_reset_flr(dev, 0); + if (!rc) + pci_info(dev, "has been reset\n"); + else + pci_info(dev, "not reset (no FLR support: %d)\n", rc); } else { rc = pci_bus_error_reset(dev); pci_info(dev, "%s Port link has been reset (%d)\n", diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index d85914afe..f977ba79a 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3819,7 +3819,7 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe) u32 cfg; if (dev->class != PCI_CLASS_STORAGE_EXPRESS || - !pcie_has_flr(dev) || !pci_resource_start(dev, 0)) + pcie_reset_flr(dev, 1) || !pci_resource_start(dev, 0)) return -ENOTTY; if (probe) @@ -3888,13 +3888,10 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe) */ static int delay_250ms_after_flr(struct pci_dev *dev, int probe) { - if (!pcie_has_flr(dev)) - return -ENOTTY; + int ret = pcie_reset_flr(dev, probe); if (probe) - return 0; - - pcie_flr(dev); + return ret; msleep(250); diff --git a/include/linux/pci.h b/include/linux/pci.h index c20211e59..20b90c205 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1225,7 +1225,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev, enum pci_bus_speed *speed, enum pcie_link_width *width); void pcie_print_link_status(struct pci_dev *dev); -bool pcie_has_flr(struct pci_dev *dev); +int pcie_reset_flr(struct pci_dev *dev, int probe); int pcie_flr(struct pci_dev *dev); int __pci_reset_function_locked(struct pci_dev *dev); int pci_reset_function(struct pci_dev *dev);