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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> 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
> >
