diff mbox series

libata: add ATA_HORKAGE_NOLPM for Pioneer BDR-207M and BDR-205

Message ID 20220926183718.480950-1-Niklas.Cassel@wdc.com
State New
Headers show
Series libata: add ATA_HORKAGE_NOLPM for Pioneer BDR-207M and BDR-205 | expand

Commit Message

Niklas Cassel Sept. 26, 2022, 6:38 p.m. UTC 
From: Niklas Cassel <niklas.cassel@wdc.com>

Commit 1527f69204fe ("ata: ahci: Add Green Sardine vendor ID as
board_ahci_mobile") added an explicit entry for AMD Green Sardine
AHCI controller using the board_ahci_mobile configuration (this
configuration has later been renamed to board_ahci_low_power).

The board_ahci_low_power configuration enables support for low power
modes.

This explicit entry takes precedence over the generic AHCI controller
entry, which does not enable support for low power modes.

Therefore, when commit 1527f69204fe ("ata: ahci: Add Green Sardine
vendor ID as board_ahci_mobile") was backported to stable kernels,
it make some Pioneer optical drives, which was working perfectly fine
before the commit was backported, stop working.

The real problem is that the Pioneer optical drives do not handle low
power modes correctly. If these optical drives would have been tested
on another AHCI controller using the board_ahci_low_power configuration,
this issue would have been detected earlier.

Unfortunately, the board_ahci_low_power configuration is only used in
less than 15% of the total AHCI controller entries, so many devices
have never been tested with an AHCI controller with low power modes.

Fixes: 1527f69204fe ("ata: ahci: Add Green Sardine vendor ID as board_ahci_mobile")
Cc: stable@vger.kernel.org
Reported-by: Jaap Berkhout <j.j.berkhout@staalenberk.nl>
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Mario Limonciello Sept. 26, 2022, 6:56 p.m. UTC | #1 
[Public]



> -----Original Message-----
> From: Niklas Cassel <Niklas.Cassel@wdc.com>
> Sent: Monday, September 26, 2022 13:38
> To: Damien Le Moal <damien.lemoal@opensource.wdc.com>; Limonciello,
> Mario <Mario.Limonciello@amd.com>
> Cc: Niklas Cassel <Niklas.Cassel@wdc.com>; stable@vger.kernel.org; Jaap
> Berkhout <j.j.berkhout@staalenberk.nl>; linux-ide@vger.kernel.org
> Subject: [PATCH] libata: add ATA_HORKAGE_NOLPM for Pioneer BDR-207M
> and BDR-205
> 
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Commit 1527f69204fe ("ata: ahci: Add Green Sardine vendor ID as
> board_ahci_mobile") added an explicit entry for AMD Green Sardine
> AHCI controller using the board_ahci_mobile configuration (this
> configuration has later been renamed to board_ahci_low_power).
> 
> The board_ahci_low_power configuration enables support for low power
> modes.
> 
> This explicit entry takes precedence over the generic AHCI controller
> entry, which does not enable support for low power modes.
> 
> Therefore, when commit 1527f69204fe ("ata: ahci: Add Green Sardine
> vendor ID as board_ahci_mobile") was backported to stable kernels,
> it make some Pioneer optical drives, which was working perfectly fine
> before the commit was backported, stop working.
> 
> The real problem is that the Pioneer optical drives do not handle low
> power modes correctly. If these optical drives would have been tested
> on another AHCI controller using the board_ahci_low_power configuration,
> this issue would have been detected earlier.
> 
> Unfortunately, the board_ahci_low_power configuration is only used in
> less than 15% of the total AHCI controller entries, so many devices
> have never been tested with an AHCI controller with low power modes.
> 
> Fixes: 1527f69204fe ("ata: ahci: Add Green Sardine vendor ID as
> board_ahci_mobile")
> Cc: stable@vger.kernel.org
> Reported-by: Jaap Berkhout <j.j.berkhout@staalenberk.nl>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

Thanks!

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Since Damien was still intending to modify the default policy so that LPM
applies everywhere I feel like more of this is going to show up.  Should we
consider to maybe keep all CD/DVD/BD devices excluded from LPM?

> ---
>  drivers/ata/libata-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 826d41f341e4..c9a9aa607b62 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3988,6 +3988,10 @@ static const struct ata_blacklist_entry
> ata_device_blacklist [] = {
>  	{ "PIONEER DVD-RW  DVR-212D",	NULL,
> 	ATA_HORKAGE_NOSETXFER },
>  	{ "PIONEER DVD-RW  DVR-216D",	NULL,
> 	ATA_HORKAGE_NOSETXFER },
> 
> +	/* These specific Pioneer models have LPM issues */
> +	{ "PIONEER BD-RW   BDR-207M",	NULL,
> 	ATA_HORKAGE_NOLPM },
> +	{ "PIONEER BD-RW   BDR-205",	NULL,	ATA_HORKAGE_NOLPM },
> +
>  	/* Crucial BX100 SSD 500GB has broken LPM support */
>  	{ "CT500BX100SSD1",		NULL,	ATA_HORKAGE_NOLPM },
> 
> --
> 2.37.3
Damien Le Moal Sept. 26, 2022, 11:12 p.m. UTC | #2 
On 9/27/22 03:56, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Niklas Cassel <Niklas.Cassel@wdc.com>
>> Sent: Monday, September 26, 2022 13:38
>> To: Damien Le Moal <damien.lemoal@opensource.wdc.com>; Limonciello,
>> Mario <Mario.Limonciello@amd.com>
>> Cc: Niklas Cassel <Niklas.Cassel@wdc.com>; stable@vger.kernel.org; Jaap
>> Berkhout <j.j.berkhout@staalenberk.nl>; linux-ide@vger.kernel.org
>> Subject: [PATCH] libata: add ATA_HORKAGE_NOLPM for Pioneer BDR-207M
>> and BDR-205
>>
>> From: Niklas Cassel <niklas.cassel@wdc.com>
>>
>> Commit 1527f69204fe ("ata: ahci: Add Green Sardine vendor ID as
>> board_ahci_mobile") added an explicit entry for AMD Green Sardine
>> AHCI controller using the board_ahci_mobile configuration (this
>> configuration has later been renamed to board_ahci_low_power).
>>
>> The board_ahci_low_power configuration enables support for low power
>> modes.
>>
>> This explicit entry takes precedence over the generic AHCI controller
>> entry, which does not enable support for low power modes.
>>
>> Therefore, when commit 1527f69204fe ("ata: ahci: Add Green Sardine
>> vendor ID as board_ahci_mobile") was backported to stable kernels,
>> it make some Pioneer optical drives, which was working perfectly fine
>> before the commit was backported, stop working.
>>
>> The real problem is that the Pioneer optical drives do not handle low
>> power modes correctly. If these optical drives would have been tested
>> on another AHCI controller using the board_ahci_low_power configuration,
>> this issue would have been detected earlier.
>>
>> Unfortunately, the board_ahci_low_power configuration is only used in
>> less than 15% of the total AHCI controller entries, so many devices
>> have never been tested with an AHCI controller with low power modes.
>>
>> Fixes: 1527f69204fe ("ata: ahci: Add Green Sardine vendor ID as
>> board_ahci_mobile")
>> Cc: stable@vger.kernel.org
>> Reported-by: Jaap Berkhout <j.j.berkhout@staalenberk.nl>
>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Thanks!
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> Since Damien was still intending to modify the default policy so that LPM
> applies everywhere I feel like more of this is going to show up.  Should we
> consider to maybe keep all CD/DVD/BD devices excluded from LPM?

I am moving carefully on that one for fear of (old) devices breaking
randomly... So not needed yet. But yeah, I definitely want to cleanup the
lpm code.

> 
>> ---
>>  drivers/ata/libata-core.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 826d41f341e4..c9a9aa607b62 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -3988,6 +3988,10 @@ static const struct ata_blacklist_entry
>> ata_device_blacklist [] = {
>>  	{ "PIONEER DVD-RW  DVR-212D",	NULL,
>> 	ATA_HORKAGE_NOSETXFER },
>>  	{ "PIONEER DVD-RW  DVR-216D",	NULL,
>> 	ATA_HORKAGE_NOSETXFER },
>>
>> +	/* These specific Pioneer models have LPM issues */
>> +	{ "PIONEER BD-RW   BDR-207M",	NULL,
>> 	ATA_HORKAGE_NOLPM },
>> +	{ "PIONEER BD-RW   BDR-205",	NULL,	ATA_HORKAGE_NOLPM },
>> +
>>  	/* Crucial BX100 SSD 500GB has broken LPM support */
>>  	{ "CT500BX100SSD1",		NULL,	ATA_HORKAGE_NOLPM },
>>
>> --
>> 2.37.3
Damien Le Moal Sept. 26, 2022, 11:22 p.m. UTC | #3 
On 9/27/22 03:38, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Commit 1527f69204fe ("ata: ahci: Add Green Sardine vendor ID as
> board_ahci_mobile") added an explicit entry for AMD Green Sardine
> AHCI controller using the board_ahci_mobile configuration (this
> configuration has later been renamed to board_ahci_low_power).
> 
> The board_ahci_low_power configuration enables support for low power
> modes.
> 
> This explicit entry takes precedence over the generic AHCI controller
> entry, which does not enable support for low power modes.
> 
> Therefore, when commit 1527f69204fe ("ata: ahci: Add Green Sardine
> vendor ID as board_ahci_mobile") was backported to stable kernels,
> it make some Pioneer optical drives, which was working perfectly fine
> before the commit was backported, stop working.
> 
> The real problem is that the Pioneer optical drives do not handle low
> power modes correctly. If these optical drives would have been tested
> on another AHCI controller using the board_ahci_low_power configuration,
> this issue would have been detected earlier.
> 
> Unfortunately, the board_ahci_low_power configuration is only used in
> less than 15% of the total AHCI controller entries, so many devices
> have never been tested with an AHCI controller with low power modes.
> 
> Fixes: 1527f69204fe ("ata: ahci: Add Green Sardine vendor ID as board_ahci_mobile")
> Cc: stable@vger.kernel.org
> Reported-by: Jaap Berkhout <j.j.berkhout@staalenberk.nl>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

Applied to for-6.0-fixes. Thanks !

> ---
>  drivers/ata/libata-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 826d41f341e4..c9a9aa607b62 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3988,6 +3988,10 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
>  	{ "PIONEER DVD-RW  DVR-212D",	NULL,	ATA_HORKAGE_NOSETXFER },
>  	{ "PIONEER DVD-RW  DVR-216D",	NULL,	ATA_HORKAGE_NOSETXFER },
>  
> +	/* These specific Pioneer models have LPM issues */
> +	{ "PIONEER BD-RW   BDR-207M",	NULL,	ATA_HORKAGE_NOLPM },
> +	{ "PIONEER BD-RW   BDR-205",	NULL,	ATA_HORKAGE_NOLPM },
> +
>  	/* Crucial BX100 SSD 500GB has broken LPM support */
>  	{ "CT500BX100SSD1",		NULL,	ATA_HORKAGE_NOLPM },
>
diff mbox series

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 826d41f341e4..c9a9aa607b62 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3988,6 +3988,10 @@  static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	{ "PIONEER DVD-RW  DVR-212D",	NULL,	ATA_HORKAGE_NOSETXFER },
 	{ "PIONEER DVD-RW  DVR-216D",	NULL,	ATA_HORKAGE_NOSETXFER },
 
+	/* These specific Pioneer models have LPM issues */
+	{ "PIONEER BD-RW   BDR-207M",	NULL,	ATA_HORKAGE_NOLPM },
+	{ "PIONEER BD-RW   BDR-205",	NULL,	ATA_HORKAGE_NOLPM },
+
 	/* Crucial BX100 SSD 500GB has broken LPM support */
 	{ "CT500BX100SSD1",		NULL,	ATA_HORKAGE_NOLPM },