| Message ID | 1414533068.27420.226.camel@ul30vt.home (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Alex Williamson wrote: > On Sat, 2014-10-25 at 08:03 +0200, Andreas Hartmann wrote: >> >> Out of interest: >> Bjorn's patch disables vc save/restore support - and the machine works >> fine again. Why is it needed at all if it seems to work perfectly w/o >> it? What's the additional benefit? Or in other words: What am I missing >> until today :-) ? What would be better? What could I do more? > > > You're right, in the configuration you have the endpoint device has a > Virtual Channel capability but the upstream root port does not. The > spec is not at all clear about defining the endpoints for enabling > Virtual Channel in each type of configuration, but I think that if we > have an upstream port that does not support Virtual Channel, we can skip > the save/restore. Please test the patch below. > > I'm also still completely confused about whether this is a VC > save/restore issue or a bus reset issue. You originally bisected this > back to the VC save/restore patch, but you also found that a manual, > setpci-based bus reset triggered a system hang. With your additional patch posted here: http://article.gmane.org/gmane.linux.kernel.pci/36162 > I believe that > re-ordering the kernel reset mechanisms also triggered this. Since > recent versions of QEMU are going to favor a bus reset over PM reset, I > don't have a lot of confidence that we're actually solving the problem > for you. Please make sure to test with a recent QEMU to be sure we'll > do a bus reset. I'm running qemu 2.1.0 (newest is 2.1.2 - but this shouldn't be a problem) and tested w/ linux 3.17. > diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c > index 7e1304d..6d13d34 100644 > --- a/drivers/pci/vc.c > +++ b/drivers/pci/vc.c > @@ -339,6 +339,25 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos, > return buf ? 0 : len; > } > > +/** > + * pci_vc_needs_save - Determine whether a VC capability needs to be saved > + * @dev: device > + * @id: VC capability ID (VC/VC9/MFVC) > + * > + * In configurations where we have a VC or MFVC capability, but the upstream > + * device does not, we assume that VC save (and therefore restore) is not > + * necessary. The intention is to only do VC save/restore in configuration > + * where it's necessary and hopefully avoid reset issues. > + */ > +static bool pci_vc_needs_save(struct pci_dev *dev, u16 id) > +{ > + if (id == PCI_EXT_CAP_ID_VC9 || pci_is_root_bus(dev->bus) || > + pci_find_ext_capability(dev->bus->self, PCI_EXT_CAP_ID_VC)) > + return true; > + > + return false; > +} > + > static struct { > u16 id; > const char *name; > @@ -362,7 +381,7 @@ int pci_save_vc_state(struct pci_dev *dev) > struct pci_cap_saved_state *save_state; > > pos = pci_find_ext_capability(dev, vc_caps[i].id); > - if (!pos) > + if (!posi || !pci_vc_needs_save(dev, vc_caps[i].id)) ^ This should be most probably !pos (and not !posi - because !posi does through a compile error). > continue; > > save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id); > @@ -422,7 +441,7 @@ void pci_allocate_vc_save_buffers(struct pci_dev *dev) > for (i = 0; i < ARRAY_SIZE(vc_caps); i++) { > int len, pos = pci_find_ext_capability(dev, vc_caps[i].id); > > - if (!pos) > + if (!pos || !pci_vc_needs_save(dev, vc_caps[i].id)) > continue; > > len = pci_vc_do_save_buffer(dev, pos, NULL, false); W/ the above patch, the machine hangs again (w/ qemu and setpci), but w/ Bjorn's patch (and nothing more applied) which disables vc save/restore, the machine just works fine ... . I especially retested this case to be really sure. I'm so sorry. But that's how it behaves here :-( Thanks, regards, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-10-29 at 17:47 +0100, Andreas Hartmann wrote: > Alex Williamson wrote: > > On Sat, 2014-10-25 at 08:03 +0200, Andreas Hartmann wrote: > >> > >> Out of interest: > >> Bjorn's patch disables vc save/restore support - and the machine works > >> fine again. Why is it needed at all if it seems to work perfectly w/o > >> it? What's the additional benefit? Or in other words: What am I missing > >> until today :-) ? What would be better? What could I do more? > > > > > > You're right, in the configuration you have the endpoint device has a > > Virtual Channel capability but the upstream root port does not. The > > spec is not at all clear about defining the endpoints for enabling > > Virtual Channel in each type of configuration, but I think that if we > > have an upstream port that does not support Virtual Channel, we can skip > > the save/restore. Please test the patch below. > > > > I'm also still completely confused about whether this is a VC > > save/restore issue or a bus reset issue. You originally bisected this > > back to the VC save/restore patch, but you also found that a manual, > > setpci-based bus reset triggered a system hang. > > With your additional patch posted here: > http://article.gmane.org/gmane.linux.kernel.pci/36162 Right, a reset via sysfs also triggered it with that patch, but the reset via setpci is independent of any VC save/restore and still hung your box. > > > I believe that > > re-ordering the kernel reset mechanisms also triggered this. Since > > recent versions of QEMU are going to favor a bus reset over PM reset, I > > don't have a lot of confidence that we're actually solving the problem > > for you. Please make sure to test with a recent QEMU to be sure we'll > > do a bus reset. > > I'm running qemu 2.1.0 (newest is 2.1.2 - but this shouldn't be a > problem) and tested w/ linux 3.17. Yep, just want to make sure it's QEMU new enough to do a bus reset and kernel with matching support. > > diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c > > index 7e1304d..6d13d34 100644 > > --- a/drivers/pci/vc.c > > +++ b/drivers/pci/vc.c > > @@ -339,6 +339,25 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos, > > return buf ? 0 : len; > > } > > > > +/** > > + * pci_vc_needs_save - Determine whether a VC capability needs to be saved > > + * @dev: device > > + * @id: VC capability ID (VC/VC9/MFVC) > > + * > > + * In configurations where we have a VC or MFVC capability, but the upstream > > + * device does not, we assume that VC save (and therefore restore) is not > > + * necessary. The intention is to only do VC save/restore in configuration > > + * where it's necessary and hopefully avoid reset issues. > > + */ > > +static bool pci_vc_needs_save(struct pci_dev *dev, u16 id) > > +{ > > + if (id == PCI_EXT_CAP_ID_VC9 || pci_is_root_bus(dev->bus) || > > + pci_find_ext_capability(dev->bus->self, PCI_EXT_CAP_ID_VC)) > > + return true; > > + > > + return false; > > +} > > + > > static struct { > > u16 id; > > const char *name; > > @@ -362,7 +381,7 @@ int pci_save_vc_state(struct pci_dev *dev) > > struct pci_cap_saved_state *save_state; > > > > pos = pci_find_ext_capability(dev, vc_caps[i].id); > > - if (!pos) > > + if (!posi || !pci_vc_needs_save(dev, vc_caps[i].id)) > ^ > This should be most probably !pos (and not !posi - because !posi does > through a compile error). Oops, sorry. > > continue; > > > > save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id); > > @@ -422,7 +441,7 @@ void pci_allocate_vc_save_buffers(struct pci_dev *dev) > > for (i = 0; i < ARRAY_SIZE(vc_caps); i++) { > > int len, pos = pci_find_ext_capability(dev, vc_caps[i].id); > > > > - if (!pos) > > + if (!pos || !pci_vc_needs_save(dev, vc_caps[i].id)) > > continue; > > > > len = pci_vc_do_save_buffer(dev, pos, NULL, false); > > W/ the above patch, the machine hangs again (w/ qemu and setpci), but w/ > Bjorn's patch (and nothing more applied) which disables vc save/restore, > the machine just works fine ... . I especially retested this case to be > really sure. I'm so sorry. But that's how it behaves here :-( Hmm, the intention was that this should effectively do the same thing as Bjorn's patch. The Atheros device (03:00.0) reports a VC capability but the root port above it (00:05.0) does not. The test in pci_vc_needs_save() should therefore be false for all tests within the if() block and the function should return false, causing us to neither allocate a save buffer or perform a save. The restore will automatically be skipped since there's no save buffer. This is what I was afraid of, on one hand you've bisected and proved via patching that the problem is exclusively due to VC save/restore, but we also have testing that indicates that we can't do a bus reset at all on this port. So I re-iterate my confusion that we don't seem to have a good idea which is the problem. Do you have any other devices that you can install in the slot for testing? Maybe if we knew that bus reset is only a problem with the Atheros card then we could blacklist it. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Alex Williamson schrieb: > On Wed, 2014-10-29 at 17:47 +0100, Andreas Hartmann wrote: >> Alex Williamson wrote: >>> On Sat, 2014-10-25 at 08:03 +0200, Andreas Hartmann wrote: >>>> >>>> Out of interest: >>>> Bjorn's patch disables vc save/restore support - and the machine works >>>> fine again. Why is it needed at all if it seems to work perfectly w/o >>>> it? What's the additional benefit? Or in other words: What am I missing >>>> until today :-) ? What would be better? What could I do more? >>> >>> >>> You're right, in the configuration you have the endpoint device has a >>> Virtual Channel capability but the upstream root port does not. The >>> spec is not at all clear about defining the endpoints for enabling >>> Virtual Channel in each type of configuration, but I think that if we >>> have an upstream port that does not support Virtual Channel, we can skip >>> the save/restore. Please test the patch below. >>> >>> I'm also still completely confused about whether this is a VC >>> save/restore issue or a bus reset issue. You originally bisected this >>> back to the VC save/restore patch, but you also found that a manual, >>> setpci-based bus reset triggered a system hang. >> >> With your additional patch posted here: >> http://article.gmane.org/gmane.linux.kernel.pci/36162 > > Right, a reset via sysfs also triggered it with that patch, but the > reset via setpci is independent of any VC save/restore and still hung > your box. > >> >>> I believe that >>> re-ordering the kernel reset mechanisms also triggered this. Since >>> recent versions of QEMU are going to favor a bus reset over PM reset, I >>> don't have a lot of confidence that we're actually solving the problem >>> for you. Please make sure to test with a recent QEMU to be sure we'll >>> do a bus reset. >> >> I'm running qemu 2.1.0 (newest is 2.1.2 - but this shouldn't be a >> problem) and tested w/ linux 3.17. > > Yep, just want to make sure it's QEMU new enough to do a bus reset and > kernel with matching support. > >>> diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c >>> index 7e1304d..6d13d34 100644 >>> --- a/drivers/pci/vc.c >>> +++ b/drivers/pci/vc.c >>> @@ -339,6 +339,25 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos, >>> return buf ? 0 : len; >>> } >>> >>> +/** >>> + * pci_vc_needs_save - Determine whether a VC capability needs to be saved >>> + * @dev: device >>> + * @id: VC capability ID (VC/VC9/MFVC) >>> + * >>> + * In configurations where we have a VC or MFVC capability, but the upstream >>> + * device does not, we assume that VC save (and therefore restore) is not >>> + * necessary. The intention is to only do VC save/restore in configuration >>> + * where it's necessary and hopefully avoid reset issues. >>> + */ >>> +static bool pci_vc_needs_save(struct pci_dev *dev, u16 id) >>> +{ >>> + if (id == PCI_EXT_CAP_ID_VC9 || pci_is_root_bus(dev->bus) || >>> + pci_find_ext_capability(dev->bus->self, PCI_EXT_CAP_ID_VC)) >>> + return true; >>> + >>> + return false; >>> +} >>> + >>> static struct { >>> u16 id; >>> const char *name; >>> @@ -362,7 +381,7 @@ int pci_save_vc_state(struct pci_dev *dev) >>> struct pci_cap_saved_state *save_state; >>> >>> pos = pci_find_ext_capability(dev, vc_caps[i].id); >>> - if (!pos) >>> + if (!posi || !pci_vc_needs_save(dev, vc_caps[i].id)) >> ^ >> This should be most probably !pos (and not !posi - because !posi does >> through a compile error). > > Oops, sorry. > >>> continue; >>> >>> save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id); >>> @@ -422,7 +441,7 @@ void pci_allocate_vc_save_buffers(struct pci_dev *dev) >>> for (i = 0; i < ARRAY_SIZE(vc_caps); i++) { >>> int len, pos = pci_find_ext_capability(dev, vc_caps[i].id); >>> >>> - if (!pos) >>> + if (!pos || !pci_vc_needs_save(dev, vc_caps[i].id)) >>> continue; >>> >>> len = pci_vc_do_save_buffer(dev, pos, NULL, false); >> >> W/ the above patch, the machine hangs again (w/ qemu and setpci), but w/ >> Bjorn's patch (and nothing more applied) which disables vc save/restore, >> the machine just works fine ... . I especially retested this case to be >> really sure. I'm so sorry. But that's how it behaves here :-( > > Hmm, the intention was that this should effectively do the same thing as > Bjorn's patch. The Atheros device (03:00.0) reports a VC capability but > the root port above it (00:05.0) does not. Are you sure, that this patch really works (-> here!) as expected? Would it be possible to add some debug output printing to the actual console (not to log file) to be sure it really works as expected? Maybe some more output to get an idea what's actually going on? Or is it just a timing issue? Thanks, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c index 7e1304d..6d13d34 100644 --- a/drivers/pci/vc.c +++ b/drivers/pci/vc.c @@ -339,6 +339,25 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos, return buf ? 0 : len; } +/** + * pci_vc_needs_save - Determine whether a VC capability needs to be saved + * @dev: device + * @id: VC capability ID (VC/VC9/MFVC) + * + * In configurations where we have a VC or MFVC capability, but the upstream + * device does not, we assume that VC save (and therefore restore) is not + * necessary. The intention is to only do VC save/restore in configuration + * where it's necessary and hopefully avoid reset issues. + */ +static bool pci_vc_needs_save(struct pci_dev *dev, u16 id) +{ + if (id == PCI_EXT_CAP_ID_VC9 || pci_is_root_bus(dev->bus) || + pci_find_ext_capability(dev->bus->self, PCI_EXT_CAP_ID_VC)) + return true; + + return false; +} + static struct { u16 id; const char *name; @@ -362,7 +381,7 @@ int pci_save_vc_state(struct pci_dev *dev) struct pci_cap_saved_state *save_state; pos = pci_find_ext_capability(dev, vc_caps[i].id); - if (!pos) + if (!posi || !pci_vc_needs_save(dev, vc_caps[i].id)) continue; save_state = pci_find_saved_ext_cap(dev, vc_caps[i].id); @@ -422,7 +441,7 @@ void pci_allocate_vc_save_buffers(struct pci_dev *dev) for (i = 0; i < ARRAY_SIZE(vc_caps); i++) { int len, pos = pci_find_ext_capability(dev, vc_caps[i].id); - if (!pos) + if (!pos || !pci_vc_needs_save(dev, vc_caps[i].id)) continue; len = pci_vc_do_save_buffer(dev, pos, NULL, false);