[PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Feiyang Chen]

When the current state is already PCI_D0, pci_power_up() will return
0 even though dev->pm_cap is not set. In that case, we should not
read the PCI_PM_CTRL register in pci_set_full_power_state().

Fixes: e200904b275c ("PCI/PM: Split pci_power_up()")
Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
---
 drivers/pci/pci.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0..7e90ab7b47a1 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1242,9 +1242,6 @@ int pci_power_up(struct pci_dev *dev)
 		else
 			dev->current_state = state;
 
-		if (state == PCI_D0)
-			return 0;
-
 		return -EIO;
 	}
 
@@ -1302,8 +1299,12 @@ static int pci_set_full_power_state(struct pci_dev *dev)
 	int ret;
 
 	ret = pci_power_up(dev);
-	if (ret < 0)
+	if (ret < 0) {
+		if (dev->current_state == PCI_D0)
+			return 0;
+
 		return ret;
+	}
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 	dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
-- 
2.39.3

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Anders Roxell]

On Tue, 22 Aug 2023 at 13:55, Feiyang Chen <chenfeiyang@loongson.cn> wrote:
>
> When the current state is already PCI_D0, pci_power_up() will return
> 0 even though dev->pm_cap is not set. In that case, we should not
> read the PCI_PM_CTRL register in pci_set_full_power_state().
>
> Fixes: e200904b275c ("PCI/PM: Split pci_power_up()")
> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
> ---
>  drivers/pci/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 60230da957e0..7e90ab7b47a1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1242,9 +1242,6 @@ int pci_power_up(struct pci_dev *dev)
>                 else
>                         dev->current_state = state;
>
> -               if (state == PCI_D0)
> -                       return 0;
> -
>                 return -EIO;
>         }
>
> @@ -1302,8 +1299,12 @@ static int pci_set_full_power_state(struct pci_dev *dev)
>         int ret;
>
>         ret = pci_power_up(dev);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               if (dev->current_state == PCI_D0)
> +                       return 0;
> +
>                 return ret;
> +       }
>
>         pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>         dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;

In fuction pci_power_up() there's another if-statement
if (state == PCI_D0)
        goto end;

That also will return 0 if need_restore isn't true.
What will happen then?


Would this work?

        ret = pci_power_up(dev);
-       if (ret < 0)
+       if (ret <= 0)
                 return ret;


Cheers,
Anders

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Ilpo Järvinen]

On Tue, 22 Aug 2023, Anders Roxell wrote:

> On Tue, 22 Aug 2023 at 13:55, Feiyang Chen <chenfeiyang@loongson.cn> wrote:
> >
> > When the current state is already PCI_D0, pci_power_up() will return
> > 0 even though dev->pm_cap is not set. In that case, we should not
> > read the PCI_PM_CTRL register in pci_set_full_power_state().
> >
> > Fixes: e200904b275c ("PCI/PM: Split pci_power_up()")
> > Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
> > ---
> >  drivers/pci/pci.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 60230da957e0..7e90ab7b47a1 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1242,9 +1242,6 @@ int pci_power_up(struct pci_dev *dev)
> >                 else
> >                         dev->current_state = state;
> >
> > -               if (state == PCI_D0)
> > -                       return 0;
> > -
> >                 return -EIO;
> >         }
> >
> > @@ -1302,8 +1299,12 @@ static int pci_set_full_power_state(struct pci_dev *dev)
> >         int ret;
> >
> >         ret = pci_power_up(dev);
> > -       if (ret < 0)
> > +       if (ret < 0) {
> > +               if (dev->current_state == PCI_D0)
> > +                       return 0;
> > +
> >                 return ret;
> > +       }
> >
> >         pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> >         dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
> 
> In fuction pci_power_up() there's another if-statement
> if (state == PCI_D0)
>         goto end;
>
> That also will return 0 if need_restore isn't true.
> What will happen then?

That case is only after pci_power_up() has returned because of 
!dev->pm_cap. As such, it looks unrelated to the case this patch is fixing 
which is the read from PCI_PM_CTRL when dev->pm_cap is not there.

> Would this work?
> 
>         ret = pci_power_up(dev);
> -       if (ret < 0)
> +       if (ret <= 0)
>                  return ret;


-- 
 i.

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Ilpo Järvinen]

On Tue, 22 Aug 2023, Feiyang Chen wrote:

> When the current state is already PCI_D0, pci_power_up() will return
> 0 even though dev->pm_cap is not set. In that case, we should not
> read the PCI_PM_CTRL register in pci_set_full_power_state().

IMHO, this is a bit misleading because after this patch, pci_power_up() 
returns always an error if dev->pm_cap is not set.

-- 
 i.


> Fixes: e200904b275c ("PCI/PM: Split pci_power_up()")
> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>
> ---
>  drivers/pci/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 60230da957e0..7e90ab7b47a1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1242,9 +1242,6 @@ int pci_power_up(struct pci_dev *dev)
>  		else
>  			dev->current_state = state;
>  
> -		if (state == PCI_D0)
> -			return 0;
> -
>  		return -EIO;
>  	}
>  
> @@ -1302,8 +1299,12 @@ static int pci_set_full_power_state(struct pci_dev *dev)
>  	int ret;
>  
>  	ret = pci_power_up(dev);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		if (dev->current_state == PCI_D0)
> +			return 0;
> +
>  		return ret;
> +	}
>  
>  	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>  	dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
> 

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Rafael J. Wysocki]

On Tue, Aug 22, 2023 at 3:24 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 22 Aug 2023, Feiyang Chen wrote:
>
> > When the current state is already PCI_D0, pci_power_up() will return
> > 0 even though dev->pm_cap is not set. In that case, we should not
> > read the PCI_PM_CTRL register in pci_set_full_power_state().
>
> IMHO, this is a bit misleading because after this patch, pci_power_up()
> returns always an error if dev->pm_cap is not set.

Yes, it does, but it has 2 callers only and the other one ignores the
return value, so this only matters here.

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Rafael J. Wysocki]

On Tue, Aug 22, 2023 at 1:55 PM Feiyang Chen <chenfeiyang@loongson.cn> wrote:
>
> When the current state is already PCI_D0, pci_power_up() will return
> 0 even though dev->pm_cap is not set. In that case, we should not
> read the PCI_PM_CTRL register in pci_set_full_power_state().
>
> Fixes: e200904b275c ("PCI/PM: Split pci_power_up()")
> Signed-off-by: Feiyang Chen <chenfeiyang@loongson.cn>

Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  drivers/pci/pci.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 60230da957e0..7e90ab7b47a1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1242,9 +1242,6 @@ int pci_power_up(struct pci_dev *dev)
>                 else
>                         dev->current_state = state;
>
> -               if (state == PCI_D0)
> -                       return 0;
> -
>                 return -EIO;
>         }
>
> @@ -1302,8 +1299,12 @@ static int pci_set_full_power_state(struct pci_dev *dev)
>         int ret;
>
>         ret = pci_power_up(dev);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               if (dev->current_state == PCI_D0)
> +                       return 0;
> +
>                 return ret;
> +       }
>
>         pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>         dev->current_state = pmcsr & PCI_PM_CTRL_STATE_MASK;
> --
> 2.39.3
>

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Tue, 22 Aug 2023, Rafael J. Wysocki wrote:

> On Tue, Aug 22, 2023 at 3:24 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Tue, 22 Aug 2023, Feiyang Chen wrote:
> >
> > > When the current state is already PCI_D0, pci_power_up() will return
> > > 0 even though dev->pm_cap is not set. In that case, we should not
> > > read the PCI_PM_CTRL register in pci_set_full_power_state().
> >
> > IMHO, this is a bit misleading because after this patch, pci_power_up()
> > returns always an error if dev->pm_cap is not set.
> 
> Yes, it does, but it has 2 callers only and the other one ignores the
> return value, so this only matters here.

I did only mean that the changelog could be more clear how it achieves 
the desired result (as currently it states opposite of what the code 
does w.r.t. that return value).

I'm not against the approach taken by patch.

-- 
 i.

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Rafael J. Wysocki]

On Wed, Aug 23, 2023 at 9:28 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Tue, 22 Aug 2023, Rafael J. Wysocki wrote:
>
> > On Tue, Aug 22, 2023 at 3:24 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Tue, 22 Aug 2023, Feiyang Chen wrote:
> > >
> > > > When the current state is already PCI_D0, pci_power_up() will return
> > > > 0 even though dev->pm_cap is not set. In that case, we should not
> > > > read the PCI_PM_CTRL register in pci_set_full_power_state().
> > >
> > > IMHO, this is a bit misleading because after this patch, pci_power_up()
> > > returns always an error if dev->pm_cap is not set.
> >
> > Yes, it does, but it has 2 callers only and the other one ignores the
> > return value, so this only matters here.
>
> I did only mean that the changelog could be more clear how it achieves
> the desired result (as currently it states opposite of what the code
> does w.r.t. that return value).

Fair enough.

It looks like the changelog has not been updated since v1.

> I'm not against the approach taken by patch.

Thanks!

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Bjorn Helgaas]

On Wed, Aug 23, 2023 at 02:46:25PM +0200, Rafael J. Wysocki wrote:
> On Wed, Aug 23, 2023 at 9:28 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> > On Tue, 22 Aug 2023, Rafael J. Wysocki wrote:
> > > On Tue, Aug 22, 2023 at 3:24 PM Ilpo Järvinen
> > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > >
> > > > On Tue, 22 Aug 2023, Feiyang Chen wrote:
> > > >
> > > > > When the current state is already PCI_D0, pci_power_up() will return
> > > > > 0 even though dev->pm_cap is not set. In that case, we should not
> > > > > read the PCI_PM_CTRL register in pci_set_full_power_state().
> > > >
> > > > IMHO, this is a bit misleading because after this patch, pci_power_up()
> > > > returns always an error if dev->pm_cap is not set.
> > >
> > > Yes, it does, but it has 2 callers only and the other one ignores the
> > > return value, so this only matters here.
> >
> > I did only mean that the changelog could be more clear how it achieves
> > the desired result (as currently it states opposite of what the code
> > does w.r.t. that return value).
> 
> Fair enough.
> 
> It looks like the changelog has not been updated since v1.
> 
> > I'm not against the approach taken by patch.

Feiyang, would you update the commit log so it matches the code and
post it as a v3?

Bjorn

Re: [PATCH v2] PCI/PM: Only read PCI_PM_CTRL register when available

[Feiyang Chen]

On Thu, Aug 24, 2023 at 4:50 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Aug 23, 2023 at 02:46:25PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Aug 23, 2023 at 9:28 AM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > > On Tue, 22 Aug 2023, Rafael J. Wysocki wrote:
> > > > On Tue, Aug 22, 2023 at 3:24 PM Ilpo Järvinen
> > > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > > >
> > > > > On Tue, 22 Aug 2023, Feiyang Chen wrote:
> > > > >
> > > > > > When the current state is already PCI_D0, pci_power_up() will return
> > > > > > 0 even though dev->pm_cap is not set. In that case, we should not
> > > > > > read the PCI_PM_CTRL register in pci_set_full_power_state().
> > > > >
> > > > > IMHO, this is a bit misleading because after this patch, pci_power_up()
> > > > > returns always an error if dev->pm_cap is not set.
> > > >
> > > > Yes, it does, but it has 2 callers only and the other one ignores the
> > > > return value, so this only matters here.
> > >
> > > I did only mean that the changelog could be more clear how it achieves
> > > the desired result (as currently it states opposite of what the code
> > > does w.r.t. that return value).
> >
> > Fair enough.
> >
> > It looks like the changelog has not been updated since v1.
> >
> > > I'm not against the approach taken by patch.
>
> Feiyang, would you update the commit log so it matches the code and
> post it as a v3?
>

Sure. I will update the commit log.

Thanks,
Feiyang

> Bjorn