Message ID | 20150825024941.GA25829@google.com (mailing list archive) |
---|---|
State | New, archived |
Delegated to: | Bjorn Helgaas |
Headers | show |
On Mon, Aug 24, 2015 at 7:49 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > commit 09981db5b0c9a6865aa39126b71bc87718577e4b > Author: Zhang Rui <rui.zhang@intel.com> > Date: Mon Aug 24 15:27:11 2015 -0500 > > PCI: Disable async suspend/resume for JMicron multi-function SATA/AHCI > > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_IDE, 8, quirk_jmicron_async_suspend); > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_SATA_AHCI, 0, quirk_jmicron_async_suspend); s/PCI_ANY/PCI_ANY_ID/g -- 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, Aug 25, 2015 at 3:21 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Mon, Aug 24, 2015 at 7:49 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> commit 09981db5b0c9a6865aa39126b71bc87718577e4b >> Author: Zhang Rui <rui.zhang@intel.com> >> Date: Mon Aug 24 15:27:11 2015 -0500 >> >> PCI: Disable async suspend/resume for JMicron multi-function SATA/AHCI >> >> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_IDE, 8, quirk_jmicron_async_suspend); >> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_SATA_AHCI, 0, quirk_jmicron_async_suspend); > > s/PCI_ANY/PCI_ANY_ID/g Done, thanks. -- 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
Hi, Bjorn, On Mon, 2015-08-24 at 21:49 -0500, Bjorn Helgaas wrote: > On Tue, Jul 28, 2015 at 09:42:27AM +0800, Zhang Rui wrote: > > From 57edba9c677e47354846db951014dc4d5b13ce54 Mon Sep 17 00:00:00 2001 > > From: Zhang Rui <rui.zhang@intel.com> > > Date: Sun, 26 Jul 2015 14:15:36 +0800 > > Subject: [PATCH] PCI: Disable async suspend/resume for Jmicron chip > > ... > > In the interest of making progress on this, I reworked this to be what I'm > looking for. It's not significantly different code-wise from what you > posted originally, so I left your Signed-off-by. But let me know if I > messed it up. > thanks for the follow up. Sorry for my late response as I was traveling and then took some vacation in the last two weeks. The patch looks fine to me. thanks, rui > We don't actually *know* whether the quirk needs to be applied before > pata_jmicron is loaded, but my guess is that is does, so I left it as a PCI > quirk rather than as a driver quirk. > Comments welcome! > > Bjorn > > > commit 09981db5b0c9a6865aa39126b71bc87718577e4b > Author: Zhang Rui <rui.zhang@intel.com> > Date: Mon Aug 24 15:27:11 2015 -0500 > > PCI: Disable async suspend/resume for JMicron multi-function SATA/AHCI > > On multi-function JMicron SATA/PATA/AHCI devices, the PATA controller at > function 1 doesn't work if it is powered on before the SATA controller at > function 0. The result is that PATA doesn't work after resume, and > messages like this: > > pata_jmicron 0000:02:00.1: Refused to change power state, currently in D3 > irq 17: nobody cared (try booting with the "irqpoll" option) > > e6b7e41cdd8c ("ata: Disabling the async PM for JMicron chip 363/361") > solved this problem for JMicron 361 and 363 devices. With async suspend > disabled, we always power on function 0 before function 1. > > Barto then reported the same problem with a JMicron 368 (see comment #57 in > the bugzilla). > > Rather than extending the blacklist piecemeal, disable async suspend for > all JMicron multi-function SATA/PATA/AHCI devices. > > This quirk could stay in the ahci and pata_jmicron drivers, but it's likely > the problem will occur even if pata_jmicron isn't loaded until after the > suspend/resume. Making it a PCI quirk ensures that we'll preserve the > power-on order even if the drivers aren't loaded. > > [bhelgaas: changelog, limit to multi-function, limit to IDE/ATA] > Link: https://bugzilla.kernel.org/show_bug.cgi?id=81551 > Reported-by: Barto <mister.freeman@laposte.net> > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index 7e62751..a466602 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -351,6 +351,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { > /* JMicron 362B and 362C have an AHCI function with IDE class code */ > { PCI_VDEVICE(JMICRON, 0x2362), board_ahci_ign_iferr }, > { PCI_VDEVICE(JMICRON, 0x236f), board_ahci_ign_iferr }, > + /* May need to update quirk_jmicron_async_suspend() for additions */ > > /* ATI */ > { PCI_VDEVICE(ATI, 0x4380), board_ahci_sb600 }, /* ATI SB600 */ > @@ -1451,18 +1452,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > else if (pdev->vendor == 0x177d && pdev->device == 0xa01c) > ahci_pci_bar = AHCI_PCI_BAR_CAVIUM; > > - /* > - * The JMicron chip 361/363 contains one SATA controller and one > - * PATA controller,for powering on these both controllers, 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 these chips. > - */ > - 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); > - > /* acquire resources */ > rc = pcim_enable_device(pdev); > if (rc) > diff --git a/drivers/ata/pata_jmicron.c b/drivers/ata/pata_jmicron.c > index 47e418b..4d1a5d2 100644 > --- a/drivers/ata/pata_jmicron.c > +++ b/drivers/ata/pata_jmicron.c > @@ -143,18 +143,6 @@ static int jmicron_init_one (struct pci_dev *pdev, const struct pci_device_id *i > }; > const struct ata_port_info *ppi[] = { &info, NULL }; > > - /* > - * The JMicron chip 361/363 contains one SATA controller and one > - * PATA controller,for powering on these both controllers, 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 these chips. > - */ > - 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); > - > return ata_pci_bmdma_init_one(pdev, ppi, &jmicron_sht, NULL, 0); > } > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index b6af4b0..5e35a9d 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -1570,6 +1570,18 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_JMICRON, PCI_DEVICE_ID_JMICRON_JMB3 > > #endif > > +static void quirk_jmicron_async_suspend(struct pci_dev *dev) > +{ > + if (dev->multifunction) { > + device_disable_async_suspend(&dev->dev); > + dev_info(&dev->dev, "async suspend disabled to avoid multi-function power-on ordering issue\n"); > + } > +} > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_IDE, 8, quirk_jmicron_async_suspend); > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_SATA_AHCI, 0, quirk_jmicron_async_suspend); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x2362, quirk_jmicron_async_suspend); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x236f, quirk_jmicron_async_suspend); > + > #ifdef CONFIG_X86_IO_APIC > static void quirk_alder_ioapic(struct pci_dev *pdev) > { -- 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
I will test this new patch at the end of this email, Jay should do the same in order to be sure that the small changes made by Zhang Rui in this new patch really works Le 28/08/2015 10:21, Zhang Rui a écrit : >> In the interest of making progress on this, I reworked this to be what I'm >> looking for. It's not significantly different code-wise from what you >> posted originally, so I left your Signed-off-by. But let me know if I >> messed it up. >> >> >> >> commit 09981db5b0c9a6865aa39126b71bc87718577e4b >> Author: Zhang Rui <rui.zhang@intel.com> >> Date: Mon Aug 24 15:27:11 2015 -0500 >> >> PCI: Disable async suspend/resume for JMicron multi-function SATA/AHCI >> >> On multi-function JMicron SATA/PATA/AHCI devices, the PATA controller at >> function 1 doesn't work if it is powered on before the SATA controller at >> function 0. The result is that PATA doesn't work after resume, and >> messages like this: >> >> pata_jmicron 0000:02:00.1: Refused to change power state, currently in D3 >> irq 17: nobody cared (try booting with the "irqpoll" option) >> >> e6b7e41cdd8c ("ata: Disabling the async PM for JMicron chip 363/361") >> solved this problem for JMicron 361 and 363 devices. With async suspend >> disabled, we always power on function 0 before function 1. >> >> Barto then reported the same problem with a JMicron 368 (see comment #57 in >> the bugzilla). >> >> Rather than extending the blacklist piecemeal, disable async suspend for >> all JMicron multi-function SATA/PATA/AHCI devices. >> >> This quirk could stay in the ahci and pata_jmicron drivers, but it's likely >> the problem will occur even if pata_jmicron isn't loaded until after the >> suspend/resume. Making it a PCI quirk ensures that we'll preserve the >> power-on order even if the drivers aren't loaded. >> >> [bhelgaas: changelog, limit to multi-function, limit to IDE/ATA] >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=81551 >> Reported-by: Barto <mister.freeman@laposte.net> >> Signed-off-by: Zhang Rui <rui.zhang@intel.com> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> >> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >> index 7e62751..a466602 100644 >> --- a/drivers/ata/ahci.c >> +++ b/drivers/ata/ahci.c >> @@ -351,6 +351,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { >> /* JMicron 362B and 362C have an AHCI function with IDE class code */ >> { PCI_VDEVICE(JMICRON, 0x2362), board_ahci_ign_iferr }, >> { PCI_VDEVICE(JMICRON, 0x236f), board_ahci_ign_iferr }, >> + /* May need to update quirk_jmicron_async_suspend() for additions */ >> >> /* ATI */ >> { PCI_VDEVICE(ATI, 0x4380), board_ahci_sb600 }, /* ATI SB600 */ >> @@ -1451,18 +1452,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> else if (pdev->vendor == 0x177d && pdev->device == 0xa01c) >> ahci_pci_bar = AHCI_PCI_BAR_CAVIUM; >> >> - /* >> - * The JMicron chip 361/363 contains one SATA controller and one >> - * PATA controller,for powering on these both controllers, 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 these chips. >> - */ >> - 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); >> - >> /* acquire resources */ >> rc = pcim_enable_device(pdev); >> if (rc) >> diff --git a/drivers/ata/pata_jmicron.c b/drivers/ata/pata_jmicron.c >> index 47e418b..4d1a5d2 100644 >> --- a/drivers/ata/pata_jmicron.c >> +++ b/drivers/ata/pata_jmicron.c >> @@ -143,18 +143,6 @@ static int jmicron_init_one (struct pci_dev *pdev, const struct pci_device_id *i >> }; >> const struct ata_port_info *ppi[] = { &info, NULL }; >> >> - /* >> - * The JMicron chip 361/363 contains one SATA controller and one >> - * PATA controller,for powering on these both controllers, 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 these chips. >> - */ >> - 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); >> - >> return ata_pci_bmdma_init_one(pdev, ppi, &jmicron_sht, NULL, 0); >> } >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index b6af4b0..5e35a9d 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -1570,6 +1570,18 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_JMICRON, PCI_DEVICE_ID_JMICRON_JMB3 >> >> #endif >> >> +static void quirk_jmicron_async_suspend(struct pci_dev *dev) >> +{ >> + if (dev->multifunction) { >> + device_disable_async_suspend(&dev->dev); >> + dev_info(&dev->dev, "async suspend disabled to avoid multi-function power-on ordering issue\n"); >> + } >> +} >> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_IDE, 8, quirk_jmicron_async_suspend); >> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_SATA_AHCI, 0, quirk_jmicron_async_suspend); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x2362, quirk_jmicron_async_suspend); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x236f, quirk_jmicron_async_suspend); >> + >> #ifdef CONFIG_X86_IO_APIC >> static void quirk_alder_ioapic(struct pci_dev *pdev) >> { > > > -- 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
I tried the new patch made by Zhang Rui, and there is a problem, I can not compile the kernel 4.6.1 with this patch, I get an error related to quirks.c file : drivers/pci/quirks.c:1586:189: error: ‘PCI_ANY’ undeclared here (not in a function) gcc doesn't know what is "PCI_ANY", this patch seems incomplete and needs some modifications : commit 09981db5b0c9a6865aa39126b71bc87718577e4b Author: Zhang Rui <rui.zhang@intel.com> Date: Mon Aug 24 15:27:11 2015 -0500 PCI: Disable async suspend/resume for JMicron multi-function SATA/AHCI On multi-function JMicron SATA/PATA/AHCI devices, the PATA controller at function 1 doesn't work if it is powered on before the SATA controller at function 0. The result is that PATA doesn't work after resume, and messages like this: pata_jmicron 0000:02:00.1: Refused to change power state, currently in D3 irq 17: nobody cared (try booting with the "irqpoll" option) e6b7e41cdd8c ("ata: Disabling the async PM for JMicron chip 363/361") solved this problem for JMicron 361 and 363 devices. With async suspend disabled, we always power on function 0 before function 1. Barto then reported the same problem with a JMicron 368 (see comment #57 in the bugzilla). Rather than extending the blacklist piecemeal, disable async suspend for all JMicron multi-function SATA/PATA/AHCI devices. This quirk could stay in the ahci and pata_jmicron drivers, but it's likely the problem will occur even if pata_jmicron isn't loaded until after the suspend/resume. Making it a PCI quirk ensures that we'll preserve the power-on order even if the drivers aren't loaded. [bhelgaas: changelog, limit to multi-function, limit to IDE/ATA] Link: https://bugzilla.kernel.org/show_bug.cgi?id=81551 Reported-by: Barto <mister.freeman@laposte.net> Signed-off-by: Zhang Rui <rui.zhang@intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 7e62751..a466602 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -351,6 +351,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { /* JMicron 362B and 362C have an AHCI function with IDE class code */ { PCI_VDEVICE(JMICRON, 0x2362), board_ahci_ign_iferr }, { PCI_VDEVICE(JMICRON, 0x236f), board_ahci_ign_iferr }, + /* May need to update quirk_jmicron_async_suspend() for additions */ /* ATI */ { PCI_VDEVICE(ATI, 0x4380), board_ahci_sb600 }, /* ATI SB600 */ @@ -1451,18 +1452,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) else if (pdev->vendor == 0x177d && pdev->device == 0xa01c) ahci_pci_bar = AHCI_PCI_BAR_CAVIUM; - /* - * The JMicron chip 361/363 contains one SATA controller and one - * PATA controller,for powering on these both controllers, 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 these chips. - */ - 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); - /* acquire resources */ rc = pcim_enable_device(pdev); if (rc) diff --git a/drivers/ata/pata_jmicron.c b/drivers/ata/pata_jmicron.c index 47e418b..4d1a5d2 100644 --- a/drivers/ata/pata_jmicron.c +++ b/drivers/ata/pata_jmicron.c @@ -143,18 +143,6 @@ static int jmicron_init_one (struct pci_dev *pdev, const struct pci_device_id *i }; const struct ata_port_info *ppi[] = { &info, NULL }; - /* - * The JMicron chip 361/363 contains one SATA controller and one - * PATA controller,for powering on these both controllers, 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 these chips. - */ - 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); - return ata_pci_bmdma_init_one(pdev, ppi, &jmicron_sht, NULL, 0); } diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index b6af4b0..5e35a9d 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -1570,6 +1570,18 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_JMICRON, PCI_DEVICE_ID_JMICRON_JMB3 #endif +static void quirk_jmicron_async_suspend(struct pci_dev *dev) +{ + if (dev->multifunction) { + device_disable_async_suspend(&dev->dev); + dev_info(&dev->dev, "async suspend disabled to avoid multi-function power-on ordering issue\n"); + } +} +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_IDE, 8, quirk_jmicron_async_suspend); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_SATA_AHCI, 0, quirk_jmicron_async_suspend); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x2362, quirk_jmicron_async_suspend); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x236f, quirk_jmicron_async_suspend); + #ifdef CONFIG_X86_IO_APIC static void quirk_alder_ioapic(struct pci_dev *pdev) { Le 28/08/2015 10:21, Zhang Rui a écrit : > Hi, Bjorn, > > On Mon, 2015-08-24 at 21:49 -0500, Bjorn Helgaas wrote: >> On Tue, Jul 28, 2015 at 09:42:27AM +0800, Zhang Rui wrote: >>> From 57edba9c677e47354846db951014dc4d5b13ce54 Mon Sep 17 00:00:00 2001 >>> From: Zhang Rui <rui.zhang@intel.com> >>> Date: Sun, 26 Jul 2015 14:15:36 +0800 >>> Subject: [PATCH] PCI: Disable async suspend/resume for Jmicron chip >> >> ... >> >> In the interest of making progress on this, I reworked this to be what I'm >> looking for. It's not significantly different code-wise from what you >> posted originally, so I left your Signed-off-by. But let me know if I >> messed it up. >> > thanks for the follow up. Sorry for my late response as I was traveling > and then took some vacation in the last two weeks. > The patch looks fine to me. > > thanks, > rui >> We don't actually *know* whether the quirk needs to be applied before >> pata_jmicron is loaded, but my guess is that is does, so I left it as a PCI >> quirk rather than as a driver quirk. > >> Comments welcome! >> >> Bjorn >> >> >> commit 09981db5b0c9a6865aa39126b71bc87718577e4b >> Author: Zhang Rui <rui.zhang@intel.com> >> Date: Mon Aug 24 15:27:11 2015 -0500 >> >> PCI: Disable async suspend/resume for JMicron multi-function SATA/AHCI >> >> On multi-function JMicron SATA/PATA/AHCI devices, the PATA controller at >> function 1 doesn't work if it is powered on before the SATA controller at >> function 0. The result is that PATA doesn't work after resume, and >> messages like this: >> >> pata_jmicron 0000:02:00.1: Refused to change power state, currently in D3 >> irq 17: nobody cared (try booting with the "irqpoll" option) >> >> e6b7e41cdd8c ("ata: Disabling the async PM for JMicron chip 363/361") >> solved this problem for JMicron 361 and 363 devices. With async suspend >> disabled, we always power on function 0 before function 1. >> >> Barto then reported the same problem with a JMicron 368 (see comment #57 in >> the bugzilla). >> >> Rather than extending the blacklist piecemeal, disable async suspend for >> all JMicron multi-function SATA/PATA/AHCI devices. >> >> This quirk could stay in the ahci and pata_jmicron drivers, but it's likely >> the problem will occur even if pata_jmicron isn't loaded until after the >> suspend/resume. Making it a PCI quirk ensures that we'll preserve the >> power-on order even if the drivers aren't loaded. >> >> [bhelgaas: changelog, limit to multi-function, limit to IDE/ATA] >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=81551 >> Reported-by: Barto <mister.freeman@laposte.net> >> Signed-off-by: Zhang Rui <rui.zhang@intel.com> >> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >> >> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >> index 7e62751..a466602 100644 >> --- a/drivers/ata/ahci.c >> +++ b/drivers/ata/ahci.c >> @@ -351,6 +351,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { >> /* JMicron 362B and 362C have an AHCI function with IDE class code */ >> { PCI_VDEVICE(JMICRON, 0x2362), board_ahci_ign_iferr }, >> { PCI_VDEVICE(JMICRON, 0x236f), board_ahci_ign_iferr }, >> + /* May need to update quirk_jmicron_async_suspend() for additions */ >> >> /* ATI */ >> { PCI_VDEVICE(ATI, 0x4380), board_ahci_sb600 }, /* ATI SB600 */ >> @@ -1451,18 +1452,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) >> else if (pdev->vendor == 0x177d && pdev->device == 0xa01c) >> ahci_pci_bar = AHCI_PCI_BAR_CAVIUM; >> >> - /* >> - * The JMicron chip 361/363 contains one SATA controller and one >> - * PATA controller,for powering on these both controllers, 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 these chips. >> - */ >> - 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); >> - >> /* acquire resources */ >> rc = pcim_enable_device(pdev); >> if (rc) >> diff --git a/drivers/ata/pata_jmicron.c b/drivers/ata/pata_jmicron.c >> index 47e418b..4d1a5d2 100644 >> --- a/drivers/ata/pata_jmicron.c >> +++ b/drivers/ata/pata_jmicron.c >> @@ -143,18 +143,6 @@ static int jmicron_init_one (struct pci_dev *pdev, const struct pci_device_id *i >> }; >> const struct ata_port_info *ppi[] = { &info, NULL }; >> >> - /* >> - * The JMicron chip 361/363 contains one SATA controller and one >> - * PATA controller,for powering on these both controllers, 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 these chips. >> - */ >> - 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); >> - >> return ata_pci_bmdma_init_one(pdev, ppi, &jmicron_sht, NULL, 0); >> } >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index b6af4b0..5e35a9d 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -1570,6 +1570,18 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_JMICRON, PCI_DEVICE_ID_JMICRON_JMB3 >> >> #endif >> >> +static void quirk_jmicron_async_suspend(struct pci_dev *dev) >> +{ >> + if (dev->multifunction) { >> + device_disable_async_suspend(&dev->dev); >> + dev_info(&dev->dev, "async suspend disabled to avoid multi-function power-on ordering issue\n"); >> + } >> +} >> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_IDE, 8, quirk_jmicron_async_suspend); >> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_SATA_AHCI, 0, quirk_jmicron_async_suspend); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x2362, quirk_jmicron_async_suspend); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x236f, quirk_jmicron_async_suspend); >> + >> #ifdef CONFIG_X86_IO_APIC >> static void quirk_alder_ioapic(struct pci_dev *pdev) >> { > > > -- 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
> > And are you sure you are already at kernel 4.6.1? ;) > > Jay > sorry, I meant "kernel 4.1.6", not 4.6.1 ;) so I will wait the final version of this patch -- 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 Fri, Aug 28, 2015 at 1:50 PM, Barto <mister.freeman@laposte.net> wrote: >> >> And are you sure you are already at kernel 4.6.1? ;) >> >> Jay >> > > sorry, I meant "kernel 4.1.6", not 4.6.1 ;) > > so I will wait the final version of this patch If you want to test this before v4.3-rc1, here it is: https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/misc&id=b95b73e30dfee0b96a3ecbb96c66486705701b9c My plan is to merge this during the v4.3 merge window, so it will appear in v4.3. I see that I probably should have added a stable tag as well. 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
I tested this patch on kernel 4.1.6, it's ok, no bugs, good job Le 28/08/2015 21:19, Bjorn Helgaas a écrit : > If you want to test this before v4.3-rc1, here it is: > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/misc&id=b95b73e30dfee0b96a3ecbb96c66486705701b9c > > My plan is to merge this during the v4.3 merge window, so it will > appear in v4.3. I see that I probably should have added a stable tag > as well. > > 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
On Fri, Aug 28, 2015 at 3:36 PM, Barto <mister.freeman@laposte.net> wrote: > I tested this patch on kernel 4.1.6, it's ok, no bugs, good job Thanks, I added your Tested-by and a stable tag for v3.15+. > Le 28/08/2015 21:19, Bjorn Helgaas a écrit : > >> If you want to test this before v4.3-rc1, here it is: >> >> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/misc&id=b95b73e30dfee0b96a3ecbb96c66486705701b9c >> >> My plan is to merge this during the v4.3 merge window, so it will >> appear in v4.3. I see that I probably should have added a stable tag >> as well. >> >> 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
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index 7e62751..a466602 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -351,6 +351,7 @@ static const struct pci_device_id ahci_pci_tbl[] = { /* JMicron 362B and 362C have an AHCI function with IDE class code */ { PCI_VDEVICE(JMICRON, 0x2362), board_ahci_ign_iferr }, { PCI_VDEVICE(JMICRON, 0x236f), board_ahci_ign_iferr }, + /* May need to update quirk_jmicron_async_suspend() for additions */ /* ATI */ { PCI_VDEVICE(ATI, 0x4380), board_ahci_sb600 }, /* ATI SB600 */ @@ -1451,18 +1452,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) else if (pdev->vendor == 0x177d && pdev->device == 0xa01c) ahci_pci_bar = AHCI_PCI_BAR_CAVIUM; - /* - * The JMicron chip 361/363 contains one SATA controller and one - * PATA controller,for powering on these both controllers, 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 these chips. - */ - 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); - /* acquire resources */ rc = pcim_enable_device(pdev); if (rc) diff --git a/drivers/ata/pata_jmicron.c b/drivers/ata/pata_jmicron.c index 47e418b..4d1a5d2 100644 --- a/drivers/ata/pata_jmicron.c +++ b/drivers/ata/pata_jmicron.c @@ -143,18 +143,6 @@ static int jmicron_init_one (struct pci_dev *pdev, const struct pci_device_id *i }; const struct ata_port_info *ppi[] = { &info, NULL }; - /* - * The JMicron chip 361/363 contains one SATA controller and one - * PATA controller,for powering on these both controllers, 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 these chips. - */ - 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); - return ata_pci_bmdma_init_one(pdev, ppi, &jmicron_sht, NULL, 0); } diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index b6af4b0..5e35a9d 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -1570,6 +1570,18 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_JMICRON, PCI_DEVICE_ID_JMICRON_JMB3 #endif +static void quirk_jmicron_async_suspend(struct pci_dev *dev) +{ + if (dev->multifunction) { + device_disable_async_suspend(&dev->dev); + dev_info(&dev->dev, "async suspend disabled to avoid multi-function power-on ordering issue\n"); + } +} +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_IDE, 8, quirk_jmicron_async_suspend); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_JMICRON, PCI_ANY, PCI_CLASS_STORAGE_SATA_AHCI, 0, quirk_jmicron_async_suspend); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x2362, quirk_jmicron_async_suspend); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_JMICRON, 0x236f, quirk_jmicron_async_suspend); + #ifdef CONFIG_X86_IO_APIC static void quirk_alder_ioapic(struct pci_dev *pdev) {