| Message ID | 20230822115514.999111-1-chenfeiyang@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI/PM: Only read PCI_PM_CTRL register when available | expand |
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
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;
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.
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.
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 >
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.
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!
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
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
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;
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(-)