| Message ID | 2282655.IicBMMa6jN@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Looks good to me.
On Sat, Mar 23, 2013 at 03:33:03PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > It turns out that _Lxx control methods provided by some BIOSes clear > the PME Status bit of PCI devices they handle, which means that > pci_acpi_wake_dev() cannot really use that bit to check whether or > not the device has signalled wakeup. > > For this reason, make pci_acpi_wake_dev() always attempt to resume > the device it is called for regardless of the device's PME Status bit > value (that bit still has to be cleared if set at this point, > though). > > Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Should this be marked for stable? I had this issue on 3.7 and 3.8 as well. Sarah > --- > drivers/pci/pci-acpi.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > Index: linux-pm/drivers/pci/pci-acpi.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-acpi.c > +++ linux-pm/drivers/pci/pci-acpi.c > @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl > return; > } > > - if (!pci_dev->pm_cap || !pci_dev->pme_support > - || pci_check_pme_status(pci_dev)) { > - if (pci_dev->pme_poll) > - pci_dev->pme_poll = false; > + /* Clear PME Status if set. */ > + if (pci_dev->pme_support) > + pci_check_pme_status(pci_dev); > > - pci_wakeup_event(pci_dev); > - pm_runtime_resume(&pci_dev->dev); > - } > + if (pci_dev->pme_poll) > + pci_dev->pme_poll = false; > + > + pci_wakeup_event(pci_dev); > + pm_runtime_resume(&pci_dev->dev); > > if (pci_dev->subordinate) > pci_pme_wakeup_bus(pci_dev->subordinate); > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, March 25, 2013 09:45:51 AM Sarah Sharp wrote: > On Sat, Mar 23, 2013 at 03:33:03PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > It turns out that _Lxx control methods provided by some BIOSes clear > > the PME Status bit of PCI devices they handle, which means that > > pci_acpi_wake_dev() cannot really use that bit to check whether or > > not the device has signalled wakeup. > > > > For this reason, make pci_acpi_wake_dev() always attempt to resume > > the device it is called for regardless of the device's PME Status bit > > value (that bit still has to be cleared if set at this point, > > though). > > > > Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Should this be marked for stable? I had this issue on 3.7 and 3.8 as > well. Yes, it probably should, but that's the maintainer's call. Thanks, Rafael > > --- > > drivers/pci/pci-acpi.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > Index: linux-pm/drivers/pci/pci-acpi.c > > =================================================================== > > --- linux-pm.orig/drivers/pci/pci-acpi.c > > +++ linux-pm/drivers/pci/pci-acpi.c > > @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl > > return; > > } > > > > - if (!pci_dev->pm_cap || !pci_dev->pme_support > > - || pci_check_pme_status(pci_dev)) { > > - if (pci_dev->pme_poll) > > - pci_dev->pme_poll = false; > > + /* Clear PME Status if set. */ > > + if (pci_dev->pme_support) > > + pci_check_pme_status(pci_dev); > > > > - pci_wakeup_event(pci_dev); > > - pm_runtime_resume(&pci_dev->dev); > > - } > > + if (pci_dev->pme_poll) > > + pci_dev->pme_poll = false; > > + > > + pci_wakeup_event(pci_dev); > > + pm_runtime_resume(&pci_dev->dev); > > > > if (pci_dev->subordinate) > > pci_pme_wakeup_bus(pci_dev->subordinate); > >
Hi Bjorn, I wonder what you think about the patch below? Rafael On Saturday, March 23, 2013 03:33:03 PM Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > It turns out that _Lxx control methods provided by some BIOSes clear > the PME Status bit of PCI devices they handle, which means that > pci_acpi_wake_dev() cannot really use that bit to check whether or > not the device has signalled wakeup. > > For this reason, make pci_acpi_wake_dev() always attempt to resume > the device it is called for regardless of the device's PME Status bit > value (that bit still has to be cleared if set at this point, > though). > > Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/pci-acpi.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > Index: linux-pm/drivers/pci/pci-acpi.c > =================================================================== > --- linux-pm.orig/drivers/pci/pci-acpi.c > +++ linux-pm/drivers/pci/pci-acpi.c > @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl > return; > } > > - if (!pci_dev->pm_cap || !pci_dev->pme_support > - || pci_check_pme_status(pci_dev)) { > - if (pci_dev->pme_poll) > - pci_dev->pme_poll = false; > + /* Clear PME Status if set. */ > + if (pci_dev->pme_support) > + pci_check_pme_status(pci_dev); > > - pci_wakeup_event(pci_dev); > - pm_runtime_resume(&pci_dev->dev); > - } > + if (pci_dev->pme_poll) > + pci_dev->pme_poll = false; > + > + pci_wakeup_event(pci_dev); > + pm_runtime_resume(&pci_dev->dev); > > if (pci_dev->subordinate) > pci_pme_wakeup_bus(pci_dev->subordinate); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > Hi Bjorn, > > I wonder what you think about the patch below? Seems fine to me (I'm trusting your and Matthew's judgment here since I don't know much about it). Why don't you resend it with Matthew's ack and the appropriate stable tags, and I'll put it in. If you have a URL for a bugzilla or mailing list report of the original problem, that would be good, too. It'd be nice if users and distros could match problem reports with this solution, but I can't tell what the user-visible issue was. I assume that Sarah tested this (or somebody else reproduced the problem and tested the fix)? > On Saturday, March 23, 2013 03:33:03 PM Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> It turns out that _Lxx control methods provided by some BIOSes clear >> the PME Status bit of PCI devices they handle, which means that >> pci_acpi_wake_dev() cannot really use that bit to check whether or >> not the device has signalled wakeup. >> >> For this reason, make pci_acpi_wake_dev() always attempt to resume >> the device it is called for regardless of the device's PME Status bit >> value (that bit still has to be cleared if set at this point, >> though). >> >> Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> drivers/pci/pci-acpi.c | 15 ++++++++------- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> Index: linux-pm/drivers/pci/pci-acpi.c >> =================================================================== >> --- linux-pm.orig/drivers/pci/pci-acpi.c >> +++ linux-pm/drivers/pci/pci-acpi.c >> @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl >> return; >> } >> >> - if (!pci_dev->pm_cap || !pci_dev->pme_support >> - || pci_check_pme_status(pci_dev)) { >> - if (pci_dev->pme_poll) >> - pci_dev->pme_poll = false; >> + /* Clear PME Status if set. */ >> + if (pci_dev->pme_support) >> + pci_check_pme_status(pci_dev); >> >> - pci_wakeup_event(pci_dev); >> - pm_runtime_resume(&pci_dev->dev); >> - } >> + if (pci_dev->pme_poll) >> + pci_dev->pme_poll = false; >> + >> + pci_wakeup_event(pci_dev); >> + pm_runtime_resume(&pci_dev->dev); >> >> if (pci_dev->subordinate) >> pci_pme_wakeup_bus(pci_dev->subordinate); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, March 28, 2013 10:21:30 AM Bjorn Helgaas wrote: > On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > Hi Bjorn, > > > > I wonder what you think about the patch below? > > Seems fine to me (I'm trusting your and Matthew's judgment here since > I don't know much about it). Why don't you resend it with Matthew's > ack and the appropriate stable tags, and I'll put it in. I will, thanks! > If you have > a URL for a bugzilla or mailing list report of the original problem, > that would be good, too. It'd be nice if users and distros could > match problem reports with this solution, but I can't tell what the > user-visible issue was. I assume that Sarah tested this (or somebody > else reproduced the problem and tested the fix)? Sarah reported it to me privately and I'm afraid I don't have any pointers to publicly available mailing list archives etc. Thanks, Rafael > > On Saturday, March 23, 2013 03:33:03 PM Rafael J. Wysocki wrote: > >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > >> It turns out that _Lxx control methods provided by some BIOSes clear > >> the PME Status bit of PCI devices they handle, which means that > >> pci_acpi_wake_dev() cannot really use that bit to check whether or > >> not the device has signalled wakeup. > >> > >> For this reason, make pci_acpi_wake_dev() always attempt to resume > >> the device it is called for regardless of the device's PME Status bit > >> value (that bit still has to be cleared if set at this point, > >> though). > >> > >> Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> > >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> --- > >> drivers/pci/pci-acpi.c | 15 ++++++++------- > >> 1 file changed, 8 insertions(+), 7 deletions(-) > >> > >> Index: linux-pm/drivers/pci/pci-acpi.c > >> =================================================================== > >> --- linux-pm.orig/drivers/pci/pci-acpi.c > >> +++ linux-pm/drivers/pci/pci-acpi.c > >> @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl > >> return; > >> } > >> > >> - if (!pci_dev->pm_cap || !pci_dev->pme_support > >> - || pci_check_pme_status(pci_dev)) { > >> - if (pci_dev->pme_poll) > >> - pci_dev->pme_poll = false; > >> + /* Clear PME Status if set. */ > >> + if (pci_dev->pme_support) > >> + pci_check_pme_status(pci_dev); > >> > >> - pci_wakeup_event(pci_dev); > >> - pm_runtime_resume(&pci_dev->dev); > >> - } > >> + if (pci_dev->pme_poll) > >> + pci_dev->pme_poll = false; > >> + > >> + pci_wakeup_event(pci_dev); > >> + pm_runtime_resume(&pci_dev->dev); > >> > >> if (pci_dev->subordinate) > >> pci_pme_wakeup_bus(pci_dev->subordinate); > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > I speak only for myself. > > Rafael J. Wysocki, Intel Open Source Technology Center.
On Thu, Mar 28, 2013 at 10:41 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Thursday, March 28, 2013 10:21:30 AM Bjorn Helgaas wrote: >> On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> > Hi Bjorn, >> > >> > I wonder what you think about the patch below? >> >> Seems fine to me (I'm trusting your and Matthew's judgment here since >> I don't know much about it). Why don't you resend it with Matthew's >> ack and the appropriate stable tags, and I'll put it in. > > I will, thanks! > >> If you have >> a URL for a bugzilla or mailing list report of the original problem, >> that would be good, too. It'd be nice if users and distros could >> match problem reports with this solution, but I can't tell what the >> user-visible issue was. I assume that Sarah tested this (or somebody >> else reproduced the problem and tested the fix)? > > Sarah reported it to me privately and I'm afraid I don't have any pointers > to publicly available mailing list archives etc. Do you at least have a description of how a user could determine whether he is seeing the problem fixed by this patch? >> > On Saturday, March 23, 2013 03:33:03 PM Rafael J. Wysocki wrote: >> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> >> >> It turns out that _Lxx control methods provided by some BIOSes clear >> >> the PME Status bit of PCI devices they handle, which means that >> >> pci_acpi_wake_dev() cannot really use that bit to check whether or >> >> not the device has signalled wakeup. >> >> >> >> For this reason, make pci_acpi_wake_dev() always attempt to resume >> >> the device it is called for regardless of the device's PME Status bit >> >> value (that bit still has to be cleared if set at this point, >> >> though). >> >> >> >> Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> >> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> --- >> >> drivers/pci/pci-acpi.c | 15 ++++++++------- >> >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> >> >> Index: linux-pm/drivers/pci/pci-acpi.c >> >> =================================================================== >> >> --- linux-pm.orig/drivers/pci/pci-acpi.c >> >> +++ linux-pm/drivers/pci/pci-acpi.c >> >> @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl >> >> return; >> >> } >> >> >> >> - if (!pci_dev->pm_cap || !pci_dev->pme_support >> >> - || pci_check_pme_status(pci_dev)) { >> >> - if (pci_dev->pme_poll) >> >> - pci_dev->pme_poll = false; >> >> + /* Clear PME Status if set. */ >> >> + if (pci_dev->pme_support) >> >> + pci_check_pme_status(pci_dev); >> >> >> >> - pci_wakeup_event(pci_dev); >> >> - pm_runtime_resume(&pci_dev->dev); >> >> - } >> >> + if (pci_dev->pme_poll) >> >> + pci_dev->pme_poll = false; >> >> + >> >> + pci_wakeup_event(pci_dev); >> >> + pm_runtime_resume(&pci_dev->dev); >> >> >> >> if (pci_dev->subordinate) >> >> pci_pme_wakeup_bus(pci_dev->subordinate); >> >> >> >> -- >> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in >> >> the body of a message to majordomo@vger.kernel.org >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- >> > I speak only for myself. >> > Rafael J. Wysocki, Intel Open Source Technology Center. > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, March 28, 2013 10:46:10 AM Bjorn Helgaas wrote: > On Thu, Mar 28, 2013 at 10:41 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Thursday, March 28, 2013 10:21:30 AM Bjorn Helgaas wrote: > >> On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > Hi Bjorn, > >> > > >> > I wonder what you think about the patch below? > >> > >> Seems fine to me (I'm trusting your and Matthew's judgment here since > >> I don't know much about it). Why don't you resend it with Matthew's > >> ack and the appropriate stable tags, and I'll put it in. > > > > I will, thanks! > > > >> If you have > >> a URL for a bugzilla or mailing list report of the original problem, > >> that would be good, too. It'd be nice if users and distros could > >> match problem reports with this solution, but I can't tell what the > >> user-visible issue was. I assume that Sarah tested this (or somebody > >> else reproduced the problem and tested the fix)? > > > > Sarah reported it to me privately and I'm afraid I don't have any pointers > > to publicly available mailing list archives etc. > > Do you at least have a description of how a user could determine > whether he is seeing the problem fixed by this patch? Yeah. For example, when the problem is visible on a USB controller and that controller is runtime-suspended, then plugging a new USB device into one of the controller's ports won't wake the controller up without the patch. I will put that information into the changelog. Thanks, Rafael > >> > On Saturday, March 23, 2013 03:33:03 PM Rafael J. Wysocki wrote: > >> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> >> > >> >> It turns out that _Lxx control methods provided by some BIOSes clear > >> >> the PME Status bit of PCI devices they handle, which means that > >> >> pci_acpi_wake_dev() cannot really use that bit to check whether or > >> >> not the device has signalled wakeup. > >> >> > >> >> For this reason, make pci_acpi_wake_dev() always attempt to resume > >> >> the device it is called for regardless of the device's PME Status bit > >> >> value (that bit still has to be cleared if set at this point, > >> >> though). > >> >> > >> >> Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> > >> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> >> --- > >> >> drivers/pci/pci-acpi.c | 15 ++++++++------- > >> >> 1 file changed, 8 insertions(+), 7 deletions(-) > >> >> > >> >> Index: linux-pm/drivers/pci/pci-acpi.c > >> >> =================================================================== > >> >> --- linux-pm.orig/drivers/pci/pci-acpi.c > >> >> +++ linux-pm/drivers/pci/pci-acpi.c > >> >> @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl > >> >> return; > >> >> } > >> >> > >> >> - if (!pci_dev->pm_cap || !pci_dev->pme_support > >> >> - || pci_check_pme_status(pci_dev)) { > >> >> - if (pci_dev->pme_poll) > >> >> - pci_dev->pme_poll = false; > >> >> + /* Clear PME Status if set. */ > >> >> + if (pci_dev->pme_support) > >> >> + pci_check_pme_status(pci_dev); > >> >> > >> >> - pci_wakeup_event(pci_dev); > >> >> - pm_runtime_resume(&pci_dev->dev); > >> >> - } > >> >> + if (pci_dev->pme_poll) > >> >> + pci_dev->pme_poll = false; > >> >> + > >> >> + pci_wakeup_event(pci_dev); > >> >> + pm_runtime_resume(&pci_dev->dev); > >> >> > >> >> if (pci_dev->subordinate) > >> >> pci_pme_wakeup_bus(pci_dev->subordinate); > >> >> > >> >> -- > >> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > >> >> the body of a message to majordomo@vger.kernel.org > >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > -- > >> > I speak only for myself. > >> > Rafael J. Wysocki, Intel Open Source Technology Center. > > -- > > I speak only for myself. > > Rafael J. Wysocki, Intel Open Source Technology Center. > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki wrote: > On Thursday, March 28, 2013 10:46:10 AM Bjorn Helgaas wrote: >> On Thu, Mar 28, 2013 at 10:41 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >>> On Thursday, March 28, 2013 10:21:30 AM Bjorn Helgaas wrote: >>>> On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >>>>> Hi Bjorn, >>>>> >>>>> I wonder what you think about the patch below? >>>> >>>> Seems fine to me (I'm trusting your and Matthew's judgment here since >>>> I don't know much about it). Why don't you resend it with Matthew's >>>> ack and the appropriate stable tags, and I'll put it in. >>> >>> I will, thanks! >>> >>>> If you have >>>> a URL for a bugzilla or mailing list report of the original problem, >>>> that would be good, too. It'd be nice if users and distros could >>>> match problem reports with this solution, but I can't tell what the >>>> user-visible issue was. I assume that Sarah tested this (or somebody >>>> else reproduced the problem and tested the fix)? >>> >>> Sarah reported it to me privately and I'm afraid I don't have any pointers >>> to publicly available mailing list archives etc. >> >> Do you at least have a description of how a user could determine >> whether he is seeing the problem fixed by this patch? > > Yeah. For example, when the problem is visible on a USB controller and that > controller is runtime-suspended, then plugging a new USB device into one > of the controller's ports won't wake the controller up without the patch. Hi, I am wondering for a week or two why nobody answered any of my bug reports, not even Sarah who asked for more details. I am think the fix is about my report under thread "Re: 3.8.2: xhci port is dead until pcieport PME# goes to disabled" and I really wonder why I wasn't Cc:ed and listed as a reporter provided it is about my report. But I should better wait what Sarah says. ;-) I would have actually same comment for the proposed patches in: Yinghai Lu "Re: [PATCH] PCI: Remove not needed check in disable aspm link" Who tested the bug, if anybody? What change(the fix) in lspci output should one observe? Thank you, Martin > > I will put that information into the changelog. > > Thanks, > Rafael > > >>>>> On Saturday, March 23, 2013 03:33:03 PM Rafael J. Wysocki wrote: >>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>> >>>>>> It turns out that _Lxx control methods provided by some BIOSes clear >>>>>> the PME Status bit of PCI devices they handle, which means that >>>>>> pci_acpi_wake_dev() cannot really use that bit to check whether or >>>>>> not the device has signalled wakeup. >>>>>> >>>>>> For this reason, make pci_acpi_wake_dev() always attempt to resume >>>>>> the device it is called for regardless of the device's PME Status bit >>>>>> value (that bit still has to be cleared if set at this point, >>>>>> though). >>>>>> >>>>>> Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> >>>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>> --- >>>>>> drivers/pci/pci-acpi.c | 15 ++++++++------- >>>>>> 1 file changed, 8 insertions(+), 7 deletions(-) >>>>>> >>>>>> Index: linux-pm/drivers/pci/pci-acpi.c >>>>>> =================================================================== >>>>>> --- linux-pm.orig/drivers/pci/pci-acpi.c >>>>>> +++ linux-pm/drivers/pci/pci-acpi.c >>>>>> @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl >>>>>> return; >>>>>> } >>>>>> >>>>>> - if (!pci_dev->pm_cap || !pci_dev->pme_support >>>>>> - || pci_check_pme_status(pci_dev)) { >>>>>> - if (pci_dev->pme_poll) >>>>>> - pci_dev->pme_poll = false; >>>>>> + /* Clear PME Status if set. */ >>>>>> + if (pci_dev->pme_support) >>>>>> + pci_check_pme_status(pci_dev); >>>>>> >>>>>> - pci_wakeup_event(pci_dev); >>>>>> - pm_runtime_resume(&pci_dev->dev); >>>>>> - } >>>>>> + if (pci_dev->pme_poll) >>>>>> + pci_dev->pme_poll = false; >>>>>> + >>>>>> + pci_wakeup_event(pci_dev); >>>>>> + pm_runtime_resume(&pci_dev->dev); >>>>>> >>>>>> if (pci_dev->subordinate) >>>>>> pci_pme_wakeup_bus(pci_dev->subordinate); >>>>>> >>>>>> -- >>>>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in >>>>>> the body of a message to majordomo@vger.kernel.org >>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> -- >>>>> I speak only for myself. >>>>> Rafael J. Wysocki, Intel Open Source Technology Center. >>> -- >>> I speak only for myself. >>> Rafael J. Wysocki, Intel Open Source Technology Center. >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" 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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 28, 2013 at 11:26 AM, Martin Mokrejs <mmokrejs@fold.natur.cuni.cz> wrote: > > > Rafael J. Wysocki wrote: >> On Thursday, March 28, 2013 10:46:10 AM Bjorn Helgaas wrote: >>> On Thu, Mar 28, 2013 at 10:41 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >>>> On Thursday, March 28, 2013 10:21:30 AM Bjorn Helgaas wrote: >>>>> On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >>>>>> Hi Bjorn, >>>>>> >>>>>> I wonder what you think about the patch below? >>>>> >>>>> Seems fine to me (I'm trusting your and Matthew's judgment here since >>>>> I don't know much about it). Why don't you resend it with Matthew's >>>>> ack and the appropriate stable tags, and I'll put it in. >>>> >>>> I will, thanks! >>>> >>>>> If you have >>>>> a URL for a bugzilla or mailing list report of the original problem, >>>>> that would be good, too. It'd be nice if users and distros could >>>>> match problem reports with this solution, but I can't tell what the >>>>> user-visible issue was. I assume that Sarah tested this (or somebody >>>>> else reproduced the problem and tested the fix)? >>>> >>>> Sarah reported it to me privately and I'm afraid I don't have any pointers >>>> to publicly available mailing list archives etc. >>> >>> Do you at least have a description of how a user could determine >>> whether he is seeing the problem fixed by this patch? >> >> Yeah. For example, when the problem is visible on a USB controller and that >> controller is runtime-suspended, then plugging a new USB device into one >> of the controller's ports won't wake the controller up without the patch. > > Hi, > I am wondering for a week or two why nobody answered any of my bug reports, > not even Sarah who asked for more details. I am think the fix is about my report > under thread "Re: 3.8.2: xhci port is dead until pcieport PME# goes to disabled" > and I really wonder why I wasn't Cc:ed and listed as a reporter provided it is > about my report. But I should better wait what Sarah says. ;-) I haven't forgotten about your hotplug issues, but I've been on vacation for a week and have been working on the similar issue reported by Chris Clayton (https://bugzilla.kernel.org/show_bug.cgi?id=54981) because it seemed a bit more tractable. But I'll get back to yours eventually :) Unfortunately nobody else seems to be jumping in to help, and I can only do so much by myself. I haven't been following your XHCI issue at all, but one thing you might consider is that it's easy for us on the receiving end to be overwhelmed by the sheer volume of information. For me personally, it's more useful to get specific answers to a few questions than it is for me to sort through a lot of speculation and other data. In some cases, "less is more" :) > I would have actually same comment for the proposed patches in: > Yinghai Lu "Re: [PATCH] PCI: Remove not needed check in disable aspm link" > Who tested the bug, if anybody? What change(the fix) in lspci output should one > observe? Yeah, that's one of the things I'm trying to sort out right now. It *was* tested by Roman, according to the changelog, and I think I can dig out the user-visible behavior from the bugzilla (https://bugzilla.kernel.org/show_bug.cgi?id=55211), but I definitely agree -- that patch needs a lot of tender loving care before I apply it. >>>>>> On Saturday, March 23, 2013 03:33:03 PM Rafael J. Wysocki wrote: >>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>>> >>>>>>> It turns out that _Lxx control methods provided by some BIOSes clear >>>>>>> the PME Status bit of PCI devices they handle, which means that >>>>>>> pci_acpi_wake_dev() cannot really use that bit to check whether or >>>>>>> not the device has signalled wakeup. >>>>>>> >>>>>>> For this reason, make pci_acpi_wake_dev() always attempt to resume >>>>>>> the device it is called for regardless of the device's PME Status bit >>>>>>> value (that bit still has to be cleared if set at this point, >>>>>>> though). >>>>>>> >>>>>>> Reported-by: Sarah Sharp <sarah.a.sharp@linux.intel.com> >>>>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>>>>> --- >>>>>>> drivers/pci/pci-acpi.c | 15 ++++++++------- >>>>>>> 1 file changed, 8 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> Index: linux-pm/drivers/pci/pci-acpi.c >>>>>>> =================================================================== >>>>>>> --- linux-pm.orig/drivers/pci/pci-acpi.c >>>>>>> +++ linux-pm/drivers/pci/pci-acpi.c >>>>>>> @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl >>>>>>> return; >>>>>>> } >>>>>>> >>>>>>> - if (!pci_dev->pm_cap || !pci_dev->pme_support >>>>>>> - || pci_check_pme_status(pci_dev)) { >>>>>>> - if (pci_dev->pme_poll) >>>>>>> - pci_dev->pme_poll = false; >>>>>>> + /* Clear PME Status if set. */ >>>>>>> + if (pci_dev->pme_support) >>>>>>> + pci_check_pme_status(pci_dev); >>>>>>> >>>>>>> - pci_wakeup_event(pci_dev); >>>>>>> - pm_runtime_resume(&pci_dev->dev); >>>>>>> - } >>>>>>> + if (pci_dev->pme_poll) >>>>>>> + pci_dev->pme_poll = false; >>>>>>> + >>>>>>> + pci_wakeup_event(pci_dev); >>>>>>> + pm_runtime_resume(&pci_dev->dev); >>>>>>> >>>>>>> if (pci_dev->subordinate) >>>>>>> pci_pme_wakeup_bus(pci_dev->subordinate); >>>>>>> >>>>>>> -- >>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in >>>>>>> the body of a message to majordomo@vger.kernel.org >>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>> -- >>>>>> I speak only for myself. >>>>>> Rafael J. Wysocki, Intel Open Source Technology Center. >>>> -- >>>> I speak only for myself. >>>> Rafael J. Wysocki, Intel Open Source Technology Center. >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" 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-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 28, 2013 at 11:49:05AM -0600, Bjorn Helgaas wrote: > On Thu, Mar 28, 2013 at 11:26 AM, Martin Mokrejs > <mmokrejs@fold.natur.cuni.cz> wrote: > > > > > > Rafael J. Wysocki wrote: > >> On Thursday, March 28, 2013 10:46:10 AM Bjorn Helgaas wrote: > >>> On Thu, Mar 28, 2013 at 10:41 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >>>> On Thursday, March 28, 2013 10:21:30 AM Bjorn Helgaas wrote: > >>>>> On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >>>>> If you have > >>>>> a URL for a bugzilla or mailing list report of the original problem, > >>>>> that would be good, too. It'd be nice if users and distros could > >>>>> match problem reports with this solution, but I can't tell what the > >>>>> user-visible issue was. I assume that Sarah tested this (or somebody > >>>>> else reproduced the problem and tested the fix)? > >>>> > >>>> Sarah reported it to me privately and I'm afraid I don't have any pointers > >>>> to publicly available mailing list archives etc. > >>> > >>> Do you at least have a description of how a user could determine > >>> whether he is seeing the problem fixed by this patch? > >> > >> Yeah. For example, when the problem is visible on a USB controller and that > >> controller is runtime-suspended, then plugging a new USB device into one > >> of the controller's ports won't wake the controller up without the patch. > > > > Hi, > > I am wondering for a week or two why nobody answered any of my bug reports, > > not even Sarah who asked for more details. I am think the fix is about my report > > under thread "Re: 3.8.2: xhci port is dead until pcieport PME# goes to disabled" > > and I really wonder why I wasn't Cc:ed and listed as a reporter provided it is > > about my report. But I should better wait what Sarah says. ;-) Actually, it didn't occur to me that your issue might be related at all. Sure, you can try this patch, on top of Greg's usb-linus branch, and see if it fixes your issue. I just reproduced this on an internal machine, and decided to report it privately to Rafael first, in case the early hardware was just broken. Bjorn, I see that you're encouraging people to have their bugs and symptoms in a bug tracker. I've also been doing that within Intel, in a private JIRA issue tracker. I've been discussing if we can duplicate some bugs or features that don't contain Intel confidential information to a public JIRA at 01.org. I don't really want to use bugzilla.kernel.org because, quite frankly, the interface is archaic, and in the past I've gotten pushback from other devs about tracking "someday" features in there. If you're interested in making bugs and features more traceable, would it help you for us to file scrubbed bugs/features in a public JIRA instance? If so, I'll talk with our admins further. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Bjorn Helgaas wrote: > On Thu, Mar 28, 2013 at 11:26 AM, Martin Mokrejs > <mmokrejs@fold.natur.cuni.cz> wrote: >> >> >> Rafael J. Wysocki wrote: >>> On Thursday, March 28, 2013 10:46:10 AM Bjorn Helgaas wrote: >>>> On Thu, Mar 28, 2013 at 10:41 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >>>>> On Thursday, March 28, 2013 10:21:30 AM Bjorn Helgaas wrote: >>>>>> On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >>>>>>> Hi Bjorn, >>>>>>> >>>>>>> I wonder what you think about the patch below? >>>>>> >>>>>> Seems fine to me (I'm trusting your and Matthew's judgment here since >>>>>> I don't know much about it). Why don't you resend it with Matthew's >>>>>> ack and the appropriate stable tags, and I'll put it in. >>>>> >>>>> I will, thanks! >>>>> >>>>>> If you have >>>>>> a URL for a bugzilla or mailing list report of the original problem, >>>>>> that would be good, too. It'd be nice if users and distros could >>>>>> match problem reports with this solution, but I can't tell what the >>>>>> user-visible issue was. I assume that Sarah tested this (or somebody >>>>>> else reproduced the problem and tested the fix)? >>>>> >>>>> Sarah reported it to me privately and I'm afraid I don't have any pointers >>>>> to publicly available mailing list archives etc. >>>> >>>> Do you at least have a description of how a user could determine >>>> whether he is seeing the problem fixed by this patch? >>> >>> Yeah. For example, when the problem is visible on a USB controller and that >>> controller is runtime-suspended, then plugging a new USB device into one >>> of the controller's ports won't wake the controller up without the patch. >> >> Hi, >> I am wondering for a week or two why nobody answered any of my bug reports, >> not even Sarah who asked for more details. I am think the fix is about my report >> under thread "Re: 3.8.2: xhci port is dead until pcieport PME# goes to disabled" >> and I really wonder why I wasn't Cc:ed and listed as a reporter provided it is >> about my report. But I should better wait what Sarah says. ;-) > > I haven't forgotten about your hotplug issues, but I've been on > vacation for a week and have been working on the similar issue > reported by Chris Clayton > (https://bugzilla.kernel.org/show_bug.cgi?id=54981) because it seemed > a bit more tractable. But I'll get back to yours eventually :) > Unfortunately nobody else seems to be jumping in to help, and I can > only do so much by myself. > > I haven't been following your XHCI issue at all, but one thing you But please do so now. If we are talking about an existing patch it should be possible to say whether what I observed is likely to be fixed by the patch. I will happily discuss then why I loose interrupts in a same way for my rtl8169 network card and why this PME# stuff happens for me only with 3.8 and not 3.7 (unlike what Sarah claims). I am not arguing that something else makes 3.7 be able to wakeup the device and overcome the same bug while "it" is gone from 3.8. I think this should be an easy task for you, pci devs. ;-) > might consider is that it's easy for us on the receiving end to be > overwhelmed by the sheer volume of information. For me personally, > it's more useful to get specific answers to a few questions than it is > for me to sort through a lot of speculation and other data. In some > cases, "less is more" :) Although in theory I agree in real, I can only collect data for you and test a patch. Even when I extracted bits which I found important into emails there was still not much answer. And if there is nobody to go through the data then it is a waste of time. But still, each thread was a different bug and I just thought you will pick any which looks edible. Why Sarah had to fix PCI/ACPI stuff I don't know but yes, that one seemed quite clear. > >> I would have actually same comment for the proposed patches in: >> Yinghai Lu "Re: [PATCH] PCI: Remove not needed check in disable aspm link" >> Who tested the bug, if anybody? What change(the fix) in lspci output should one >> observe? > > Yeah, that's one of the things I'm trying to sort out right now. It > *was* tested by Roman, according to the changelog, and I think I can > dig out the user-visible behavior from the bugzilla > (https://bugzilla.kernel.org/show_bug.cgi?id=55211), but I definitely > agree -- that patch needs a lot of tender loving care before I apply > it. Good. I think all of these relate to the issues I saw, and I don't believe one cannot find the *now described* broken behavior in my test outputs and verbosely explain what went on. Best, Martin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 28, 2013 at 12:23 PM, Sarah Sharp <sarah.a.sharp@linux.intel.com> wrote: > On Thu, Mar 28, 2013 at 11:49:05AM -0600, Bjorn Helgaas wrote: >> On Thu, Mar 28, 2013 at 11:26 AM, Martin Mokrejs >> <mmokrejs@fold.natur.cuni.cz> wrote: >> > >> > >> > Rafael J. Wysocki wrote: >> >> On Thursday, March 28, 2013 10:46:10 AM Bjorn Helgaas wrote: >> >>> On Thu, Mar 28, 2013 at 10:41 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> >>>> On Thursday, March 28, 2013 10:21:30 AM Bjorn Helgaas wrote: >> >>>>> On Thu, Mar 28, 2013 at 6:57 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> >>>>> If you have >> >>>>> a URL for a bugzilla or mailing list report of the original problem, >> >>>>> that would be good, too. It'd be nice if users and distros could >> >>>>> match problem reports with this solution, but I can't tell what the >> >>>>> user-visible issue was. I assume that Sarah tested this (or somebody >> >>>>> else reproduced the problem and tested the fix)? >> >>>> >> >>>> Sarah reported it to me privately and I'm afraid I don't have any pointers >> >>>> to publicly available mailing list archives etc. >> >>> >> >>> Do you at least have a description of how a user could determine >> >>> whether he is seeing the problem fixed by this patch? >> >> >> >> Yeah. For example, when the problem is visible on a USB controller and that >> >> controller is runtime-suspended, then plugging a new USB device into one >> >> of the controller's ports won't wake the controller up without the patch. >> > >> > Hi, >> > I am wondering for a week or two why nobody answered any of my bug reports, >> > not even Sarah who asked for more details. I am think the fix is about my report >> > under thread "Re: 3.8.2: xhci port is dead until pcieport PME# goes to disabled" >> > and I really wonder why I wasn't Cc:ed and listed as a reporter provided it is >> > about my report. But I should better wait what Sarah says. ;-) > > Actually, it didn't occur to me that your issue might be related at all. > Sure, you can try this patch, on top of Greg's usb-linus branch, and see > if it fixes your issue. > > I just reproduced this on an internal machine, and decided to report it > privately to Rafael first, in case the early hardware was just broken. > > Bjorn, I see that you're encouraging people to have their bugs and > symptoms in a bug tracker. I've also been doing that within Intel, in a > private JIRA issue tracker. I've been discussing if we can duplicate > some bugs or features that don't contain Intel confidential information > to a public JIRA at 01.org. I don't really want to use > bugzilla.kernel.org because, quite frankly, the interface is archaic, > and in the past I've gotten pushback from other devs about tracking > "someday" features in there. My main concern is that often there's more information relevant to a change than it makes sense to put in the changelog, so I like to include a URL to that additional info. I don't really care if that's for a mailing list archive, a bugzilla, a JIRA instance, etc. Issue trackers are more convenient than mailing lists for collecting dmesg logs, acpidumps, etc. The archaic bugzilla interface notwithstanding, I'm not sure it would be an improvement to have a collection of dozens of issue trackers controlled by random organizations. I'd rather have a single place and confidence that it will stick around. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas wrote: > On Thu, Mar 28, 2013 at 12:23 PM, Sarah Sharp >> >> Bjorn, I see that you're encouraging people to have their bugs and >> symptoms in a bug tracker. I've also been doing that within Intel, in a >> private JIRA issue tracker. I've been discussing if we can duplicate >> some bugs or features that don't contain Intel confidential information >> to a public JIRA at 01.org. I don't really want to use >> bugzilla.kernel.org because, quite frankly, the interface is archaic, >> and in the past I've gotten pushback from other devs about tracking >> "someday" features in there. > > My main concern is that often there's more information relevant to a > change than it makes sense to put in the changelog, so I like to > include a URL to that additional info. I don't really care if that's > for a mailing list archive, a bugzilla, a JIRA instance, etc. Issue > trackers are more convenient than mailing lists for collecting dmesg > logs, acpidumps, etc. The archaic bugzilla interface notwithstanding, > I'm not sure it would be an improvement to have a collection of dozens > of issue trackers controlled by random organizations. I'd rather have > a single place and confidence that it will stick around. What's wrong with bugzilla? It's nice and more appealing than Jira. From a user perspective I always found Jira ugly. sorry to say that. ;-) Martin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/drivers/pci/pci-acpi.c =================================================================== --- linux-pm.orig/drivers/pci/pci-acpi.c +++ linux-pm/drivers/pci/pci-acpi.c @@ -53,14 +53,15 @@ static void pci_acpi_wake_dev(acpi_handl return; } - if (!pci_dev->pm_cap || !pci_dev->pme_support - || pci_check_pme_status(pci_dev)) { - if (pci_dev->pme_poll) - pci_dev->pme_poll = false; + /* Clear PME Status if set. */ + if (pci_dev->pme_support) + pci_check_pme_status(pci_dev); - pci_wakeup_event(pci_dev); - pm_runtime_resume(&pci_dev->dev); - } + if (pci_dev->pme_poll) + pci_dev->pme_poll = false; + + pci_wakeup_event(pci_dev); + pm_runtime_resume(&pci_dev->dev); if (pci_dev->subordinate) pci_pme_wakeup_bus(pci_dev->subordinate);