On Thu 2015-01-15 17:55:59, Bjorn Helgaas wrote:
> [+cc Rafael, Pavel, linux-pm]
>
> On Wed, Jan 14, 2015 at 09:36:48PM -0800, Yinghai Lu wrote:
> > Wilmer reported continuous suspend/resume does not work after
> > commit 928bea964827 ("PCI: Delay enabling bridges until they're needed").
> >
> > For pci bridge without driver, FW enable it already.
> > In pci_pm_resume/pci_pm_reenable_device after first resume
> > will not reenable the device, aka the status is not the same
> > as that before first suspend.
> >
> > Try to update enable status according to register value before
> > calling pci_reenable_device, so we will not miss those pm
> > operation calling for next suspend/resume.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=86421
> > Fixes: 928bea964827 ("PCI: Delay enabling bridges until they're needed")
> > Reported-by: Wilmer van der Gaast <kernel@xxxxxxxxxxxxxxxx>
> > Bisected-by: Wilmer van der Gaast <kernel@xxxxxxxxxxxxxxxx>
> > Tested-by: Wilmer van der Gaast <kernel@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> > Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> > CC: stable@xxxxxxxxxxxxxxx # v3.10+
>
> 928bea964827 appeared in v3.12. Did you mean v3.12+ instead of v3.10+?
>
> I'd really like to get Rafael and Pavel to take a look at this.
>
> > ---
> > drivers/pci/pci-driver.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > Index: linux-2.6/drivers/pci/pci-driver.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/pci-driver.c
> > +++ linux-2.6/drivers/pci/pci-driver.c
> > @@ -519,8 +519,17 @@ static void pci_pm_set_unknown_state(str
> > */
> > static int pci_pm_reenable_device(struct pci_dev *pci_dev)
> > {
> > + u16 cmd;
> > int retval;
> >
> > + /* update enable_cnt according to cmd register */
> > + pci_read_config_word(pci_dev, PCI_COMMAND, &cmd);
> > + if (!pci_dev->is_busmaster && (cmd & PCI_COMMAND_MASTER))
> > + pci_dev->is_busmaster = true;
>
>
> > + if (!pci_is_enabled(pci_dev) &&
> > + (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)))
> > + atomic_inc(&pci_dev->enable_cnt);
>
> This doesn't feel right because we're handling enable_cnt differently here
> than we do on initial boot.
>
> On initial boot, I don't think we set enable_cnt based on whether firmware
> left the IO or MEMORY bits set in the command register. Why should we
> modify enable_cnt based on the command register during resume?
>
> I could certainly believe we should do something during initial boot, too.
> It just seems like we should look at the command register in both places or
> neither place.
>
> I have the same question about is_busmaster above.
Yep, that sounds like strange thing to do.
Adding more cc's.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
