On Fri, Oct 19, 2018 at 03:21:05PM +0200, Jean Delvare wrote:
> On Thu, 18 Oct 2018 15:30:38 +0300, Jarkko Nikula wrote:
> > Allow PCI core to do runtime PM to devices without needing to use dummy
> > runtime PM callback functions if there is no need to do anything device
> > specific beyond PCI device power state management.
> >
> > Implement this by letting core to change device power state during
> > runtime PM transitions even if no callback functions are defined.
>
> Thank you very much for looking into this and providing a fix.
>
> > Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM")
> > Reported-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>
> > ---
> > This is related to my i2c-i801.c fix thread back in June which I completely
> > forgot till now: https://lkml.org/lkml/2018/6/27/642
> > Discussion back then was that it should be handled in the PCI PM instead
> > of having dummy functions in the drivers. I wanted to respin with a
> > patch.
> > ---
> > drivers/pci/pci-driver.c | 16 ++++++----------
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index bef17c3fca67..6185b878ede1 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1239,7 +1239,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > pci_power_t prev = pci_dev->current_state;
> > - int error;
> > + int error = 0;
> >
> > /*
> > * If pci_dev->driver is not set (unbound), we leave the device in D0,
> > @@ -1251,11 +1251,9 @@ static int pci_pm_runtime_suspend(struct device *dev)
> > return 0;
> > }
> >
> > - if (!pm || !pm->runtime_suspend)
> > - return -ENOSYS;
> > -
> > pci_dev->state_saved = false;
> > - error = pm->runtime_suspend(dev);
> > + if (pm && pm->runtime_suspend)
> > + error = pm->runtime_suspend(dev);
> > if (error) {
> > /*
> > * -EBUSY and -EAGAIN is used to request the runtime PM core
>
> Later in this function, pm is dereferenced again. It happens twice in
> the "if (error)" condition where it is currently safe (error can't be
> non-zero if pm->runtime_suspend() has not been called, and obviously
> pm->runtime_suspend() can't have been called if pm was NULL). However
> it also happens later without the condition:
>
> if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
> && pci_dev->current_state != PCI_UNKNOWN) {
> WARN_ONCE(pci_dev->current_state != prev,
> "PCI PM: State of device not saved by %pF\n",
> pm->runtime_suspend);
> return 0;
> }
>
> I am no expert of the PM framework but is there no risk to dereference
> NULL at this point? Or even if pm is non-NULL, pm->runtime_suspend may
> be NULL, leading to a confusing warning message?
>
> More generally, I would feel better if instead of initializing error to
> 0, we would move under the "if (pm && pm->runtime_suspend)" condition
> everything that must not be run if pm->runtime_suspend is not defined.
> That would make the possible code flows a lot clearer.
I agree, this isn't good. Even if it's safe (and I don't think that
second spot is safe), it's too hard to analyze. I'm going to drop
this for now.
Jarkko, I assume a9c8088c7988 ("i2c: i801: Don't restore config
registers on runtime PM"), which you reference as "Fixes:" is what
causes the regression. If you can update the changelog so it mentions
the regression, why it happens, and why this patch fixes it, we'll be
in a better spot to backport it to stable kernels.
Bjorn
