| Message ID | 1415151063-2530-1-git-send-email-chuansheng.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Nov 4, 2014 at 6:31 PM, Chuansheng Liu <chuansheng.liu@intel.com> wrote: > The JMicron chip 361/363/368 contains one SATA controller and > one PATA controller, they are brother-relation ship in PCI tree, > but for powering on these both controller, we must follow the > sequence one by one, otherwise one of them can not be powered on > successfully. This should mention what's broken and what problem a user would see. This changelog sounds a lot like the one for e6b7e41cdd8c, so I don't know if this is for a new, related problem, or what. > So here we disable the async suspend method for Jmicron chip. > > Bug link: > https://bugzilla.kernel.org/show_bug.cgi?id=81551 > https://bugzilla.kernel.org/show_bug.cgi?id=84861 > > And we can revert the below commit after this patch is applied: > e6b7e41(ata: Disabling the async PM for JMicron chip 363/361) > > Cc: stable@vger.kernel.org # 3.15+ > Acked-by: Aaron Lu <aaron.lu@intel.com> > Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com> > --- > drivers/pci/pci.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 625a4ac..53128f0 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2046,7 +2046,17 @@ void pci_pm_init(struct pci_dev *dev) > pm_runtime_forbid(&dev->dev); > pm_runtime_set_active(&dev->dev); > pm_runtime_enable(&dev->dev); > - device_enable_async_suspend(&dev->dev); > + > + /* > + * The JMicron chip 361/363/368 contains one SATA controller and > + * one PATA controller, they are brother-relation ship in PCI tree, > + * but for powering on these both controller, we must follow the > + * sequence one by one, otherwise one of them can not be powered on > + * successfully, so here we disable the async suspend method for > + * Jmicron chip. > + */ > + if (dev->vendor != PCI_VENDOR_ID_JMICRON) > + device_enable_async_suspend(&dev->dev); I don't like littering the core PCI code with vendor tests like this. This would be the only one, except for an ancient DECchip 21050 bridge erratum. And why would we want a test for *all* JMicron devices here, when you claim the problem only affects a few specific ones? And what's the story with the e6b7e41cdd8c ("ata: Disabling the async PM for JMicron chip 363/361") connection? Is something broken even with e6b7e41cdd8c, and this is a better fix? Or is this simply a different way of fixing the same problem? > dev->wakeup_prepared = false; > > dev->pm_cap = 0; > -- > 1.7.9.5 > -- 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
this patch solves these 2 bug reports : https://bugzilla.kernel.org/show_bug.cgi?id=84861 https://bugzilla.kernel.org/show_bug.cgi?id=81551 in simple words : JMicron IDE/Sata controlers family ( JMBxxx ) are not fully compatible with async_suspend feature, when a user tries to put his PC on standby mode then at wake-up JMicron IDE/Sata controlers will not work, because of a brother-relation between the SATA and IDE part on this JMicron PCI card Le 05/11/2014 19:01, Bjorn Helgaas a écrit : > On Tue, Nov 4, 2014 at 6:31 PM, Chuansheng Liu <chuansheng.liu@intel.com> wrote: >> The JMicron chip 361/363/368 contains one SATA controller and >> one PATA controller, they are brother-relation ship in PCI tree, >> but for powering on these both controller, we must follow the >> sequence one by one, otherwise one of them can not be powered on >> successfully. > > This should mention what's broken and what problem a user would see. > This changelog sounds a lot like the one for e6b7e41cdd8c, so I don't > know if this is for a new, related problem, or what. > >> So here we disable the async suspend method for Jmicron chip. >> >> Bug link: >> https://bugzilla.kernel.org/show_bug.cgi?id=81551 >> https://bugzilla.kernel.org/show_bug.cgi?id=84861 >> >> And we can revert the below commit after this patch is applied: >> e6b7e41(ata: Disabling the async PM for JMicron chip 363/361) >> >> Cc: stable@vger.kernel.org # 3.15+ >> Acked-by: Aaron Lu <aaron.lu@intel.com> >> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com> >> --- >> drivers/pci/pci.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 625a4ac..53128f0 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -2046,7 +2046,17 @@ void pci_pm_init(struct pci_dev *dev) >> pm_runtime_forbid(&dev->dev); >> pm_runtime_set_active(&dev->dev); >> pm_runtime_enable(&dev->dev); >> - device_enable_async_suspend(&dev->dev); >> + >> + /* >> + * The JMicron chip 361/363/368 contains one SATA controller and >> + * one PATA controller, they are brother-relation ship in PCI tree, >> + * but for powering on these both controller, we must follow the >> + * sequence one by one, otherwise one of them can not be powered on >> + * successfully, so here we disable the async suspend method for >> + * Jmicron chip. >> + */ >> + if (dev->vendor != PCI_VENDOR_ID_JMICRON) >> + device_enable_async_suspend(&dev->dev); > > I don't like littering the core PCI code with vendor tests like this. > This would be the only one, except for an ancient DECchip 21050 bridge > erratum. > > And why would we want a test for *all* JMicron devices here, when you > claim the problem only affects a few specific ones? > > And what's the story with the e6b7e41cdd8c ("ata: Disabling the async > PM for JMicron chip 363/361") connection? Is something broken even > with e6b7e41cdd8c, and this is a better fix? Or is this simply a > different way of fixing the same problem? > >> dev->wakeup_prepared = false; >> >> dev->pm_cap = 0; >> -- >> 1.7.9.5 >> > -- 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, Nov 5, 2014 at 11:46 AM, Barto <mister.freeman@laposte.net> wrote: > this patch solves these 2 bug reports : > > https://bugzilla.kernel.org/show_bug.cgi?id=84861 > > https://bugzilla.kernel.org/show_bug.cgi?id=81551 Those bugs were already mentioned. But e6b7e41cdd8c claims to solve https://bugzilla.kernel.org/show_bug.cgi?id=81551, and 84861 is a duplicate of 81551, so it should also be fixed by e6b7e41cdd8c. So the question is, why was e6b7e41cdd8c insufficient? Presumably it was tested and somebody thought it did fix the problem. > in simple words : JMicron IDE/Sata controlers family ( JMBxxx ) are not > fully compatible with async_suspend feature, when a user tries to put > his PC on standby mode then at wake-up JMicron IDE/Sata controlers will > not work, because of a brother-relation between the SATA and IDE part on > this JMicron PCI card > > > > > Le 05/11/2014 19:01, Bjorn Helgaas a écrit : >> On Tue, Nov 4, 2014 at 6:31 PM, Chuansheng Liu <chuansheng.liu@intel.com> wrote: >>> The JMicron chip 361/363/368 contains one SATA controller and >>> one PATA controller, they are brother-relation ship in PCI tree, >>> but for powering on these both controller, we must follow the >>> sequence one by one, otherwise one of them can not be powered on >>> successfully. >> >> This should mention what's broken and what problem a user would see. >> This changelog sounds a lot like the one for e6b7e41cdd8c, so I don't >> know if this is for a new, related problem, or what. >> >>> So here we disable the async suspend method for Jmicron chip. >>> >>> Bug link: >>> https://bugzilla.kernel.org/show_bug.cgi?id=81551 >>> https://bugzilla.kernel.org/show_bug.cgi?id=84861 >>> >>> And we can revert the below commit after this patch is applied: >>> e6b7e41(ata: Disabling the async PM for JMicron chip 363/361) >>> >>> Cc: stable@vger.kernel.org # 3.15+ >>> Acked-by: Aaron Lu <aaron.lu@intel.com> >>> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com> >>> --- >>> drivers/pci/pci.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>> index 625a4ac..53128f0 100644 >>> --- a/drivers/pci/pci.c >>> +++ b/drivers/pci/pci.c >>> @@ -2046,7 +2046,17 @@ void pci_pm_init(struct pci_dev *dev) >>> pm_runtime_forbid(&dev->dev); >>> pm_runtime_set_active(&dev->dev); >>> pm_runtime_enable(&dev->dev); >>> - device_enable_async_suspend(&dev->dev); >>> + >>> + /* >>> + * The JMicron chip 361/363/368 contains one SATA controller and >>> + * one PATA controller, they are brother-relation ship in PCI tree, >>> + * but for powering on these both controller, we must follow the >>> + * sequence one by one, otherwise one of them can not be powered on >>> + * successfully, so here we disable the async suspend method for >>> + * Jmicron chip. >>> + */ >>> + if (dev->vendor != PCI_VENDOR_ID_JMICRON) >>> + device_enable_async_suspend(&dev->dev); >> >> I don't like littering the core PCI code with vendor tests like this. >> This would be the only one, except for an ancient DECchip 21050 bridge >> erratum. >> >> And why would we want a test for *all* JMicron devices here, when you >> claim the problem only affects a few specific ones? >> >> And what's the story with the e6b7e41cdd8c ("ata: Disabling the async >> PM for JMicron chip 363/361") connection? Is something broken even >> with e6b7e41cdd8c, and this is a better fix? Or is this simply a >> different way of fixing the same problem? >> >>> dev->wakeup_prepared = false; >>> >>> dev->pm_cap = 0; >>> -- >>> 1.7.9.5 >>> >> -- 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
Bjorn : the patch initialy created for bug 81551 ( ATAPI-CD-ROM-drive dead after resume from suspend/s2disk ) was not enough for the bug 84861 ( JMicron Technology Corp. JMB368 IDE controller dead after resume when async suspend is enabled ), the reason : there are too much models inside the family of JMBxxx JMicron SATA/IDE controlers PCI cards, and the first patch targets ONLY the JMB363/361 model, which is not enough : if (pdev->vendor == PCI_VENDOR_ID_JMICRON && + (pdev->device == PCI_DEVICE_ID_JMICRON_JMB363 || + pdev->device == PCI_DEVICE_ID_JMICRON_JMB361)) + device_disable_async_suspend(&pdev->dev); for example I have a JMB363/368 JMicron SATA/IDE PCI card, and the first patch created for the bug 81551 is not enough, that's why Chuansheng Liu created a new patch who targets ALL models of JMicron JMBxxx SATA/IDE cards, in order to be sure that these models of JMicron will have "async_suspend feature disabled, the good patch who works for all models of JMicron JMBxx SATA/IDE controlers : + if (dev->vendor != PCI_VENDOR_ID_JMICRON) + device_enable_async_suspend(&dev->dev); dev->wakeup_prepared = false; Le 05/11/2014 20:03, Bjorn Helgaas a écrit : > On Wed, Nov 5, 2014 at 11:46 AM, Barto <mister.freeman@laposte.net> wrote: >> this patch solves these 2 bug reports : >> >> https://bugzilla.kernel.org/show_bug.cgi?id= >> >> https://bugzilla.kernel.org/show_bug.cgi?id=81551 > > Those bugs were already mentioned. But e6b7e41cdd8c claims to solve > https://bugzilla.kernel.org/show_bug.cgi?id=81551, and 84861 is a > duplicate of 81551, so it should also be fixed by e6b7e41cdd8c. > > So the question is, why was e6b7e41cdd8c insufficient? Presumably it > was tested and somebody thought it did fix the problem. > >> in simple words : JMicron IDE/Sata controlers family ( JMBxxx ) are not >> fully compatible with async_suspend feature, when a user tries to put >> his PC on standby mode then at wake-up JMicron IDE/Sata controlers will >> not work, because of a brother-relation between the SATA and IDE part on >> this JMicron PCI card >> >> >> >> >> Le 05/11/2014 19:01, Bjorn Helgaas a écrit : >>> On Tue, Nov 4, 2014 at 6:31 PM, Chuansheng Liu <chuansheng.liu@intel.com> wrote: >>>> The JMicron chip 361/363/368 contains one SATA controller and >>>> one PATA controller, they are brother-relation ship in PCI tree, >>>> but for powering on these both controller, we must follow the >>>> sequence one by one, otherwise one of them can not be powered on >>>> successfully. >>> >>> This should mention what's broken and what problem a user would see. >>> This changelog sounds a lot like the one for e6b7e41cdd8c, so I don't >>> know if this is for a new, related problem, or what. >>> >>>> So here we disable the async suspend method for Jmicron chip. >>>> >>>> Bug link: >>>> https://bugzilla.kernel.org/show_bug.cgi?id=81551 >>>> https://bugzilla.kernel.org/show_bug.cgi?id=84861 >>>> >>>> And we can revert the below commit after this patch is applied: >>>> e6b7e41(ata: Disabling the async PM for JMicron chip 363/361) >>>> >>>> Cc: stable@vger.kernel.org # 3.15+ >>>> Acked-by: Aaron Lu <aaron.lu@intel.com> >>>> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com> >>>> --- >>>> drivers/pci/pci.c | 12 +++++++++++- >>>> 1 file changed, 11 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>>> index 625a4ac..53128f0 100644 >>>> --- a/drivers/pci/pci.c >>>> +++ b/drivers/pci/pci.c >>>> @@ -2046,7 +2046,17 @@ void pci_pm_init(struct pci_dev *dev) >>>> pm_runtime_forbid(&dev->dev); >>>> pm_runtime_set_active(&dev->dev); >>>> pm_runtime_enable(&dev->dev); >>>> - device_enable_async_suspend(&dev->dev); >>>> + >>>> + /* >>>> + * The JMicron chip 361/363/368 contains one SATA controller and >>>> + * one PATA controller, they are brother-relation ship in PCI tree, >>>> + * but for powering on these both controller, we must follow the >>>> + * sequence one by one, otherwise one of them can not be powered on >>>> + * successfully, so here we disable the async suspend method for >>>> + * Jmicron chip. >>>> + */ >>>> + if (dev->vendor != PCI_VENDOR_ID_JMICRON) >>>> + device_enable_async_suspend(&dev->dev); >>> >>> I don't like littering the core PCI code with vendor tests like this. >>> This would be the only one, except for an ancient DECchip 21050 bridge >>> erratum. >>> >>> And why would we want a test for *all* JMicron devices here, when you >>> claim the problem only affects a few specific ones? >>> >>> And what's the story with the e6b7e41cdd8c ("ata: Disabling the async >>> PM for JMicron chip 363/361") connection? Is something broken even >>> with e6b7e41cdd8c, and this is a better fix? Or is this simply a >>> different way of fixing the same problem? >>> >>>> dev->wakeup_prepared = false; >>>> >>>> dev->pm_cap = 0; >>>> -- >>>> 1.7.9.5 >>>> >>> > -- 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
Hello Bjorn, > -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > Sent: Thursday, November 06, 2014 3:04 AM > To: Barto > Cc: Liu, Chuansheng; Lu, Aaron; Tejun Heo; Rafael Wysocki; > linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] PCI: Do not enable async suspend for JMicron chips > > On Wed, Nov 5, 2014 at 11:46 AM, Barto <mister.freeman@laposte.net> > wrote: > > this patch solves these 2 bug reports : > > > > https://bugzilla.kernel.org/show_bug.cgi?id=84861 > > > > https://bugzilla.kernel.org/show_bug.cgi?id=81551 > > Those bugs were already mentioned. But e6b7e41cdd8c claims to solve > https://bugzilla.kernel.org/show_bug.cgi?id=81551, and 84861 is a > duplicate of 81551, so it should also be fixed by e6b7e41cdd8c. > > So the question is, why was e6b7e41cdd8c insufficient? Presumably it > was tested and somebody thought it did fix the problem. The first patch e6b7e41cdd8c which is just exclude some of JMicron chips(363/361) out of async_suspend, then Barto found the same issue on JMicron 368, so we need one more general patch to let JMicron chips out of async_suspend, so we make this patch. Bjorn, tj, Could you kindly take this patch? As Barto said, it effected the user experience indeed, thanks.
On Wed, Nov 5, 2014 at 6:48 PM, Liu, Chuansheng <chuansheng.liu@intel.com> wrote: > Hello Bjorn, > >> -----Original Message----- >> From: Bjorn Helgaas [mailto:bhelgaas@google.com] >> Sent: Thursday, November 06, 2014 3:04 AM >> To: Barto >> Cc: Liu, Chuansheng; Lu, Aaron; Tejun Heo; Rafael Wysocki; >> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH] PCI: Do not enable async suspend for JMicron chips >> >> On Wed, Nov 5, 2014 at 11:46 AM, Barto <mister.freeman@laposte.net> >> wrote: >> > this patch solves these 2 bug reports : >> > >> > https://bugzilla.kernel.org/show_bug.cgi?id=84861 >> > >> > https://bugzilla.kernel.org/show_bug.cgi?id=81551 >> >> Those bugs were already mentioned. But e6b7e41cdd8c claims to solve >> https://bugzilla.kernel.org/show_bug.cgi?id=81551, and 84861 is a >> duplicate of 81551, so it should also be fixed by e6b7e41cdd8c. >> >> So the question is, why was e6b7e41cdd8c insufficient? Presumably it >> was tested and somebody thought it did fix the problem. > > The first patch e6b7e41cdd8c which is just exclude some of JMicron chips(363/361) out of async_suspend, > then Barto found the same issue on JMicron 368, so we need one more general patch to let JMicron chips > out of async_suspend, so we make this patch. > > Bjorn, tj, > Could you kindly take this patch? As Barto said, it effected the user experience indeed, thanks. Thanks for clarifying the changelog as far as the different chips and the different bugzillas. But you haven't addressed my concerns about (1) putting a PCI vendor ID check in the generic PCI core code, and (2) applying this to *all* JMicron devices. You might want to explore a quirk-type solution or maybe just add the JMicron 368 to the checks added by e6b7e41cdd8c. Bjorn -- 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
Hello Bjorn, in my bugreport I have already tried to add the JMicron 368 in the "if statement" and it didn't work, check my message here : https://bugzilla.kernel.org/show_bug.cgi?id=84861#c11 if Chuansheng has choosen a more generic way ( applying the patch to all JMicron devices ) it's because also because we don't know how many JMBxxx models could be affected by this bug, tomorrow maybe one user would create a bug report about a "JMB369" pci card who have again this bug, and maybe on month later another user with a "JMB382", it could be a nightmare for Chuanseng if he had to create every time a new patch for each model of JMicron, so for the moment the better approach for me is to disable async_suspend for all JMBxxx JMicron, Chuanseng's patch seems reasonnable, as long as we don't know the exact list of JMBxxx models we can assume that all JMicron SATA/IDE are affected by this problem Le 06/11/2014 05:08, Bjorn Helgaas a écrit : > > But you haven't addressed my concerns about (1) putting a PCI vendor > ID check in the generic PCI core code, and (2) applying this to *all* > JMicron devices. You might want to explore a quirk-type solution or > maybe just add the JMicron 368 to the checks added by e6b7e41cdd8c. > > Bjorn > -- 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
SGVsbG8gQmpvcm4sDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmpv cm4gSGVsZ2FhcyBbbWFpbHRvOmJoZWxnYWFzQGdvb2dsZS5jb21dDQo+IFNlbnQ6IFRodXJzZGF5 LCBOb3ZlbWJlciAwNiwgMjAxNCAxMjowOSBQTQ0KPiBUbzogTGl1LCBDaHVhbnNoZW5nDQo+IENj OiBCYXJ0bzsgVGVqdW4gSGVvICh0akBrZXJuZWwub3JnKTsgTHUsIEFhcm9uOyBSYWZhZWwgV3lz b2NraTsNCj4gbGludXgtcGNpQHZnZXIua2VybmVsLm9yZzsgbGludXgta2VybmVsQHZnZXIua2Vy bmVsLm9yZw0KPiBTdWJqZWN0OiBSZTogW1BBVENIXSBQQ0k6IERvIG5vdCBlbmFibGUgYXN5bmMg c3VzcGVuZCBmb3IgSk1pY3JvbiBjaGlwcw0KPiANCj4gT24gV2VkLCBOb3YgNSwgMjAxNCBhdCA2 OjQ4IFBNLCBMaXUsIENodWFuc2hlbmcNCj4gPGNodWFuc2hlbmcubGl1QGludGVsLmNvbT4gd3Jv dGU6DQo+ID4gSGVsbG8gQmpvcm4sDQo+ID4NCj4gPj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0t LS0NCj4gPj4gRnJvbTogQmpvcm4gSGVsZ2FhcyBbbWFpbHRvOmJoZWxnYWFzQGdvb2dsZS5jb21d DQo+ID4+IFNlbnQ6IFRodXJzZGF5LCBOb3ZlbWJlciAwNiwgMjAxNCAzOjA0IEFNDQo+ID4+IFRv OiBCYXJ0bw0KPiA+PiBDYzogTGl1LCBDaHVhbnNoZW5nOyBMdSwgQWFyb247IFRlanVuIEhlbzsg UmFmYWVsIFd5c29ja2k7DQo+ID4+IGxpbnV4LXBjaUB2Z2VyLmtlcm5lbC5vcmc7IGxpbnV4LWtl cm5lbEB2Z2VyLmtlcm5lbC5vcmcNCj4gPj4gU3ViamVjdDogUmU6IFtQQVRDSF0gUENJOiBEbyBu b3QgZW5hYmxlIGFzeW5jIHN1c3BlbmQgZm9yIEpNaWNyb24gY2hpcHMNCj4gPj4NCj4gPj4gT24g V2VkLCBOb3YgNSwgMjAxNCBhdCAxMTo0NiBBTSwgQmFydG8gPG1pc3Rlci5mcmVlbWFuQGxhcG9z dGUubmV0Pg0KPiA+PiB3cm90ZToNCj4gPj4gPiB0aGlzIHBhdGNoIHNvbHZlcyB0aGVzZSAyIGJ1 ZyByZXBvcnRzIDoNCj4gPj4gPg0KPiA+PiA+IGh0dHBzOi8vYnVnemlsbGEua2VybmVsLm9yZy9z aG93X2J1Zy5jZ2k/aWQ9ODQ4NjENCj4gPj4gPg0KPiA+PiA+IGh0dHBzOi8vYnVnemlsbGEua2Vy bmVsLm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODE1NTENCj4gPj4NCj4gPj4gVGhvc2UgYnVncyB3ZXJl IGFscmVhZHkgbWVudGlvbmVkLiAgQnV0IGU2YjdlNDFjZGQ4YyBjbGFpbXMgdG8gc29sdmUNCj4g Pj4gaHR0cHM6Ly9idWd6aWxsYS5rZXJuZWwub3JnL3Nob3dfYnVnLmNnaT9pZD04MTU1MSwgYW5k IDg0ODYxIGlzIGENCj4gPj4gZHVwbGljYXRlIG9mIDgxNTUxLCBzbyBpdCBzaG91bGQgYWxzbyBi ZSBmaXhlZCBieSBlNmI3ZTQxY2RkOGMuDQo+ID4+DQo+ID4+IFNvIHRoZSBxdWVzdGlvbiBpcywg d2h5IHdhcyBlNmI3ZTQxY2RkOGMgaW5zdWZmaWNpZW50PyAgUHJlc3VtYWJseSBpdA0KPiA+PiB3 YXMgdGVzdGVkIGFuZCBzb21lYm9keSB0aG91Z2h0IGl0IGRpZCBmaXggdGhlIHByb2JsZW0uDQo+ ID4NCj4gPiBUaGUgZmlyc3QgcGF0Y2ggZTZiN2U0MWNkZDhjIHdoaWNoIGlzIGp1c3QgZXhjbHVk ZSBzb21lIG9mIEpNaWNyb24NCj4gY2hpcHMoMzYzLzM2MSkgb3V0IG9mIGFzeW5jX3N1c3BlbmQs DQo+ID4gdGhlbiBCYXJ0byBmb3VuZCB0aGUgc2FtZSBpc3N1ZSBvbiBKTWljcm9uIDM2OCwgc28g d2UgbmVlZCBvbmUgbW9yZQ0KPiBnZW5lcmFsIHBhdGNoIHRvIGxldCBKTWljcm9uIGNoaXBzDQo+ ID4gb3V0IG9mIGFzeW5jX3N1c3BlbmQsIHNvIHdlIG1ha2UgdGhpcyBwYXRjaC4NCj4gPg0KPiA+ IEJqb3JuLCB0aiwNCj4gPiBDb3VsZCB5b3Uga2luZGx5IHRha2UgdGhpcyBwYXRjaD8gQXMgQmFy dG8gc2FpZCwgaXQgZWZmZWN0ZWQgdGhlIHVzZXINCj4gZXhwZXJpZW5jZSBpbmRlZWQsIHRoYW5r cy4NCj4gDQo+IFRoYW5rcyBmb3IgY2xhcmlmeWluZyB0aGUgY2hhbmdlbG9nIGFzIGZhciBhcyB0 aGUgZGlmZmVyZW50IGNoaXBzIGFuZA0KPiB0aGUgZGlmZmVyZW50IGJ1Z3ppbGxhcy4NCj4gDQo+ IEJ1dCB5b3UgaGF2ZW4ndCBhZGRyZXNzZWQgbXkgY29uY2VybnMgYWJvdXQgKDEpIHB1dHRpbmcg YSBQQ0kgdmVuZG9yDQo+IElEIGNoZWNrIGluIHRoZSBnZW5lcmljIFBDSSBjb3JlIGNvZGUsIGFu ZCAoMikgYXBwbHlpbmcgdGhpcyB0byAqYWxsKg0KPiBKTWljcm9uIGRldmljZXMuICBZb3UgbWln aHQgd2FudCB0byBleHBsb3JlIGEgcXVpcmstdHlwZSBzb2x1dGlvbiBvcg0KPiBtYXliZSBqdXN0 IGFkZCB0aGUgSk1pY3JvbiAzNjggdG8gdGhlIGNoZWNrcyBhZGRlZCBieSBlNmI3ZTQxY2RkOGMu DQpVbmRlcnN0YW5kIHlvdXIgcG9pbnQsIGluIGZhY3QsIGJlZm9yZSB0aGlzIHBhdGNoIHN1Ym1p dHRlZCwgSSBoYWQgd3JpdHRlbiBhbm90aGVyIHBhdGNoIGh0dHBzOi8vbGttbC5vcmcvbGttbC8y MDE0LzkvMjQvNjgNCndoaWNoIGFkZHJlc3NlZCB0byBhZGQgdGhlIHF1aXJrLXR5cGUgc29sdXRp b24gaW4gQVRBIGNvZGUsIGFuZCBBYXJvbiBnaXZlbiBiZXR0ZXIgc3VnZ2VzdGlvbiB0aGF0IGlt cGxlbWVudGVkIGF0IHBjaV9wbV9pbml0KCkuDQpIb3cgZG8geW91IHRoaW5rIG9mIGl0PyBUaGFu a3MuDQoNCg0K -- 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 11/06/2014 01:29 PM, Liu, Chuansheng wrote: > Hello Bjorn, > >> -----Original Message----- >> From: Bjorn Helgaas [mailto:bhelgaas@google.com] >> Sent: Thursday, November 06, 2014 12:09 PM >> To: Liu, Chuansheng >> Cc: Barto; Tejun Heo (tj@kernel.org); Lu, Aaron; Rafael Wysocki; >> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH] PCI: Do not enable async suspend for JMicron chips >> >> On Wed, Nov 5, 2014 at 6:48 PM, Liu, Chuansheng >> <chuansheng.liu@intel.com> wrote: >>> Hello Bjorn, >>> >>>> -----Original Message----- >>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com] >>>> Sent: Thursday, November 06, 2014 3:04 AM >>>> To: Barto >>>> Cc: Liu, Chuansheng; Lu, Aaron; Tejun Heo; Rafael Wysocki; >>>> linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org >>>> Subject: Re: [PATCH] PCI: Do not enable async suspend for JMicron chips >>>> >>>> On Wed, Nov 5, 2014 at 11:46 AM, Barto <mister.freeman@laposte.net> >>>> wrote: >>>>> this patch solves these 2 bug reports : >>>>> >>>>> https://bugzilla.kernel.org/show_bug.cgi?id=84861 >>>>> >>>>> https://bugzilla.kernel.org/show_bug.cgi?id=81551 >>>> >>>> Those bugs were already mentioned. But e6b7e41cdd8c claims to solve >>>> https://bugzilla.kernel.org/show_bug.cgi?id=81551, and 84861 is a >>>> duplicate of 81551, so it should also be fixed by e6b7e41cdd8c. >>>> >>>> So the question is, why was e6b7e41cdd8c insufficient? Presumably it >>>> was tested and somebody thought it did fix the problem. >>> >>> The first patch e6b7e41cdd8c which is just exclude some of JMicron >> chips(363/361) out of async_suspend, >>> then Barto found the same issue on JMicron 368, so we need one more >> general patch to let JMicron chips >>> out of async_suspend, so we make this patch. >>> >>> Bjorn, tj, >>> Could you kindly take this patch? As Barto said, it effected the user >> experience indeed, thanks. >> >> Thanks for clarifying the changelog as far as the different chips and >> the different bugzillas. >> >> But you haven't addressed my concerns about (1) putting a PCI vendor >> ID check in the generic PCI core code, and (2) applying this to *all* >> JMicron devices. You might want to explore a quirk-type solution or >> maybe just add the JMicron 368 to the checks added by e6b7e41cdd8c. > Understand your point, in fact, before this patch submitted, I had written another patch https://lkml.org/lkml/2014/9/24/68 > which addressed to add the quirk-type solution in ATA code, and Aaron given better suggestion that implemented at pci_pm_init(). > How do you think of it? Thanks. I think Bjorn means that we should place the code as a fixup somewhere in the quirks.c. I didn't take a closer look but DECLARE_PCI_FIXUP_FINAL for those JMicron PCI devices seems to be a proper phase. Thanks, Aaron -- 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
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 625a4ac..53128f0 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2046,7 +2046,17 @@ void pci_pm_init(struct pci_dev *dev) pm_runtime_forbid(&dev->dev); pm_runtime_set_active(&dev->dev); pm_runtime_enable(&dev->dev); - device_enable_async_suspend(&dev->dev); + + /* + * The JMicron chip 361/363/368 contains one SATA controller and + * one PATA controller, they are brother-relation ship in PCI tree, + * but for powering on these both controller, we must follow the + * sequence one by one, otherwise one of them can not be powered on + * successfully, so here we disable the async suspend method for + * Jmicron chip. + */ + if (dev->vendor != PCI_VENDOR_ID_JMICRON) + device_enable_async_suspend(&dev->dev); dev->wakeup_prepared = false; dev->pm_cap = 0;