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@xxxxxxxxxx
> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> > 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@xxxxxxxxxx>
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?
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
