[PATCH] PCI: keep enable status consistent for device without driver

[PATCH] PCI: keep enable status consistent for device without driver

[Yinghai Lu]

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@wilmer.gaast.net>
Bisected-by: Wilmer van der Gaast <kernel@wilmer.gaast.net>
Tested-by: Wilmer van der Gaast <kernel@wilmer.gaast.net>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Zhang Rui <rui.zhang@intel.com>
CC: stable@vger.kernel.org  # v3.10+

---
 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);
+
 	/* if the device was enabled before suspend, reenable */
 	retval = pci_reenable_device(pci_dev);
 	/*

Re: [PATCH] PCI: keep enable status consistent for device without driver

[Bjorn Helgaas]

[+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@wilmer.gaast.net>
> Bisected-by: Wilmer van der Gaast <kernel@wilmer.gaast.net>
> Tested-by: Wilmer van der Gaast <kernel@wilmer.gaast.net>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> CC: stable@vger.kernel.org  # 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.

> +
>  	/* if the device was enabled before suspend, reenable */
>  	retval = pci_reenable_device(pci_dev);
>  	/*

Re: [PATCH] PCI: keep enable status consistent for device without driver

[Pavel Machek]

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@wilmer.gaast.net>
> > Bisected-by: Wilmer van der Gaast <kernel@wilmer.gaast.net>
> > Tested-by: Wilmer van der Gaast <kernel@wilmer.gaast.net>
> > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > CC: stable@vger.kernel.org  # 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

Re: [PATCH] PCI: keep enable status consistent for device without driver

[Yinghai Lu]

On Thu, Jan 15, 2015 at 3:55 PM, Bjorn Helgaas <bhelgaas@google.com> 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@wilmer.gaast.net>
>> Bisected-by: Wilmer van der Gaast <kernel@wilmer.gaast.net>
>> Tested-by: Wilmer van der Gaast <kernel@wilmer.gaast.net>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> CC: stable@vger.kernel.org  # v3.10+
>
> 928bea964827 appeared in v3.12.  Did you mean v3.12+ instead of v3.10+?

Yes.

get confused that Wilmer can not use 3.10 stable kernel.

>
> 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.
>

BIOS set power state correctly, so first suspend/resume will work.

late because setting power state will rely on the enable-cnt, first resume would
not set the power state related correctly, then second suspend/resume will not
work anymore.

will check if we can change to:
check hw status, and then enable the pci device on boot path instead.

Thanks

Yinghai