[PATCH #upstream-fixes] libata: disable ATAPI AN by default

[PATCH #upstream-fixes] libata: disable ATAPI AN by default

[Tejun Heo]

There are ATAPI devices which raise AN when hit by commands issued by
open().  This leads to infinite loop of AN -> MEDIA_CHANGE uevent ->
udev open() to check media -> AN.

Both ACS and SerialATA standards don't define in which case ATAPI
devices are supposed to raise or not raise AN.  They both list media
insertion event as a possible use case for ATAPI ANs but there is no
clear description of what constitutes such events.  As such, it seems
a bit too naive to export ANs directly to userland as MEDIA_CHANGE
events without further verification (which should behave similarly to
windows as it apparently is the only thing that some hardware vendors
are testing against).

This patch adds libata.atapi_an module parameter and disables ATAPI AN
by default for now.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Cc: Nick Bowler <nbowler@elliptictech.com>
Cc: David Zeuthen <david@fubar.dk>
Cc: stable@kernel.org
---
 drivers/ata/libata-core.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 49cffb6..5abab5d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -160,6 +160,10 @@ int libata_allow_tpm = 0;
 module_param_named(allow_tpm, libata_allow_tpm, int, 0444);
 MODULE_PARM_DESC(allow_tpm, "Permit the use of TPM commands (0=off [default], 1=on)");

+static int atapi_an;
+module_param(atapi_an, int, 0444);
+MODULE_PARM_DESC(atapi_an, "Enable ATAPI AN media presence notification (0=0ff [default], 1=on)");
+
 MODULE_AUTHOR("Jeff Garzik");
 MODULE_DESCRIPTION("Library module for ATA devices");
 MODULE_LICENSE("GPL");
@@ -2572,7 +2576,8 @@ int ata_dev_configure(struct ata_device *dev)
 		 * to enable ATAPI AN to discern between PHY status
 		 * changed notifications and ATAPI ANs.
 		 */
-		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_atapi_AN(id) &&
+		if (atapi_an &&
+		    (ap->flags & ATA_FLAG_AN) && ata_id_has_atapi_AN(id) &&
 		    (!sata_pmp_attached(ap) ||
 		     sata_scr_read(&ap->link, SCR_NOTIFICATION, &sntf) == 0)) {
 			unsigned int err_mask;

Re: [PATCH #upstream-fixes] libata: disable ATAPI AN by default

[Jeff Garzik]

On 05/19/2010 09:38 AM, Tejun Heo wrote:
> There are ATAPI devices which raise AN when hit by commands issued by
> open().  This leads to infinite loop of AN ->  MEDIA_CHANGE uevent ->
> udev open() to check media ->  AN.
>
> Both ACS and SerialATA standards don't define in which case ATAPI
> devices are supposed to raise or not raise AN.  They both list media
> insertion event as a possible use case for ATAPI ANs but there is no
> clear description of what constitutes such events.  As such, it seems
> a bit too naive to export ANs directly to userland as MEDIA_CHANGE
> events without further verification (which should behave similarly to
> windows as it apparently is the only thing that some hardware vendors
> are testing against).

Windows completely disables AN?

Or do we simply need to be less naive about _delivery_ of AN's?

	Jeff

Re: [PATCH #upstream-fixes] libata: disable ATAPI AN by default

[Tejun Heo]

Hello,

On 05/19/2010 06:14 PM, Jeff Garzik wrote:
> Windows completely disables AN?
> 
> Or do we simply need to be less naive about _delivery_ of AN's?

I think we first need to verify what the event is about before
delivering it to userland; IOW, it should kick a full polling op.  The
problem is the same with periodic polling.  Windows does it with
single command but we can't do that from userland as open() involves
issuing more commands.  That discrepancy is basically we're having all
these problems with periodic polling and AN infinite looping.

Hardware vendors don't consider cases where their devices are hit with
a series of commands periodically or after raising an AN event.  We
have quite a few drives which just die after being hit repeatedly with
media presence polling commands and this one is causing infinite loop
by re-raising AN.

So, until we can replicate the windows behavior (which actually is
pretty reasonable - just use single GET_CONFIGURATION call for polling
and status check), I think it's wiser to disable AN.

Thanks.

-- 
tejun

Re: [PATCH #upstream-fixes] libata: disable ATAPI AN by default

[Nick Bowler]

On 15:38 Wed 19 May     , Tejun Heo wrote:
> There are ATAPI devices which raise AN when hit by commands issued by
> open().  This leads to infinite loop of AN -> MEDIA_CHANGE uevent ->
> udev open() to check media -> AN.
> 
> Both ACS and SerialATA standards don't define in which case ATAPI
> devices are supposed to raise or not raise AN.  They both list media
> insertion event as a possible use case for ATAPI ANs but there is no
> clear description of what constitutes such events.  As such, it seems
> a bit too naive to export ANs directly to userland as MEDIA_CHANGE
> events without further verification (which should behave similarly to
> windows as it apparently is the only thing that some hardware vendors
> are testing against).
> 
> This patch adds libata.atapi_an module parameter and disables ATAPI AN
> by default for now.

Works here, drive now spins down properly.

Tested-By: Nick Bowler <nbowler@draconx.ca>

-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: [PATCH #upstream-fixes] libata: disable ATAPI AN by default

[Robert Hancock]

On 05/19/2010 10:53 AM, Tejun Heo wrote:
> Hello,
>
> On 05/19/2010 06:14 PM, Jeff Garzik wrote:
>> Windows completely disables AN?
>>
>> Or do we simply need to be less naive about _delivery_ of AN's?
>
> I think we first need to verify what the event is about before
> delivering it to userland; IOW, it should kick a full polling op.  The
> problem is the same with periodic polling.  Windows does it with
> single command but we can't do that from userland as open() involves
> issuing more commands.  That discrepancy is basically we're having all
> these problems with periodic polling and AN infinite looping.
>
> Hardware vendors don't consider cases where their devices are hit with
> a series of commands periodically or after raising an AN event.  We
> have quite a few drives which just die after being hit repeatedly with
> media presence polling commands and this one is causing infinite loop
> by re-raising AN.
>
> So, until we can replicate the windows behavior (which actually is
> pretty reasonable - just use single GET_CONFIGURATION call for polling
> and status check), I think it's wiser to disable AN.

There are also some drives where AN is just broken - I have a Lite-ON 
DH-401S BD-ROM drive which raises AN on opening the tray, but not on 
closing with media inserted.

I don't know if Windows is using AN at all yet or not. Seems like a lot 
of drives aren't tested well with it though.

Re: [PATCH #upstream-fixes] libata: disable ATAPI AN by default

[Jeff Garzik]

On 05/19/2010 09:38 AM, Tejun Heo wrote:
> There are ATAPI devices which raise AN when hit by commands issued by
> open().  This leads to infinite loop of AN ->  MEDIA_CHANGE uevent ->
> udev open() to check media ->  AN.
>
> Both ACS and SerialATA standards don't define in which case ATAPI
> devices are supposed to raise or not raise AN.  They both list media
> insertion event as a possible use case for ATAPI ANs but there is no
> clear description of what constitutes such events.  As such, it seems
> a bit too naive to export ANs directly to userland as MEDIA_CHANGE
> events without further verification (which should behave similarly to
> windows as it apparently is the only thing that some hardware vendors
> are testing against).
>
> This patch adds libata.atapi_an module parameter and disables ATAPI AN
> by default for now.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> Cc: Kay Sievers<kay.sievers@vrfy.org>
> Cc: Nick Bowler<nbowler@elliptictech.com>
> Cc: David Zeuthen<david@fubar.dk>
> Cc: stable@kernel.org
> ---
>   drivers/ata/libata-core.c |    7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)

applied