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

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