[PATCH] PCI: Delay after FLR of Intel DC P4510 NVMe
Like the Intel DC P3700 NVMe, the Intel P4510 NVMe exhibits a timeout
failure when the driver tries to interact with the device to soon after
an FLR. The same reset quirk the P3700 uses also resolves the failure
for the P4510, so this change introduces the same reset quirk for the
P4510.
Reviewed-by: Alex Williamson
Signed-off-by: Alay Shah
Signed-off-by: Suresh Gumpula
Signed-off-by: Raphael Norwitz
---
drivers/pci/quirks.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..5a8c059b848d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3922,6 +3922,7 @@ static const struct pci_dev_reset_methods
pci_dev_reset_methods[] = {
reset_ivb_igd },
{ PCI_VENDOR_ID_SAMSUNG, 0xa804, nvme_disable_and_flr },
{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
+ { PCI_VENDOR_ID_INTEL, 0x0a54, delay_250ms_after_flr },
{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
reset_chelsio_generic_dev },
{ 0 }
--
2.20.1
Re: [PATCH] PCI: merge slot and bus reset implementations
On Wed, Apr 07, 2021 at 04:37:23PM +0300, Leon Romanovsky wrote:
> On Wed, Apr 07, 2021 at 06:36:01PM +0530, ameynarkhed...@gmail.com wrote:
> > On 21/04/07 03:30PM, Leon Romanovsky wrote:
> > > On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhed...@gmail.com wrote:
> > > > On 21/04/07 10:23AM, Leon Romanovsky wrote:
> > > > > On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> > > > > > On Sun, 4 Apr 2021 11:04:32 +0300
> > > > > > Leon Romanovsky wrote:
> > > > > >
> > > > > > > On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > > > > > > > On Thu, 1 Apr 2021 15:27:37 +0300
> > > > > > > > Leon Romanovsky wrote:
> > > > > > > >
> > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, Raphael Norwitz
> > > > > > > > > wrote:
> > > > > > > > > > Slot resets are bus resets with additional logic to prevent
> > > > > > > > > > a device
> > > > > > > > > > from being removed during the reset. Currently slot and bus
> > > > > > > > > > resets have
> > > > > > > > > > separate implementations in pci.c, complicating higher
> > > > > > > > > > level logic. As
> > > > > > > > > > discussed on the mailing list, they should be combined into
> > > > > > > > > > a generic
> > > > > > > > > > function which performs an SBR. This change adds a function,
> > > > > > > > > > pci_reset_bus_function(), which first attempts a slot reset
> > > > > > > > > > and then
> > > > > > > > > > attempts a bus reset if -ENOTTY is returned, such that
> > > > > > > > > > there is now a
> > > > > > > > > > single device agnostic function to perform an SBR.
> > > > > > > > > >
> > > > > > > > > > This new function is also needed to add SBR reset quirks
> > > > > > > > > > and therefore
> > > > > > > > > > is exposed in pci.h.
> > > > > > > > > >
> > > > > > > > > > Link:
> > > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2021_3_23_911=DwIBAg=s883GpUCOChKOHiocYtGcg=In4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk=dn12ruIb9lwgcFMNKBZzri1m3zoTBFlkHnrF48PChs4=iEm1FGjLlWUpKJQYMwCHc1crraEzAgN10pCzyEzbrWI=
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Suggested-by: Alex Williamson
> > > > > > > > > > Signed-off-by: Amey Narkhede
> > > > > > > > > > Signed-off-by: Raphael Norwitz
> > > > > > > > > > ---
> > > > > > > > > > drivers/pci/pci.c | 17 +
> > > > > > > > > > include/linux/pci.h | 1 +
> > > > > > > > > > 2 files changed, 10 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > > > > > index 16a17215f633..12a91af2ade4 100644
> > > > > > > > > > --- a/drivers/pci/pci.c
> > > > > > > > > > +++ b/drivers/pci/pci.c
> > > > > > > > > > @@ -4982,6 +4982,13 @@ static int
> > > > > > > > > > pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > > > > > > > > return pci_reset_hotplug_slot(dev->slot->hotplug,
> > > > > > > > > > probe);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > > > > > > > +{
> > > > > > > > > > + int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > > > > +
> > > > > > > > > > + return (rc == -ENOTTY) ? pci_parent_bus_reset(dev,
> > > > > > > > > > probe) : rc;
> > > > >
[PATCH v2] PCI: merge slot and bus reset implementations
Slot resets are bus resets with additional logic to prevent a device
from being removed during the reset. Currently slot and bus resets have
separate implementations in pci.c, complicating higher level logic. As
discussed on the mailing list, they should be combined into a generic
function which performs an SBR. This change adds a function,
pci_reset_bus_function(), which first attempts a slot reset and then
attempts a bus reset if -ENOTTY is returned, such that there is now a
single device agnostic function to perform an SBR.
This new function is also needed to add SBR reset quirks and therefore
is exposed in pci.h.
Link: https://lkml.org/lkml/2021/3/23/911
Suggested-by: Alex Williamson
Signed-off-by: Amey Narkhede
Signed-off-by: Raphael Norwitz
---
drivers/pci/pci.c | 19 +++
include/linux/pci.h | 1 +
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 16a17215f633..a8f8dd588d15 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4982,6 +4982,15 @@ static int pci_dev_reset_slot_function(struct pci_dev
*dev, int probe)
return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
}
+int pci_reset_bus_function(struct pci_dev *dev, int probe)
+{
+ int rc = pci_dev_reset_slot_function(dev, probe);
+
+ if (rc != -ENOTTY)
+ return rc;
+ return pci_parent_bus_reset(dev, probe);
+}
+
static void pci_dev_lock(struct pci_dev *dev)
{
pci_cfg_access_lock(dev);
@@ -5102,10 +5111,7 @@ int __pci_reset_function_locked(struct pci_dev *dev)
rc = pci_pm_reset(dev, 0);
if (rc != -ENOTTY)
return rc;
- rc = pci_dev_reset_slot_function(dev, 0);
- if (rc != -ENOTTY)
- return rc;
- return pci_parent_bus_reset(dev, 0);
+ return pci_reset_bus_function(dev, 0);
}
EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
@@ -5135,13 +5141,10 @@ int pci_probe_reset_function(struct pci_dev *dev)
if (rc != -ENOTTY)
return rc;
rc = pci_pm_reset(dev, 1);
- if (rc != -ENOTTY)
- return rc;
- rc = pci_dev_reset_slot_function(dev, 1);
if (rc != -ENOTTY)
return rc;
- return pci_parent_bus_reset(dev, 1);
+ return pci_reset_bus_function(dev, 1);
}
/**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..979d54335ac1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1228,6 +1228,7 @@ int pci_probe_reset_bus(struct pci_bus *bus);
int pci_reset_bus(struct pci_dev *dev);
void pci_reset_secondary_bus(struct pci_dev *dev);
void pcibios_reset_secondary_bus(struct pci_dev *dev);
+int pci_reset_bus_function(struct pci_dev *dev, int probe);
void pci_update_resource(struct pci_dev *dev, int resno);
int __must_check pci_assign_resource(struct pci_dev *dev, int i);
int __must_check pci_reassign_resource(struct pci_dev *dev, int i,
resource_size_t add_size, resource_size_t align);
--
2.20.1
[PATCH] PCI: merge slot and bus reset implementations
Slot resets are bus resets with additional logic to prevent a device
from being removed during the reset. Currently slot and bus resets have
separate implementations in pci.c, complicating higher level logic. As
discussed on the mailing list, they should be combined into a generic
function which performs an SBR. This change adds a function,
pci_reset_bus_function(), which first attempts a slot reset and then
attempts a bus reset if -ENOTTY is returned, such that there is now a
single device agnostic function to perform an SBR.
This new function is also needed to add SBR reset quirks and therefore
is exposed in pci.h.
Link: https://lkml.org/lkml/2021/3/23/911
Suggested-by: Alex Williamson
Signed-off-by: Amey Narkhede
Signed-off-by: Raphael Norwitz
---
drivers/pci/pci.c | 17 +
include/linux/pci.h | 1 +
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 16a17215f633..12a91af2ade4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev
*dev, int probe)
return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
}
+int pci_reset_bus_function(struct pci_dev *dev, int probe)
+{
+ int rc = pci_dev_reset_slot_function(dev, probe);
+
+ return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
+}
+
static void pci_dev_lock(struct pci_dev *dev)
{
pci_cfg_access_lock(dev);
@@ -5102,10 +5109,7 @@ int __pci_reset_function_locked(struct pci_dev *dev)
rc = pci_pm_reset(dev, 0);
if (rc != -ENOTTY)
return rc;
- rc = pci_dev_reset_slot_function(dev, 0);
- if (rc != -ENOTTY)
- return rc;
- return pci_parent_bus_reset(dev, 0);
+ return pci_reset_bus_function(dev, 0);
}
EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
@@ -5135,13 +5139,10 @@ int pci_probe_reset_function(struct pci_dev *dev)
if (rc != -ENOTTY)
return rc;
rc = pci_pm_reset(dev, 1);
- if (rc != -ENOTTY)
- return rc;
- rc = pci_dev_reset_slot_function(dev, 1);
if (rc != -ENOTTY)
return rc;
- return pci_parent_bus_reset(dev, 1);
+ return pci_reset_bus_function(dev, 1);
}
/**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..979d54335ac1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1228,6 +1228,7 @@ int pci_probe_reset_bus(struct pci_bus *bus);
int pci_reset_bus(struct pci_dev *dev);
void pci_reset_secondary_bus(struct pci_dev *dev);
void pcibios_reset_secondary_bus(struct pci_dev *dev);
+int pci_reset_bus_function(struct pci_dev *dev, int probe);
void pci_update_resource(struct pci_dev *dev, int resno);
int __must_check pci_assign_resource(struct pci_dev *dev, int i);
int __must_check pci_reassign_resource(struct pci_dev *dev, int i,
resource_size_t add_size, resource_size_t align);
--
2.20.1
Re: [PATCH 4/4] PCI/sysfs: Allow userspace to query and set device reset mechanism
On Mon, Mar 15, 2021 at 10:29:50AM -0600, Alex Williamson wrote: > On Mon, 15 Mar 2021 21:03:41 +0530 > Amey Narkhede wrote: > > > On 21/03/15 05:07PM, Leon Romanovsky wrote: > > > On Mon, Mar 15, 2021 at 08:34:09AM -0600, Alex Williamson wrote: > > > > On Mon, 15 Mar 2021 14:52:26 +0100 > > > > Pali Rohár wrote: > > > > > > > > > On Monday 15 March 2021 19:13:23 Amey Narkhede wrote: > > > > > > slot reset (pci_dev_reset_slot_function) and secondary bus > > > > > > reset(pci_parent_bus_reset) which I think are hot reset and > > > > > > warm reset respectively. > > > > > > > > > > No. PCI secondary bus reset = PCIe Hot Reset. Slot reset is just > > > > > another > > > > > type of reset, which is currently implemented only for PCIe hot plug > > > > > bridges and for PowerPC PowerNV platform and it just call PCI > > > > > secondary > > > > > bus reset with some other hook. PCIe Warm Reset does not have API in > > > > > kernel and therefore drivers do not export this type of reset via any > > > > > kernel function (yet). > > > > > > > > Warm reset is beyond the scope of this series, but could be implemented > > > > in a compatible way to fit within the pci_reset_fn_methods[] array > > > > defined here. Note that with this series the resets available through > > > > pci_reset_function() and the per device reset attribute is sysfs remain > > > > exactly the same as they are currently. The bus and slot reset > > > > methods used here are limited to devices where only a single function is > > > > affected by the reset, therefore it is not like the patch you proposed > > > > which performed a reset irrespective of the downstream devices. This > > > > series only enables selection of the existing methods. Thanks, > > > > > > Alex, > > > > > > I asked the patch author here [1], but didn't get any response, maybe > > > you can answer me. What is the use case scenario for this functionality? > > > > > > Thanks > > > > > > [1] https://lore.kernel.org/lkml/YE389lAqjJSeTolM@unreal/ > > > > > Sorry for not responding immediately. There were some buggy wifi cards > > which needed FLR explicitly not sure if that behavior is fixed in > > drivers. Also there is use a case at Nutanix but the engineer who > > is involved is on PTO that is why I did not respond immediately as > > I don't know the details yet. > > And more generally, devices continue to have reset issues and we > impose a fixed priority in our ordering. We can and probably should > continue to quirk devices when we find broken resets so that we have > the best default behavior, but it's currently not easy for an end user > to experiment, ie. this reset works, that one doesn't. We might also > have platform issues where a given reset works better on a certain > platform. Exposing a way to test these things might lead to better > quirks. In the case I think Pali was looking for, they wanted a > mechanism to force a bus reset, if this was in reference to a single > function device, this could be accomplished by setting a priority for > that mechanism, which would translate to not only the sysfs reset > attribute, but also the reset mechanism used by vfio-pci. Thanks, > > Alex > To confirm from our end - we have seen many such instances where default reset methods have not worked well on our platform. Debugging these issues is painful in practice, and this interface would make it far easier. Having an interface like this would also help us better communicate the issues we find with upstream. Allowing others to more easily test our (or other entities') findings should give better visibility into which issues apply to the device in general and which are platform specific. In disambiguating the former from the latter, we should be able to better quirk devices for everyone, and in the latter cases, this interface allows for a safer and more elegant solution than any of the current alternatives. CC Alay, Suresh, Shyam and Felipe in case they have anything to add.
Re: [PATCH 0/4] Expose and manage PCI device reset
On Sat, Mar 13, 2021 at 12:10:38AM +0530, Amey Narkhede wrote: > On 21/03/12 11:20AM, Alex Williamson wrote: > > On Fri, 12 Mar 2021 23:04:48 +0530 > > ameynarkhed...@gmail.com wrote: > > > > > From: Amey Narkhede > > > > > > PCI and PCIe devices may support a number of possible reset mechanisms > > > for example Function Level Reset (FLR) provided via Advanced Feature or > > > PCIe capabilities, Power Management reset, bus reset, or device specific > > > reset. > > > Currently the PCI subsystem creates a policy prioritizing these reset > > > methods > > > which provides neither visibility nor control to userspace. > > > > > > Expose the reset methods available per device to userspace, via sysfs > > > and allow an administrative user or device owner to have ability to > > > manage per device reset method priorities or exclusions. > > > This feature aims to allow greater control of a device for use cases > > > as device assignment, where specific device or platform issues may > > > interact poorly with a given reset method, and for which device specific > > > quirks have not been developed. > > > > > > Suggested-by: Alex Williamson > > > Reviewed-by: Alex Williamson > > > Reviewed-by: Raphael Norwitz > > > > Reviews/Acks/Sign-off-by from others (aside from Tested/Reported-by) > > really need to be explicit, IMO. This is a common issue for new > > developers, but it really needs to be more formal. I wouldn't claim to > > be able to speak for Raphael and interpret his comments so far as his > > final seal of approval. > > > > Also in the patches, all Sign-offs/Reviews/Acks need to be above the > > triple dash '---' line. Anything between that line and the beginning > > of the diff is discarded by tools. People will often use that for > > difference between version since it will be discarded on commit. > > Likewise, the cover letter is not committed, so Review-by there are > > generally not done. I generally make my Sign-off last in the chain and > > maintainers will generally add theirs after that. This makes for a > > chain where someone can read up from the bottom to see how this commit > > entered the kernel. Reviews, Acks, and whatnot will therefore usually > > be collected above the author posting the patch. > > > > Since this is a v1 patch and it's likely there will be more revisions, > > rather than send a v2 immediately with corrections, I'd probably just > > reply to the cover letter retracting Raphael's Review-by for him to > > send his own and noting that you'll fix the commit reviews formatting, > > but will wait for a bit for further comments before sending a new > > version. > > > > No big deal, nice work getting it sent out. Thanks, > > > > Alex > > > Raphael sent me the email with > Reviewed-by: Raphael Norwitz that > is why I included it. > So basically in v2 I should reorder tags such that Sign-off will be > the last. Did I get that right? Or am I missing something? > Just to confirm, I did send Reviewed-by: Raphael Norwitz for the latest version and I'm happy to have it on this series. > Thanks, > Amey > > > > Amey Narkhede (4): > > > PCI: Refactor pcie_flr to follow calling convention of other reset > > > methods > > > PCI: Add new bitmap for keeping track of supported reset mechanisms > > > PCI: Remove reset_fn field from pci_dev > > > PCI/sysfs: Allow userspace to query and set device reset mechanism > > > > > > Documentation/ABI/testing/sysfs-bus-pci | 15 ++ > > > drivers/crypto/cavium/nitrox/nitrox_main.c| 4 +- > > > drivers/crypto/qat/qat_common/adf_aer.c | 2 +- > > > drivers/infiniband/hw/hfi1/chip.c | 4 +- > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +- > > > .../ethernet/cavium/liquidio/lio_vf_main.c| 4 +- > > > .../ethernet/cavium/liquidio/octeon_mailbox.c | 2 +- > > > drivers/net/ethernet/freescale/enetc/enetc.c | 2 +- > > > .../ethernet/freescale/enetc/enetc_pci_mdio.c | 2 +- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 +- > > > drivers/pci/pci-sysfs.c | 68 +++- > > > drivers/pci/pci.c | 160 ++ > > > drivers/pci/pci.h | 11 +- > > > drivers/pci/pcie/aer.c| 12 +- > > > drivers/pci/probe.c | 4 +- > > > drivers/pci/quirks.c | 17 +- > > > include/linux/pci.h | 17 +- > > > 17 files changed, 213 insertions(+), 117 deletions(-) > > > > > > -- > > > 2.30.2 > > > > >
Re: [PATCH] Fix typo in irq_domain documentation
Ping - looks like this was never applied? On Tue, Aug 25, 2020 at 6:23 AM Marc Zyngier wrote: > > On 2020-08-19 22:53, Raphael Norwitz wrote: > > The irq_domain documentation states that "Here the interrupt number > > loose all kind of correspondence to hardware interrupt numbers:...". > > It's clear from the context that the author means to use "loses" > > instead > > of "loose". To avoid future confusion, this change fixes the > > aforementioned wording. > > > > Signed-off-by: Raphael Norwitz > > --- > > Documentation/core-api/irq/irq-domain.rst | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/Documentation/core-api/irq/irq-domain.rst > > b/Documentation/core-api/irq/irq-domain.rst > > index 096db12..eba5e41 100644 > > --- a/Documentation/core-api/irq/irq-domain.rst > > +++ b/Documentation/core-api/irq/irq-domain.rst > > @@ -15,7 +15,7 @@ such as GPIO controllers avoid reimplementing > > identical callback > > mechanisms as the IRQ core system by modelling their interrupt > > handlers as irqchips, i.e. in effect cascading interrupt controllers. > > > > -Here the interrupt number loose all kind of correspondence to > > +Here the interrupt number loses all kind of correspondence to > > hardware interrupt numbers: whereas in the past, IRQ numbers could > > be chosen so they matched the hardware IRQ line into the root > > interrupt controller (i.e. the component actually fireing the > > Acked-by: Marc Zyngier > > M. > -- > Jazz is not dead. It just smells funny...
[PATCH] Fix typo in irq_domain documentation
The irq_domain documentation states that "Here the interrupt number loose all kind of correspondence to hardware interrupt numbers:...". It's clear from the context that the author means to use "loses" instead of "loose". To avoid future confusion, this change fixes the aforementioned wording. Signed-off-by: Raphael Norwitz --- Documentation/core-api/irq/irq-domain.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/core-api/irq/irq-domain.rst b/Documentation/core-api/irq/irq-domain.rst index 096db12..eba5e41 100644 --- a/Documentation/core-api/irq/irq-domain.rst +++ b/Documentation/core-api/irq/irq-domain.rst @@ -15,7 +15,7 @@ such as GPIO controllers avoid reimplementing identical callback mechanisms as the IRQ core system by modelling their interrupt handlers as irqchips, i.e. in effect cascading interrupt controllers. -Here the interrupt number loose all kind of correspondence to +Here the interrupt number loses all kind of correspondence to hardware interrupt numbers: whereas in the past, IRQ numbers could be chosen so they matched the hardware IRQ line into the root interrupt controller (i.e. the component actually fireing the -- 2.9.3
