| Message ID | 20170516001736.4001-3-david.daney@cavium.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 16/05/2017 02:17, David Daney wrote: > Root ports of cn8xxx do not function after bus reset when used with > some e1000e and LSI HBA devices. Add a quirk to prevent bus reset on > these root ports. I understand the bus reset would work along with a variety of other child devices. I guess there is no way to be more accurate and forbid the bus reset only when incompatible child devices are found? > > Signed-off-by: David Daney <david.daney@cavium.com> > --- > drivers/pci/quirks.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 085fb78..02cd847 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -3347,6 +3347,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0032, quirk_no_bus_reset); > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x003c, quirk_no_bus_reset); > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0033, quirk_no_bus_reset); > > +/* > + * Root port on some Cavium CN8xxx chips do not successfully complete > + * a bus reset when used with certain types child devices. Config s/types /types of Thanks Eric > + * space access to the child may quit responding. Flag the root port > + * as not supporting bus reset. > + */ > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset); > + > static void quirk_no_pm_reset(struct pci_dev *dev) > { > /* >
On 05/16/2017 01:14 PM, Auger Eric wrote: > Hi, > > On 16/05/2017 02:17, David Daney wrote: >> Root ports of cn8xxx do not function after bus reset when used with >> some e1000e and LSI HBA devices. Add a quirk to prevent bus reset on >> these root ports. > I understand the bus reset would work along with a variety of other > child devices. I guess there is no way to be more accurate and forbid > the bus reset only when incompatible child devices are found? That's right. Conceptually we have a 2 dimensional array of all possible upstream devices vs. all possible downstream devices. The maintenance burden of correctly populating this and keeping it up to date would be impossible. The e1000e works well, I am sure, with most Intel root ports. We haven't investigated exactly which end of the link is responsible for the failures against the cn8890 root port, but the simplest way forward is to just say it cannot support bus reset. >> >> Signed-off-by: David Daney <david.daney@cavium.com> >> --- >> drivers/pci/quirks.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index 085fb78..02cd847 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -3347,6 +3347,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0032, quirk_no_bus_reset); >> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x003c, quirk_no_bus_reset); >> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0033, quirk_no_bus_reset); >> >> +/* >> + * Root port on some Cavium CN8xxx chips do not successfully complete >> + * a bus reset when used with certain types child devices. Config > s/types /types of Yeah, I need to be more careful. We really need to improve the grammatical analysis capabilities of checkpatch.pl, I think I will lay the blame there. I can resubmit or Bjorn can fix it before committing, I will let him decide. David. > Thanks > > Eric >> + * space access to the child may quit responding. Flag the root port >> + * as not supporting bus reset. >> + */ >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset); >> + >> static void quirk_no_pm_reset(struct pci_dev *dev) >> { >> /* >>
On Tue, 16 May 2017 13:29:58 -0700 David Daney <ddaney@caviumnetworks.com> wrote: > On 05/16/2017 01:14 PM, Auger Eric wrote: > > Hi, > > > > On 16/05/2017 02:17, David Daney wrote: > >> Root ports of cn8xxx do not function after bus reset when used with > >> some e1000e and LSI HBA devices. Add a quirk to prevent bus reset on > >> these root ports. > > I understand the bus reset would work along with a variety of other > > child devices. I guess there is no way to be more accurate and forbid > > the bus reset only when incompatible child devices are found? > > That's right. Conceptually we have a 2 dimensional array of all > possible upstream devices vs. all possible downstream devices. The > maintenance burden of correctly populating this and keeping it up to > date would be impossible. > > The e1000e works well, I am sure, with most Intel root ports. We > haven't investigated exactly which end of the link is responsible for > the failures against the cn8890 root port, but the simplest way forward > is to just say it cannot support bus reset. That's a pretty serious limitation though, you only need to look at the world of graphics cards where the device offers no capability for function level reset and PM reset does nothing useful. The only way we can get the device to a fairly reproducible state is via bus reset. So let's just throw out GPU assignment on this platform as unsupportable. What other sorts of devices are we going to find have inconsistent behavior compared to platforms where bus reset is possible? What's going to be the user experience? Tracking which devices have trouble with bus reset is a problem, but so is limiting access to bus reset where it really is useful. Thanks, Alex > >> > >> Signed-off-by: David Daney <david.daney@cavium.com> > >> --- > >> drivers/pci/quirks.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > >> index 085fb78..02cd847 100644 > >> --- a/drivers/pci/quirks.c > >> +++ b/drivers/pci/quirks.c > >> @@ -3347,6 +3347,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0032, quirk_no_bus_reset); > >> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x003c, quirk_no_bus_reset); > >> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0033, quirk_no_bus_reset); > >> > >> +/* > >> + * Root port on some Cavium CN8xxx chips do not successfully complete > >> + * a bus reset when used with certain types child devices. Config > > s/types /types of > > Yeah, I need to be more careful. We really need to improve the > grammatical analysis capabilities of checkpatch.pl, I think I will lay > the blame there. > > I can resubmit or Bjorn can fix it before committing, I will let him decide. > > David. > > > > Thanks > > > > Eric > >> + * space access to the child may quit responding. Flag the root port > >> + * as not supporting bus reset. > >> + */ > >> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset); > >> + > >> static void quirk_no_pm_reset(struct pci_dev *dev) > >> { > >> /* > >> >
On Tue, 2017-05-16 at 13:29 -0700, David Daney wrote: > We really need to improve the > grammatical analysis capabilities of checkpatch.pl, I think I will lay > the blame there. Uh huh. Best of luck. Submit a patch... ;)
Hahahahahaha :)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 085fb78..02cd847 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3347,6 +3347,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0032, quirk_no_bus_reset); DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x003c, quirk_no_bus_reset); DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ATHEROS, 0x0033, quirk_no_bus_reset); +/* + * Root port on some Cavium CN8xxx chips do not successfully complete + * a bus reset when used with certain types child devices. Config + * space access to the child may quit responding. Flag the root port + * as not supporting bus reset. + */ +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_CAVIUM, 0xa100, quirk_no_bus_reset); + static void quirk_no_pm_reset(struct pci_dev *dev) { /*
Root ports of cn8xxx do not function after bus reset when used with some e1000e and LSI HBA devices. Add a quirk to prevent bus reset on these root ports. Signed-off-by: David Daney <david.daney@cavium.com> --- drivers/pci/quirks.c | 8 ++++++++ 1 file changed, 8 insertions(+)