2007-05-09 23:41:43

by Kristen Carlson Accardi

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

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

Signed-off-by: Kristen Carlson Accardi <[email protected]>

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,

--


2007-05-10 05:10:20

by Andrew Morton

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

On Wed, 9 May 2007 16:38:09 -0700 Kristen Carlson Accardi <[email protected]> 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.

2007-05-10 05:14:53

by Jeff Garzik

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

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


2007-05-10 05:26:14

by Andrew Morton

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

On Thu, 10 May 2007 01:14:38 -0400 Jeff Garzik <[email protected]> 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? ;)

2007-05-10 15:15:42

by Randy Dunlap

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

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

> On Wed, 9 May 2007 16:38:09 -0700 Kristen Carlson Accardi <[email protected]> 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 ***

2007-05-10 17:05:31

by Kristen Carlson Accardi

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

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

Signed-off-by: Kristen Carlson Accardi <[email protected]>
---
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,

2007-05-25 03:16:18

by Jeff Garzik

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

Kristen Carlson Accardi wrote:
> Check to see if an ATAPI device supports Asynchronous Notification.
> If so, enable it.
>
> Signed-off-by: Kristen Carlson Accardi <[email protected]>
> ---
> 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.

2007-06-11 20:24:19

by Kristen Carlson Accardi

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

On Thu, 24 May 2007 23:15:56 -0400
Jeff Garzik <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>

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;