[patch 1/7] libata: check for AN support

[patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

Check to see if an ATAPI device supports Asynchronous Notification.
If so, enable it.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -70,6 +70,7 @@ const unsigned long sata_deb_timing_long
 static unsigned int ata_dev_init_params(struct ata_device *dev,
 					u16 heads, u16 sectors);
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
+static unsigned int ata_dev_set_AN(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 
 static unsigned int ata_print_id = 1;
@@ -1744,6 +1745,23 @@ int ata_dev_configure(struct ata_device 
 		}
 		dev->cdb_len = (unsigned int) rc;
 
+		/*
+		 * check to see if this ATAPI device supports
+		 * Asynchronous Notification
+		 */
+		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id))
+		{
+			/* issue SET feature command to turn this on */
+			rc = ata_dev_set_AN(dev);
+			if (rc) {
+				ata_dev_printk(dev, KERN_ERR,
+						"unable to set AN\n");
+				rc = -EINVAL;
+				goto err_out_nosup;
+			}
+			dev->flags |= ATA_DFLAG_AN;
+		}
+
 		if (ata_id_cdb_intr(dev->id)) {
 			dev->flags |= ATA_DFLAG_CDB_INTR;
 			cdb_intr_string = ", CDB intr";
@@ -3525,6 +3543,42 @@ static unsigned int ata_dev_set_xfermode
 }
 
 /**
+ *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
+ *                       with sector count set to indicate
+ *                       Asynchronous Notification feature
+ *	@dev: Device to which command will be sent
+ *
+ *	Issue SET FEATURES - SATA FEATURES command to device @dev
+ *	on port @ap.
+ *
+ *	LOCKING:
+ *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, AC_ERR_* mask otherwise.
+ */
+static unsigned int ata_dev_set_AN(struct ata_device *dev)
+{
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+
+	/* set up set-features taskfile */
+	DPRINTK("set features - SATA features\n");
+
+	ata_tf_init(dev, &tf);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_SATA_ENABLE;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = SATA_AN;
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
+
+	DPRINTK("EXIT, err_mask=%x\n", err_mask);
+	return err_mask;
+}
+
+/**
  *	ata_dev_init_params - Issue INIT DEV PARAMS command
  *	@dev: Device to which command will be sent
  *	@heads: Number of heads (taskfile parameter)
Index: 2.6-git/include/linux/ata.h
===================================================================
--- 2.6-git.orig/include/linux/ata.h
+++ 2.6-git/include/linux/ata.h
@@ -194,6 +194,12 @@ enum {
 	SETFEATURES_WC_ON	= 0x02, /* Enable write cache */
 	SETFEATURES_WC_OFF	= 0x82, /* Disable write cache */
 
+	SETFEATURES_SATA_ENABLE = 0x10, /* Enable use of SATA feature */
+	SETFEATURES_SATA_DISABLE = 0x90, /* Disable use of SATA feature */
+
+	/* SETFEATURE Sector counts for SATA features */
+	SATA_AN			= 0x05,  /* Asynchronous Notification */
+
 	/* ATAPI stuff */
 	ATAPI_PKT_DMA		= (1 << 0),
 	ATAPI_DMADIR		= (1 << 2),	/* ATAPI data dir:
@@ -299,6 +305,8 @@ struct ata_taskfile {
 #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
 #define ata_id_removeable(id)	((id)[0] & (1 << 7))
 #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
+#define ata_id_has_AN(id)	\
+	((id[76] && (~id[76])) & ((id)[78] & (1 << 5)))
 #define ata_id_iordy_disable(id) ((id)[49] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[49] & (1 << 9))
 #define ata_id_u32(id,n)	\
Index: 2.6-git/include/linux/libata.h
===================================================================
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
@@ -174,6 +175,7 @@ enum {
 	ATA_FLAG_SETXFER_POLLING= (1 << 14), /* use polling for SETXFER */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
+	ATA_FLAG_AN		= (1 << 17), /* controller supports AN */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be

-- 

Re: [patch 1/7] libata: check for AN support

[Tejun Heo]

Hello,

Kristen Carlson Accardi wrote:
>  static unsigned int ata_print_id = 1;
> @@ -1744,6 +1745,23 @@ int ata_dev_configure(struct ata_device 
>  		}
>  		dev->cdb_len = (unsigned int) rc;
>  
> +		/*
> +		 * check to see if this ATAPI device supports
> +		 * Asynchronous Notification
> +		 */
> +		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id))
> +		{
> +			/* issue SET feature command to turn this on */
> +			rc = ata_dev_set_AN(dev);

Please don't store err_mask into int rc.  Please store it to a separate
err_mask variable and report it when printing error message.

> +			if (rc) {
> +				ata_dev_printk(dev, KERN_ERR,
> +						"unable to set AN\n");
> +				rc = -EINVAL;

Wouldn't -EIO be more appropriate?

> +				goto err_out_nosup;
> +			}
> +			dev->flags |= ATA_DFLAG_AN;
> +		}
> +

Not NACKing.  Just notes for future improvements.  We need to be more
careful here.  ATA/ATAPI world is filled with braindamaged devices and I
bet there are devices which advertises it can do AN but chokes when AN
is enabled.

This should be handled similarly to ACPI failure.  Currently ACPI does
the following.

1. try once, if fail, record that ACPI failed.  return error to trigger
retry.
2. try again, if fail again, ignore error if possible (!FROZEN) and turn
off ACPI.

This fallback mechanism for optional features can probably be
generalized and used for both ACPI and AN.

-- 
tejun

Re: [patch 1/7] libata: check for AN support

[Alan Cox]

> +		/*
> +		 * check to see if this ATAPI device supports
> +		 * Asynchronous Notification
> +		 */
> +		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id))
> +		{

Bracketing police ^^^

> +			/* issue SET feature command to turn this on */
> +			rc = ata_dev_set_AN(dev);
> +			if (rc) {
> +				ata_dev_printk(dev, KERN_ERR,
> +						"unable to set AN\n");
> +				rc = -EINVAL;
> +				goto err_out_nosup;

How fatal is this - do we need to ignore the device at this point or
should we just pretend (possibly correctly) that the device itself does
not support notification. 

> @@ -299,6 +305,8 @@ struct ata_taskfile {
>  #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
>  #define ata_id_removeable(id)	((id)[0] & (1 << 7))
>  #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
> +#define ata_id_has_AN(id)	\
> +	((id[76] && (~id[76])) & ((id)[78] & (1 << 5)))

Might be nice to check ATA version as well to be paranoid but this all
looks ok as its a reserved field since way back when.

Re: [patch 1/7] libata: check for AN support

[Olivier Galibert]

Sorry for replying to Alan's reply, I missed the original mail.

> > +#define ata_id_has_AN(id)	\
> > +	((id[76] && (~id[76])) & ((id)[78] & (1 << 5)))

(a && ~a) & (b & 32)

I don't think that does what you think it does, because at that point
it's a funny way to write 0 ((0 or 1) binary-and (0 or 32)).

I'm not even sure what it is you want.  If for the first part you
wanted (id[76] != 0x00 && id[76] != 0xff), please write just that,
thanks :-)

  OG.

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

On Tue, 24 Apr 2007 12:23:04 +0200
Olivier Galibert <galibert@pobox.com> wrote:

> Sorry for replying to Alan's reply, I missed the original mail.
> 
> > > +#define ata_id_has_AN(id)	\
> > > +	((id[76] && (~id[76])) & ((id)[78] & (1 << 5)))
> 
> (a && ~a) & (b & 32)
> 
> I don't think that does what you think it does, because at that point
> it's a funny way to write 0 ((0 or 1) binary-and (0 or 32)).
> 
> I'm not even sure what it is you want.  If for the first part you
> wanted (id[76] != 0x00 && id[76] != 0xff), please write just that,
> thanks :-)
> 
>   OG.
> 

>From the serial ata spec, we have:

13.2.1.18        Word 78: Serial ATA features supported
If Word 76 is not 0000h or FFFFh, Word 78 reports the optional features 
supported by the device.  Support for this word is optional and if not 
supported the word shall be zero indicating the device has no support for new 
Serial ATA capabilities.

so, basically yes, I'm really testing to make sure that word 76 isn't 0 or all
one then using that value & with value of bit in work 78 to determine AN
support - if you think this is really obfuscated, I've got no problem changing 
it - there's obviously many ways to mess around with bits.

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

On Tue, 24 Apr 2007 17:03:29 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Hello,
> 
> Kristen Carlson Accardi wrote:
> >  static unsigned int ata_print_id = 1;
> > @@ -1744,6 +1745,23 @@ int ata_dev_configure(struct ata_device 
> >  		}
> >  		dev->cdb_len = (unsigned int) rc;
> >  
> > +		/*
> > +		 * check to see if this ATAPI device supports
> > +		 * Asynchronous Notification
> > +		 */
> > +		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id))
> > +		{
> > +			/* issue SET feature command to turn this on */
> > +			rc = ata_dev_set_AN(dev);
> 
> Please don't store err_mask into int rc.  Please store it to a separate
> err_mask variable and report it when printing error message.
> 
> > +			if (rc) {
> > +				ata_dev_printk(dev, KERN_ERR,
> > +						"unable to set AN\n");
> > +				rc = -EINVAL;
> 
> Wouldn't -EIO be more appropriate?

I think Alan is right - and being unable to turn on AN should not be fatal.
I'll just change all this code to just print the err and keep going.

> 
> > +				goto err_out_nosup;
> > +			}
> > +			dev->flags |= ATA_DFLAG_AN;
> > +		}
> > +
> 
> Not NACKing.  Just notes for future improvements.  We need to be more
> careful here.  ATA/ATAPI world is filled with braindamaged devices and I
> bet there are devices which advertises it can do AN but chokes when AN
> is enabled.
> 
> This should be handled similarly to ACPI failure.  Currently ACPI does
> the following.
> 
> 1. try once, if fail, record that ACPI failed.  return error to trigger
> retry.
> 2. try again, if fail again, ignore error if possible (!FROZEN) and turn
> off ACPI.
> 
> This fallback mechanism for optional features can probably be
> generalized and used for both ACPI and AN.

Ok - meanwhile I think it's appropriate here to just do try-once-fail-give-up.

Re: [patch 1/7] libata: check for AN support

[Olivier Galibert]

On Tue, Apr 24, 2007 at 08:49:04AM -0700, Kristen Carlson Accardi wrote:
> On Tue, 24 Apr 2007 12:23:04 +0200
> Olivier Galibert <galibert@pobox.com> wrote:
> 
> > Sorry for replying to Alan's reply, I missed the original mail.
> > 
> > > > +#define ata_id_has_AN(id)	\
> > > > +	((id[76] && (~id[76])) & ((id)[78] & (1 << 5)))
> > 
> > (a && ~a) & (b & 32)
> > 
> > I don't think that does what you think it does, because at that point
> > it's a funny way to write 0 ((0 or 1) binary-and (0 or 32)).
> > 
> > I'm not even sure what it is you want.  If for the first part you
> > wanted (id[76] != 0x00 && id[76] != 0xff), please write just that,
> > thanks :-)
> > 
> >   OG.
> > 
> 
> >From the serial ata spec, we have:
> 
> 13.2.1.18        Word 78: Serial ATA features supported
> If Word 76 is not 0000h or FFFFh, Word 78 reports the optional features 
> supported by the device.  Support for this word is optional and if not 
> supported the word shall be zero indicating the device has no support for new 
> Serial ATA capabilities.
> 
> so, basically yes, I'm really testing to make sure that word 76 isn't 0 or all
> one then using that value & with value of bit in work 78 to determine AN
> support - if you think this is really obfuscated, I've got no problem changing 
> it - there's obviously many ways to mess around with bits.

& is not &&, so right now it's really incorrect.  1 & 32 is 0.

((id)[76] != 0x0000 && (id)[76] != 0xffff && ((id)[78] & (1 << 5)))

The implicit typing of id looks dangerous to me, but you're not the
one who has started it.

  OG.

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

On Tue, 24 Apr 2007 20:05:52 +0200
Olivier Galibert <galibert@pobox.com> wrote:

> On Tue, Apr 24, 2007 at 08:49:04AM -0700, Kristen Carlson Accardi wrote:
> > On Tue, 24 Apr 2007 12:23:04 +0200
> > Olivier Galibert <galibert@pobox.com> wrote:
> > 
> > > Sorry for replying to Alan's reply, I missed the original mail.
> > > 
> > > > > +#define ata_id_has_AN(id)	\
> > > > > +	((id[76] && (~id[76])) & ((id)[78] & (1 << 5)))
> > > 
> > > (a && ~a) & (b & 32)
> > > 
> > > I don't think that does what you think it does, because at that point
> > > it's a funny way to write 0 ((0 or 1) binary-and (0 or 32)).
> > > 
> > > I'm not even sure what it is you want.  If for the first part you
> > > wanted (id[76] != 0x00 && id[76] != 0xff), please write just that,
> > > thanks :-)
> > > 
> > >   OG.
> > > 
> > 
> > >From the serial ata spec, we have:
> > 
> > 13.2.1.18        Word 78: Serial ATA features supported
> > If Word 76 is not 0000h or FFFFh, Word 78 reports the optional features 
> > supported by the device.  Support for this word is optional and if not 
> > supported the word shall be zero indicating the device has no support for new 
> > Serial ATA capabilities.
> > 
> > so, basically yes, I'm really testing to make sure that word 76 isn't 0 or all
> > one then using that value & with value of bit in work 78 to determine AN
> > support - if you think this is really obfuscated, I've got no problem changing 
> > it - there's obviously many ways to mess around with bits.
> 
> & is not &&, so right now it's really incorrect.  1 & 32 is 0.

ah - ok, gotcha, thanks.

> 
> ((id)[76] != 0x0000 && (id)[76] != 0xffff && ((id)[78] & (1 << 5)))
> 
> The implicit typing of id looks dangerous to me, but you're not the
> one who has started it.
> 
>   OG.
> 

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

Check to see if an ATAPI device supports Asynchronous Notification.
If so, enable it.

changes from last version: 
* fix typo in ata_id_has_AN and make word 76 test more clear
* If we fail to set the AN feature, just print a warning and continue
 
Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -70,6 +70,7 @@ const unsigned long sata_deb_timing_long
 static unsigned int ata_dev_init_params(struct ata_device *dev,
 					u16 heads, u16 sectors);
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
+static unsigned int ata_dev_set_AN(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 
 static unsigned int ata_print_id = 1;
@@ -1744,6 +1745,22 @@ int ata_dev_configure(struct ata_device 
 		}
 		dev->cdb_len = (unsigned int) rc;
 
+		/*
+		 * check to see if this ATAPI device supports
+		 * Asynchronous Notification
+		 */
+		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id)) {
+			int err;
+			/* issue SET feature command to turn this on */
+			err = ata_dev_set_AN(dev);
+			if (err)
+				ata_dev_printk(dev, KERN_ERR,
+						"unable to set AN, err %x\n",
+						err);
+			else
+				dev->flags |= ATA_DFLAG_AN;
+		}
+
 		if (ata_id_cdb_intr(dev->id)) {
 			dev->flags |= ATA_DFLAG_CDB_INTR;
 			cdb_intr_string = ", CDB intr";
@@ -3525,6 +3542,42 @@ static unsigned int ata_dev_set_xfermode
 }
 
 /**
+ *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
+ *                       with sector count set to indicate
+ *                       Asynchronous Notification feature
+ *	@dev: Device to which command will be sent
+ *
+ *	Issue SET FEATURES - SATA FEATURES command to device @dev
+ *	on port @ap.
+ *
+ *	LOCKING:
+ *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, AC_ERR_* mask otherwise.
+ */
+static unsigned int ata_dev_set_AN(struct ata_device *dev)
+{
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+
+	/* set up set-features taskfile */
+	DPRINTK("set features - SATA features\n");
+
+	ata_tf_init(dev, &tf);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_SATA_ENABLE;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = SATA_AN;
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
+
+	DPRINTK("EXIT, err_mask=%x\n", err_mask);
+	return err_mask;
+}
+
+/**
  *	ata_dev_init_params - Issue INIT DEV PARAMS command
  *	@dev: Device to which command will be sent
  *	@heads: Number of heads (taskfile parameter)
Index: 2.6-git/include/linux/ata.h
===================================================================
--- 2.6-git.orig/include/linux/ata.h
+++ 2.6-git/include/linux/ata.h
@@ -194,6 +194,12 @@ enum {
 	SETFEATURES_WC_ON	= 0x02, /* Enable write cache */
 	SETFEATURES_WC_OFF	= 0x82, /* Disable write cache */
 
+	SETFEATURES_SATA_ENABLE = 0x10, /* Enable use of SATA feature */
+	SETFEATURES_SATA_DISABLE = 0x90, /* Disable use of SATA feature */
+
+	/* SETFEATURE Sector counts for SATA features */
+	SATA_AN			= 0x05,  /* Asynchronous Notification */
+
 	/* ATAPI stuff */
 	ATAPI_PKT_DMA		= (1 << 0),
 	ATAPI_DMADIR		= (1 << 2),	/* ATAPI data dir:
@@ -299,6 +305,8 @@ struct ata_taskfile {
 #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
 #define ata_id_removeable(id)	((id)[0] & (1 << 7))
 #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
+#define ata_id_has_AN(id)	\
+	(((id[76] != 0x0000) && (id[76] != 0xffff)) && ((id)[78] & (1 << 5)))
 #define ata_id_iordy_disable(id) ((id)[49] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[49] & (1 << 9))
 #define ata_id_u32(id,n)	\
Index: 2.6-git/include/linux/libata.h
===================================================================
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
@@ -174,6 +175,7 @@ enum {
 	ATA_FLAG_SETXFER_POLLING= (1 << 14), /* use polling for SETXFER */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
+	ATA_FLAG_AN		= (1 << 17), /* controller supports AN */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be

Re: [patch 1/7] libata: check for AN support

[Olivier Galibert]

On Tue, Apr 24, 2007 at 01:53:27PM -0700, Kristen Carlson Accardi wrote:
> Check to see if an ATAPI device supports Asynchronous Notification.
> If so, enable it.
> 
> changes from last version: 
> * fix typo in ata_id_has_AN and make word 76 test more clear
> * If we fail to set the AN feature, just print a warning and continue
>  
> Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> @@ -299,6 +305,8 @@ struct ata_taskfile {
>  #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
>  #define ata_id_removeable(id)	((id)[0] & (1 << 7))
>  #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
> +#define ata_id_has_AN(id)	\
> +	(((id[76] != 0x0000) && (id[76] != 0xffff)) && ((id)[78] & (1 << 5)))

(id)[76] I guess ?  Sorry for being a pain :/

  OG.

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

On Wed, 25 Apr 2007 02:49:46 +0200
Olivier Galibert <galibert@pobox.com> wrote:

> On Tue, Apr 24, 2007 at 01:53:27PM -0700, Kristen Carlson Accardi wrote:
> > Check to see if an ATAPI device supports Asynchronous Notification.
> > If so, enable it.
> > 
> > changes from last version: 
> > * fix typo in ata_id_has_AN and make word 76 test more clear
> > * If we fail to set the AN feature, just print a warning and continue
> >  
> > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > 
> > @@ -299,6 +305,8 @@ struct ata_taskfile {
> >  #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
> >  #define ata_id_removeable(id)	((id)[0] & (1 << 7))
> >  #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
> > +#define ata_id_has_AN(id)	\
> > +	(((id[76] != 0x0000) && (id[76] != 0xffff)) && ((id)[78] & (1 << 5)))
> 
> (id)[76] I guess ?  Sorry for being a pain :/
> 
>   OG.
> 

Ok - I'll fix that.  Thank you for being a pain :), I really appreciate
the time you are taking to review my patches.

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

Check to see if an ATAPI device supports Asynchronous Notification.
If so, enable it.

Changes from last version:
* use parens around id in ata.h

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -70,6 +70,7 @@ const unsigned long sata_deb_timing_long
 static unsigned int ata_dev_init_params(struct ata_device *dev,
 					u16 heads, u16 sectors);
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
+static unsigned int ata_dev_set_AN(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 
 static unsigned int ata_print_id = 1;
@@ -1744,6 +1745,22 @@ int ata_dev_configure(struct ata_device 
 		}
 		dev->cdb_len = (unsigned int) rc;
 
+		/*
+		 * check to see if this ATAPI device supports
+		 * Asynchronous Notification
+		 */
+		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id)) {
+			int err;
+			/* issue SET feature command to turn this on */
+			err = ata_dev_set_AN(dev);
+			if (err)
+				ata_dev_printk(dev, KERN_ERR,
+						"unable to set AN, err %x\n",
+						err);
+			else
+				dev->flags |= ATA_DFLAG_AN;
+		}
+
 		if (ata_id_cdb_intr(dev->id)) {
 			dev->flags |= ATA_DFLAG_CDB_INTR;
 			cdb_intr_string = ", CDB intr";
@@ -3525,6 +3542,42 @@ static unsigned int ata_dev_set_xfermode
 }
 
 /**
+ *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
+ *                       with sector count set to indicate
+ *                       Asynchronous Notification feature
+ *	@dev: Device to which command will be sent
+ *
+ *	Issue SET FEATURES - SATA FEATURES command to device @dev
+ *	on port @ap.
+ *
+ *	LOCKING:
+ *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, AC_ERR_* mask otherwise.
+ */
+static unsigned int ata_dev_set_AN(struct ata_device *dev)
+{
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+
+	/* set up set-features taskfile */
+	DPRINTK("set features - SATA features\n");
+
+	ata_tf_init(dev, &tf);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_SATA_ENABLE;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = SATA_AN;
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
+
+	DPRINTK("EXIT, err_mask=%x\n", err_mask);
+	return err_mask;
+}
+
+/**
  *	ata_dev_init_params - Issue INIT DEV PARAMS command
  *	@dev: Device to which command will be sent
  *	@heads: Number of heads (taskfile parameter)
Index: 2.6-git/include/linux/ata.h
===================================================================
--- 2.6-git.orig/include/linux/ata.h
+++ 2.6-git/include/linux/ata.h
@@ -194,6 +194,12 @@ enum {
 	SETFEATURES_WC_ON	= 0x02, /* Enable write cache */
 	SETFEATURES_WC_OFF	= 0x82, /* Disable write cache */
 
+	SETFEATURES_SATA_ENABLE = 0x10, /* Enable use of SATA feature */
+	SETFEATURES_SATA_DISABLE = 0x90, /* Disable use of SATA feature */
+
+	/* SETFEATURE Sector counts for SATA features */
+	SATA_AN			= 0x05,  /* Asynchronous Notification */
+
 	/* ATAPI stuff */
 	ATAPI_PKT_DMA		= (1 << 0),
 	ATAPI_DMADIR		= (1 << 2),	/* ATAPI data dir:
@@ -299,6 +305,9 @@ struct ata_taskfile {
 #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
 #define ata_id_removeable(id)	((id)[0] & (1 << 7))
 #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
+#define ata_id_has_AN(id)	\
+	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
+	  ((id)[78] & (1 << 5)) )
 #define ata_id_iordy_disable(id) ((id)[49] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[49] & (1 << 9))
 #define ata_id_u32(id,n)	\
Index: 2.6-git/include/linux/libata.h
===================================================================
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
@@ -174,6 +175,7 @@ enum {
 	ATA_FLAG_SETXFER_POLLING= (1 << 14), /* use polling for SETXFER */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
+	ATA_FLAG_AN		= (1 << 17), /* controller supports AN */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be

Re: [patch 1/7] libata: check for AN support

[Matt Sealey]

Kristen Carlson Accardi wrote:
> Check to see if an ATAPI device supports Asynchronous Notification.
> If so, enable it.
> 
> Changes from last version:
> * use parens around id in ata.h
> 
> Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> Index: 2.6-git/drivers/ata/libata-core.c
> ===================================================================
> --- 2.6-git.orig/drivers/ata/libata-core.c
> +++ 2.6-git/drivers/ata/libata-core.c
> @@ -70,6 +70,7 @@ const unsigned long sata_deb_timing_long
>  static unsigned int ata_dev_init_params(struct ata_device *dev,
>  					u16 heads, u16 sectors);
>  static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
> +static unsigned int ata_dev_set_AN(struct ata_device *dev);
>  static void ata_dev_xfermask(struct ata_device *dev);
>  
>  static unsigned int ata_print_id = 1;
> @@ -1744,6 +1745,22 @@ int ata_dev_configure(struct ata_device 
>  		}
>  		dev->cdb_len = (unsigned int) rc;
>  
> +		/*
> +		 * check to see if this ATAPI device supports
> +		 * Asynchronous Notification
> +		 */
> +		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id)) {
> +			int err;
> +			/* issue SET feature command to turn this on */
> +			err = ata_dev_set_AN(dev);
> +			if (err)
> +				ata_dev_printk(dev, KERN_ERR,
> +						"unable to set AN, err %x\n",
> +						err);
> +			else
> +				dev->flags |= ATA_DFLAG_AN;
> +		}
> +
>  		if (ata_id_cdb_intr(dev->id)) {
>  			dev->flags |= ATA_DFLAG_CDB_INTR;
>  			cdb_intr_string = ", CDB intr";
> @@ -3525,6 +3542,42 @@ static unsigned int ata_dev_set_xfermode
>  }
>  
>  /**
> + *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
> + *                       with sector count set to indicate
> + *                       Asynchronous Notification feature
> + *	@dev: Device to which command will be sent
> + *
> + *	Issue SET FEATURES - SATA FEATURES command to device @dev
> + *	on port @ap.
> + *
> + *	LOCKING:
> + *	PCI/etc. bus probe sem.
> + *
> + *	RETURNS:
> + *	0 on success, AC_ERR_* mask otherwise.
> + */
> +static unsigned int ata_dev_set_AN(struct ata_device *dev)
> +{
> +	struct ata_taskfile tf;
> +	unsigned int err_mask;
> +
> +	/* set up set-features taskfile */
> +	DPRINTK("set features - SATA features\n");
> +
> +	ata_tf_init(dev, &tf);
> +	tf.command = ATA_CMD_SET_FEATURES;
> +	tf.feature = SETFEATURES_SATA_ENABLE;
> +	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> +	tf.protocol = ATA_PROT_NODATA;
> +	tf.nsect = SATA_AN;
> +
> +	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
> +
> +	DPRINTK("EXIT, err_mask=%x\n", err_mask);
> +	return err_mask;
> +}
> +
> +/**
>   *	ata_dev_init_params - Issue INIT DEV PARAMS command
>   *	@dev: Device to which command will be sent
>   *	@heads: Number of heads (taskfile parameter)
> Index: 2.6-git/include/linux/ata.h
> ===================================================================
> --- 2.6-git.orig/include/linux/ata.h
> +++ 2.6-git/include/linux/ata.h
> @@ -194,6 +194,12 @@ enum {
>  	SETFEATURES_WC_ON	= 0x02, /* Enable write cache */
>  	SETFEATURES_WC_OFF	= 0x82, /* Disable write cache */
>  
> +	SETFEATURES_SATA_ENABLE = 0x10, /* Enable use of SATA feature */
> +	SETFEATURES_SATA_DISABLE = 0x90, /* Disable use of SATA feature */
> +
> +	/* SETFEATURE Sector counts for SATA features */
> +	SATA_AN			= 0x05,  /* Asynchronous Notification */
> +
>  	/* ATAPI stuff */
>  	ATAPI_PKT_DMA		= (1 << 0),
>  	ATAPI_DMADIR		= (1 << 2),	/* ATAPI data dir:
> @@ -299,6 +305,9 @@ struct ata_taskfile {
>  #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
>  #define ata_id_removeable(id)	((id)[0] & (1 << 7))
>  #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
> +#define ata_id_has_AN(id)	\
> +	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
> +	  ((id)[78] & (1 << 5)) )

??

> --- 2.6-git.orig/include/linux/libata.h
> +++ 2.6-git/include/linux/libata.h
> @@ -136,6 +136,7 @@ enum {
>  	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
>  	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
>  	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
> +	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
>  	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,

Why don't the macros use the enums? It makes the code hard to read without
painful cross-reference doesn't it? Surely (id)[76] & (ATA_DFLAG_AN) is a
lot more readable than 1 << 5 - even if the flag is obviously that, a lot
of values and registers can have 1 << 5 as a flag and mean a lot of different
things.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

On Wed, 25 Apr 2007 20:16:51 +0100
Matt Sealey <matt@genesi-usa.com> wrote:

> > +#define ata_id_has_AN(id)	\
> > +	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
> > +	  ((id)[78] & (1 << 5)) )
> 
> ??
> 
> > --- 2.6-git.orig/include/linux/libata.h
> > +++ 2.6-git/include/linux/libata.h
> > @@ -136,6 +136,7 @@ enum {
> >  	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
> >  	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
> >  	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
> > +	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
> >  	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
> 
> Why don't the macros use the enums? It makes the code hard to read without
> painful cross-reference doesn't it? Surely (id)[76] & (ATA_DFLAG_AN) is a
> lot more readable than 1 << 5 - even if the flag is obviously that, a lot
> of values and registers can have 1 << 5 as a flag and mean a lot of different
> things.

It's really just a coincidence that the ATA_DFLAG_AN bit is the same as the bit
in the identify device word, so this would not be appropriate.

Re: [patch 1/7] libata: check for AN support

[Olivier Galibert]

On Wed, Apr 25, 2007 at 08:16:51PM +0100, Matt Sealey wrote:
> > +#define ata_id_has_AN(id)	\
> > +	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
> > +	  ((id)[78] & (1 << 5)) )
> 
> ??
> 
> > --- 2.6-git.orig/include/linux/libata.h
> > +++ 2.6-git/include/linux/libata.h
> > @@ -136,6 +136,7 @@ enum {
> >  	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
> >  	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
> >  	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
> > +	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
> >  	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
> 
> Why don't the macros use the enums? It makes the code hard to read without
> painful cross-reference doesn't it? Surely (id)[76] & (ATA_DFLAG_AN) is a
> lot more readable than 1 << 5 - even if the flag is obviously that, a lot
> of values and registers can have 1 << 5 as a flag and mean a lot of different
> things.

The two being 32 is just a coincidence.  One is a hardware register
bit, the other the signification of the bits of ata_device->flags.

  OG.

Re: [patch 1/7] libata: check for AN support

[Matt Sealey]

Kristen Carlson Accardi wrote:
> On Wed, 25 Apr 2007 20:16:51 +0100
> Matt Sealey <matt@genesi-usa.com> wrote:
> 
>>> +#define ata_id_has_AN(id)	\
>>> +	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
>>> +	  ((id)[78] & (1 << 5)) )
>> ??
>>
>>> --- 2.6-git.orig/include/linux/libata.h
>>> +++ 2.6-git/include/linux/libata.h
>>> @@ -136,6 +136,7 @@ enum {
>>>  	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
>>>  	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
>>>  	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
>>> +	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
>>>  	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
>> Why don't the macros use the enums? It makes the code hard to read without
>> painful cross-reference doesn't it? Surely (id)[76] & (ATA_DFLAG_AN) is a
>> lot more readable than 1 << 5 - even if the flag is obviously that, a lot
>> of values and registers can have 1 << 5 as a flag and mean a lot of different
>> things.
> 
> It's really just a coincidence that the ATA_DFLAG_AN bit is the same as the bit
> in the identify device word, so this would not be appropriate.

Okay, that makes sense.. I just had a bad day cross-referencing some terrible
code in another project, was in the mood to nit :D

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

[patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

Check to see if an ATAPI device supports Asynchronous Notification.
If so, enable it.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -70,6 +70,7 @@ const unsigned long sata_deb_timing_long
 static unsigned int ata_dev_init_params(struct ata_device *dev,
 					u16 heads, u16 sectors);
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
+static unsigned int ata_dev_set_AN(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 
 unsigned int ata_print_id = 1;
@@ -1981,6 +1982,22 @@ int ata_dev_configure(struct ata_device 
 		}
 		dev->cdb_len = (unsigned int) rc;
 
+		/*
+		 * check to see if this ATAPI device supports
+		 * Asynchronous Notification
+		 */
+		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id)) {
+			int err;
+			/* issue SET feature command to turn this on */
+			err = ata_dev_set_AN(dev);
+			if (err)
+				ata_dev_printk(dev, KERN_ERR,
+						"unable to set AN, err %x\n",
+						err);
+			else
+				dev->flags |= ATA_DFLAG_AN;
+		}
+
 		if (ata_id_cdb_intr(dev->id)) {
 			dev->flags |= ATA_DFLAG_CDB_INTR;
 			cdb_intr_string = ", CDB intr";
@@ -3908,6 +3925,42 @@ static unsigned int ata_dev_set_xfermode
 }
 
 /**
+ *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
+ *                       with sector count set to indicate
+ *                       Asynchronous Notification feature
+ *	@dev: Device to which command will be sent
+ *
+ *	Issue SET FEATURES - SATA FEATURES command to device @dev
+ *	on port @ap.
+ *
+ *	LOCKING:
+ *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, AC_ERR_* mask otherwise.
+ */
+static unsigned int ata_dev_set_AN(struct ata_device *dev)
+{
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+
+	/* set up set-features taskfile */
+	DPRINTK("set features - SATA features\n");
+
+	ata_tf_init(dev, &tf);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_SATA_ENABLE;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = SATA_AN;
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
+
+	DPRINTK("EXIT, err_mask=%x\n", err_mask);
+	return err_mask;
+}
+
+/**
  *	ata_dev_init_params - Issue INIT DEV PARAMS command
  *	@dev: Device to which command will be sent
  *	@heads: Number of heads (taskfile parameter)
Index: 2.6-git/include/linux/ata.h
===================================================================
--- 2.6-git.orig/include/linux/ata.h
+++ 2.6-git/include/linux/ata.h
@@ -204,6 +204,12 @@ enum {
 
 	SETFEATURES_SPINUP	= 0x07, /* Spin-up drive */
 
+	SETFEATURES_SATA_ENABLE = 0x10, /* Enable use of SATA feature */
+	SETFEATURES_SATA_DISABLE = 0x90, /* Disable use of SATA feature */
+
+	/* SETFEATURE Sector counts for SATA features */
+	SATA_AN			= 0x05,  /* Asynchronous Notification */
+
 	/* ATAPI stuff */
 	ATAPI_PKT_DMA		= (1 << 0),
 	ATAPI_DMADIR		= (1 << 2),	/* ATAPI data dir:
@@ -309,6 +315,9 @@ struct ata_taskfile {
 #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
 #define ata_id_removeable(id)	((id)[0] & (1 << 7))
 #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
+#define ata_id_has_AN(id)	\
+	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
+	  ((id)[78] & (1 << 5)) )
 #define ata_id_iordy_disable(id) ((id)[49] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[49] & (1 << 9))
 #define ata_id_u32(id,n)	\
Index: 2.6-git/include/linux/libata.h
===================================================================
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
@@ -174,6 +175,7 @@ enum {
 	ATA_FLAG_SETXFER_POLLING= (1 << 14), /* use polling for SETXFER */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
+	ATA_FLAG_AN		= (1 << 17), /* controller supports AN */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be
Index: 2.6-git/drivers/ata/ahci.c
===================================================================
--- 2.6-git.orig/drivers/ata/ahci.c
+++ 2.6-git/drivers/ata/ahci.c
@@ -327,14 +327,15 @@ static const struct ata_port_operations 
 static const struct ata_port_info ahci_port_info[] = {
 	/* board_ahci */
 	{
-		.flags		= AHCI_FLAG_COMMON,
+		.flags		= AHCI_FLAG_COMMON | ATA_FLAG_AN,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
 	/* board_ahci_pi */
 	{
-		.flags		= AHCI_FLAG_COMMON | AHCI_FLAG_HONOR_PI,
+		.flags		= AHCI_FLAG_COMMON | AHCI_FLAG_HONOR_PI |
+				  ATA_FLAG_AN,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,

-- 

[patch 2/7] genhd: expose AN to user space

[Kristen Carlson Accardi]

Allow user space to determine if a disk supports Asynchronous Notification
of media changes.  This is done by adding a new sysfs file "capability_flags",
which is documented in (insert file name).  This sysfs file will export all
disk capabilities flags to user space.  We also define a new flag to define
the media change notification capability.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/block/genhd.c
===================================================================
--- 2.6-git.orig/block/genhd.c
+++ 2.6-git/block/genhd.c
@@ -370,7 +370,10 @@ static ssize_t disk_size_read(struct gen
 {
 	return sprintf(page, "%llu\n", (unsigned long long)get_capacity(disk));
 }
-
+static ssize_t disk_capability_read(struct gendisk *disk, char *page)
+{
+	return sprintf(page, "%x\n", disk->flags);
+}
 static ssize_t disk_stats_read(struct gendisk * disk, char *page)
 {
 	preempt_disable();
@@ -413,6 +416,10 @@ static struct disk_attribute disk_attr_s
 	.attr = {.name = "size", .mode = S_IRUGO },
 	.show	= disk_size_read
 };
+static struct disk_attribute disk_attr_capability = {
+	.attr = {.name = "capability", .mode = S_IRUGO },
+	.show	= disk_capability_read
+};
 static struct disk_attribute disk_attr_stat = {
 	.attr = {.name = "stat", .mode = S_IRUGO },
 	.show	= disk_stats_read
@@ -453,6 +460,7 @@ static struct attribute * default_attrs[
 	&disk_attr_removable.attr,
 	&disk_attr_size.attr,
 	&disk_attr_stat.attr,
+	&disk_attr_capability.attr,
 #ifdef CONFIG_FAIL_MAKE_REQUEST
 	&disk_attr_fail.attr,
 #endif
Index: 2.6-git/include/linux/genhd.h
===================================================================
--- 2.6-git.orig/include/linux/genhd.h
+++ 2.6-git/include/linux/genhd.h
@@ -94,6 +94,7 @@ struct hd_struct {
 
 #define GENHD_FL_REMOVABLE			1
 #define GENHD_FL_DRIVERFS			2
+#define GENHD_FL_MEDIA_CHANGE_NOTIFY		4
 #define GENHD_FL_CD				8
 #define GENHD_FL_UP				16
 #define GENHD_FL_SUPPRESS_PARTITION_INFO	32
Index: 2.6-git/Documentation/block/capability.txt
===================================================================
--- /dev/null
+++ 2.6-git/Documentation/block/capability.txt
@@ -0,0 +1,15 @@
+Generic Block Device Capability
+===============================================================================
+This file documents the sysfs file block/<disk>/capability
+
+capability is a hex word indicating which capabilities a specific disk
+supports.  For more information on bits not listed here, see
+include/linux/genhd.h
+
+Capability				Value
+-------------------------------------------------------------------------------
+GENHD_FL_MEDIA_CHANGE_NOTIFY		4
+	When this bit is set, the disk supports Asynchronous Notification
+	of media change events.  These events will be broadcast to user
+	space via kernel uevent.
+

-- 

[patch 3/7] scsi: expose AN to user space

[Kristen Carlson Accardi]

Get media change notification capability from disk and pass this information
to genhd by setting appropriate flag.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/scsi/sr.c
===================================================================
--- 2.6-git.orig/drivers/scsi/sr.c
+++ 2.6-git/drivers/scsi/sr.c
@@ -603,6 +603,8 @@ static int sr_probe(struct device *dev)
 
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
+	if (sdev->media_change_notify)
+		disk->flags |= GENHD_FL_MEDIA_CHANGE_NOTIFY;
 	add_disk(disk);
 
 	sdev_printk(KERN_DEBUG, sdev,
Index: 2.6-git/include/scsi/scsi_device.h
===================================================================
--- 2.6-git.orig/include/scsi/scsi_device.h
+++ 2.6-git/include/scsi/scsi_device.h
@@ -126,7 +126,7 @@ struct scsi_device {
 	unsigned fix_capacity:1;	/* READ_CAPACITY is too high by 1 */
 	unsigned guess_capacity:1;	/* READ_CAPACITY might be too high by 1 */
 	unsigned retry_hwerror:1;	/* Retry HARDWARE_ERROR */
-
+	unsigned media_change_notify:1; /* dev supports async media notify */
 	unsigned int device_blocked;	/* Device returned QUEUE_FULL. */
 
 	unsigned int max_device_blocked; /* what device_blocked counts down from  */
Index: 2.6-git/drivers/scsi/sd.c
===================================================================
--- 2.6-git.orig/drivers/scsi/sd.c
+++ 2.6-git/drivers/scsi/sd.c
@@ -1668,6 +1668,9 @@ static int sd_probe(struct device *dev)
 	if (sdp->removable)
 		gd->flags |= GENHD_FL_REMOVABLE;
 
+	if (sdp->media_change_notify)
+		gd->flags |= GENHD_FL_MEDIA_CHANGE_NOTIFY;
+
 	dev_set_drvdata(dev, sdkp);
 	add_disk(gd);
 

-- 

[patch 4/7] libata: expose AN to user space

[Kristen Carlson Accardi]

If Asynchronous Notification of media change events is supported,
pass that information up to the SCSI layer.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/libata-scsi.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-scsi.c
+++ 2.6-git/drivers/ata/libata-scsi.c
@@ -899,6 +899,9 @@ static void ata_scsi_dev_config(struct s
 		blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
 	}
 
+	if (dev->flags & ATA_DFLAG_AN)
+		sdev->media_change_notify = 1;
+
 	if (dev->flags & ATA_DFLAG_NCQ) {
 		int depth;
 

-- 

[patch 5/7] genhd: send async notification on media change

[Kristen Carlson Accardi]

Send an uevent to user space to indicate that a media change event has occurred.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/block/genhd.c
===================================================================
--- 2.6-git.orig/block/genhd.c
+++ 2.6-git/block/genhd.c
@@ -643,6 +643,27 @@ struct seq_operations diskstats_op = {
 	.show	= diskstats_show
 };
 
+static void media_change_notify_thread(struct work_struct *work)
+{
+	struct gendisk *gd = container_of(work, struct gendisk, async_notify);
+	char event[] = "MEDIA_CHANGE=1";
+	char *envp[] = { event, NULL };
+
+	/*
+	 * set enviroment vars to indicate which event this is for
+	 * so that user space will know to go check the media status.
+	 */
+	kobject_uevent_env(&gd->kobj, KOBJ_CHANGE, envp);
+	put_device(gd->driverfs_dev);
+}
+
+void genhd_media_change_notify(struct gendisk *disk)
+{
+	get_device(disk->driverfs_dev);
+	schedule_work(&disk->async_notify);
+}
+EXPORT_SYMBOL_GPL(genhd_media_change_notify);
+
 struct gendisk *alloc_disk(int minors)
 {
 	return alloc_disk_node(minors, -1);
@@ -672,6 +693,8 @@ struct gendisk *alloc_disk_node(int mino
 		kobj_set_kset_s(disk,block_subsys);
 		kobject_init(&disk->kobj);
 		rand_initialize_disk(disk);
+		INIT_WORK(&disk->async_notify,
+			media_change_notify_thread);
 	}
 	return disk;
 }
Index: 2.6-git/include/linux/genhd.h
===================================================================
--- 2.6-git.orig/include/linux/genhd.h
+++ 2.6-git/include/linux/genhd.h
@@ -66,6 +66,7 @@ struct partition {
 #include <linux/smp.h>
 #include <linux/string.h>
 #include <linux/fs.h>
+#include <linux/workqueue.h>
 
 struct partition {
 	unsigned char boot_ind;		/* 0x80 - active */
@@ -139,6 +140,7 @@ struct gendisk {
 #else
 	struct disk_stats dkstats;
 #endif
+	struct work_struct async_notify;
 };
 
 /* Structure for sysfs attributes on block devices */
@@ -419,7 +421,7 @@ extern struct gendisk *alloc_disk_node(i
 extern struct gendisk *alloc_disk(int minors);
 extern struct kobject *get_disk(struct gendisk *disk);
 extern void put_disk(struct gendisk *disk);
-
+extern void genhd_media_change_notify(struct gendisk *disk);
 extern void blk_register_region(dev_t dev, unsigned long range,
 			struct module *module,
 			struct kobject *(*probe)(dev_t, int *, void *),

-- 

[patch 6/7] SCSI: save disk in scsi_device

[Kristen Carlson Accardi]

Give anyone who has access to scsi_device access to the genhd struct as well.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
Index: 2.6-git/drivers/scsi/sd.c
===================================================================
--- 2.6-git.orig/drivers/scsi/sd.c
+++ 2.6-git/drivers/scsi/sd.c
@@ -1673,6 +1673,7 @@ static int sd_probe(struct device *dev)
 
 	dev_set_drvdata(dev, sdkp);
 	add_disk(gd);
+	sdp->disk = gd;
 
 	sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
 		  sdp->removable ? "removable " : "");
Index: 2.6-git/drivers/scsi/sr.c
===================================================================
--- 2.6-git.orig/drivers/scsi/sr.c
+++ 2.6-git/drivers/scsi/sr.c
@@ -606,6 +606,7 @@ static int sr_probe(struct device *dev)
 	if (sdev->media_change_notify)
 		disk->flags |= GENHD_FL_MEDIA_CHANGE_NOTIFY;
 	add_disk(disk);
+	sdev->disk = disk;
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
Index: 2.6-git/include/scsi/scsi_device.h
===================================================================
--- 2.6-git.orig/include/scsi/scsi_device.h
+++ 2.6-git/include/scsi/scsi_device.h
@@ -140,7 +140,7 @@ struct scsi_device {
 
 	struct device		sdev_gendev;
 	struct class_device	sdev_classdev;
-
+	struct gendisk 		*disk;
 	struct execute_work	ew; /* used to get process context on put */
 
 	enum scsi_device_state sdev_state;

-- 

[patch 7/7] libata: send event when AN received

[Kristen Carlson Accardi]

When we get an SDB FIS with the 'N' bit set, we should send
an event to user space to indicate that there has been a
media change.  This will be done via the block device. 

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
Index: 2.6-git/drivers/ata/ahci.c
===================================================================
--- 2.6-git.orig/drivers/ata/ahci.c
+++ 2.6-git/drivers/ata/ahci.c
@@ -1218,6 +1218,28 @@ static void ahci_host_intr(struct ata_po
 		return;
 	}
 
+	if (status & PORT_IRQ_SDB_FIS) {
+		/*
+		 * if this is an ATAPI device with AN turned on,
+		 * then we should interrogate the device to
+		 * determine the cause of the interrupt
+		 *
+		 * for AN - this we should check the SDB FIS
+		 * and find the I and N bits set
+		 */
+		const u32 *f = pp->rx_fis + RX_FIS_SDB;
+
+		/* check the 'N' bit in word 0 of the FIS */
+		if (f[0] & (1 << 15)) {
+			int port_addr =  ((f[0] & 0x00000f00) >> 8);
+			struct ata_device *adev;
+			if (port_addr < ATA_MAX_DEVICES) {
+				adev = &ap->device[port_addr];
+				if (adev->flags & ATA_DFLAG_AN)
+					ata_scsi_media_change_notify(adev);
+			}
+		}
+	}
 	if (ap->sactive)
 		qc_active = readl(port_mmio + PORT_SCR_ACT);
 	else
Index: 2.6-git/include/linux/libata.h
===================================================================
--- 2.6-git.orig/include/linux/libata.h
+++ 2.6-git/include/linux/libata.h
@@ -721,6 +721,7 @@ extern void ata_host_init(struct ata_hos
 extern int ata_scsi_detect(struct scsi_host_template *sht);
 extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
 extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
+extern void ata_scsi_media_change_notify(struct ata_device *atadev);
 extern void ata_sas_port_destroy(struct ata_port *);
 extern struct ata_port *ata_sas_port_alloc(struct ata_host *,
 					   struct ata_port_info *, struct Scsi_Host *);
Index: 2.6-git/drivers/ata/libata-scsi.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-scsi.c
+++ 2.6-git/drivers/ata/libata-scsi.c
@@ -3112,6 +3112,22 @@ static void ata_scsi_remove_dev(struct a
 }
 
 /**
+ *	ata_scsi_media_change_notify - send media change event
+ *	@atadev: Pointer to the disk device with media change event
+ *
+ *	Tell the block layer to send a media change notification
+ *	event.
+ *
+ * 	LOCKING:
+ * 	interrupt context, may not sleep.
+ */
+void ata_scsi_media_change_notify(struct ata_device *atadev)
+{
+	genhd_media_change_notify(atadev->sdev->disk);
+}
+EXPORT_SYMBOL_GPL(ata_scsi_media_change_notify);
+
+/**
  *	ata_scsi_hotplug - SCSI part of hotplug
  *	@work: Pointer to ATA port to perform SCSI hotplug on
  *

-- 

Re: [patch 1/7] libata: check for AN support

[Andrew Morton]

On Wed, 9 May 2007 16:38:09 -0700 Kristen Carlson Accardi <kristen.c.accardi@intel.com> wrote:

>  /**
> + *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
> + *                       with sector count set to indicate
> + *                       Asynchronous Notification feature

I think kenreldoc requires that all this be on a single line?

> + *	@dev: Device to which command will be sent
> + *
> + *	Issue SET FEATURES - SATA FEATURES command to device @dev
> + *	on port @ap.
> + *
> + *	LOCKING:
> + *	PCI/etc. bus probe sem.
> + *
> + *	RETURNS:
> + *	0 on success, AC_ERR_* mask otherwise.
> + */

ooh, locking and return value documentation.  Often missed, and nice.

Re: [patch 1/7] libata: check for AN support

[Jeff Garzik]

Andrew Morton wrote:
>> + *	@dev: Device to which command will be sent
>> + *
>> + *	Issue SET FEATURES - SATA FEATURES command to device @dev
>> + *	on port @ap.
>> + *
>> + *	LOCKING:
>> + *	PCI/etc. bus probe sem.
>> + *
>> + *	RETURNS:
>> + *	0 on success, AC_ERR_* mask otherwise.
>> + */
> 
> ooh, locking and return value documentation.  Often missed, and nice.


We set high standards in the libata world ;-)

	Jeff

Re: [patch 1/7] libata: check for AN support

[Andrew Morton]

On Thu, 10 May 2007 01:14:38 -0400 Jeff Garzik <jeff@garzik.org> wrote:

> Andrew Morton wrote:
> >> + *	@dev: Device to which command will be sent
> >> + *
> >> + *	Issue SET FEATURES - SATA FEATURES command to device @dev
> >> + *	on port @ap.
> >> + *
> >> + *	LOCKING:
> >> + *	PCI/etc. bus probe sem.
> >> + *
> >> + *	RETURNS:
> >> + *	0 on success, AC_ERR_* mask otherwise.
> >> + */
> > 
> > ooh, locking and return value documentation.  Often missed, and nice.
> 
> 
> We set high standards in the libata world ;-)
> 

It seems to be working.  This series is perhaps the most idiomatic and
generally kernelly-looking code I've seen in ages.

Who cares if it works? ;)

Re: [patch 1/7] libata: check for AN support

[Randy Dunlap]

On Wed, 9 May 2007 22:09:52 -0700 Andrew Morton wrote:

> On Wed, 9 May 2007 16:38:09 -0700 Kristen Carlson Accardi <kristen.c.accardi@intel.com> wrote:
> 
> >  /**
> > + *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
> > + *                       with sector count set to indicate
> > + *                       Asynchronous Notification feature
> 
> I think kenreldoc requires that all this be on a single line?

Correct.

> > + *	@dev: Device to which command will be sent
> > + *
> > + *	Issue SET FEATURES - SATA FEATURES command to device @dev
> > + *	on port @ap.
> > + *
> > + *	LOCKING:
> > + *	PCI/etc. bus probe sem.
> > + *
> > + *	RETURNS:
> > + *	0 on success, AC_ERR_* mask otherwise.
> > + */
> 
> ooh, locking and return value documentation.  Often missed, and nice.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

Check to see if an ATAPI device supports Asynchronous Notification.
If so, enable it.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
---
Andrew, I cleaned up the function header to properly comply with kernel
doc requirements.  Other than that, this patch is the same.  

Index: 2.6-mm/drivers/ata/libata-core.c
===================================================================
--- 2.6-mm.orig/drivers/ata/libata-core.c
+++ 2.6-mm/drivers/ata/libata-core.c
@@ -70,6 +70,7 @@ const unsigned long sata_deb_timing_long
 static unsigned int ata_dev_init_params(struct ata_device *dev,
 					u16 heads, u16 sectors);
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
+static unsigned int ata_dev_set_AN(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 
 unsigned int ata_print_id = 1;
@@ -1983,6 +1984,22 @@ int ata_dev_configure(struct ata_device 
 		}
 		dev->cdb_len = (unsigned int) rc;
 
+		/*
+		 * check to see if this ATAPI device supports
+		 * Asynchronous Notification
+		 */
+		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id)) {
+			int err;
+			/* issue SET feature command to turn this on */
+			err = ata_dev_set_AN(dev);
+			if (err)
+				ata_dev_printk(dev, KERN_ERR,
+						"unable to set AN, err %x\n",
+						err);
+			else
+				dev->flags |= ATA_DFLAG_AN;
+		}
+
 		if (ata_id_cdb_intr(dev->id)) {
 			dev->flags |= ATA_DFLAG_CDB_INTR;
 			cdb_intr_string = ", CDB intr";
@@ -3910,6 +3927,41 @@ static unsigned int ata_dev_set_xfermode
 }
 
 /**
+ *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
+ *	@dev: Device to which command will be sent
+ *
+ *	Issue SET FEATURES - SATA FEATURES command to device @dev
+ *	on port @ap with sector count set to indicate Asynchronous
+ *	Notification feature
+ *
+ *	LOCKING:
+ *	PCI/etc. bus probe sem.
+ *
+ *	RETURNS:
+ *	0 on success, AC_ERR_* mask otherwise.
+ */
+static unsigned int ata_dev_set_AN(struct ata_device *dev)
+{
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+
+	/* set up set-features taskfile */
+	DPRINTK("set features - SATA features\n");
+
+	ata_tf_init(dev, &tf);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_SATA_ENABLE;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = SATA_AN;
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
+
+	DPRINTK("EXIT, err_mask=%x\n", err_mask);
+	return err_mask;
+}
+
+/**
  *	ata_dev_init_params - Issue INIT DEV PARAMS command
  *	@dev: Device to which command will be sent
  *	@heads: Number of heads (taskfile parameter)
Index: 2.6-mm/include/linux/ata.h
===================================================================
--- 2.6-mm.orig/include/linux/ata.h
+++ 2.6-mm/include/linux/ata.h
@@ -204,6 +204,12 @@ enum {
 
 	SETFEATURES_SPINUP	= 0x07, /* Spin-up drive */
 
+	SETFEATURES_SATA_ENABLE = 0x10, /* Enable use of SATA feature */
+	SETFEATURES_SATA_DISABLE = 0x90, /* Disable use of SATA feature */
+
+	/* SETFEATURE Sector counts for SATA features */
+	SATA_AN			= 0x05,  /* Asynchronous Notification */
+
 	/* ATAPI stuff */
 	ATAPI_PKT_DMA		= (1 << 0),
 	ATAPI_DMADIR		= (1 << 2),	/* ATAPI data dir:
@@ -309,6 +315,9 @@ struct ata_taskfile {
 #define ata_id_queue_depth(id)	(((id)[75] & 0x1f) + 1)
 #define ata_id_removeable(id)	((id)[0] & (1 << 7))
 #define ata_id_has_dword_io(id)	((id)[50] & (1 << 0))
+#define ata_id_has_AN(id)	\
+	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
+	  ((id)[78] & (1 << 5)) )
 #define ata_id_iordy_disable(id) ((id)[49] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[49] & (1 << 9))
 #define ata_id_u32(id,n)	\
Index: 2.6-mm/include/linux/libata.h
===================================================================
--- 2.6-mm.orig/include/linux/libata.h
+++ 2.6-mm/include/linux/libata.h
@@ -136,6 +136,7 @@ enum {
 	ATA_DFLAG_CDB_INTR	= (1 << 2), /* device asserts INTRQ when ready for CDB */
 	ATA_DFLAG_NCQ		= (1 << 3), /* device supports NCQ */
 	ATA_DFLAG_FLUSH_EXT	= (1 << 4), /* do FLUSH_EXT instead of FLUSH */
+	ATA_DFLAG_AN		= (1 << 5), /* device supports Async notification */
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device limited to PIO mode */
@@ -174,6 +175,7 @@ enum {
 	ATA_FLAG_SETXFER_POLLING= (1 << 14), /* use polling for SETXFER */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
+	ATA_FLAG_AN		= (1 << 17), /* controller supports AN */
 
 	/* The following flag belongs to ap->pflags but is kept in
 	 * ap->flags because it's referenced in many LLDs and will be
Index: 2.6-mm/drivers/ata/ahci.c
===================================================================
--- 2.6-mm.orig/drivers/ata/ahci.c
+++ 2.6-mm/drivers/ata/ahci.c
@@ -327,14 +327,15 @@ static const struct ata_port_operations 
 static const struct ata_port_info ahci_port_info[] = {
 	/* board_ahci */
 	{
-		.flags		= AHCI_FLAG_COMMON,
+		.flags		= AHCI_FLAG_COMMON | ATA_FLAG_AN,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,
 	},
 	/* board_ahci_pi */
 	{
-		.flags		= AHCI_FLAG_COMMON | AHCI_FLAG_HONOR_PI,
+		.flags		= AHCI_FLAG_COMMON | AHCI_FLAG_HONOR_PI |
+				  ATA_FLAG_AN,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
 		.port_ops	= &ahci_ops,

Re: [patch 5/7] genhd: send async notification on media change

[Andrew Morton]

On Wed, 9 May 2007 16:38:35 -0700
Kristen Carlson Accardi <kristen.c.accardi@intel.com> wrote:

> Send an uevent to user space to indicate that a media change event has occurred.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> Index: 2.6-git/block/genhd.c
> ===================================================================
> --- 2.6-git.orig/block/genhd.c
> +++ 2.6-git/block/genhd.c
> @@ -643,6 +643,27 @@ struct seq_operations diskstats_op = {
>  	.show	= diskstats_show
>  };
>  
> +static void media_change_notify_thread(struct work_struct *work)
> +{
> +	struct gendisk *gd = container_of(work, struct gendisk, async_notify);
> +	char event[] = "MEDIA_CHANGE=1";
> +	char *envp[] = { event, NULL };
> +
> +	/*
> +	 * set enviroment vars to indicate which event this is for
> +	 * so that user space will know to go check the media status.
> +	 */
> +	kobject_uevent_env(&gd->kobj, KOBJ_CHANGE, envp);
> +	put_device(gd->driverfs_dev);
> +}
> +
> +void genhd_media_change_notify(struct gendisk *disk)
> +{
> +	get_device(disk->driverfs_dev);
> +	schedule_work(&disk->async_notify);
> +}
> +EXPORT_SYMBOL_GPL(genhd_media_change_notify);
> +
>  struct gendisk *alloc_disk(int minors)
>  {
>  	return alloc_disk_node(minors, -1);
> @@ -672,6 +693,8 @@ struct gendisk *alloc_disk_node(int mino
>  		kobj_set_kset_s(disk,block_subsys);
>  		kobject_init(&disk->kobj);
>  		rand_initialize_disk(disk);
> +		INIT_WORK(&disk->async_notify,
> +			media_change_notify_thread);
>  	}
>  	return disk;

Why does this do a schedule_work() rather than calling kobject_uevent_env()
directly?

Re: [patch 5/7] genhd: send async notification on media change

[Kristen Carlson Accardi]

On Tue, 22 May 2007 14:12:11 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 9 May 2007 16:38:35 -0700
> Kristen Carlson Accardi <kristen.c.accardi@intel.com> wrote:
> 
> > Send an uevent to user space to indicate that a media change event has occurred.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > 
> > Index: 2.6-git/block/genhd.c
> > ===================================================================
> > --- 2.6-git.orig/block/genhd.c
> > +++ 2.6-git/block/genhd.c
> > @@ -643,6 +643,27 @@ struct seq_operations diskstats_op = {
> >  	.show	= diskstats_show
> >  };
> >  
> > +static void media_change_notify_thread(struct work_struct *work)
> > +{
> > +	struct gendisk *gd = container_of(work, struct gendisk, async_notify);
> > +	char event[] = "MEDIA_CHANGE=1";
> > +	char *envp[] = { event, NULL };
> > +
> > +	/*
> > +	 * set enviroment vars to indicate which event this is for
> > +	 * so that user space will know to go check the media status.
> > +	 */
> > +	kobject_uevent_env(&gd->kobj, KOBJ_CHANGE, envp);
> > +	put_device(gd->driverfs_dev);
> > +}
> > +
> > +void genhd_media_change_notify(struct gendisk *disk)
> > +{
> > +	get_device(disk->driverfs_dev);
> > +	schedule_work(&disk->async_notify);
> > +}
> > +EXPORT_SYMBOL_GPL(genhd_media_change_notify);
> > +
> >  struct gendisk *alloc_disk(int minors)
> >  {
> >  	return alloc_disk_node(minors, -1);
> > @@ -672,6 +693,8 @@ struct gendisk *alloc_disk_node(int mino
> >  		kobj_set_kset_s(disk,block_subsys);
> >  		kobject_init(&disk->kobj);
> >  		rand_initialize_disk(disk);
> > +		INIT_WORK(&disk->async_notify,
> > +			media_change_notify_thread);
> >  	}
> >  	return disk;
> 
> Why does this do a schedule_work() rather than calling kobject_uevent_env()
> directly?
> 

Because it is called at Interrupt time.

Kristen

Re: [patch 5/7] genhd: send async notification on media change

[James Bottomley]

On Wed, 2007-05-23 at 09:31 -0700, Kristen Carlson Accardi wrote:
> On Tue, 22 May 2007 14:12:11 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Wed, 9 May 2007 16:38:35 -0700
> > Kristen Carlson Accardi <kristen.c.accardi@intel.com> wrote:
> > 
> > > Send an uevent to user space to indicate that a media change event has occurred.
> > > 
> > > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > > 
> > > Index: 2.6-git/block/genhd.c
> > > ===================================================================
> > > --- 2.6-git.orig/block/genhd.c
> > > +++ 2.6-git/block/genhd.c
> > > @@ -643,6 +643,27 @@ struct seq_operations diskstats_op = {
> > >  	.show	= diskstats_show
> > >  };
> > >  
> > > +static void media_change_notify_thread(struct work_struct *work)
> > > +{
> > > +	struct gendisk *gd = container_of(work, struct gendisk, async_notify);
> > > +	char event[] = "MEDIA_CHANGE=1";
> > > +	char *envp[] = { event, NULL };
> > > +
> > > +	/*
> > > +	 * set enviroment vars to indicate which event this is for
> > > +	 * so that user space will know to go check the media status.
> > > +	 */
> > > +	kobject_uevent_env(&gd->kobj, KOBJ_CHANGE, envp);
> > > +	put_device(gd->driverfs_dev);
> > > +}
> > > +
> > > +void genhd_media_change_notify(struct gendisk *disk)
> > > +{
> > > +	get_device(disk->driverfs_dev);
> > > +	schedule_work(&disk->async_notify);
> > > +}
> > > +EXPORT_SYMBOL_GPL(genhd_media_change_notify);
> > > +
> > >  struct gendisk *alloc_disk(int minors)
> > >  {
> > >  	return alloc_disk_node(minors, -1);
> > > @@ -672,6 +693,8 @@ struct gendisk *alloc_disk_node(int mino
> > >  		kobj_set_kset_s(disk,block_subsys);
> > >  		kobject_init(&disk->kobj);
> > >  		rand_initialize_disk(disk);
> > > +		INIT_WORK(&disk->async_notify,
> > > +			media_change_notify_thread);
> > >  	}
> > >  	return disk;
> > 
> > Why does this do a schedule_work() rather than calling kobject_uevent_env()
> > directly?
> > 
> 
> Because it is called at Interrupt time.

It better not be ... there's two GFP_KERNEL allocations just above this
line in the file.

James

Re: [patch 5/7] genhd: send async notification on media change

[Kristen Carlson Accardi]

On Wed, 23 May 2007 12:03:55 -0500
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Wed, 2007-05-23 at 09:31 -0700, Kristen Carlson Accardi wrote:
> > On Tue, 22 May 2007 14:12:11 -0700
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Wed, 9 May 2007 16:38:35 -0700
> > > Kristen Carlson Accardi <kristen.c.accardi@intel.com> wrote:
> > > 
> > > > Send an uevent to user space to indicate that a media change event has occurred.
> > > > 
> > > > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > > > 
> > > > Index: 2.6-git/block/genhd.c
> > > > ===================================================================
> > > > --- 2.6-git.orig/block/genhd.c
> > > > +++ 2.6-git/block/genhd.c
> > > > @@ -643,6 +643,27 @@ struct seq_operations diskstats_op = {
> > > >  	.show	= diskstats_show
> > > >  };
> > > >  
> > > > +static void media_change_notify_thread(struct work_struct *work)
> > > > +{
> > > > +	struct gendisk *gd = container_of(work, struct gendisk, async_notify);
> > > > +	char event[] = "MEDIA_CHANGE=1";
> > > > +	char *envp[] = { event, NULL };
> > > > +
> > > > +	/*
> > > > +	 * set enviroment vars to indicate which event this is for
> > > > +	 * so that user space will know to go check the media status.
> > > > +	 */
> > > > +	kobject_uevent_env(&gd->kobj, KOBJ_CHANGE, envp);
> > > > +	put_device(gd->driverfs_dev);
> > > > +}
> > > > +
> > > > +void genhd_media_change_notify(struct gendisk *disk)
> > > > +{
> > > > +	get_device(disk->driverfs_dev);
> > > > +	schedule_work(&disk->async_notify);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(genhd_media_change_notify);
> > > > +
> > > >  struct gendisk *alloc_disk(int minors)
> > > >  {
> > > >  	return alloc_disk_node(minors, -1);
> > > > @@ -672,6 +693,8 @@ struct gendisk *alloc_disk_node(int mino
> > > >  		kobj_set_kset_s(disk,block_subsys);
> > > >  		kobject_init(&disk->kobj);
> > > >  		rand_initialize_disk(disk);
> > > > +		INIT_WORK(&disk->async_notify,
> > > > +			media_change_notify_thread);
> > > >  	}
> > > >  	return disk;
> > > 
> > > Why does this do a schedule_work() rather than calling kobject_uevent_env()
> > > directly?
> > > 
> > 
> > Because it is called at Interrupt time.
> 
> It better not be ... there's two GFP_KERNEL allocations just above this
> line in the file.
> 
> James
> 

Where?  We are talking about the call to genhd_media_change_notify().
the calling path is this:
ahci_host_intr()->ata_scsi_media_change_notify()->genhd_media_change_notify().

I don't see the allocations - are they hidden somewhere?

thanks,
Kristen
 

Re: [patch 5/7] genhd: send async notification on media change

[James Bottomley]

On Wed, 2007-05-23 at 11:26 -0700, Kristen Carlson Accardi wrote:
> On Wed, 23 May 2007 12:03:55 -0500
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> > On Wed, 2007-05-23 at 09:31 -0700, Kristen Carlson Accardi wrote:
> > > On Tue, 22 May 2007 14:12:11 -0700
> > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > 
> > > > On Wed, 9 May 2007 16:38:35 -0700
> > > > Kristen Carlson Accardi <kristen.c.accardi@intel.com> wrote:
> > > > 
> > > > > Send an uevent to user space to indicate that a media change event has occurred.
> > > > > 
> > > > > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > > > > 
> > > > > Index: 2.6-git/block/genhd.c
> > > > > ===================================================================
> > > > > --- 2.6-git.orig/block/genhd.c
> > > > > +++ 2.6-git/block/genhd.c
> > > > > @@ -643,6 +643,27 @@ struct seq_operations diskstats_op = {
> > > > >  	.show	= diskstats_show
> > > > >  };
> > > > >  
> > > > > +static void media_change_notify_thread(struct work_struct *work)
> > > > > +{
> > > > > +	struct gendisk *gd = container_of(work, struct gendisk, async_notify);
> > > > > +	char event[] = "MEDIA_CHANGE=1";
> > > > > +	char *envp[] = { event, NULL };
> > > > > +
> > > > > +	/*
> > > > > +	 * set enviroment vars to indicate which event this is for
> > > > > +	 * so that user space will know to go check the media status.
> > > > > +	 */
> > > > > +	kobject_uevent_env(&gd->kobj, KOBJ_CHANGE, envp);
> > > > > +	put_device(gd->driverfs_dev);
> > > > > +}
> > > > > +
> > > > > +void genhd_media_change_notify(struct gendisk *disk)
> > > > > +{
> > > > > +	get_device(disk->driverfs_dev);
> > > > > +	schedule_work(&disk->async_notify);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(genhd_media_change_notify);
> > > > > +
> > > > >  struct gendisk *alloc_disk(int minors)
> > > > >  {
> > > > >  	return alloc_disk_node(minors, -1);
> > > > > @@ -672,6 +693,8 @@ struct gendisk *alloc_disk_node(int mino
> > > > >  		kobj_set_kset_s(disk,block_subsys);
> > > > >  		kobject_init(&disk->kobj);
> > > > >  		rand_initialize_disk(disk);
> > > > > +		INIT_WORK(&disk->async_notify,
> > > > > +			media_change_notify_thread);
> > > > >  	}
> > > > >  	return disk;
> > > > 
> > > > Why does this do a schedule_work() rather than calling kobject_uevent_env()
> > > > directly?
> > > > 
> > > 
> > > Because it is called at Interrupt time.
> > 
> > It better not be ... there's two GFP_KERNEL allocations just above this
> > line in the file.
> > 
> > James
> > 
> 
> Where?  We are talking about the call to genhd_media_change_notify().
> the calling path is this:
> ahci_host_intr()->ata_scsi_media_change_notify()->genhd_media_change_notify().
> 
> I don't see the allocations - are they hidden somewhere?

Sorry, I thought you were saying alloc_disk_node() could be called from
interrupt context.

<gets back on ball>

If you just want to invoke guaranteed user context from a place in the
code which *may* be called from interrupt, then
execute_in_process_context() might be a better way of doing it ... at
least it avoids setting up a workqueue where none is needed.

James

Re: [patch 5/7] genhd: send async notification on media change

[Kristen Carlson Accardi]

On Wed, 23 May 2007 13:51:51 -0500
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Wed, 2007-05-23 at 11:26 -0700, Kristen Carlson Accardi wrote:
> > On Wed, 23 May 2007 12:03:55 -0500
> > James Bottomley <James.Bottomley@SteelEye.com> wrote:
> > 
> > > On Wed, 2007-05-23 at 09:31 -0700, Kristen Carlson Accardi wrote:
> > > > On Tue, 22 May 2007 14:12:11 -0700
> > > > Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > 
> > > > > On Wed, 9 May 2007 16:38:35 -0700
> > > > > Kristen Carlson Accardi <kristen.c.accardi@intel.com> wrote:
> > > > > 
> > > > > > Send an uevent to user space to indicate that a media change event has occurred.
> > > > > > 
> > > > > > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > > > > > 
> > > > > > Index: 2.6-git/block/genhd.c
> > > > > > ===================================================================
> > > > > > --- 2.6-git.orig/block/genhd.c
> > > > > > +++ 2.6-git/block/genhd.c
> > > > > > @@ -643,6 +643,27 @@ struct seq_operations diskstats_op = {
> > > > > >  	.show	= diskstats_show
> > > > > >  };
> > > > > >  
> > > > > > +static void media_change_notify_thread(struct work_struct *work)
> > > > > > +{
> > > > > > +	struct gendisk *gd = container_of(work, struct gendisk, async_notify);
> > > > > > +	char event[] = "MEDIA_CHANGE=1";
> > > > > > +	char *envp[] = { event, NULL };
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * set enviroment vars to indicate which event this is for
> > > > > > +	 * so that user space will know to go check the media status.
> > > > > > +	 */
> > > > > > +	kobject_uevent_env(&gd->kobj, KOBJ_CHANGE, envp);
> > > > > > +	put_device(gd->driverfs_dev);
> > > > > > +}
> > > > > > +
> > > > > > +void genhd_media_change_notify(struct gendisk *disk)
> > > > > > +{
> > > > > > +	get_device(disk->driverfs_dev);
> > > > > > +	schedule_work(&disk->async_notify);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(genhd_media_change_notify);
> > > > > > +
> > > > > >  struct gendisk *alloc_disk(int minors)
> > > > > >  {
> > > > > >  	return alloc_disk_node(minors, -1);
> > > > > > @@ -672,6 +693,8 @@ struct gendisk *alloc_disk_node(int mino
> > > > > >  		kobj_set_kset_s(disk,block_subsys);
> > > > > >  		kobject_init(&disk->kobj);
> > > > > >  		rand_initialize_disk(disk);
> > > > > > +		INIT_WORK(&disk->async_notify,
> > > > > > +			media_change_notify_thread);
> > > > > >  	}
> > > > > >  	return disk;
> > > > > 
> > > > > Why does this do a schedule_work() rather than calling kobject_uevent_env()
> > > > > directly?
> > > > > 
> > > > 
> > > > Because it is called at Interrupt time.
> > > 
> > > It better not be ... there's two GFP_KERNEL allocations just above this
> > > line in the file.
> > > 
> > > James
> > > 
> > 
> > Where?  We are talking about the call to genhd_media_change_notify().
> > the calling path is this:
> > ahci_host_intr()->ata_scsi_media_change_notify()->genhd_media_change_notify().
> > 
> > I don't see the allocations - are they hidden somewhere?
> 
> Sorry, I thought you were saying alloc_disk_node() could be called from
> interrupt context.
> 
> <gets back on ball>
> 
> If you just want to invoke guaranteed user context from a place in the
> code which *may* be called from interrupt, then
> execute_in_process_context() might be a better way of doing it ... at
> least it avoids setting up a workqueue where none is needed.
> 
> James
> 

That is good to know - although in this particular case we are guaranteed
to always be called from interrupt context since our uevent needs to be
sent in response to an Interrupt received from the disk, so it wouldn't
buy us anything.

Kristen

Re: [patch 1/7] libata: check for AN support

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> Check to see if an ATAPI device supports Asynchronous Notification.
> If so, enable it.
> 
> Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> ---
> Andrew, I cleaned up the function header to properly comply with kernel
> doc requirements.  Other than that, this patch is the same.  

I would ask for a simple revision:  update ata_dev_set_AN() such that it 
takes a second argument 'enable'.  This boolean indicates to the 
function whether SETFEATURES_SATA_ENABLE or SETFEATURES_SATA_DISABLE 
should be passed to the device.

Otherwise than that, it's ready to merge I would say.

Re: [patch 1/7] libata: check for AN support

[Kristen Carlson Accardi]

On Thu, 24 May 2007 23:15:56 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Kristen Carlson Accardi wrote:
> > Check to see if an ATAPI device supports Asynchronous Notification.
> > If so, enable it.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > ---
> > Andrew, I cleaned up the function header to properly comply with kernel
> > doc requirements.  Other than that, this patch is the same.  
> 
> I would ask for a simple revision:  update ata_dev_set_AN() such that it 
> takes a second argument 'enable'.  This boolean indicates to the 
> function whether SETFEATURES_SATA_ENABLE or SETFEATURES_SATA_DISABLE 
> should be passed to the device.
> 
> Otherwise than that, it's ready to merge I would say.
> 

Jeff - can you fold this into the original patch, or would you like me
to resubmit the whole thing?

Kristen

Modify ata_dev_set_AN to take a second argument 'enable'.  This
boolean indicates to the function whether SETFEATURES_SATA_ENABLE
or SETFEATURES_SATA_DISABLE should be passed to the device.

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: 2.6-git/drivers/ata/libata-core.c
===================================================================
--- 2.6-git.orig/drivers/ata/libata-core.c
+++ 2.6-git/drivers/ata/libata-core.c
@@ -70,7 +70,7 @@ const unsigned long sata_deb_timing_long
 static unsigned int ata_dev_init_params(struct ata_device *dev,
 					u16 heads, u16 sectors);
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
-static unsigned int ata_dev_set_AN(struct ata_device *dev);
+static unsigned int ata_dev_set_AN(struct ata_device *dev, u8 enable);
 static void ata_dev_xfermask(struct ata_device *dev);
 
 unsigned int ata_print_id = 1;
@@ -2010,7 +2010,7 @@ int ata_dev_configure(struct ata_device 
 		if ((ap->flags & ATA_FLAG_AN) && ata_id_has_AN(id)) {
 			int err;
 			/* issue SET feature command to turn this on */
-			err = ata_dev_set_AN(dev);
+			err = ata_dev_set_AN(dev, SETFEATURES_SATA_ENABLE);
 			if (err)
 				ata_dev_printk(dev, KERN_ERR,
 						"unable to set AN, err %x\n",
@@ -3966,6 +3966,7 @@ static unsigned int ata_dev_set_xfermode
 /**
  *	ata_dev_set_AN - Issue SET FEATURES - SATA FEATURES
  *	@dev: Device to which command will be sent
+ *	@enable: Whether to enable or disable the feature
  *
  *	Issue SET FEATURES - SATA FEATURES command to device @dev
  *	on port @ap with sector count set to indicate Asynchronous
@@ -3977,7 +3978,7 @@ static unsigned int ata_dev_set_xfermode
  *	RETURNS:
  *	0 on success, AC_ERR_* mask otherwise.
  */
-static unsigned int ata_dev_set_AN(struct ata_device *dev)
+static unsigned int ata_dev_set_AN(struct ata_device *dev, u8 enable)
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
@@ -3987,7 +3988,7 @@ static unsigned int ata_dev_set_AN(struc
 
 	ata_tf_init(dev, &tf);
 	tf.command = ATA_CMD_SET_FEATURES;
-	tf.feature = SETFEATURES_SATA_ENABLE;
+	tf.feature = enable;
 	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf.protocol = ATA_PROT_NODATA;
 	tf.nsect = SATA_AN;