PCI: Disable async suspend/resume for Jmicron chip

Commit Message

Bjorn Helgaas Aug. 25, 2015, 2:49 a.m. UTC

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 <[e-mail]>
> 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.

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 <[e-mail]>
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 <[e-mail]>
    Signed-off-by: Zhang Rui <[e-mail]>
    Signed-off-by: Bjorn Helgaas <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Yinghai Lu Aug. 25, 2015, 8:21 p.m. UTC | #1

On Mon, Aug 24, 2015 at 7:49 PM, Bjorn Helgaas <[e-mail]> wrote:
> commit 09981db5b0c9a6865aa39126b71bc87718577e4b
> Author: Zhang Rui <[e-mail]>
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Aug. 25, 2015, 8:50 p.m. UTC | #2

On Tue, Aug 25, 2015 at 3:21 PM, Yinghai Lu <[e-mail]> wrote:
> On Mon, Aug 24, 2015 at 7:49 PM, Bjorn Helgaas <[e-mail]> wrote:
>> commit 09981db5b0c9a6865aa39126b71bc87718577e4b
>> Author: Zhang Rui <[e-mail]>
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zhang Rui Aug. 28, 2015, 8:21 a.m. UTC | #3

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 <[e-mail]>
> > 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 <[e-mail]>
> 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 <[e-mail]>
>     Signed-off-by: Zhang Rui <[e-mail]>
>     Signed-off-by: Bjorn Helgaas <[e-mail]>
> 
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Barto Aug. 28, 2015, 5:53 p.m. UTC | #4

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 <[e-mail]>
>> 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 <[e-mail]>
>>     Signed-off-by: Zhang Rui <[e-mail]>
>>     Signed-off-by: Bjorn Helgaas <[e-mail]>
>>
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Barto Aug. 28, 2015, 6:30 p.m. UTC | #5

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 <[e-mail]>
 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 <[e-mail]>
     Signed-off-by: Zhang Rui <[e-mail]>
     Signed-off-by: Bjorn Helgaas <[e-mail]>

 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 <[e-mail]>
>>> 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 <[e-mail]>
>> 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 <[e-mail]>
>>     Signed-off-by: Zhang Rui <[e-mail]>
>>     Signed-off-by: Bjorn Helgaas <[e-mail]>
>>
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Barto Aug. 28, 2015, 6:50 p.m. UTC | #6

> 
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Aug. 28, 2015, 7:19 p.m. UTC | #7

On Fri, Aug 28, 2015 at 1:50 PM, Barto <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Barto Aug. 28, 2015, 8:36 p.m. UTC | #8

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas Aug. 28, 2015, 8:54 p.m. UTC | #9

On Fri, Aug 28, 2015 at 3:36 PM, Barto <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)
 {