| Message ID | 20220830104913.1620539-1-rajvi.jingar@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [RESEND,v3,1/2] PCI/PM: refactor pci_pm_suspend_noirq() | expand |
Hi Bjorn, On Tue, Aug 30, 2022 at 12:49 PM Rajvi Jingar <rajvi.jingar@linux.intel.com> wrote: > > The state of the device is saved during pci_pm_suspend_noirq(), if it > has not already been saved, regardless of the skip_bus_pm flag value. So > skip_bus_pm check is removed before saving the device state. > > Signed-off-by: Rajvi Jingar <rajvi.jingar@linux.intel.com> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> I have reviewed this and the [2/2] already and they are clear improvements to me. Do you have any concerns regarding any of them? If not, do you want me to pick them up or do you plan to take care of them yourself? > --- > v1->v2: no change > v2->v3: no change > --- > drivers/pci/pci-driver.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > index 49238ddd39ee..1f64de3e5280 100644 > --- a/drivers/pci/pci-driver.c > +++ b/drivers/pci/pci-driver.c > @@ -867,20 +867,14 @@ static int pci_pm_suspend_noirq(struct device *dev) > } > } > > - if (pci_dev->skip_bus_pm) { > + if (!pci_dev->state_saved) { > + pci_save_state(pci_dev); > /* > - * Either the device is a bridge with a child in D0 below it, or > - * the function is running for the second time in a row without > - * going through full resume, which is possible only during > - * suspend-to-idle in a spurious wakeup case. The device should > - * be in D0 at this point, but if it is a bridge, it may be > - * necessary to save its state. > + * If the device is a bridge with a child in D0 below it, it needs to > + * stay in D0, so check skip_bus_pm to avoid putting it into a > + * low-power state in that case. > */ > - if (!pci_dev->state_saved) > - pci_save_state(pci_dev); > - } else if (!pci_dev->state_saved) { > - pci_save_state(pci_dev); > - if (pci_power_manageable(pci_dev)) > + if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev)) > pci_prepare_to_sleep(pci_dev); > } > > -- > 2.25.1 >
On Tue, Aug 30, 2022 at 01:44:43PM +0200, Rafael J. Wysocki wrote: > Hi Bjorn, > > On Tue, Aug 30, 2022 at 12:49 PM Rajvi Jingar > <rajvi.jingar@linux.intel.com> wrote: > > > > The state of the device is saved during pci_pm_suspend_noirq(), if it > > has not already been saved, regardless of the skip_bus_pm flag value. So > > skip_bus_pm check is removed before saving the device state. > > > > Signed-off-by: Rajvi Jingar <rajvi.jingar@linux.intel.com> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > I have reviewed this and the [2/2] already and they are clear > improvements to me. > > Do you have any concerns regarding any of them? Since the log doesn't mention fixing a problem, I guess this one is only a simplification, right? It looks functionally equivalent to me. > If not, do you want me to pick them up or do you plan to take care of > them yourself? Let me take them; I want to at least wrap the comment to align with the rest of the file. > > --- > > v1->v2: no change > > v2->v3: no change Why are we bumping the version numbers if there's truly no change? > > --- > > drivers/pci/pci-driver.c | 18 ++++++------------ > > 1 file changed, 6 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c > > index 49238ddd39ee..1f64de3e5280 100644 > > --- a/drivers/pci/pci-driver.c > > +++ b/drivers/pci/pci-driver.c > > @@ -867,20 +867,14 @@ static int pci_pm_suspend_noirq(struct device *dev) > > } > > } > > > > - if (pci_dev->skip_bus_pm) { > > + if (!pci_dev->state_saved) { > > + pci_save_state(pci_dev); > > /* > > - * Either the device is a bridge with a child in D0 below it, or > > - * the function is running for the second time in a row without > > - * going through full resume, which is possible only during > > - * suspend-to-idle in a spurious wakeup case. The device should > > - * be in D0 at this point, but if it is a bridge, it may be > > - * necessary to save its state. > > + * If the device is a bridge with a child in D0 below it, it needs to > > + * stay in D0, so check skip_bus_pm to avoid putting it into a > > + * low-power state in that case. > > */ > > - if (!pci_dev->state_saved) > > - pci_save_state(pci_dev); > > - } else if (!pci_dev->state_saved) { > > - pci_save_state(pci_dev); > > - if (pci_power_manageable(pci_dev)) > > + if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev)) > > pci_prepare_to_sleep(pci_dev); > > } > > > > -- > > 2.25.1 > >
On Tue, Aug 30, 2022 at 11:17:43AM -0500, Bjorn Helgaas wrote: > On Tue, Aug 30, 2022 at 01:44:43PM +0200, Rafael J. Wysocki wrote: > > On Tue, Aug 30, 2022 at 12:49 PM Rajvi Jingar > > > v1->v2: no change > > > v2->v3: no change > > Why are we bumping the version numbers if there's truly no change? Sorry, ignore this question; I see that patch 2/2 is changing.
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 49238ddd39ee..1f64de3e5280 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -867,20 +867,14 @@ static int pci_pm_suspend_noirq(struct device *dev) } } - if (pci_dev->skip_bus_pm) { + if (!pci_dev->state_saved) { + pci_save_state(pci_dev); /* - * Either the device is a bridge with a child in D0 below it, or - * the function is running for the second time in a row without - * going through full resume, which is possible only during - * suspend-to-idle in a spurious wakeup case. The device should - * be in D0 at this point, but if it is a bridge, it may be - * necessary to save its state. + * If the device is a bridge with a child in D0 below it, it needs to + * stay in D0, so check skip_bus_pm to avoid putting it into a + * low-power state in that case. */ - if (!pci_dev->state_saved) - pci_save_state(pci_dev); - } else if (!pci_dev->state_saved) { - pci_save_state(pci_dev); - if (pci_power_manageable(pci_dev)) + if (!pci_dev->skip_bus_pm && pci_power_manageable(pci_dev)) pci_prepare_to_sleep(pci_dev); }