* Re: [PATCH] PCI / PM: Allow runtime PM without callback functions
2018-10-18 12:30 [PATCH] PCI / PM: Allow runtime PM without callback functions Jarkko Nikula
@ 2018-10-18 15:08 ` Rafael J. Wysocki
2018-10-18 21:25 ` Bjorn Helgaas
2018-10-19 13:21 ` Jean Delvare
2 siblings, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2018-10-18 15:08 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Linux PCI, Linux PM, Bjorn Helgaas, Rafael J. Wysocki,
Mika Westerberg, Jean Delvare, Wolfram Sang, Stable
On Thu, Oct 18, 2018 at 2:30 PM Jarkko Nikula
<jarkko.nikula@linux.intel.com> 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.
>
> Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM")
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> 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.
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 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
> @@ -1292,7 +1290,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
>
> static int pci_pm_runtime_resume(struct device *dev)
> {
> - int rc;
> + int rc = 0;
> struct pci_dev *pci_dev = to_pci_dev(dev);
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>
> @@ -1306,14 +1304,12 @@ static int pci_pm_runtime_resume(struct device *dev)
> if (!pci_dev->driver)
> return 0;
>
> - if (!pm || !pm->runtime_resume)
> - return -ENOSYS;
> -
> pci_fixup_device(pci_fixup_resume_early, pci_dev);
> pci_enable_wake(pci_dev, PCI_D0, false);
> pci_fixup_device(pci_fixup_resume, pci_dev);
>
> - rc = pm->runtime_resume(dev);
> + if (pm && pm->runtime_resume)
> + rc = pm->runtime_resume(dev);
>
> pci_dev->runtime_d3cold = false;
>
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] PCI / PM: Allow runtime PM without callback functions
2018-10-18 12:30 [PATCH] PCI / PM: Allow runtime PM without callback functions Jarkko Nikula
2018-10-18 15:08 ` Rafael J. Wysocki
@ 2018-10-18 21:25 ` Bjorn Helgaas
2018-10-19 11:45 ` Jarkko Nikula
2018-10-19 13:21 ` Jean Delvare
2 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2018-10-18 21:25 UTC (permalink / raw)
To: Jarkko Nikula
Cc: linux-pci, linux-pm, Bjorn Helgaas, Rafael J . Wysocki,
Mika Westerberg, Jean Delvare, Wolfram Sang, stable
On Thu, Oct 18, 2018 at 03:30:38PM +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.
>
> Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM")
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: <stable@vger.kernel.org>
I applied this with Rafael's reviewed-by to pci/misc for v4.20,
thanks!
But I dropped the stable tag because if I understand correctly, the
point of this is to avoid the need for SIMPLE_DEV_PM_OPS() in drivers
that don't need to do anything for PM.
That's worthwhile, but it's not transparently obvious that it would
qualify for a stable backport based on this:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst
If there's an argument for adding a stable tag, I'll add it , but that
justification should be explicit in the changelog.
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> 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
> @@ -1292,7 +1290,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
>
> static int pci_pm_runtime_resume(struct device *dev)
> {
> - int rc;
> + int rc = 0;
> struct pci_dev *pci_dev = to_pci_dev(dev);
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>
> @@ -1306,14 +1304,12 @@ static int pci_pm_runtime_resume(struct device *dev)
> if (!pci_dev->driver)
> return 0;
>
> - if (!pm || !pm->runtime_resume)
> - return -ENOSYS;
> -
> pci_fixup_device(pci_fixup_resume_early, pci_dev);
> pci_enable_wake(pci_dev, PCI_D0, false);
> pci_fixup_device(pci_fixup_resume, pci_dev);
>
> - rc = pm->runtime_resume(dev);
> + if (pm && pm->runtime_resume)
> + rc = pm->runtime_resume(dev);
>
> pci_dev->runtime_d3cold = false;
>
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] PCI / PM: Allow runtime PM without callback functions
2018-10-18 21:25 ` Bjorn Helgaas
@ 2018-10-19 11:45 ` Jarkko Nikula
0 siblings, 0 replies; 8+ messages in thread
From: Jarkko Nikula @ 2018-10-19 11:45 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, linux-pm, Bjorn Helgaas, Rafael J . Wysocki,
Mika Westerberg, Jean Delvare, Wolfram Sang, stable
On 10/19/2018 12:25 AM, Bjorn Helgaas wrote:
> On Thu, Oct 18, 2018 at 03:30:38PM +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.
>>
>> Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM")
>> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Cc: <stable@vger.kernel.org>
>
> I applied this with Rafael's reviewed-by to pci/misc for v4.20,
> thanks!
>
> But I dropped the stable tag because if I understand correctly, the
> point of this is to avoid the need for SIMPLE_DEV_PM_OPS() in drivers
> that don't need to do anything for PM.
>
> That's worthwhile, but it's not transparently obvious that it would
> qualify for a stable backport based on this:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst
>
> If there's an argument for adding a stable tag, I'll add it , but that
> justification should be explicit in the changelog.
>
No disagreement here. I was thinking a second or two before sending the
patch may I Cc or not the stable. I went adding it since this actually
fixes a PM regression on i2c-i801 after v4.18. SMBus PCI device doesn't
go to D3 and
/sys/bus/pci/devices/[0000:00:1f.3 etc]/power/runtime_status
shows "error" when runtime PM framework attempts to autosuspend the device.
But given that most of the platforms I have seen don't implement the PM
capabilities for SMBus PCI device, a few do implement though, and we
haven't got any regression reports either so this is not very fatal IMHO.
I definitely don't want to see massive regression from different PCI
systems either. Queueing for v4.20 sounds reasonable and we can ask to
cherry-pick the commit into v4.18 and v4.19 stable kernels if nothing
fatal shows up.
--
Jarkko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] PCI / PM: Allow runtime PM without callback functions
2018-10-18 12:30 [PATCH] PCI / PM: Allow runtime PM without callback functions Jarkko Nikula
2018-10-18 15:08 ` Rafael J. Wysocki
2018-10-18 21:25 ` Bjorn Helgaas
@ 2018-10-19 13:21 ` Jean Delvare
2018-10-20 16:19 ` Bjorn Helgaas
2 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2018-10-19 13:21 UTC (permalink / raw)
To: Jarkko Nikula
Cc: linux-pci, linux-pm, Bjorn Helgaas, Rafael J . Wysocki,
Mika Westerberg, Wolfram Sang, stable
Hi Jarkko,
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@linux.intel.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> 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.
> @@ -1292,7 +1290,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
>
> static int pci_pm_runtime_resume(struct device *dev)
> {
> - int rc;
> + int rc = 0;
> struct pci_dev *pci_dev = to_pci_dev(dev);
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>
> @@ -1306,14 +1304,12 @@ static int pci_pm_runtime_resume(struct device *dev)
> if (!pci_dev->driver)
> return 0;
>
> - if (!pm || !pm->runtime_resume)
> - return -ENOSYS;
> -
> pci_fixup_device(pci_fixup_resume_early, pci_dev);
> pci_enable_wake(pci_dev, PCI_D0, false);
> pci_fixup_device(pci_fixup_resume, pci_dev);
>
> - rc = pm->runtime_resume(dev);
> + if (pm && pm->runtime_resume)
> + rc = pm->runtime_resume(dev);
>
> pci_dev->runtime_d3cold = false;
>
The rest looks good to me.
Thanks,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] PCI / PM: Allow runtime PM without callback functions
2018-10-19 13:21 ` Jean Delvare
@ 2018-10-20 16:19 ` Bjorn Helgaas
2018-10-22 6:04 ` Jarkko Nikula
2018-10-22 6:06 ` Rafael J. Wysocki
0 siblings, 2 replies; 8+ messages in thread
From: Bjorn Helgaas @ 2018-10-20 16:19 UTC (permalink / raw)
To: Jean Delvare
Cc: Jarkko Nikula, linux-pci, linux-pm, Bjorn Helgaas,
Rafael J . Wysocki, Mika Westerberg, Wolfram Sang, stable
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@linux.intel.com>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] PCI / PM: Allow runtime PM without callback functions
2018-10-20 16:19 ` Bjorn Helgaas
@ 2018-10-22 6:04 ` Jarkko Nikula
2018-10-22 6:06 ` Rafael J. Wysocki
1 sibling, 0 replies; 8+ messages in thread
From: Jarkko Nikula @ 2018-10-22 6:04 UTC (permalink / raw)
To: Bjorn Helgaas, Jean Delvare
Cc: linux-pci, linux-pm, Bjorn Helgaas, Rafael J . Wysocki,
Mika Westerberg, Wolfram Sang, stable
On 10/20/2018 07:19 PM, Bjorn Helgaas wrote:
> On Fri, Oct 19, 2018 at 03:21:05PM +0200, Jean Delvare wrote:
>> 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?
>>
Thanks for spotting this! I don't have any excuse why I was so
completely blind.
>> 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.
>
Thanks. I'll cook a better version.
--
Jarkko
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] PCI / PM: Allow runtime PM without callback functions
2018-10-20 16:19 ` Bjorn Helgaas
2018-10-22 6:04 ` Jarkko Nikula
@ 2018-10-22 6:06 ` Rafael J. Wysocki
1 sibling, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2018-10-22 6:06 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jean Delvare, Jarkko Nikula, Linux PCI, Linux PM, Bjorn Helgaas,
Rafael J. Wysocki, Mika Westerberg, Wolfram Sang, Stable
On Sat, Oct 20, 2018 at 6:19 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> 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@linux.intel.com>
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > ---
> > > 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.
Yeah, sorry for missing this.
[Note to self: be more careful with patch reviews in the future.]
Cheers,
Rafael
^ permalink raw reply [flat|nested] 8+ messages in thread