| Message ID | 63b7393731a5708dbbf107055e3fd9801c3c00b3.1476007467.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Sun, 2016-10-09 at 14:46 +0200, Lukas Wunner wrote: > Commit cc7cc02bada8 ("PCI: Query platform firmware for device power > state") augmented struct pci_platform_pm_ops with a ->get_state hook > and > implemented it for acpi_pci_platform_pm, the only pci_platform_pm_ops > existing till v4.7. > > However v4.8 introduced another pci_platform_pm_ops for Intel Mobile > Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid: > Add > Power Management Unit driver"). It is missing the ->get_state hook, > which is fatal since pci_set_platform_pm() enforces its presence. > > Retrofit mid_pci_platform_pm with the missing callback to fix the > breakage. > +Ingo. I guess it should go via tip/x86/urgent tree. > Cc: x86@kernel.org > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Acked-and-tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.co > m> > --- > Changes v1 -> v2: > - Cast return value of intel_mid_pci_get_power_state() to > (__force pci_power_t) to avoid "sparse -D__CHECK_ENDIAN__" warning. > > arch/x86/include/asm/intel-mid.h | 1 + > arch/x86/platform/intel-mid/pwr.c | 19 +++++++++++++++++++ > drivers/pci/pci-mid.c | 6 ++++++ > 3 files changed, 26 insertions(+) > > diff --git a/arch/x86/include/asm/intel-mid.h > b/arch/x86/include/asm/intel-mid.h > index 9d6b097..4511c10 100644 > --- a/arch/x86/include/asm/intel-mid.h > +++ b/arch/x86/include/asm/intel-mid.h > @@ -17,6 +17,7 @@ > > extern int intel_mid_pci_init(void); > extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, > pci_power_t state); > +extern pci_power_t intel_mid_pci_get_power_state(struct pci_dev > *pdev); > > #define INTEL_MID_PWR_LSS_OFFSET 4 > #define INTEL_MID_PWR_LSS_TYPE (1 << 7) > diff --git a/arch/x86/platform/intel-mid/pwr.c > b/arch/x86/platform/intel-mid/pwr.c > index c901a34..cae8d9b 100644 > --- a/arch/x86/platform/intel-mid/pwr.c > +++ b/arch/x86/platform/intel-mid/pwr.c > @@ -260,6 +260,25 @@ int intel_mid_pci_set_power_state(struct pci_dev > *pdev, pci_power_t state) > } > EXPORT_SYMBOL_GPL(intel_mid_pci_set_power_state); > > +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev) > +{ > + struct mid_pwr *pwr = midpwr; > + int id, reg, bit; > + u32 power; > + > + if (!pwr || !pwr->available) > + return PCI_UNKNOWN; > + > + id = intel_mid_pwr_get_lss_id(pdev); > + if (id < 0) > + return PCI_UNKNOWN; > + > + reg = (id * LSS_PWS_BITS) / 32; > + bit = (id * LSS_PWS_BITS) % 32; > + power = mid_pwr_get_state(pwr, reg); > + return (__force pci_power_t)((power >> bit) & 3); > +} > + > int intel_mid_pwr_get_lss_id(struct pci_dev *pdev) > { > int vndr; > diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c > index c878aa7..a8b52dc 100644 > --- a/drivers/pci/pci-mid.c > +++ b/drivers/pci/pci-mid.c > @@ -29,6 +29,11 @@ static int mid_pci_set_power_state(struct pci_dev > *pdev, pci_power_t state) > return intel_mid_pci_set_power_state(pdev, state); > } > > +static pci_power_t mid_pci_get_power_state(struct pci_dev *pdev) > +{ > + return intel_mid_pci_get_power_state(pdev); > +} > + > static pci_power_t mid_pci_choose_state(struct pci_dev *pdev) > { > return PCI_D3hot; > @@ -52,6 +57,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev) > static struct pci_platform_pm_ops mid_pci_platform_pm = { > .is_manageable = mid_pci_power_manageable, > .set_state = mid_pci_set_power_state, > + .get_state = mid_pci_get_power_state, > .choose_state = mid_pci_choose_state, > .sleep_wake = mid_pci_sleep_wake, > .run_wake = mid_pci_run_wake,
* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sun, 2016-10-09 at 14:46 +0200, Lukas Wunner wrote: > > Commit cc7cc02bada8 ("PCI: Query platform firmware for device power > > state") augmented struct pci_platform_pm_ops with a ->get_state hook > > and > > implemented it for acpi_pci_platform_pm, the only pci_platform_pm_ops > > existing till v4.7. > > > > However v4.8 introduced another pci_platform_pm_ops for Intel Mobile > > Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid: > > Add > > Power Management Unit driver"). It is missing the ->get_state hook, > > which is fatal since pci_set_platform_pm() enforces its presence. > > > > Retrofit mid_pci_platform_pm with the missing callback to fix the > > breakage. > > > > +Ingo. > > I guess it should go via tip/x86/urgent tree. Can do, if Bjorn is fine with it as well - the patch is touching drivers/pci/pci-mid.c. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2016-10-11 at 10:37 +0200, Ingo Molnar wrote: > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Sun, 2016-10-09 at 14:46 +0200, Lukas Wunner wrote: > > > > > > Commit cc7cc02bada8 ("PCI: Query platform firmware for device > > > power > > > state") augmented struct pci_platform_pm_ops with a ->get_state > > > hook > > > and > > > implemented it for acpi_pci_platform_pm, the only > > > pci_platform_pm_ops > > > existing till v4.7. > > > > > > However v4.8 introduced another pci_platform_pm_ops for Intel > > > Mobile > > > Internet Devices with commit 5823d0893ec2 ("x86/platform/intel- > > > mid: > > > Add > > > Power Management Unit driver"). It is missing the ->get_state > > > hook, > > > which is fatal since pci_set_platform_pm() enforces its presence. > > > > > > Retrofit mid_pci_platform_pm with the missing callback to fix the > > > breakage. > > > > > > > +Ingo. > > > > I guess it should go via tip/x86/urgent tree. > > Can do, if Bjorn is fine with it as well - the patch is touching > drivers/pci/pci-mid.c. Bjorn, we are at 4.9-rc1 already and it has this regression. Can you, please, ACK it?
On Sun, Oct 09, 2016 at 02:46:29PM +0200, Lukas Wunner wrote: > Commit cc7cc02bada8 ("PCI: Query platform firmware for device power > state") augmented struct pci_platform_pm_ops with a ->get_state hook and > implemented it for acpi_pci_platform_pm, the only pci_platform_pm_ops > existing till v4.7. > > However v4.8 introduced another pci_platform_pm_ops for Intel Mobile > Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid: Add > Power Management Unit driver"). It is missing the ->get_state hook, > which is fatal since pci_set_platform_pm() enforces its presence. I assume Andy tested 5823d0893ec2, so apparently "fatal" here doesn't mean a panic on the MIDs? I'm wondering (1) exactly what the user- visible failure mode is, and (2) whether there's anything we can do to avoid omissions like this in the future. pci_set_platform_pm() does indeed return -EINVAL if it receives a pci_platform_pm_ops with a NULL ops->get_state pointer, but unfortunately neither of the callers checks that return code. > Retrofit mid_pci_platform_pm with the missing callback to fix the > breakage. > > Cc: x86@kernel.org > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Acked-and-tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Fixes: 5823d0893ec2 ("x86/platform/intel-mid: Add Power Management Unit driver") Acked-by: Bjorn Helgaas <bhelgaas@google.com> Sounds like this should be marked for stable, since v4.8 contains 5823d0893ec2 and is apparently broken? I agree this should go via the x86 tree, since that's how 5823d0893ec2 was merged. > --- > Changes v1 -> v2: > - Cast return value of intel_mid_pci_get_power_state() to > (__force pci_power_t) to avoid "sparse -D__CHECK_ENDIAN__" warning. > > arch/x86/include/asm/intel-mid.h | 1 + > arch/x86/platform/intel-mid/pwr.c | 19 +++++++++++++++++++ > drivers/pci/pci-mid.c | 6 ++++++ > 3 files changed, 26 insertions(+) > > diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h > index 9d6b097..4511c10 100644 > --- a/arch/x86/include/asm/intel-mid.h > +++ b/arch/x86/include/asm/intel-mid.h > @@ -17,6 +17,7 @@ > > extern int intel_mid_pci_init(void); > extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state); > +extern pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev); > > #define INTEL_MID_PWR_LSS_OFFSET 4 > #define INTEL_MID_PWR_LSS_TYPE (1 << 7) > diff --git a/arch/x86/platform/intel-mid/pwr.c b/arch/x86/platform/intel-mid/pwr.c > index c901a34..cae8d9b 100644 > --- a/arch/x86/platform/intel-mid/pwr.c > +++ b/arch/x86/platform/intel-mid/pwr.c > @@ -260,6 +260,25 @@ int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state) > } > EXPORT_SYMBOL_GPL(intel_mid_pci_set_power_state); > > +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev) > +{ > + struct mid_pwr *pwr = midpwr; > + int id, reg, bit; > + u32 power; > + > + if (!pwr || !pwr->available) > + return PCI_UNKNOWN; > + > + id = intel_mid_pwr_get_lss_id(pdev); > + if (id < 0) > + return PCI_UNKNOWN; > + > + reg = (id * LSS_PWS_BITS) / 32; > + bit = (id * LSS_PWS_BITS) % 32; > + power = mid_pwr_get_state(pwr, reg); > + return (__force pci_power_t)((power >> bit) & 3); > +} > + > int intel_mid_pwr_get_lss_id(struct pci_dev *pdev) > { > int vndr; > diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c > index c878aa7..a8b52dc 100644 > --- a/drivers/pci/pci-mid.c > +++ b/drivers/pci/pci-mid.c > @@ -29,6 +29,11 @@ static int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state) > return intel_mid_pci_set_power_state(pdev, state); > } > > +static pci_power_t mid_pci_get_power_state(struct pci_dev *pdev) > +{ > + return intel_mid_pci_get_power_state(pdev); > +} > + > static pci_power_t mid_pci_choose_state(struct pci_dev *pdev) > { > return PCI_D3hot; > @@ -52,6 +57,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev) > static struct pci_platform_pm_ops mid_pci_platform_pm = { > .is_manageable = mid_pci_power_manageable, > .set_state = mid_pci_set_power_state, > + .get_state = mid_pci_get_power_state, > .choose_state = mid_pci_choose_state, > .sleep_wake = mid_pci_sleep_wake, > .run_wake = mid_pci_run_wake, > -- > 2.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-10-19 at 10:10 -0500, Bjorn Helgaas wrote: > On Sun, Oct 09, 2016 at 02:46:29PM +0200, Lukas Wunner wrote: > > > > Commit cc7cc02bada8 ("PCI: Query platform firmware for device power > > state") augmented struct pci_platform_pm_ops with a ->get_state hook > > and > > implemented it for acpi_pci_platform_pm, the only > > pci_platform_pm_ops > > existing till v4.7. > > > > However v4.8 introduced another pci_platform_pm_ops for Intel Mobile > > Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid: > > Add > > Power Management Unit driver"). It is missing the ->get_state hook, > > which is fatal since pci_set_platform_pm() enforces its presence. > > I assume Andy tested 5823d0893ec2, so apparently "fatal" here doesn't > mean a panic on the MIDs? I'm wondering (1) exactly what the user- > visible failure mode is, Fatal means it crashes without even a character printed out on serial console and reboot (since watchdog). > and (2) whether there's anything we can do to > avoid omissions like this in the future. It happened because the feature was developed in PCI subsystem namespace (mailing lists, etc.) without knowing anything about new coming users. I have no idea how to avoid this except browsing / grepping linux-next on regular basis when developing either "feature" or "user" of the API in question. > > pci_set_platform_pm() does indeed return -EINVAL if it receives a > pci_platform_pm_ops with a NULL ops->get_state pointer, but > unfortunately neither of the callers checks that return code. Yeah. > > > > > Retrofit mid_pci_platform_pm with the missing callback to fix the > > breakage. > > > > Cc: x86@kernel.org > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > Acked-and-tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel. > > com> > > Fixes: 5823d0893ec2 ("x86/platform/intel-mid: Add Power Management > Unit driver") > Acked-by: Bjorn Helgaas <bhelgaas@google.com> Thanks! > Sounds like this should be marked for stable, since v4.8 contains > 5823d0893ec2 and is apparently broken? At that point there was no "feature" commit in the tree. Perhaps the Fixes tag should point to the "feature" commit instead. Am I right, Lukas?
On Wed, Oct 19, 2016 at 06:48:01PM +0300, Andy Shevchenko wrote: > On Wed, 2016-10-19 at 10:10 -0500, Bjorn Helgaas wrote: > > On Sun, Oct 09, 2016 at 02:46:29PM +0200, Lukas Wunner wrote: > > > Commit cc7cc02bada8 ("PCI: Query platform firmware for device power > > > state") augmented struct pci_platform_pm_ops with a ->get_state hook > > > and > > > implemented it for acpi_pci_platform_pm, the only > > > pci_platform_pm_ops > > > existing till v4.7. > > > > > > However v4.8 introduced another pci_platform_pm_ops for Intel Mobile > > > Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid: > > > Add > > > Power Management Unit driver"). It is missing the ->get_state hook, > > > which is fatal since pci_set_platform_pm() enforces its presence. > > > > I assume Andy tested 5823d0893ec2, so apparently "fatal" here doesn't > > mean a panic on the MIDs? I'm wondering (1) exactly what the user- > > visible failure mode is, > > Fatal means it crashes without even a character printed out on serial > console and reboot (since watchdog). > > > and (2) whether there's anything we can do to > > avoid omissions like this in the future. > > It happened because the feature was developed in PCI subsystem namespace > (mailing lists, etc.) without knowing anything about new coming users. I > have no idea how to avoid this except browsing / grepping linux-next on > regular basis when developing either "feature" or "user" of the API in > question. Basically it was my fault because my development tree lags behind mainline for technical reasons. I use ZFS for my root partition and every new kernel release requires adjustments to the out-of-tree ZoL. So before I can upgrade I have to either wait until the ZoL folks make those adjustments or I have to make them myself (usually I'm too lazy). In this case I didn't rebase my tree on 4.8 until the merge window for 4.9 had already opened, so I noticed the breakage fairly late. However so far only 4.9-rc1 is broken and we still have like 7 weeks to get the fix in. > > > > pci_set_platform_pm() does indeed return -EINVAL if it receives a > > pci_platform_pm_ops with a NULL ops->get_state pointer, but > > unfortunately neither of the callers checks that return code. At least in the case of intel-mid it wouldn't matter if the return code would be checked and passed up the call chain because the ultimate callee, do_initcall_level(), doesn't check the return value of do_one_initcall(). Perhaps the solution would be to not return -EINVAL but to throw a WARN and limp on. That way the user or developer at least gets an error message and knows what's wrong. > > Fixes: 5823d0893ec2 ("x86/platform/intel-mid: Add Power Management > > Unit driver") > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > Sounds like this should be marked for stable, since v4.8 contains > > 5823d0893ec2 and is apparently broken? > > At that point there was no "feature" commit in the tree. Perhaps the > Fixes tag should point to the "feature" commit instead. Am I right, > Lukas? Right, the Fixes tag should be: Fixes: cc7cc02bada8 ("PCI: Query platform firmware for device power state") And this shouldn't get backported to 4.8, the issue is only present in 4.9-rc1 so far. @Ingo: Do you want me to repost with Bjorn's ack and the correct Fixes: tag? Thanks! Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-10-19 at 20:04 +0200, Lukas Wunner wrote: > On Wed, Oct 19, 2016 at 06:48:01PM +0300, Andy Shevchenko wrote: > > On Wed, 2016-10-19 at 10:10 -0500, Bjorn Helgaas wrote: > > > On Sun, Oct 09, 2016 at 02:46:29PM +0200, Lukas Wunner wrote: > However so far only 4.9-rc1 is broken and we still have like 7 weeks > to get the fix in. One week less. :-) > > > Fixes: 5823d0893ec2 ("x86/platform/intel-mid: Add Power Management > > > Unit driver") > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Sounds like this should be marked for stable, since v4.8 contains > > > 5823d0893ec2 and is apparently broken? > > > > At that point there was no "feature" commit in the tree. Perhaps the > > Fixes tag should point to the "feature" commit instead. Am I right, > > Lukas? > > Right, the Fixes tag should be: > > Fixes: cc7cc02bada8 ("PCI: Query platform firmware for device power > state") > > And this shouldn't get backported to 4.8, the issue is only present > in 4.9-rc1 so far. > > @Ingo: Do you want me to repost with Bjorn's ack and the correct > Fixes: tag? I didn't noticed the patch in last urgent pull request, so, I assume you need to send v3.
diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h index 9d6b097..4511c10 100644 --- a/arch/x86/include/asm/intel-mid.h +++ b/arch/x86/include/asm/intel-mid.h @@ -17,6 +17,7 @@ extern int intel_mid_pci_init(void); extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state); +extern pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev); #define INTEL_MID_PWR_LSS_OFFSET 4 #define INTEL_MID_PWR_LSS_TYPE (1 << 7) diff --git a/arch/x86/platform/intel-mid/pwr.c b/arch/x86/platform/intel-mid/pwr.c index c901a34..cae8d9b 100644 --- a/arch/x86/platform/intel-mid/pwr.c +++ b/arch/x86/platform/intel-mid/pwr.c @@ -260,6 +260,25 @@ int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state) } EXPORT_SYMBOL_GPL(intel_mid_pci_set_power_state); +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev) +{ + struct mid_pwr *pwr = midpwr; + int id, reg, bit; + u32 power; + + if (!pwr || !pwr->available) + return PCI_UNKNOWN; + + id = intel_mid_pwr_get_lss_id(pdev); + if (id < 0) + return PCI_UNKNOWN; + + reg = (id * LSS_PWS_BITS) / 32; + bit = (id * LSS_PWS_BITS) % 32; + power = mid_pwr_get_state(pwr, reg); + return (__force pci_power_t)((power >> bit) & 3); +} + int intel_mid_pwr_get_lss_id(struct pci_dev *pdev) { int vndr; diff --git a/drivers/pci/pci-mid.c b/drivers/pci/pci-mid.c index c878aa7..a8b52dc 100644 --- a/drivers/pci/pci-mid.c +++ b/drivers/pci/pci-mid.c @@ -29,6 +29,11 @@ static int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state) return intel_mid_pci_set_power_state(pdev, state); } +static pci_power_t mid_pci_get_power_state(struct pci_dev *pdev) +{ + return intel_mid_pci_get_power_state(pdev); +} + static pci_power_t mid_pci_choose_state(struct pci_dev *pdev) { return PCI_D3hot; @@ -52,6 +57,7 @@ static bool mid_pci_need_resume(struct pci_dev *dev) static struct pci_platform_pm_ops mid_pci_platform_pm = { .is_manageable = mid_pci_power_manageable, .set_state = mid_pci_set_power_state, + .get_state = mid_pci_get_power_state, .choose_state = mid_pci_choose_state, .sleep_wake = mid_pci_sleep_wake, .run_wake = mid_pci_run_wake,
Commit cc7cc02bada8 ("PCI: Query platform firmware for device power state") augmented struct pci_platform_pm_ops with a ->get_state hook and implemented it for acpi_pci_platform_pm, the only pci_platform_pm_ops existing till v4.7. However v4.8 introduced another pci_platform_pm_ops for Intel Mobile Internet Devices with commit 5823d0893ec2 ("x86/platform/intel-mid: Add Power Management Unit driver"). It is missing the ->get_state hook, which is fatal since pci_set_platform_pm() enforces its presence. Retrofit mid_pci_platform_pm with the missing callback to fix the breakage. Cc: x86@kernel.org Signed-off-by: Lukas Wunner <lukas@wunner.de> Acked-and-tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- Changes v1 -> v2: - Cast return value of intel_mid_pci_get_power_state() to (__force pci_power_t) to avoid "sparse -D__CHECK_ENDIAN__" warning. arch/x86/include/asm/intel-mid.h | 1 + arch/x86/platform/intel-mid/pwr.c | 19 +++++++++++++++++++ drivers/pci/pci-mid.c | 6 ++++++ 3 files changed, 26 insertions(+)