* [RFC][pci/pm] pci config space save restore issues during suspend/resume @ 2020-02-11 5:57 Chen Yu 2020-02-11 13:50 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Chen Yu @ 2020-02-11 5:57 UTC (permalink / raw) To: linux-pci Cc: Bjorn Helgaas, Len Brown, Rafael J. Wysocki, Zhang Rui, Andy Shevchenko, Chen Yu Hi, We found two issues in the code during suspend: 1. Andy Shevchenko found that, the save restore of pci config space might cause potential issue. Current code uses pci_read_config_dword() to read pci config header. However hardware is not obliged to react correctly when trying to read two/three 'adjacent' pci config registers with one dword read. Q1: Should we save/restore the pci config space header according to the PCI spec strictly(pci_read_config_dword() for 32bit, while pci_read_config_word() for 16bits, etc)? 2. The pci config space of some problematic devices(or due to firmware bug) might become inaccessible after resumed from S3(suspend to mem) on VM. Q2: Should we do sanity check on pci config space before saving them? Say, invoke pci_dev_is_present() before suspend, if the pci config space is not sane, bypass the config space saving process, because there's no need to save invalid pci config space. Comments would be appreciated. -- thanks, Chenyu ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][pci/pm] pci config space save restore issues during suspend/resume 2020-02-11 5:57 [RFC][pci/pm] pci config space save restore issues during suspend/resume Chen Yu @ 2020-02-11 13:50 ` Bjorn Helgaas 2020-02-12 6:29 ` Chen Yu 0 siblings, 1 reply; 4+ messages in thread From: Bjorn Helgaas @ 2020-02-11 13:50 UTC (permalink / raw) To: Chen Yu Cc: linux-pci, Len Brown, Rafael J. Wysocki, Zhang Rui, Andy Shevchenko, Chen Yu On Tue, Feb 11, 2020 at 01:57:06PM +0800, Chen Yu wrote: > Hi, > We found two issues in the code during suspend: > > 1. Andy Shevchenko found that, the save restore of pci config space > might cause potential issue. Current code uses > pci_read_config_dword() to read pci config header. However > hardware is not obliged to react correctly when trying to read > two/three 'adjacent' pci config registers with one dword read. > > Q1: Should we save/restore the pci config space header according > to the PCI spec strictly(pci_read_config_dword() for 32bit, > while pci_read_config_word() for 16bits, etc)? I'm sure you know my first question will be for a spec reference for this requirement that we read registers with the correct size :) If there is such a requirement, then of course we should follow it. > 2. The pci config space of some problematic devices(or due to firmware > bug) might become inaccessible after resumed from S3(suspend to > mem) on VM. > > Q2: Should we do sanity check on pci config space before saving > them? Say, invoke pci_dev_is_present() before suspend, if the > pci config space is not sane, bypass the config space saving > process, because there's no need to save invalid pci config > space. I'm not in favor of a sanity check, at least not yet. This sounds like a problem that has not been debugged yet. If the device is broken in some way, maybe a quirk would be appropriate. Otherwise, maybe there's some Linux issue in the resume from S3 path that we should fix. Bjorn ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][pci/pm] pci config space save restore issues during suspend/resume 2020-02-11 13:50 ` Bjorn Helgaas @ 2020-02-12 6:29 ` Chen Yu 2020-02-12 14:16 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ messages in thread From: Chen Yu @ 2020-02-12 6:29 UTC (permalink / raw) To: Bjorn Helgaas Cc: Chen Yu, linux-pci, Len Brown, Rafael J. Wysocki, Zhang Rui, Andy Shevchenko Hi Bjorn, On Tue, Feb 11, 2020 at 07:50:43AM -0600, Bjorn Helgaas wrote: > On Tue, Feb 11, 2020 at 01:57:06PM +0800, Chen Yu wrote: > > Hi, > > We found two issues in the code during suspend: > > > > 1. Andy Shevchenko found that, the save restore of pci config space > > might cause potential issue. Current code uses > > pci_read_config_dword() to read pci config header. However > > hardware is not obliged to react correctly when trying to read > > two/three 'adjacent' pci config registers with one dword read. > > > > Q1: Should we save/restore the pci config space header according > > to the PCI spec strictly(pci_read_config_dword() for 32bit, > > while pci_read_config_word() for 16bits, etc)? > > I'm sure you know my first question will be for a spec reference for > this requirement that we read registers with the correct size :) If > there is such a requirement, then of course we should follow it. > There seems to be no explicit request in the spec that the config space header should be read according to each register's size. And after recheck the PCI Express Base Specification, Rev. 4.0 Version 1.0 pg703, it mentions: "An implemented 64-bit Base Address register consumes two consecutive DWORD locations." It looks like content within a DWORD io space should be adjacent. So pci_read_config_dword() should be applicable. But this is just my understanding, since we have not encountered issues caused by dword reading yet, we might let it be for now. > > 2. The pci config space of some problematic devices(or due to firmware > > bug) might become inaccessible after resumed from S3(suspend to > > mem) on VM. > > > > Q2: Should we do sanity check on pci config space before saving > > them? Say, invoke pci_dev_is_present() before suspend, if the > > pci config space is not sane, bypass the config space saving > > process, because there's no need to save invalid pci config > > space. > > I'm not in favor of a sanity check, at least not yet. This sounds > like a problem that has not been debugged yet. If the device is > broken in some way, maybe a quirk would be appropriate. Otherwise, > maybe there's some Linux issue in the resume from S3 path that we > should fix. Got it. Thanks, Chenyu > > Bjorn ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][pci/pm] pci config space save restore issues during suspend/resume 2020-02-12 6:29 ` Chen Yu @ 2020-02-12 14:16 ` Bjorn Helgaas 0 siblings, 0 replies; 4+ messages in thread From: Bjorn Helgaas @ 2020-02-12 14:16 UTC (permalink / raw) To: Chen Yu Cc: Chen Yu, linux-pci, Len Brown, Rafael J. Wysocki, Zhang Rui, Andy Shevchenko On Wed, Feb 12, 2020 at 02:29:42PM +0800, Chen Yu wrote: > On Tue, Feb 11, 2020 at 07:50:43AM -0600, Bjorn Helgaas wrote: > > On Tue, Feb 11, 2020 at 01:57:06PM +0800, Chen Yu wrote: > > > Hi, > > > We found two issues in the code during suspend: > > > > > > 1. Andy Shevchenko found that, the save restore of pci config space > > > might cause potential issue. Current code uses > > > pci_read_config_dword() to read pci config header. However > > > hardware is not obliged to react correctly when trying to read > > > two/three 'adjacent' pci config registers with one dword read. > > > > > > Q1: Should we save/restore the pci config space header according > > > to the PCI spec strictly(pci_read_config_dword() for 32bit, > > > while pci_read_config_word() for 16bits, etc)? > > > > I'm sure you know my first question will be for a spec reference > > for this requirement that we read registers with the correct size > > :) If there is such a requirement, then of course we should > > follow it. > > > There seems to be no explicit request in the spec that the config > space header should be read according to each register's size. And > after recheck the PCI Express Base Specification, Rev. 4.0 Version > 1.0 pg703, it mentions: "An implemented 64-bit Base Address register > consumes two consecutive DWORD locations." It looks like content > within a DWORD io space should be adjacent. So > pci_read_config_dword() should be applicable. But this is just my > understanding, since we have not encountered issues caused by dword > reading yet, we might let it be for now. > > > > 2. The pci config space of some problematic devices(or due to firmware > > > bug) might become inaccessible after resumed from S3(suspend to > > > mem) on VM. > > > > > > Q2: Should we do sanity check on pci config space before saving > > > them? Say, invoke pci_dev_is_present() before suspend, if the > > > pci config space is not sane, bypass the config space saving > > > process, because there's no need to save invalid pci config > > > space. > > > > I'm not in favor of a sanity check, at least not yet. This sounds > > like a problem that has not been debugged yet. If the device is > > broken in some way, maybe a quirk would be appropriate. Otherwise, > > maybe there's some Linux issue in the resume from S3 path that we > > should fix. > > Got it. OK. Obviously you're tripping over some problem, but you haven't given any details about what it is, so I don't think there's anything I can help with yet. When you have a potential workaround patch or more details, don't hesitate to post them! Bjorn ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-12 14:16 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-11 5:57 [RFC][pci/pm] pci config space save restore issues during suspend/resume Chen Yu 2020-02-11 13:50 ` Bjorn Helgaas 2020-02-12 6:29 ` Chen Yu 2020-02-12 14:16 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).