[PATCHSET 01/11] prep for new EH

[PATCHSET 01/11] prep for new EH

[Tejun Heo]

Hello, all.

This is part of patchset series described in [T].

This is the first take of prep-for-new-EH patchset.  This patchset
contains 22 patches and can be grouped as follows.

#01-02: SCSI support for libata EH
#03-09: various fixes
#10-22: various new features

This patchset is against

  upstream (acc696d93dcf993dec123d69d599979e1456ffec)

New features worth mentioning are...

* Use statically allocated buffers.  dev->id[] is now statically
  allocated and ap->sector_buf[512] is added so that revalidation and
  NCQ read log don't have to allocate buffers during EH.  With this no
  allocation is necessary during EH except for PM link allocation
  which is done with GFP_NOIO and doesn't hamper operation of existing
  devices when it fails.

* qc->result_tf to carry resulting TF after command excution.  TF can
  be explicitly requested via ATA_QCFLAG_RESULT_TF.  LLDDs are
  responsible for filling it when a qc fails (done automatically for
  drivers implementing ->tf_read).  This change is necessary to
  natively support controllers which don't have single TF image like
  sil24 and makes the completion path more efficient.

* New SCR access and on/offline test functions defined.  This cleans
  up scattered SCR availability tests and allows for later PM
  extensions.

* Added dev->ap and remove many @ap arguments.  This simplifies codes
  and makes it much easier to add ata_link inbetween later.

* ata_*_printk() helpers.  links and devices will be identified
  differently on PM and no-PM cases.  Adding printk helpers seems to
  be the cleanest solution.  Maybe debug message handling can be
  integrated, too.  I don't know.

Thanks.

--
tejun

[T] http://article.gmane.org/gmane.linux.ide/9957

[PATCH 10/22] libata: use preallocated buffers

[Tejun Heo]

It's not a very good idea to allocate memory during EH.  Use
statically allocated buffer for dev->id[] and add 512byte buffer
ap->sector_buf.  This buffer is owned by EH (or probing) and to be
used as temporary buffer for various purposes (IDENTIFY, NCQ log page
10h, PM GSCR block).

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   32 ++++++++------------------------
 include/linux/libata.h     |    4 +++-
 2 files changed, 11 insertions(+), 25 deletions(-)

dbdf1ff319e3e4f70ea9582f9db67b613c02ab7e
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 4c8e602..1806c7c 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1099,7 +1099,7 @@ unsigned int ata_pio_need_iordy(const st
  *	@dev: target device
  *	@p_class: pointer to class of the target device (may be changed)
  *	@post_reset: is this read ID post-reset?
- *	@p_id: read IDENTIFY page (newly allocated)
+ *	@id: buffer to read IDENTIFY data into
  *
  *	Read ID data from the specified device.  ATA_CMD_ID_ATA is
  *	performed on ATA devices and ATA_CMD_ID_ATAPI on ATAPI
@@ -1113,12 +1113,11 @@ unsigned int ata_pio_need_iordy(const st
  *	0 on success, -errno otherwise.
  */
 static int ata_dev_read_id(struct ata_port *ap, struct ata_device *dev,
-			   unsigned int *p_class, int post_reset, u16 **p_id)
+			   unsigned int *p_class, int post_reset, u16 *id)
 {
 	unsigned int class = *p_class;
 	struct ata_taskfile tf;
 	unsigned int err_mask = 0;
-	u16 *id;
 	const char *reason;
 	int rc;
 
@@ -1126,13 +1125,6 @@ static int ata_dev_read_id(struct ata_po
 
 	ata_dev_select(ap, dev->devno, 1, 1); /* select device 0/1 */
 
-	id = kmalloc(sizeof(id[0]) * ATA_ID_WORDS, GFP_KERNEL);
-	if (id == NULL) {
-		rc = -ENOMEM;
-		reason = "out of memory";
-		goto err_out;
-	}
-
  retry:
 	ata_tf_init(ap, &tf, dev->devno);
 
@@ -1194,13 +1186,12 @@ static int ata_dev_read_id(struct ata_po
 	}
 
 	*p_class = class;
-	*p_id = id;
+
 	return 0;
 
  err_out:
 	printk(KERN_WARNING "ata%u: dev %u failed to IDENTIFY (%s)\n",
 	       ap->id, dev->devno, reason);
-	kfree(id);
 	return rc;
 }
 
@@ -1425,9 +1416,7 @@ static int ata_bus_probe(struct ata_port
 		if (!ata_dev_enabled(dev))
 			continue;
 
-		kfree(dev->id);
-		dev->id = NULL;
-		rc = ata_dev_read_id(ap, dev, &dev->class, 1, &dev->id);
+		rc = ata_dev_read_id(ap, dev, &dev->class, 1, dev->id);
 		if (rc)
 			goto fail;
 
@@ -2788,7 +2777,7 @@ int ata_dev_revalidate(struct ata_port *
 		       int post_reset)
 {
 	unsigned int class = dev->class;
-	u16 *id = NULL;
+	u16 *id = (void *)ap->sector_buf;
 	int rc;
 
 	if (!ata_dev_enabled(dev)) {
@@ -2796,8 +2785,8 @@ int ata_dev_revalidate(struct ata_port *
 		goto fail;
 	}
 
-	/* allocate & read ID data */
-	rc = ata_dev_read_id(ap, dev, &class, post_reset, &id);
+	/* read ID data */
+	rc = ata_dev_read_id(ap, dev, &class, post_reset, id);
 	if (rc)
 		goto fail;
 
@@ -2807,8 +2796,7 @@ int ata_dev_revalidate(struct ata_port *
 		goto fail;
 	}
 
-	kfree(dev->id);
-	dev->id = id;
+	memcpy(dev->id, id, sizeof(id[0]) * ATA_ID_WORDS);
 
 	/* configure device according to the new ID */
 	rc = ata_dev_configure(ap, dev, 0);
@@ -2818,7 +2806,6 @@ int ata_dev_revalidate(struct ata_port *
  fail:
 	printk(KERN_ERR "ata%u: dev %u revalidation failed (errno=%d)\n",
 	       ap->id, dev->devno, rc);
-	kfree(id);
 	return rc;
 }
 
@@ -4873,14 +4860,11 @@ void ata_host_set_remove(struct ata_host
 int ata_scsi_release(struct Scsi_Host *host)
 {
 	struct ata_port *ap = ata_shost_to_port(host);
-	int i;
 
 	DPRINTK("ENTER\n");
 
 	ap->ops->port_disable(ap);
 	ata_host_remove(ap, 0);
-	for (i = 0; i < ATA_MAX_DEVICES; i++)
-		kfree(ap->device[i].id);
 
 	DPRINTK("EXIT\n");
 	return 1;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9c2d4bb..a117ca2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -360,7 +360,7 @@ struct ata_device {
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	unsigned int		class;		/* ATA_DEV_xxx */
 	unsigned int		devno;		/* 0 or 1 */
-	u16			*id;		/* IDENTIFY xxx DEVICE data */
+	u16			id[ATA_ID_WORDS]; /* IDENTIFY xxx DEVICE data */
 	u8			pio_mode;
 	u8			dma_mode;
 	u8			xfer_mode;
@@ -425,6 +425,8 @@ struct ata_port {
 	struct list_head	eh_done_q;
 
 	void			*private_data;
+
+	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
 };
 
 struct ata_port_operations {
-- 
1.2.4

Re: [PATCH 10/22] libata: use preallocated buffers

[Albert Lee]

Tejun Heo wrote:
> It's not a very good idea to allocate memory during EH.  Use
> statically allocated buffer for dev->id[] and add 512byte buffer
> ap->sector_buf.  This buffer is owned by EH (or probing) and to be
> used as temporary buffer for various purposes (IDENTIFY, NCQ log page
> 10h, PM GSCR block).
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> <snip>
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 9c2d4bb..a117ca2 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -360,7 +360,7 @@ struct ata_device {
>  	unsigned long		flags;		/* ATA_DFLAG_xxx */
>  	unsigned int		class;		/* ATA_DEV_xxx */
>  	unsigned int		devno;		/* 0 or 1 */
> -	u16			*id;		/* IDENTIFY xxx DEVICE data */
> +	u16			id[ATA_ID_WORDS]; /* IDENTIFY xxx DEVICE data */
>  	u8			pio_mode;
>  	u8			dma_mode;
>  	u8			xfer_mode;
> @@ -425,6 +425,8 @@ struct ata_port {
>  	struct list_head	eh_done_q;
>  
>  	void			*private_data;
> +
> +	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
>  };
>  
>  struct ata_port_operations {

Reorder ata_host_set->private_data and ata_port->private_data.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Just a minor fix.
Maybe we can make the ->private_data the last one for easier allocation.

--- upstream/include/linux/libata.h	2006-05-16 11:08:54.000000000 +0800
+++ upstream1/include/linux/libata.h	2006-05-17 13:25:24.000000000 +0800
@@ -330,12 +330,12 @@ struct ata_host_set {
 	unsigned long		irq;
 	void __iomem		*mmio_base;
 	unsigned int		n_ports;
-	void			*private_data;
 	const struct ata_port_operations *ops;
 	unsigned long		flags;
 	int			simplex_claimed;	/* Keep seperate in case we
 							   ever need to do this locked */
 	struct ata_port *	ports[0];
+	void			*private_data;
 };
 
 struct ata_queued_cmd {
@@ -492,9 +492,8 @@ struct ata_port {
 	u32			msg_enable;
 	struct list_head	eh_done_q;
 
-	void			*private_data;
-
 	u8			sector_buf[ATA_SECT_SIZE]; /* owned by EH */
+	void			*private_data;
 };
 
 struct ata_port_operations {

Re: [PATCH 10/22] libata: use preallocated buffers

[Jeff Garzik]

Albert Lee wrote:
> --- upstream/include/linux/libata.h	2006-05-16 11:08:54.000000000 +0800
> +++ upstream1/include/linux/libata.h	2006-05-17 13:25:24.000000000 +0800
> @@ -330,12 +330,12 @@ struct ata_host_set {
>  	unsigned long		irq;
>  	void __iomem		*mmio_base;
>  	unsigned int		n_ports;
> -	void			*private_data;
>  	const struct ata_port_operations *ops;
>  	unsigned long		flags;
>  	int			simplex_claimed;	/* Keep seperate in case we
>  							   ever need to do this locked */
>  	struct ata_port *	ports[0];
> +	void			*private_data;
>  };

The zero length array needs to remain the last element in the struct...

	Jeff

Re: [PATCH 10/22] libata: use preallocated buffers

[Albert Lee]

Jeff Garzik wrote:
> Albert Lee wrote:
> 
>> --- upstream/include/linux/libata.h    2006-05-16 11:08:54.000000000
>> +0800
>> +++ upstream1/include/linux/libata.h    2006-05-17 13:25:24.000000000
>> +0800
>> @@ -330,12 +330,12 @@ struct ata_host_set {
>>      unsigned long        irq;
>>      void __iomem        *mmio_base;
>>      unsigned int        n_ports;
>> -    void            *private_data;
>>      const struct ata_port_operations *ops;
>>      unsigned long        flags;
>>      int            simplex_claimed;    /* Keep seperate in case we
>>                                 ever need to do this locked */
>>      struct ata_port *    ports[0];
>> +    void            *private_data;
>>  };
> 
> 
> The zero length array needs to remain the last element in the struct...
> 

Ah, I see. Sorry for the noise.

--
albert

[PATCH 01/22] SCSI: Introduce scsi_req_abort_cmd (REPOST)

[Tejun Heo]

Introduce scsi_req_abort_cmd(struct scsi_cmnd *).
This function requests that SCSI Core start recovery for the
command by deleting the timer and adding the command to the eh
queue.  It can be called by either LLDDs or SCSI Core.  LLDDs who
implement their own error recovery MAY ignore the timeout event if
they generated scsi_req_abort_cmd.

First post:
http://marc.theaimsgroup.com/?l=linux-scsi&m=113833937421677&w=2

Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/scsi.c      |   18 ++++++++++++++++++
 include/scsi/scsi_cmnd.h |    1 +
 2 files changed, 19 insertions(+), 0 deletions(-)

2dcc69f8559ce963d637e881ca168afd91ee2478
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 73994e2..dae4f08 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -720,6 +720,24 @@ void scsi_init_cmd_from_req(struct scsi_
 static DEFINE_PER_CPU(struct list_head, scsi_done_q);
 
 /**
+ * scsi_req_abort_cmd -- Request command recovery for the specified command
+ * cmd: pointer to the SCSI command of interest
+ *
+ * This function requests that SCSI Core start recovery for the
+ * command by deleting the timer and adding the command to the eh
+ * queue.  It can be called by either LLDDs or SCSI Core.  LLDDs who
+ * implement their own error recovery MAY ignore the timeout event if
+ * they generated scsi_req_abort_cmd.
+ */
+void scsi_req_abort_cmd(struct scsi_cmnd *cmd)
+{
+	if (!scsi_delete_timer(cmd))
+		return;
+	scsi_times_out(cmd);
+}
+EXPORT_SYMBOL(scsi_req_abort_cmd);
+
+/**
  * scsi_done - Enqueue the finished SCSI command into the done queue.
  * @cmd: The SCSI Command for which a low-level device driver (LLDD) gives
  * ownership back to SCSI Core -- i.e. the LLDD has finished with it.
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 1ace1b9..88c6c4d 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -151,5 +151,6 @@ extern struct scsi_cmnd *scsi_get_comman
 extern void scsi_put_command(struct scsi_cmnd *);
 extern void scsi_io_completion(struct scsi_cmnd *, unsigned int, unsigned int);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
+extern void scsi_req_abort_cmd(struct scsi_cmnd *cmd);
 
 #endif /* _SCSI_SCSI_CMND_H */
-- 
1.2.4

Re: [PATCH 01/22] SCSI: Introduce scsi_req_abort_cmd (REPOST)

[Jeff Garzik]

Tejun Heo wrote:
> Introduce scsi_req_abort_cmd(struct scsi_cmnd *).
> This function requests that SCSI Core start recovery for the
> command by deleting the timer and adding the command to the eh
> queue.  It can be called by either LLDDs or SCSI Core.  LLDDs who
> implement their own error recovery MAY ignore the timeout event if
> they generated scsi_req_abort_cmd.

ACK...


> 2dcc69f8559ce963d637e881ca168afd91ee2478
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 73994e2..dae4f08 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -720,6 +720,24 @@ void scsi_init_cmd_from_req(struct scsi_
>  static DEFINE_PER_CPU(struct list_head, scsi_done_q);
>  
>  /**
> + * scsi_req_abort_cmd -- Request command recovery for the specified command
> + * cmd: pointer to the SCSI command of interest
> + *
> + * This function requests that SCSI Core start recovery for the
> + * command by deleting the timer and adding the command to the eh
> + * queue.  It can be called by either LLDDs or SCSI Core.  LLDDs who
> + * implement their own error recovery MAY ignore the timeout event if
> + * they generated scsi_req_abort_cmd.
> + */
> +void scsi_req_abort_cmd(struct scsi_cmnd *cmd)
> +{
> +	if (!scsi_delete_timer(cmd))
> +		return;
> +	scsi_times_out(cmd);
> +}
> +EXPORT_SYMBOL(scsi_req_abort_cmd);
> +
> +/**
>   * scsi_done - Enqueue the finished SCSI command into the done queue.
>   * @cmd: The SCSI Command for which a low-level device driver (LLDD) gives
>   * ownership back to SCSI Core -- i.e. the LLDD has finished with it.
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 1ace1b9..88c6c4d 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -151,5 +151,6 @@ extern struct scsi_cmnd *scsi_get_comman
>  extern void scsi_put_command(struct scsi_cmnd *);
>  extern void scsi_io_completion(struct scsi_cmnd *, unsigned int, unsigned int);
>  extern void scsi_finish_command(struct scsi_cmnd *cmd);
> +extern void scsi_req_abort_cmd(struct scsi_cmnd *cmd);
>  
>  #endif /* _SCSI_SCSI_CMND_H */

Patch quoted in its entirety, since this copy wasn't CC'd to linux-scsi.

	Jeff

Re: [PATCH 01/22] SCSI: Introduce scsi_req_abort_cmd (REPOST)

[Luben Tuikov]

--- Jeff Garzik <jgarzik@pobox.com> wrote:

> Tejun Heo wrote:
> > Introduce scsi_req_abort_cmd(struct scsi_cmnd *).
> > This function requests that SCSI Core start recovery for the
> > command by deleting the timer and adding the command to the eh
> > queue.  It can be called by either LLDDs or SCSI Core.  LLDDs who
> > implement their own error recovery MAY ignore the timeout event if
> > they generated scsi_req_abort_cmd.
> 
> ACK...
> 
> 
> > 2dcc69f8559ce963d637e881ca168afd91ee2478
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index 73994e2..dae4f08 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -720,6 +720,24 @@ void scsi_init_cmd_from_req(struct scsi_
> >  static DEFINE_PER_CPU(struct list_head, scsi_done_q);
> >  
> >  /**
> > + * scsi_req_abort_cmd -- Request command recovery for the specified command
> > + * cmd: pointer to the SCSI command of interest
> > + *
> > + * This function requests that SCSI Core start recovery for the
> > + * command by deleting the timer and adding the command to the eh
> > + * queue.  It can be called by either LLDDs or SCSI Core.  LLDDs who
> > + * implement their own error recovery MAY ignore the timeout event if
> > + * they generated scsi_req_abort_cmd.
> > + */
> > +void scsi_req_abort_cmd(struct scsi_cmnd *cmd)
> > +{
> > +	if (!scsi_delete_timer(cmd))
> > +		return;
> > +	scsi_times_out(cmd);
> > +}
> > +EXPORT_SYMBOL(scsi_req_abort_cmd);
> > +
> > +/**
> >   * scsi_done - Enqueue the finished SCSI command into the done queue.
> >   * @cmd: The SCSI Command for which a low-level device driver (LLDD) gives
> >   * ownership back to SCSI Core -- i.e. the LLDD has finished with it.
> > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> > index 1ace1b9..88c6c4d 100644
> > --- a/include/scsi/scsi_cmnd.h
> > +++ b/include/scsi/scsi_cmnd.h
> > @@ -151,5 +151,6 @@ extern struct scsi_cmnd *scsi_get_comman
> >  extern void scsi_put_command(struct scsi_cmnd *);
> >  extern void scsi_io_completion(struct scsi_cmnd *, unsigned int, unsigned int);
> >  extern void scsi_finish_command(struct scsi_cmnd *cmd);
> > +extern void scsi_req_abort_cmd(struct scsi_cmnd *cmd);
> >  
> >  #endif /* _SCSI_SCSI_CMND_H */
> 
> Patch quoted in its entirety, since this copy wasn't CC'd to linux-scsi.

No, the patch isn't quoted in its entirety.  You deleted a bunch of lines.
See my next email where I simply CC lsml, and don't delete any lines.

Good luck,
      Luben

Re: [PATCH 01/22] SCSI: Introduce scsi_req_abort_cmd (REPOST)

[Luben Tuikov]

--- Tejun Heo <htejun@gmail.com> wrote:

> Introduce scsi_req_abort_cmd(struct scsi_cmnd *).
> This function requests that SCSI Core start recovery for the
> command by deleting the timer and adding the command to the eh
> queue.  It can be called by either LLDDs or SCSI Core.  LLDDs who
> implement their own error recovery MAY ignore the timeout event if
> they generated scsi_req_abort_cmd.
> 
> First post:
> http://marc.theaimsgroup.com/?l=linux-scsi&m=113833937421677&w=2
> 
> Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/scsi.c      |   18 ++++++++++++++++++
>  include/scsi/scsi_cmnd.h |    1 +
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> 2dcc69f8559ce963d637e881ca168afd91ee2478
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 73994e2..dae4f08 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -720,6 +720,24 @@ void scsi_init_cmd_from_req(struct scsi_
>  static DEFINE_PER_CPU(struct list_head, scsi_done_q);
>  
>  /**
> + * scsi_req_abort_cmd -- Request command recovery for the specified command
> + * cmd: pointer to the SCSI command of interest
> + *
> + * This function requests that SCSI Core start recovery for the
> + * command by deleting the timer and adding the command to the eh
> + * queue.  It can be called by either LLDDs or SCSI Core.  LLDDs who
> + * implement their own error recovery MAY ignore the timeout event if
> + * they generated scsi_req_abort_cmd.
> + */
> +void scsi_req_abort_cmd(struct scsi_cmnd *cmd)
> +{
> +	if (!scsi_delete_timer(cmd))
> +		return;
> +	scsi_times_out(cmd);
> +}
> +EXPORT_SYMBOL(scsi_req_abort_cmd);
> +
> +/**
>   * scsi_done - Enqueue the finished SCSI command into the done queue.
>   * @cmd: The SCSI Command for which a low-level device driver (LLDD) gives
>   * ownership back to SCSI Core -- i.e. the LLDD has finished with it.
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 1ace1b9..88c6c4d 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -151,5 +151,6 @@ extern struct scsi_cmnd *scsi_get_comman
>  extern void scsi_put_command(struct scsi_cmnd *);
>  extern void scsi_io_completion(struct scsi_cmnd *, unsigned int, unsigned int);
>  extern void scsi_finish_command(struct scsi_cmnd *cmd);
> +extern void scsi_req_abort_cmd(struct scsi_cmnd *cmd);
>  
>  #endif /* _SCSI_SCSI_CMND_H */
> -- 
> 1.2.4

Now that's the patch in its entirety, Jeff.

     Luben

Re: [PATCH 01/22] SCSI: Introduce scsi_req_abort_cmd (REPOST)

[Jeff Garzik]

Luben Tuikov wrote:
> Now that's the patch in its entirety, Jeff.
> 
>      Luben


hehe, thanks Dad :)

	Jeff

Re: [PATCH 01/22] SCSI: Introduce scsi_req_abort_cmd (REPOST)

[Luben Tuikov]

--- Jeff Garzik <jgarzik@pobox.com> wrote:

> Luben Tuikov wrote:
> > Now that's the patch in its entirety, Jeff.
> > 
> >      Luben
> 
> 
> hehe, thanks Dad :)

It is not very nice to chop off the patch email which GIT generated,
and then claim, as you did, that that was the whole thing, when
it clearly it was not.

      Luben

Re: [PATCH 01/22] SCSI: Introduce scsi_req_abort_cmd (REPOST)

[Jeff Garzik]

Luben Tuikov wrote:
> --- Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>> Luben Tuikov wrote:
>>> Now that's the patch in its entirety, Jeff.
>>>
>>>      Luben
>>
>> hehe, thanks Dad :)
> 
> It is not very nice to chop off the patch email which GIT generated,
> and then claim, as you did, that that was the whole thing, when
> it clearly it was not.

Clearly, its a conspiracy.  No one ever makes mistakes.

	Jeff

[PATCH 04/22] ahci: hardreset classification fix

[Tejun Heo]

AHCI calls ata_dev_classify() even when no device is attached which
results in false class code.  Fix it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fab598203c75f0ad53a57a2c629433586036e1f0
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index d23f002..c332554 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -670,7 +670,7 @@ static int ahci_hardreset(struct ata_por
 	rc = sata_std_hardreset(ap, class);
 	ahci_start_engine(ap);
 
-	if (rc == 0)
+	if (rc == 0 && sata_dev_present(ap))
 		*class = ahci_dev_classify(ap);
 	if (*class == ATA_DEV_UNKNOWN)
 		*class = ATA_DEV_NONE;
-- 
1.2.4

[PATCH 06/22] libata: kill duplicate prototypes

[Tejun Heo]

Kill duplicate prototypes for ata_eh_qc_complete/retry() in libata.h.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 include/linux/libata.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

24e675bc2d266ce75ca95663b3e92d97beb8bbaf
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d35b1e3..9c2d4bb 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -530,8 +530,6 @@ extern void ata_host_set_remove(struct a
 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_eh_qc_complete(struct ata_queued_cmd *qc);
-extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
 extern int ata_scsi_release(struct Scsi_Host *host);
 extern unsigned int ata_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
 extern int ata_scsi_device_resume(struct scsi_device *);
-- 
1.2.4

[PATCH 03/22] libata: silly fix in ata_scsi_start_stop_xlat()

[Tejun Heo]

Don't directly access &qc->tf when tf == &qc->tf.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-scsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

0dc90181abc07a5fa439bc099620f34381ec293b
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 9871f82..b0c83c2 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -748,7 +748,7 @@ static unsigned int ata_scsi_start_stop_
 		tf->nsect = 1;	/* 1 sector, lba=0 */
 
 		if (qc->dev->flags & ATA_DFLAG_LBA) {
-			qc->tf.flags |= ATA_TFLAG_LBA;
+			tf->flags |= ATA_TFLAG_LBA;
 
 			tf->lbah = 0x0;
 			tf->lbam = 0x0;
-- 
1.2.4

[PATCH 05/22] libata: unexport ata_scsi_error()

[Tejun Heo]

While moving ata_scsi_error() from LLDD sht to libata transportt,
EXPORT_SYMBOL_GPL() entry was left out.  Kill it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

b6212a123649958b5502a95e235807e2176bb247
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 509178c..8ac560c 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5161,7 +5161,6 @@ EXPORT_SYMBOL_GPL(ata_device_resume);
 EXPORT_SYMBOL_GPL(ata_scsi_device_suspend);
 EXPORT_SYMBOL_GPL(ata_scsi_device_resume);
 
-EXPORT_SYMBOL_GPL(ata_scsi_error);
 EXPORT_SYMBOL_GPL(ata_eng_timeout);
 EXPORT_SYMBOL_GPL(ata_eh_qc_complete);
 EXPORT_SYMBOL_GPL(ata_eh_qc_retry);
-- 
1.2.4

[PATCH 08/22] libata: clear ap->active_tag atomically w.r.t. command completion

[Tejun Heo]

ap->active_tag was cleared in ata_qc_free().  This left ap->active_tag
dangling after ata_qc_complete().  Spurious interrupts inbetween could
incorrectly access the qc.  Clear active_tag in ata_qc_complete().
This change is necessary for later EH changes.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

7054ad3067bb27d6070a75f90afd14fafd3f5527
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 142a3a8..763dd66 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4083,8 +4083,6 @@ void ata_qc_free(struct ata_queued_cmd *
 	qc->flags = 0;
 	tag = qc->tag;
 	if (likely(ata_tag_valid(tag))) {
-		if (tag == ap->active_tag)
-			ap->active_tag = ATA_TAG_POISON;
 		qc->tag = ATA_TAG_POISON;
 		clear_bit(tag, &ap->qactive);
 	}
@@ -4098,6 +4096,9 @@ void __ata_qc_complete(struct ata_queued
 	if (likely(qc->flags & ATA_QCFLAG_DMAMAP))
 		ata_sg_clean(qc);
 
+	/* command should be marked inactive atomically with qc completion */
+	qc->ap->active_tag = ATA_TAG_POISON;
+
 	/* atapi: mark qc as inactive to prevent the interrupt handler
 	 * from completing the command twice later, before the error handler
 	 * is called. (when rc != 0 and atapi request sense is needed)
-- 
1.2.4

[PATCH 07/22] libata: fix ->phy_reset class code handling in ata_bus_probe()

[Tejun Heo]

ata_bus_probe() doesn't clear dev->class after ->phy_reset().  This
can result in falsely enabled devices if probing fails.  Clear
dev->class to ATA_DEV_UNKNOWN after fetching it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

2c6d8801ef9205d36d646cd8a0bf81a27b6c46d2
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 8ac560c..142a3a8 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1397,9 +1397,11 @@ static int ata_bus_probe(struct ata_port
 	} else {
 		ap->ops->phy_reset(ap);
 
-		if (!(ap->flags & ATA_FLAG_DISABLED))
-			for (i = 0; i < ATA_MAX_DEVICES; i++)
+		for (i = 0; i < ATA_MAX_DEVICES; i++) {
+			if (!(ap->flags & ATA_FLAG_DISABLED))
 				classes[i] = ap->device[i].class;
+			ap->device[i].class = ATA_DEV_UNKNOWN;
+		}
 
 		ata_port_probe(ap);
 	}
-- 
1.2.4

[PATCH 09/22] libata: hold host_set lock while finishing internal qc

[Tejun Heo]

Hold host_set lock while finishing internal qc.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

b187d8d384d552b351c297e6e3e8276781e5ffd5
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 763dd66..4c8e602 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1031,6 +1031,9 @@ unsigned ata_exec_internal(struct ata_po
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 	}
 
+	/* finish up */
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+
 	*tf = qc->tf;
 	err_mask = qc->err_mask;
 
@@ -1052,6 +1055,8 @@ unsigned ata_exec_internal(struct ata_po
 		ata_port_probe(ap);
 	}
 
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
 	return err_mask;
 }
 
-- 
1.2.4

[PATCH 02/22] SCSI: implement host_eh_scheduled hack for libata

[Tejun Heo]

libata needs to invoke EH without scmd.  This patch adds
shost->host_eh_scheduled to implement such behavior.

As this is a temporary hack for libata, no general interface is
defined.  This patch simply adds handling for host_eh_scheduled where
needed and exports scsi_eh_wakeup() to modules.  The rest is upto
libata.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/scsi_error.c |    3 ++-
 drivers/scsi/scsi_lib.c   |    2 +-
 drivers/scsi/scsi_priv.h  |    1 -
 include/scsi/scsi_eh.h    |    1 +
 include/scsi/scsi_host.h  |    1 +
 5 files changed, 5 insertions(+), 3 deletions(-)

b3095135fd8d02d5b31591d87243afb3aeb217da
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 1c75646..442ae59 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -56,6 +56,7 @@ void scsi_eh_wakeup(struct Scsi_Host *sh
 				printk("Waking error handler thread\n"));
 	}
 }
+EXPORT_SYMBOL_GPL(scsi_eh_wakeup);	/* exported only for libata */
 
 /**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
@@ -1517,7 +1518,7 @@ int scsi_error_handler(void *data)
 	 */
 	set_current_state(TASK_INTERRUPTIBLE);
 	while (!kthread_should_stop()) {
-		if (shost->host_failed == 0 ||
+		if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
 		    shost->host_failed != shost->host_busy) {
 			SCSI_LOG_ERROR_RECOVERY(1,
 				printk("Error handler scsi_eh_%d sleeping\n",
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7b0f9a3..c55d195 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -566,7 +566,7 @@ void scsi_device_unbusy(struct scsi_devi
 	spin_lock_irqsave(shost->host_lock, flags);
 	shost->host_busy--;
 	if (unlikely(scsi_host_in_recovery(shost) &&
-		     shost->host_failed))
+		     (shost->host_failed || shost->host_eh_scheduled)))
 		scsi_eh_wakeup(shost);
 	spin_unlock(shost->host_lock);
 	spin_lock(sdev->request_queue->queue_lock);
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 27c4827..0b39081 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -63,7 +63,6 @@ extern int scsi_delete_timer(struct scsi
 extern void scsi_times_out(struct scsi_cmnd *cmd);
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
-extern void scsi_eh_wakeup(struct Scsi_Host *shost);
 extern int scsi_eh_scmd_add(struct scsi_cmnd *, int);
 
 /* scsi_lib.c */
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index d160880..e31ada2 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -35,6 +35,7 @@ static inline int scsi_sense_valid(struc
 }
 
 
+extern void scsi_eh_wakeup(struct Scsi_Host *shost);	/* libata only */
 extern void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
 			       struct list_head *done_q);
 extern void scsi_eh_flush_done_q(struct list_head *done_q);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index de6ce54..3b454c2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -472,6 +472,7 @@ struct Scsi_Host {
 	 */
 	unsigned int host_busy;		   /* commands actually active on low-level */
 	unsigned int host_failed;	   /* commands that failed. */
+	unsigned int host_eh_scheduled;	   /* XXX - libata hack, do NOT use */
     
 	unsigned short host_no;  /* Used for IOCTL_GET_IDLUN, /proc/scsi et al. */
 	int resetting; /* if set, it means that last_reset is a valid value */
-- 
1.2.4

Re: [PATCH 02/22] SCSI: implement host_eh_scheduled hack for libata

[Jeff Garzik]

Tejun Heo wrote:
> libata needs to invoke EH without scmd.  This patch adds
> shost->host_eh_scheduled to implement such behavior.
> 
> As this is a temporary hack for libata, no general interface is
> defined.  This patch simply adds handling for host_eh_scheduled where
> needed and exports scsi_eh_wakeup() to modules.  The rest is upto
> libata.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

I ACK the change (the important part), but not the description or comments.

Rationale for ACK (if it matters):  Back when the first version of this 
was posted, after our initial discussion I reviewed the uses of 
->host_failed and ->host_busy, and had considered twiddling those rather 
than adding another state variable.  Being in EH when host_failed==0 
made me nervous.  However, after reviewing the uses, I now feel that 
your approach presented probably creates less complex code.



> b3095135fd8d02d5b31591d87243afb3aeb217da
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 1c75646..442ae59 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -56,6 +56,7 @@ void scsi_eh_wakeup(struct Scsi_Host *sh
>  				printk("Waking error handler thread\n"));
>  	}
>  }
> +EXPORT_SYMBOL_GPL(scsi_eh_wakeup);	/* exported only for libata */
>  
>  /**
>   * scsi_eh_scmd_add - add scsi cmd to error handling.
> @@ -1517,7 +1518,7 @@ int scsi_error_handler(void *data)
>  	 */
>  	set_current_state(TASK_INTERRUPTIBLE);
>  	while (!kthread_should_stop()) {
> -		if (shost->host_failed == 0 ||
> +		if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
>  		    shost->host_failed != shost->host_busy) {
>  			SCSI_LOG_ERROR_RECOVERY(1,
>  				printk("Error handler scsi_eh_%d sleeping\n",
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7b0f9a3..c55d195 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -566,7 +566,7 @@ void scsi_device_unbusy(struct scsi_devi
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	shost->host_busy--;
>  	if (unlikely(scsi_host_in_recovery(shost) &&
> -		     shost->host_failed))
> +		     (shost->host_failed || shost->host_eh_scheduled)))
>  		scsi_eh_wakeup(shost);
>  	spin_unlock(shost->host_lock);
>  	spin_lock(sdev->request_queue->queue_lock);
> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
> index d160880..e31ada2 100644
> --- a/include/scsi/scsi_eh.h
> +++ b/include/scsi/scsi_eh.h
> @@ -35,6 +35,7 @@ static inline int scsi_sense_valid(struc
>  }
>  
>  
> +extern void scsi_eh_wakeup(struct Scsi_Host *shost);	/* libata only */
>  extern void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
>  			       struct list_head *done_q);
>  extern void scsi_eh_flush_done_q(struct list_head *done_q);
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index de6ce54..3b454c2 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -472,6 +472,7 @@ struct Scsi_Host {
>  	 */
>  	unsigned int host_busy;		   /* commands actually active on low-level */
>  	unsigned int host_failed;	   /* commands that failed. */
> +	unsigned int host_eh_scheduled;	   /* XXX - libata hack, do NOT use */

However, the nitpicks I have with the comments and patch description:

* Although your comments about "libata only" are true, it is more 
correct to say that these are exclusive to ->eh_strategy_handler() 
users.  It just so happens that libata is the only in-tree user... 
however when making changes to the SCSI driver API, you should be in a 
"I'm updating the SCSI EH API" frame of mine as well.

* In your patch description you mention this change (or a subset 
thereof) is temporary, but do not explain why it is temporary, and what 
will be the eventual replacement.

* In general this change should be described as an addition to the SCSI 
LLDD EH API...  I even disagree to some extent of calling it a hack. 
One must Do What Needs To Be Done, And No More.  There is no shame in 
that :)

The ->eh_strategy_handler API is guaranteed to be rough, because there 
weren't really any in-tree users until libata came along.  Now we're 
exposing the rough edges, and fixing them.

	Jeff

[PATCH 12/22] libata: remove postreset handling from ata_do_reset()

[Tejun Heo]

Make ata_do_reset() deal only with reset.  postreset is now the
responsibility of the caller.  This is simpler and eases later
prereset addition.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   19 ++++++++++---------
 drivers/scsi/libata.h      |    2 +-
 2 files changed, 11 insertions(+), 10 deletions(-)

1875f703363435b19f33166a74ef3081a1969967
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index ec40c79..464fd46 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2585,7 +2585,7 @@ int ata_std_probe_reset(struct ata_port 
 }
 
 int ata_do_reset(struct ata_port *ap, ata_reset_fn_t reset,
-		 ata_postreset_fn_t postreset, unsigned int *classes)
+		 unsigned int *classes)
 {
 	int i, rc;
 
@@ -2609,9 +2609,6 @@ int ata_do_reset(struct ata_port *ap, at
 			if (classes[i] == ATA_DEV_UNKNOWN)
 				classes[i] = ATA_DEV_NONE;
 
-	if (postreset)
-		postreset(ap, classes);
-
 	return 0;
 }
 
@@ -2655,7 +2652,7 @@ int ata_drive_probe_reset(struct ata_por
 		probeinit(ap);
 
 	if (softreset && !ata_set_sata_spd_needed(ap)) {
-		rc = ata_do_reset(ap, softreset, postreset, classes);
+		rc = ata_do_reset(ap, softreset, classes);
 		if (rc == 0 && classes[0] != ATA_DEV_UNKNOWN)
 			goto done;
 		printk(KERN_INFO "ata%u: softreset failed, will try "
@@ -2667,7 +2664,7 @@ int ata_drive_probe_reset(struct ata_por
 		goto done;
 
 	while (1) {
-		rc = ata_do_reset(ap, hardreset, postreset, classes);
+		rc = ata_do_reset(ap, hardreset, classes);
 		if (rc == 0) {
 			if (classes[0] != ATA_DEV_UNKNOWN)
 				goto done;
@@ -2688,12 +2685,16 @@ int ata_drive_probe_reset(struct ata_por
 		       ap->id);
 		ssleep(5);
 
-		rc = ata_do_reset(ap, softreset, postreset, classes);
+		rc = ata_do_reset(ap, softreset, classes);
 	}
 
  done:
-	if (rc == 0 && classes[0] == ATA_DEV_UNKNOWN)
-		rc = -ENODEV;
+	if (rc == 0) {
+		if (postreset)
+			postreset(ap, classes);
+		if (classes[0] == ATA_DEV_UNKNOWN)
+			rc = -ENODEV;
+	}
 	return rc;
 }
 
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index 3f8b0a8..43a56e5 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -57,7 +57,7 @@ extern int ata_down_xfermask_limit(struc
 				   int force_pio0);
 extern int ata_set_mode(struct ata_port *ap, struct ata_device **r_failed_dev);
 extern int ata_do_reset(struct ata_port *ap, ata_reset_fn_t reset,
-			ata_postreset_fn_t postreset, unsigned int *classes);
+			unsigned int *classes);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern void ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
-- 
1.2.4

[PATCH 20/22] libata: use dev->ap

[Tejun Heo]

Use dev->ap where possible and eliminate superflous @ap from functions
and structures.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c        |    2 -
 drivers/scsi/libata-core.c |  160 +++++++++++++++++++++-----------------------
 drivers/scsi/libata-scsi.c |   40 +++++------
 drivers/scsi/libata.h      |   11 +--
 include/linux/libata.h     |   21 ++----
 5 files changed, 106 insertions(+), 128 deletions(-)

350ae14b3a5639351348cb30b2ddd50022d6bdab
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index c2298fb..f6e4c8e 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -597,7 +597,7 @@ static int ahci_softreset(struct ata_por
 	/* restart engine */
 	ahci_start_engine(ap);
 
-	ata_tf_init(ap, &tf, 0);
+	ata_tf_init(ap->device, &tf);
 	fis = pp->cmd_tbl;
 
 	/* issue the first D2H Register FIS */
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 38a9833..8eee5af 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -61,13 +61,10 @@
 
 #include "libata.h"
 
-static unsigned int ata_dev_init_params(struct ata_port *ap,
-					struct ata_device *dev,
-					u16 heads,
-					u16 sectors);
-static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
-					 struct ata_device *dev);
-static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev);
+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 void ata_dev_xfermask(struct ata_device *dev);
 
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
@@ -412,11 +409,11 @@ static const char *sata_spd_string(unsig
 	return spd_str[spd - 1];
 }
 
-void ata_dev_disable(struct ata_port *ap, struct ata_device *dev)
+void ata_dev_disable(struct ata_device *dev)
 {
 	if (ata_dev_enabled(dev)) {
 		printk(KERN_WARNING "ata%u: dev %u disabled\n",
-		       ap->id, dev->devno);
+		       dev->ap->id, dev->devno);
 		dev->class++;
 	}
 }
@@ -960,7 +957,6 @@ void ata_qc_complete_internal(struct ata
 
 /**
  *	ata_exec_internal - execute libata internal command
- *	@ap: Port to which the command is sent
  *	@dev: Device to which the command is sent
  *	@tf: Taskfile registers for the command and the result
  *	@cdb: CDB for packet command
@@ -978,10 +974,11 @@ void ata_qc_complete_internal(struct ata
  *	None.  Should be called with kernel context, might sleep.
  */
 
-unsigned ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
+unsigned ata_exec_internal(struct ata_device *dev,
 			   struct ata_taskfile *tf, const u8 *cdb,
 			   int dma_dir, void *buf, unsigned int buflen)
 {
+	struct ata_port *ap = dev->ap;
 	u8 command = tf->command;
 	struct ata_queued_cmd *qc;
 	DECLARE_COMPLETION(wait);
@@ -990,7 +987,7 @@ unsigned ata_exec_internal(struct ata_po
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 
-	qc = ata_qc_new_init(ap, dev);
+	qc = ata_qc_new_init(dev);
 	BUG_ON(qc == NULL);
 
 	qc->tf = *tf;
@@ -1095,7 +1092,6 @@ unsigned int ata_pio_need_iordy(const st
 
 /**
  *	ata_dev_read_id - Read ID data from the specified device
- *	@ap: port on which target device resides
  *	@dev: target device
  *	@p_class: pointer to class of the target device (may be changed)
  *	@post_reset: is this read ID post-reset?
@@ -1112,9 +1108,10 @@ unsigned int ata_pio_need_iordy(const st
  *	RETURNS:
  *	0 on success, -errno otherwise.
  */
-static int ata_dev_read_id(struct ata_port *ap, struct ata_device *dev,
-			   unsigned int *p_class, int post_reset, u16 *id)
+static int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
+			   int post_reset, u16 *id)
 {
+	struct ata_port *ap = dev->ap;
 	unsigned int class = *p_class;
 	struct ata_taskfile tf;
 	unsigned int err_mask = 0;
@@ -1126,7 +1123,7 @@ static int ata_dev_read_id(struct ata_po
 	ata_dev_select(ap, dev->devno, 1, 1); /* select device 0/1 */
 
  retry:
-	ata_tf_init(ap, &tf, dev->devno);
+	ata_tf_init(dev, &tf);
 
 	switch (class) {
 	case ATA_DEV_ATA:
@@ -1143,7 +1140,7 @@ static int ata_dev_read_id(struct ata_po
 
 	tf.protocol = ATA_PROT_PIO;
 
-	err_mask = ata_exec_internal(ap, dev, &tf, NULL, DMA_FROM_DEVICE,
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     id, sizeof(id[0]) * ATA_ID_WORDS);
 	if (err_mask) {
 		rc = -EIO;
@@ -1170,7 +1167,7 @@ static int ata_dev_read_id(struct ata_po
 		 * Some drives were very specific about that exact sequence.
 		 */
 		if (ata_id_major_version(id) < 4 || !ata_id_has_lba(id)) {
-			err_mask = ata_dev_init_params(ap, dev, id[3], id[6]);
+			err_mask = ata_dev_init_params(dev, id[3], id[6]);
 			if (err_mask) {
 				rc = -EIO;
 				reason = "INIT_DEV_PARAMS failed";
@@ -1195,15 +1192,13 @@ static int ata_dev_read_id(struct ata_po
 	return rc;
 }
 
-static inline u8 ata_dev_knobble(const struct ata_port *ap,
-				 struct ata_device *dev)
+static inline u8 ata_dev_knobble(struct ata_device *dev)
 {
-	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
+	return ((dev->ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
 }
 
 /**
  *	ata_dev_configure - Configure the specified ATA/ATAPI device
- *	@ap: Port on which target device resides
  *	@dev: Target device to configure
  *	@print_info: Enable device info printout
  *
@@ -1216,9 +1211,9 @@ static inline u8 ata_dev_knobble(const s
  *	RETURNS:
  *	0 on success, -errno otherwise
  */
-static int ata_dev_configure(struct ata_port *ap, struct ata_device *dev,
-			     int print_info)
+static int ata_dev_configure(struct ata_device *dev, int print_info)
 {
+	struct ata_port *ap = dev->ap;
 	const u16 *id = dev->id;
 	unsigned int xfer_mask;
 	int i, rc;
@@ -1331,7 +1326,7 @@ static int ata_dev_configure(struct ata_
 					      ap->device[i].cdb_len);
 
 	/* limit bridge transfers to udma5, 200 sectors */
-	if (ata_dev_knobble(ap, dev)) {
+	if (ata_dev_knobble(dev)) {
 		if (print_info)
 			printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
 			       ap->id, dev->devno);
@@ -1416,11 +1411,11 @@ static int ata_bus_probe(struct ata_port
 		if (!ata_dev_enabled(dev))
 			continue;
 
-		rc = ata_dev_read_id(ap, dev, &dev->class, 1, dev->id);
+		rc = ata_dev_read_id(dev, &dev->class, 1, dev->id);
 		if (rc)
 			goto fail;
 
-		rc = ata_dev_configure(ap, dev, 1);
+		rc = ata_dev_configure(dev, 1);
 		if (rc)
 			goto fail;
 	}
@@ -1453,13 +1448,13 @@ static int ata_bus_probe(struct ata_port
 	default:
 		tries[dev->devno]--;
 		if (down_xfermask &&
-		    ata_down_xfermask_limit(ap, dev, tries[dev->devno] == 1))
+		    ata_down_xfermask_limit(dev, tries[dev->devno] == 1))
 			tries[dev->devno] = 0;
 	}
 
 	if (!tries[dev->devno]) {
-		ata_down_xfermask_limit(ap, dev, 1);
-		ata_dev_disable(ap, dev);
+		ata_down_xfermask_limit(dev, 1);
+		ata_dev_disable(dev);
 	}
 
 	goto retry;
@@ -1586,15 +1581,15 @@ void sata_phy_reset(struct ata_port *ap)
 
 /**
  *	ata_dev_pair		-	return other device on cable
- *	@ap: port
  *	@adev: device
  *
  *	Obtain the other device on the same cable, or if none is
  *	present NULL is returned
  */
 
-struct ata_device *ata_dev_pair(struct ata_port *ap, struct ata_device *adev)
+struct ata_device *ata_dev_pair(struct ata_device *adev)
 {
+	struct ata_port *ap = adev->ap;
 	struct ata_device *pair = &ap->device[1 - adev->devno];
 	if (!ata_dev_enabled(pair))
 		return NULL;
@@ -1885,7 +1880,6 @@ int ata_timing_compute(struct ata_device
 
 /**
  *	ata_down_xfermask_limit - adjust dev xfer masks downward
- *	@ap: Port associated with device @dev
  *	@dev: Device to adjust xfer masks
  *	@force_pio0: Force PIO0
  *
@@ -1899,9 +1893,9 @@ int ata_timing_compute(struct ata_device
  *	RETURNS:
  *	0 on success, negative errno on failure
  */
-int ata_down_xfermask_limit(struct ata_port *ap, struct ata_device *dev,
-			    int force_pio0)
+int ata_down_xfermask_limit(struct ata_device *dev, int force_pio0)
 {
+	struct ata_port *ap = dev->ap;
 	unsigned long xfer_mask;
 	int highbit;
 
@@ -1933,8 +1927,9 @@ int ata_down_xfermask_limit(struct ata_p
 	return -EINVAL;
 }
 
-static int ata_dev_set_mode(struct ata_port *ap, struct ata_device *dev)
+static int ata_dev_set_mode(struct ata_device *dev)
 {
+	struct ata_port *ap = dev->ap;
 	unsigned int err_mask;
 	int rc;
 
@@ -1942,7 +1937,7 @@ static int ata_dev_set_mode(struct ata_p
 	if (dev->xfer_shift == ATA_SHIFT_PIO)
 		dev->flags |= ATA_DFLAG_PIO;
 
-	err_mask = ata_dev_set_xfermode(ap, dev);
+	err_mask = ata_dev_set_xfermode(dev);
 	if (err_mask) {
 		printk(KERN_ERR
 		       "ata%u: failed to set xfermode (err_mask=0x%x)\n",
@@ -1950,7 +1945,7 @@ static int ata_dev_set_mode(struct ata_p
 		return -EIO;
 	}
 
-	rc = ata_dev_revalidate(ap, dev, 0);
+	rc = ata_dev_revalidate(dev, 0);
 	if (rc)
 		return rc;
 
@@ -2006,7 +2001,7 @@ int ata_set_mode(struct ata_port *ap, st
 		if (!ata_dev_enabled(dev))
 			continue;
 
-		ata_dev_xfermask(ap, dev);
+		ata_dev_xfermask(dev);
 
 		pio_mask = ata_pack_xfermask(dev->pio_mask, 0, 0);
 		dma_mask = ata_pack_xfermask(0, dev->mwdma_mask, dev->udma_mask);
@@ -2059,7 +2054,7 @@ int ata_set_mode(struct ata_port *ap, st
 		if (!ata_dev_enabled(dev))
 			continue;
 
-		rc = ata_dev_set_mode(ap, dev);
+		rc = ata_dev_set_mode(dev);
 		if (rc)
 			goto out;
 	}
@@ -2711,7 +2706,6 @@ int ata_drive_probe_reset(struct ata_por
 
 /**
  *	ata_dev_same_device - Determine whether new ID matches configured device
- *	@ap: port on which the device to compare against resides
  *	@dev: device to compare against
  *	@new_class: class of the new device
  *	@new_id: IDENTIFY page of the new device
@@ -2726,9 +2720,10 @@ int ata_drive_probe_reset(struct ata_por
  *	RETURNS:
  *	1 if @dev matches @new_class and @new_id, 0 otherwise.
  */
-static int ata_dev_same_device(struct ata_port *ap, struct ata_device *dev,
-			       unsigned int new_class, const u16 *new_id)
+static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
+			       const u16 *new_id)
 {
+	struct ata_port *ap = dev->ap;
 	const u16 *old_id = dev->id;
 	unsigned char model[2][41], serial[2][21];
 	u64 new_n_sectors;
@@ -2773,7 +2768,6 @@ static int ata_dev_same_device(struct at
 
 /**
  *	ata_dev_revalidate - Revalidate ATA device
- *	@ap: port on which the device to revalidate resides
  *	@dev: device to revalidate
  *	@post_reset: is this revalidation after reset?
  *
@@ -2786,9 +2780,9 @@ static int ata_dev_same_device(struct at
  *	RETURNS:
  *	0 on success, negative errno otherwise
  */
-int ata_dev_revalidate(struct ata_port *ap, struct ata_device *dev,
-		       int post_reset)
+int ata_dev_revalidate(struct ata_device *dev, int post_reset)
 {
+	struct ata_port *ap = dev->ap;
 	unsigned int class = dev->class;
 	u16 *id = (void *)ap->sector_buf;
 	int rc;
@@ -2799,12 +2793,12 @@ int ata_dev_revalidate(struct ata_port *
 	}
 
 	/* read ID data */
-	rc = ata_dev_read_id(ap, dev, &class, post_reset, id);
+	rc = ata_dev_read_id(dev, &class, post_reset, id);
 	if (rc)
 		goto fail;
 
 	/* is the device still there? */
-	if (!ata_dev_same_device(ap, dev, class, id)) {
+	if (!ata_dev_same_device(dev, class, id)) {
 		rc = -ENODEV;
 		goto fail;
 	}
@@ -2812,7 +2806,7 @@ int ata_dev_revalidate(struct ata_port *
 	memcpy(dev->id, id, sizeof(id[0]) * ATA_ID_WORDS);
 
 	/* configure device according to the new ID */
-	rc = ata_dev_configure(ap, dev, 0);
+	rc = ata_dev_configure(dev, 0);
 	if (rc == 0)
 		return 0;
 
@@ -2894,7 +2888,6 @@ static int ata_dma_blacklisted(const str
 
 /**
  *	ata_dev_xfermask - Compute supported xfermask of the given device
- *	@ap: Port on which the device to compute xfermask for resides
  *	@dev: Device to compute xfermask for
  *
  *	Compute supported xfermask of @dev and store it in
@@ -2909,8 +2902,9 @@ static int ata_dma_blacklisted(const str
  *	LOCKING:
  *	None.
  */
-static void ata_dev_xfermask(struct ata_port *ap, struct ata_device *dev)
+static void ata_dev_xfermask(struct ata_device *dev)
 {
+	struct ata_port *ap = dev->ap;
 	struct ata_host_set *hs = ap->host_set;
 	unsigned long xfer_mask;
 	int i;
@@ -2963,7 +2957,6 @@ static void ata_dev_xfermask(struct ata_
 
 /**
  *	ata_dev_set_xfermode - Issue SET FEATURES - XFER MODE command
- *	@ap: Port associated with device @dev
  *	@dev: Device to which command will be sent
  *
  *	Issue SET FEATURES - XFER MODE command to device @dev
@@ -2976,8 +2969,7 @@ static void ata_dev_xfermask(struct ata_
  *	0 on success, AC_ERR_* mask otherwise.
  */
 
-static unsigned int ata_dev_set_xfermode(struct ata_port *ap,
-					 struct ata_device *dev)
+static unsigned int ata_dev_set_xfermode(struct ata_device *dev)
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
@@ -2985,14 +2977,14 @@ static unsigned int ata_dev_set_xfermode
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
 
-	ata_tf_init(ap, &tf, dev->devno);
+	ata_tf_init(dev, &tf);
 	tf.command = ATA_CMD_SET_FEATURES;
 	tf.feature = SETFEATURES_XFER;
 	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf.protocol = ATA_PROT_NODATA;
 	tf.nsect = dev->xfer_mode;
 
-	err_mask = ata_exec_internal(ap, dev, &tf, NULL, DMA_NONE, NULL, 0);
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
 
 	DPRINTK("EXIT, err_mask=%x\n", err_mask);
 	return err_mask;
@@ -3000,8 +2992,9 @@ static unsigned int ata_dev_set_xfermode
 
 /**
  *	ata_dev_init_params - Issue INIT DEV PARAMS command
- *	@ap: Port associated with device @dev
  *	@dev: Device to which command will be sent
+ *	@heads: Number of heads
+ *	@sectors: Number of sectors
  *
  *	LOCKING:
  *	Kernel thread context (may sleep)
@@ -3009,11 +3002,8 @@ static unsigned int ata_dev_set_xfermode
  *	RETURNS:
  *	0 on success, AC_ERR_* mask otherwise.
  */
-
-static unsigned int ata_dev_init_params(struct ata_port *ap,
-					struct ata_device *dev,
-					u16 heads,
-					u16 sectors)
+static unsigned int ata_dev_init_params(struct ata_device *dev,
+					u16 heads, u16 sectors)
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
@@ -3025,14 +3015,14 @@ static unsigned int ata_dev_init_params(
 	/* set up init dev params taskfile */
 	DPRINTK("init dev params \n");
 
-	ata_tf_init(ap, &tf, dev->devno);
+	ata_tf_init(dev, &tf);
 	tf.command = ATA_CMD_INIT_DEV_PARAMS;
 	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf.protocol = ATA_PROT_NODATA;
 	tf.nsect = sectors;
 	tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
 
-	err_mask = ata_exec_internal(ap, dev, &tf, NULL, DMA_NONE, NULL, 0);
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
 
 	DPRINTK("EXIT, err_mask=%x\n", err_mask);
 	return err_mask;
@@ -4044,16 +4034,15 @@ static struct ata_queued_cmd *ata_qc_new
 
 /**
  *	ata_qc_new_init - Request an available ATA command, and initialize it
- *	@ap: Port associated with device @dev
  *	@dev: Device from whom we request an available command structure
  *
  *	LOCKING:
  *	None.
  */
 
-struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
-				      struct ata_device *dev)
+struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev)
 {
+	struct ata_port *ap = dev->ap;
 	struct ata_queued_cmd *qc;
 
 	qc = ata_qc_new(ap);
@@ -4519,19 +4508,18 @@ int ata_port_offline(struct ata_port *ap
  * Execute a 'simple' command, that only consists of the opcode 'cmd' itself,
  * without filling any other registers
  */
-static int ata_do_simple_cmd(struct ata_port *ap, struct ata_device *dev,
-			     u8 cmd)
+static int ata_do_simple_cmd(struct ata_device *dev, u8 cmd)
 {
 	struct ata_taskfile tf;
 	int err;
 
-	ata_tf_init(ap, &tf, dev->devno);
+	ata_tf_init(dev, &tf);
 
 	tf.command = cmd;
 	tf.flags |= ATA_TFLAG_DEVICE;
 	tf.protocol = ATA_PROT_NODATA;
 
-	err = ata_exec_internal(ap, dev, &tf, NULL, DMA_NONE, NULL, 0);
+	err = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
 	if (err)
 		printk(KERN_ERR "%s: ata command failed: %d\n",
 				__FUNCTION__, err);
@@ -4539,7 +4527,7 @@ static int ata_do_simple_cmd(struct ata_
 	return err;
 }
 
-static int ata_flush_cache(struct ata_port *ap, struct ata_device *dev)
+static int ata_flush_cache(struct ata_device *dev)
 {
 	u8 cmd;
 
@@ -4551,22 +4539,21 @@ static int ata_flush_cache(struct ata_po
 	else
 		cmd = ATA_CMD_FLUSH;
 
-	return ata_do_simple_cmd(ap, dev, cmd);
+	return ata_do_simple_cmd(dev, cmd);
 }
 
-static int ata_standby_drive(struct ata_port *ap, struct ata_device *dev)
+static int ata_standby_drive(struct ata_device *dev)
 {
-	return ata_do_simple_cmd(ap, dev, ATA_CMD_STANDBYNOW1);
+	return ata_do_simple_cmd(dev, ATA_CMD_STANDBYNOW1);
 }
 
-static int ata_start_drive(struct ata_port *ap, struct ata_device *dev)
+static int ata_start_drive(struct ata_device *dev)
 {
-	return ata_do_simple_cmd(ap, dev, ATA_CMD_IDLEIMMEDIATE);
+	return ata_do_simple_cmd(dev, ATA_CMD_IDLEIMMEDIATE);
 }
 
 /**
  *	ata_device_resume - wakeup a previously suspended devices
- *	@ap: port the device is connected to
  *	@dev: the device to resume
  *
  *	Kick the drive back into action, by sending it an idle immediate
@@ -4574,39 +4561,42 @@ static int ata_start_drive(struct ata_po
  *	and host.
  *
  */
-int ata_device_resume(struct ata_port *ap, struct ata_device *dev)
+int ata_device_resume(struct ata_device *dev)
 {
+	struct ata_port *ap = dev->ap;
+
 	if (ap->flags & ATA_FLAG_SUSPENDED) {
 		struct ata_device *failed_dev;
 		ap->flags &= ~ATA_FLAG_SUSPENDED;
 		while (ata_set_mode(ap, &failed_dev))
-			ata_dev_disable(ap, failed_dev);
+			ata_dev_disable(failed_dev);
 	}
 	if (!ata_dev_enabled(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
-		ata_start_drive(ap, dev);
+		ata_start_drive(dev);
 
 	return 0;
 }
 
 /**
  *	ata_device_suspend - prepare a device for suspend
- *	@ap: port the device is connected to
  *	@dev: the device to suspend
  *
  *	Flush the cache on the drive, if appropriate, then issue a
  *	standbynow command.
  */
-int ata_device_suspend(struct ata_port *ap, struct ata_device *dev, pm_message_t state)
+int ata_device_suspend(struct ata_device *dev, pm_message_t state)
 {
+	struct ata_port *ap = dev->ap;
+
 	if (!ata_dev_enabled(dev))
 		return 0;
 	if (dev->class == ATA_DEV_ATA)
-		ata_flush_cache(ap, dev);
+		ata_flush_cache(dev);
 
 	if (state.event != PM_EVENT_FREEZE)
-		ata_standby_drive(ap, dev);
+		ata_standby_drive(dev);
 	ap->flags |= ATA_FLAG_SUSPENDED;
 	return 0;
 }
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index ce90b63..fcbf64e 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -302,7 +302,6 @@ int ata_scsi_ioctl(struct scsi_device *s
 
 /**
  *	ata_scsi_qc_new - acquire new ata_queued_cmd reference
- *	@ap: ATA port to which the new command is attached
  *	@dev: ATA device to which the new command is attached
  *	@cmd: SCSI command that originated this ATA command
  *	@done: SCSI command completion function
@@ -321,14 +320,13 @@ int ata_scsi_ioctl(struct scsi_device *s
  *	RETURNS:
  *	Command allocated, or %NULL if none available.
  */
-struct ata_queued_cmd *ata_scsi_qc_new(struct ata_port *ap,
-				       struct ata_device *dev,
+struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
 				       struct scsi_cmnd *cmd,
 				       void (*done)(struct scsi_cmnd *))
 {
 	struct ata_queued_cmd *qc;
 
-	qc = ata_qc_new_init(ap, dev);
+	qc = ata_qc_new_init(dev);
 	if (qc) {
 		qc->scsicmd = cmd;
 		qc->scsidone = done;
@@ -398,7 +396,7 @@ int ata_scsi_device_resume(struct scsi_d
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	struct ata_device *dev = &ap->device[sdev->id];
 
-	return ata_device_resume(ap, dev);
+	return ata_device_resume(dev);
 }
 
 int ata_scsi_device_suspend(struct scsi_device *sdev, pm_message_t state)
@@ -406,7 +404,7 @@ int ata_scsi_device_suspend(struct scsi_
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	struct ata_device *dev = &ap->device[sdev->id];
 
-	return ata_device_suspend(ap, dev, state);
+	return ata_device_suspend(dev, state);
 }
 
 /**
@@ -1224,7 +1222,6 @@ static void ata_scsi_qc_complete(struct 
 
 /**
  *	ata_scsi_translate - Translate then issue SCSI command to ATA device
- *	@ap: ATA port to which the command is addressed
  *	@dev: ATA device to which the command is addressed
  *	@cmd: SCSI command to execute
  *	@done: SCSI command completion function
@@ -1247,17 +1244,16 @@ static void ata_scsi_qc_complete(struct 
  *	spin_lock_irqsave(host_set lock)
  */
 
-static void ata_scsi_translate(struct ata_port *ap, struct ata_device *dev,
-			      struct scsi_cmnd *cmd,
-			      void (*done)(struct scsi_cmnd *),
-			      ata_xlat_func_t xlat_func)
+static void ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
+			       void (*done)(struct scsi_cmnd *),
+			       ata_xlat_func_t xlat_func)
 {
 	struct ata_queued_cmd *qc;
 	u8 *scsicmd = cmd->cmnd;
 
 	VPRINTK("ENTER\n");
 
-	qc = ata_scsi_qc_new(ap, dev, cmd, done);
+	qc = ata_scsi_qc_new(dev, cmd, done);
 	if (!qc)
 		goto err_mem;
 
@@ -1266,7 +1262,7 @@ static void ata_scsi_translate(struct at
 	    cmd->sc_data_direction == DMA_TO_DEVICE) {
 		if (unlikely(cmd->request_bufflen < 1)) {
 			printk(KERN_WARNING "ata%u(%u): WARNING: zero len r/w req\n",
-			       ap->id, dev->devno);
+			       dev->ap->id, dev->devno);
 			goto err_did;
 		}
 
@@ -2433,19 +2429,20 @@ static inline void ata_scsi_dump_cdb(str
 #endif
 }
 
-static inline void __ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *),
-				       struct ata_port *ap, struct ata_device *dev)
+static inline void __ata_scsi_queuecmd(struct scsi_cmnd *cmd,
+				       void (*done)(struct scsi_cmnd *),
+				       struct ata_device *dev)
 {
 	if (dev->class == ATA_DEV_ATA) {
 		ata_xlat_func_t xlat_func = ata_get_xlat_func(dev,
 							      cmd->cmnd[0]);
 
 		if (xlat_func)
-			ata_scsi_translate(ap, dev, cmd, done, xlat_func);
+			ata_scsi_translate(dev, cmd, done, xlat_func);
 		else
-			ata_scsi_simulate(ap, dev, cmd, done);
+			ata_scsi_simulate(dev, cmd, done);
 	} else
-		ata_scsi_translate(ap, dev, cmd, done, atapi_xlat);
+		ata_scsi_translate(dev, cmd, done, atapi_xlat);
 }
 
 /**
@@ -2483,7 +2480,7 @@ int ata_scsi_queuecmd(struct scsi_cmnd *
 
 	dev = ata_scsi_find_dev(ap, scsidev);
 	if (likely(dev))
-		__ata_scsi_queuecmd(cmd, done, ap, dev);
+		__ata_scsi_queuecmd(cmd, done, dev);
 	else {
 		cmd->result = (DID_BAD_TARGET << 16);
 		done(cmd);
@@ -2496,7 +2493,6 @@ int ata_scsi_queuecmd(struct scsi_cmnd *
 
 /**
  *	ata_scsi_simulate - simulate SCSI command on ATA device
- *	@ap: port the device is connected to
  *	@dev: the target device
  *	@cmd: SCSI command being sent to device.
  *	@done: SCSI command completion function.
@@ -2508,14 +2504,12 @@ int ata_scsi_queuecmd(struct scsi_cmnd *
  *	spin_lock_irqsave(host_set lock)
  */
 
-void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
-		      struct scsi_cmnd *cmd,
+void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd,
 		      void (*done)(struct scsi_cmnd *))
 {
 	struct ata_scsi_args args;
 	const u8 *scsicmd = cmd->cmnd;
 
-	args.ap = ap;
 	args.dev = dev;
 	args.id = dev->id;
 	args.cmd = cmd;
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index 43a56e5..a810c91 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -32,7 +32,6 @@
 #define DRV_VERSION	"1.30"	/* must be exactly four chars */
 
 struct ata_scsi_args {
-	struct ata_port		*ap;
 	struct ata_device	*dev;
 	u16			*id;
 	struct scsi_cmnd	*cmd;
@@ -43,18 +42,16 @@ struct ata_scsi_args {
 extern int atapi_enabled;
 extern int atapi_dmadir;
 extern int libata_fua;
-extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
-				      struct ata_device *dev);
+extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);
 extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
-extern void ata_dev_disable(struct ata_port *ap, struct ata_device *dev);
+extern void ata_dev_disable(struct ata_device *dev);
 extern void ata_port_flush_task(struct ata_port *ap);
-extern unsigned ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
+extern unsigned ata_exec_internal(struct ata_device *dev,
 				  struct ata_taskfile *tf, const u8 *cdb,
 				  int dma_dir, void *buf, unsigned int buflen);
 extern int ata_down_sata_spd_limit(struct ata_port *ap);
 extern int ata_set_sata_spd_needed(struct ata_port *ap);
-extern int ata_down_xfermask_limit(struct ata_port *ap, struct ata_device *dev,
-				   int force_pio0);
+extern int ata_down_xfermask_limit(struct ata_device *dev, int force_pio0);
 extern int ata_set_mode(struct ata_port *ap, struct ata_device **r_failed_dev);
 extern int ata_do_reset(struct ata_port *ap, ata_reset_fn_t reset,
 			unsigned int *classes);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 8d7fbb3..bd8cd3b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -518,8 +518,7 @@ extern void ata_std_probeinit(struct ata
 extern int ata_std_softreset(struct ata_port *ap, unsigned int *classes);
 extern int sata_std_hardreset(struct ata_port *ap, unsigned int *class);
 extern void ata_std_postreset(struct ata_port *ap, unsigned int *classes);
-extern int ata_dev_revalidate(struct ata_port *ap, struct ata_device *dev,
-			      int post_reset);
+extern int ata_dev_revalidate(struct ata_device *dev, int post_reset);
 extern void ata_port_disable(struct ata_port *);
 extern void ata_std_ports(struct ata_ioports *ioaddr);
 #ifdef CONFIG_PCI
@@ -545,8 +544,8 @@ extern int ata_port_online(struct ata_po
 extern int ata_port_offline(struct ata_port *ap);
 extern int ata_scsi_device_resume(struct scsi_device *);
 extern int ata_scsi_device_suspend(struct scsi_device *, pm_message_t state);
-extern int ata_device_resume(struct ata_port *, struct ata_device *);
-extern int ata_device_suspend(struct ata_port *, struct ata_device *, pm_message_t state);
+extern int ata_device_resume(struct ata_device *);
+extern int ata_device_suspend(struct ata_device *, pm_message_t state);
 extern int ata_ratelimit(void);
 extern unsigned int ata_busy_sleep(struct ata_port *ap,
 				   unsigned long timeout_pat,
@@ -592,15 +591,13 @@ extern void ata_bmdma_stop(struct ata_qu
 extern u8   ata_bmdma_status(struct ata_port *ap);
 extern void ata_bmdma_irq_clear(struct ata_port *ap);
 extern void __ata_qc_complete(struct ata_queued_cmd *qc);
-extern void ata_scsi_simulate(struct ata_port *ap, struct ata_device *dev,
-			      struct scsi_cmnd *cmd,
+extern void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd,
 			      void (*done)(struct scsi_cmnd *));
 extern int ata_std_bios_param(struct scsi_device *sdev,
 			      struct block_device *bdev,
 			      sector_t capacity, int geom[]);
 extern int ata_scsi_slave_config(struct scsi_device *sdev);
-extern struct ata_device *ata_dev_pair(struct ata_port *ap, 
-				       struct ata_device *adev);
+extern struct ata_device *ata_dev_pair(struct ata_device *adev);
 
 /*
  * Timing helpers
@@ -812,12 +809,12 @@ static inline struct ata_queued_cmd *ata
 	return NULL;
 }
 
-static inline void ata_tf_init(struct ata_port *ap, struct ata_taskfile *tf, unsigned int device)
+static inline void ata_tf_init(struct ata_device *dev, struct ata_taskfile *tf)
 {
 	memset(tf, 0, sizeof(*tf));
 
-	tf->ctl = ap->ctl;
-	if (device == 0)
+	tf->ctl = dev->ap->ctl;
+	if (dev->devno == 0)
 		tf->device = ATA_DEVICE_OBS;
 	else
 		tf->device = ATA_DEVICE_OBS | ATA_DEV1;
@@ -832,7 +829,7 @@ static inline void ata_qc_reinit(struct 
 	qc->nbytes = qc->curbytes = 0;
 	qc->err_mask = 0;
 
-	ata_tf_init(qc->ap, &qc->tf, qc->dev->devno);
+	ata_tf_init(qc->dev, &qc->tf);
 
 	/* init result_tf such that it indicates normal completion */
 	qc->result_tf.command = ATA_DRDY;
-- 
1.2.4

[PATCH 15/22] libata: init ap->cbl to ATA_CBL_SATA early

[Tejun Heo]

Init ap->cbl to ATA_CBL_SATA in ata_host_init().  This is necessary
for soon-to-follow SCR handling function changes.  LLDDs are free to
change ap->cbl during probing.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

acb4ec253fe1fd0879d1afa56a97c688424d2772
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 4cb7828..3d44cff 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2370,8 +2370,7 @@ void ata_std_probeinit(struct ata_port *
 	if ((ap->flags & ATA_FLAG_SATA) && ap->ops->scr_read) {
 		u32 spd;
 
-		/* set cable type and resume link */
-		ap->cbl = ATA_CBL_SATA;
+		/* resume link */
 		sata_phy_resume(ap);
 
 		/* init sata_spd_limit to the current value */
@@ -4586,7 +4585,6 @@ static void ata_host_init(struct ata_por
 	ap->udma_mask = ent->udma_mask;
 	ap->flags |= ent->host_flags;
 	ap->ops = ent->port_ops;
-	ap->cbl = ATA_CBL_NONE;
 	ap->sata_spd_limit = UINT_MAX;
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
@@ -4594,6 +4592,11 @@ static void ata_host_init(struct ata_por
 	INIT_WORK(&ap->port_task, NULL, NULL);
 	INIT_LIST_HEAD(&ap->eh_done_q);
 
+	/* set cable type */
+	ap->cbl = ATA_CBL_NONE;
+	if (ap->flags & ATA_FLAG_SATA)
+		ap->cbl = ATA_CBL_SATA;
+
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 		dev->devno = i;
-- 
1.2.4

Re: [PATCH 15/22] libata: init ap->cbl to ATA_CBL_SATA early

[Jeff Garzik]

Tejun Heo wrote:
> Init ap->cbl to ATA_CBL_SATA in ata_host_init().  This is necessary
> for soon-to-follow SCR handling function changes.  LLDDs are free to
> change ap->cbl during probing.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-core.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> acb4ec253fe1fd0879d1afa56a97c688424d2772
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 4cb7828..3d44cff 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -2370,8 +2370,7 @@ void ata_std_probeinit(struct ata_port *
>  	if ((ap->flags & ATA_FLAG_SATA) && ap->ops->scr_read) {
>  		u32 spd;
>  
> -		/* set cable type and resume link */
> -		ap->cbl = ATA_CBL_SATA;
> +		/* resume link */
>  		sata_phy_resume(ap);
>  
>  		/* init sata_spd_limit to the current value */
> @@ -4586,7 +4585,6 @@ static void ata_host_init(struct ata_por
>  	ap->udma_mask = ent->udma_mask;
>  	ap->flags |= ent->host_flags;
>  	ap->ops = ent->port_ops;
> -	ap->cbl = ATA_CBL_NONE;
>  	ap->sata_spd_limit = UINT_MAX;
>  	ap->active_tag = ATA_TAG_POISON;
>  	ap->last_ctl = 0xFF;
> @@ -4594,6 +4592,11 @@ static void ata_host_init(struct ata_por
>  	INIT_WORK(&ap->port_task, NULL, NULL);
>  	INIT_LIST_HEAD(&ap->eh_done_q);
>  
> +	/* set cable type */
> +	ap->cbl = ATA_CBL_NONE;
> +	if (ap->flags & ATA_FLAG_SATA)
> +		ap->cbl = ATA_CBL_SATA;

ACK, though I worry about a growing inconsistency of ap->cbl use between 
PATA and SATA.

[PATCH 17/22] libata: use new SCR and on/offline functions

[Tejun Heo]

Use new SCR and on/offline functions.  Note that for LLDD which know
it implements SCR callbacks, SCR functions are guaranteed to succeed
and ata_port_online() == !ata_port_offline().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c        |    6 +-
 drivers/scsi/libata-core.c |  106 ++++++++++++++++++++++++--------------------
 drivers/scsi/sata_mv.c     |   16 ++++---
 drivers/scsi/sata_sil24.c  |    6 +-
 4 files changed, 74 insertions(+), 60 deletions(-)

1868a1691a56029b8e5f1378afbc56ca00145e3f
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index c332554..c2298fb 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -567,7 +567,7 @@ static int ahci_softreset(struct ata_por
 
 	DPRINTK("ENTER\n");
 
-	if (!sata_dev_present(ap)) {
+	if (ata_port_offline(ap)) {
 		DPRINTK("PHY reports no device\n");
 		*class = ATA_DEV_NONE;
 		return 0;
@@ -640,7 +640,7 @@ static int ahci_softreset(struct ata_por
 	msleep(150);
 
 	*class = ATA_DEV_NONE;
-	if (sata_dev_present(ap)) {
+	if (ata_port_online(ap)) {
 		if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
 			rc = -EIO;
 			reason = "device not ready";
@@ -670,7 +670,7 @@ static int ahci_hardreset(struct ata_por
 	rc = sata_std_hardreset(ap, class);
 	ahci_start_engine(ap);
 
-	if (rc == 0 && sata_dev_present(ap))
+	if (rc == 0 && ata_port_online(ap))
 		*class = ahci_dev_classify(ap);
 	if (*class == ATA_DEV_UNKNOWN)
 		*class = ATA_DEV_NONE;
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index e3c579e..128f84d 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1494,13 +1494,11 @@ static void sata_print_link_status(struc
 {
 	u32 sstatus, scontrol, tmp;
 
-	if (!ap->ops->scr_read)
+	if (ata_scr_read(ap, SCR_STATUS, &sstatus))
 		return;
+	ata_scr_read(ap, SCR_CONTROL, &scontrol);
 
-	sstatus = scr_read(ap, SCR_STATUS);
-	scontrol = scr_read(ap, SCR_CONTROL);
-
-	if (sata_dev_present(ap)) {
+	if (ata_port_online(ap)) {
 		tmp = (sstatus >> 4) & 0xf;
 		printk(KERN_INFO
 		       "ata%u: SATA link up %s (SStatus %X SControl %X)\n",
@@ -1531,17 +1529,18 @@ void __sata_phy_reset(struct ata_port *a
 
 	if (ap->flags & ATA_FLAG_SATA_RESET) {
 		/* issue phy wake/reset */
-		scr_write_flush(ap, SCR_CONTROL, 0x301);
+		ata_scr_write_flush(ap, SCR_CONTROL, 0x301);
 		/* Couldn't find anything in SATA I/II specs, but
 		 * AHCI-1.1 10.4.2 says at least 1 ms. */
 		mdelay(1);
 	}
-	scr_write_flush(ap, SCR_CONTROL, 0x300); /* phy wake/clear reset */
+	/* phy wake/clear reset */
+	ata_scr_write_flush(ap, SCR_CONTROL, 0x300);
 
 	/* wait for phy to become ready, if necessary */
 	do {
 		msleep(200);
-		sstatus = scr_read(ap, SCR_STATUS);
+		ata_scr_read(ap, SCR_STATUS, &sstatus);
 		if ((sstatus & 0xf) != 1)
 			break;
 	} while (time_before(jiffies, timeout));
@@ -1550,7 +1549,7 @@ void __sata_phy_reset(struct ata_port *a
 	sata_print_link_status(ap);
 
 	/* TODO: phy layer with polling, timeouts, etc. */
-	if (sata_dev_present(ap))
+	if (!ata_port_offline(ap))
 		ata_port_probe(ap);
 	else
 		ata_port_disable(ap);
@@ -1638,11 +1637,12 @@ void ata_port_disable(struct ata_port *a
  */
 int ata_down_sata_spd_limit(struct ata_port *ap)
 {
-	u32 spd, mask;
-	int highbit;
+	u32 sstatus, spd, mask;
+	int rc, highbit;
 
-	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
-		return -EOPNOTSUPP;
+	rc = ata_scr_read(ap, SCR_STATUS, &sstatus);
+	if (rc)
+		return rc;
 
 	mask = ap->sata_spd_limit;
 	if (mask <= 1)
@@ -1650,7 +1650,7 @@ int ata_down_sata_spd_limit(struct ata_p
 	highbit = fls(mask) - 1;
 	mask &= ~(1 << highbit);
 
-	spd = (scr_read(ap, SCR_STATUS) >> 4) & 0xf;
+	spd = (sstatus >> 4) & 0xf;
 	if (spd <= 1)
 		return -EINVAL;
 	spd--;
@@ -1700,11 +1700,9 @@ int ata_set_sata_spd_needed(struct ata_p
 {
 	u32 scontrol;
 
-	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
+	if (ata_scr_read(ap, SCR_CONTROL, &scontrol))
 		return 0;
 
-	scontrol = scr_read(ap, SCR_CONTROL);
-
 	return __ata_set_sata_spd_needed(ap, &scontrol);
 }
 
@@ -1719,20 +1717,21 @@ int ata_set_sata_spd_needed(struct ata_p
  *
  *	RETURNS:
  *	0 if spd doesn't need to be changed, 1 if spd has been
- *	changed.  -EOPNOTSUPP if SCR registers are inaccessible.
+ *	changed.  Negative errno if SCR registers are inaccessible.
  */
 int ata_set_sata_spd(struct ata_port *ap)
 {
 	u32 scontrol;
+	int rc;
 
-	if (ap->cbl != ATA_CBL_SATA || !ap->ops->scr_read)
-		return -EOPNOTSUPP;
+	if ((rc = ata_scr_read(ap, SCR_CONTROL, &scontrol)))
+		return rc;
 
-	scontrol = scr_read(ap, SCR_CONTROL);
 	if (!__ata_set_sata_spd_needed(ap, &scontrol))
 		return 0;
 
-	scr_write(ap, SCR_CONTROL, scontrol);
+	if ((rc = ata_scr_write(ap, SCR_CONTROL, scontrol)))
+		return rc;
 	return 1;
 }
 
@@ -2336,20 +2335,26 @@ static int sata_phy_resume(struct ata_po
 {
 	unsigned long timeout = jiffies + (HZ * 5);
 	u32 scontrol, sstatus;
+	int rc;
+
+	if ((rc = ata_scr_read(ap, SCR_CONTROL, &scontrol)))
+		return rc;
 
-	scontrol = scr_read(ap, SCR_CONTROL);
 	scontrol = (scontrol & 0x0f0) | 0x300;
-	scr_write_flush(ap, SCR_CONTROL, scontrol);
+
+	if ((rc = ata_scr_write(ap, SCR_CONTROL, scontrol)))
+		return rc;
 
 	/* Wait for phy to become ready, if necessary. */
 	do {
 		msleep(200);
-		sstatus = scr_read(ap, SCR_STATUS);
+		if ((rc = ata_scr_read(ap, SCR_STATUS, &sstatus)))
+			return rc;
 		if ((sstatus & 0xf) != 1)
 			return 0;
 	} while (time_before(jiffies, timeout));
 
-	return -1;
+	return -EBUSY;
 }
 
 /**
@@ -2367,21 +2372,20 @@ static int sata_phy_resume(struct ata_po
  */
 void ata_std_probeinit(struct ata_port *ap)
 {
-	if ((ap->flags & ATA_FLAG_SATA) && ap->ops->scr_read) {
-		u32 spd;
+	u32 scontrol;
 
-		/* resume link */
-		sata_phy_resume(ap);
+	/* resume link */
+	sata_phy_resume(ap);
 
-		/* init sata_spd_limit to the current value */
-		spd = (scr_read(ap, SCR_CONTROL) & 0xf0) >> 4;
-		if (spd)
-			ap->sata_spd_limit &= (1 << spd) - 1;
-
-		/* wait for device */
-		if (sata_dev_present(ap))
-			ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT);
+	/* init sata_spd_limit to the current value */
+	if (ata_scr_read(ap, SCR_CONTROL, &scontrol) == 0) {
+		int spd = (scontrol >> 4) & 0xf;
+		ap->sata_spd_limit &= (1 << spd) - 1;
 	}
+
+	/* wait for device */
+	if (ata_port_online(ap))
+		ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT);
 }
 
 /**
@@ -2406,7 +2410,7 @@ int ata_std_softreset(struct ata_port *a
 
 	DPRINTK("ENTER\n");
 
-	if (ap->ops->scr_read && !sata_dev_present(ap)) {
+	if (ata_port_offline(ap)) {
 		classes[0] = ATA_DEV_NONE;
 		goto out;
 	}
@@ -2457,6 +2461,7 @@ int ata_std_softreset(struct ata_port *a
 int sata_std_hardreset(struct ata_port *ap, unsigned int *class)
 {
 	u32 scontrol;
+	int rc;
 
 	DPRINTK("ENTER\n");
 
@@ -2466,17 +2471,25 @@ int sata_std_hardreset(struct ata_port *
 		 * reconfiguration.  This works for at least ICH7 AHCI
 		 * and Sil3124.
 		 */
-		scontrol = scr_read(ap, SCR_CONTROL);
+		if ((rc = ata_scr_read(ap, SCR_CONTROL, &scontrol)))
+			return rc;
+
 		scontrol = (scontrol & 0x0f0) | 0x302;
-		scr_write_flush(ap, SCR_CONTROL, scontrol);
+
+		if ((rc = ata_scr_write(ap, SCR_CONTROL, scontrol)))
+			return rc;
 
 		ata_set_sata_spd(ap);
 	}
 
 	/* issue phy wake/reset */
-	scontrol = scr_read(ap, SCR_CONTROL);
+	if ((rc = ata_scr_read(ap, SCR_CONTROL, &scontrol)))
+		return rc;
+
 	scontrol = (scontrol & 0x0f0) | 0x301;
-	scr_write_flush(ap, SCR_CONTROL, scontrol);
+
+	if ((rc = ata_scr_write_flush(ap, SCR_CONTROL, scontrol)))
+		return rc;
 
 	/* Couldn't find anything in SATA I/II specs, but AHCI-1.1
 	 * 10.4.2 says at least 1 ms.
@@ -2487,7 +2500,7 @@ int sata_std_hardreset(struct ata_port *
 	sata_phy_resume(ap);
 
 	/* TODO: phy layer with polling, timeouts, etc. */
-	if (!sata_dev_present(ap)) {
+	if (ata_port_offline(ap)) {
 		*class = ATA_DEV_NONE;
 		DPRINTK("EXIT, link offline\n");
 		return 0;
@@ -2527,8 +2540,7 @@ void ata_std_postreset(struct ata_port *
 	DPRINTK("ENTER\n");
 
 	/* print link status */
-	if (ap->cbl == ATA_CBL_SATA)
-		sata_print_link_status(ap);
+	sata_print_link_status(ap);
 
 	/* re-enable interrupts */
 	if (ap->ioaddr.ctl_addr)	/* FIXME: hack. create a hook instead */
@@ -2575,7 +2587,7 @@ int ata_std_probe_reset(struct ata_port 
 	ata_reset_fn_t hardreset;
 
 	hardreset = NULL;
-	if (ap->cbl == ATA_CBL_SATA && ap->ops->scr_read)
+	if (ata_scr_valid(ap))
 		hardreset = sata_std_hardreset;
 
 	return ata_drive_probe_reset(ap, ata_std_probeinit,
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 181917a..97aad57 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -1309,8 +1309,8 @@ static void mv_err_intr(struct ata_port 
 	edma_err_cause = readl(port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
 
 	if (EDMA_ERR_SERR & edma_err_cause) {
-		serr = scr_read(ap, SCR_ERROR);
-		scr_write_flush(ap, SCR_ERROR, serr);
+		ata_scr_read(ap, SCR_ERROR, &serr);
+		ata_scr_write_flush(ap, SCR_ERROR, serr);
 	}
 	if (EDMA_ERR_SELF_DIS & edma_err_cause) {
 		struct mv_port_priv *pp	= ap->private_data;
@@ -1934,15 +1934,16 @@ static void __mv_phy_reset(struct ata_po
 
 	/* Issue COMRESET via SControl */
 comreset_retry:
-	scr_write_flush(ap, SCR_CONTROL, 0x301);
+	ata_scr_write_flush(ap, SCR_CONTROL, 0x301);
 	__msleep(1, can_sleep);
 
-	scr_write_flush(ap, SCR_CONTROL, 0x300);
+	ata_scr_write_flush(ap, SCR_CONTROL, 0x300);
 	__msleep(20, can_sleep);
 
 	timeout = jiffies + msecs_to_jiffies(200);
 	do {
-		sstatus = scr_read(ap, SCR_STATUS) & 0x3;
+		ata_scr_read(ap, SCR_STATUS, &sstatus);
+		sstatus &= 0x3;
 		if ((sstatus == 3) || (sstatus == 0))
 			break;
 
@@ -1959,11 +1960,12 @@ comreset_retry:
 		"SCtrl 0x%08x\n", mv_scr_read(ap, SCR_STATUS),
 		mv_scr_read(ap, SCR_ERROR), mv_scr_read(ap, SCR_CONTROL));
 
-	if (sata_dev_present(ap)) {
+	if (ata_port_online(ap)) {
 		ata_port_probe(ap);
 	} else {
+		ata_scr_read(ap, SCR_STATUS, &sstatus);
 		printk(KERN_INFO "ata%u: no device found (phy stat %08x)\n",
-		       ap->id, scr_read(ap, SCR_STATUS));
+		       ap->id, sstatus);
 		ata_port_disable(ap);
 		return;
 	}
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index a9c8484..664b138 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -464,7 +464,7 @@ static int sil24_softreset(struct ata_po
 
 	DPRINTK("ENTER\n");
 
-	if (!sata_dev_present(ap)) {
+	if (ata_port_offline(ap)) {
 		DPRINTK("PHY reports no device\n");
 		*class = ATA_DEV_NONE;
 		goto out;
@@ -531,7 +531,7 @@ static int sil24_hardreset(struct ata_po
 	ata_set_sata_spd(ap);
 
 	tout_msec = 100;
-	if (sata_dev_present(ap))
+	if (ata_port_online(ap))
 		tout_msec = 5000;
 
 	writel(PORT_CS_DEV_RST, port + PORT_CTRL_STAT);
@@ -544,7 +544,7 @@ static int sil24_hardreset(struct ata_po
 	msleep(100);
 
 	if (tmp & PORT_CS_DEV_RST) {
-		if (!sata_dev_present(ap))
+		if (ata_port_offline(ap))
 			return 0;
 		reason = "link not ready";
 		goto err;
-- 
1.2.4

[PATCH 21/22] libata: implement ATA printk helpers

[Tejun Heo]

Implement ata_{port|dev}_printk() which prefixes the message with
proper identification string.  This change is necessary for later PM
support because devices and links should be identified differently
depending on how they are attached.

This also helps unifying device id strings.  Currently, there are two
forms in use (P is the port number D device number) - 'ataP(D):', and
'ataP: dev D '.  These macros also make it harder to forget proper ID
string (e.g. printing only port number when a device is in question).

Debug message handling can be integrated into these printk macros by
passing debug type and level via @lv.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 include/linux/libata.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

387b4ba08bdce6d729cf441e7c1173d9863f4c04
diff --git a/include/linux/libata.h b/include/linux/libata.h
index bd8cd3b..7b54d92 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -650,7 +650,18 @@ extern void ata_eng_timeout(struct ata_p
 extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
 
+/*
+ * printk helpers
+ */
+#define ata_port_printk(ap, lv, fmt, args...) \
+	printk(lv"ata%u: "fmt, (ap)->id , ##args)
+
+#define ata_dev_printk(dev, lv, fmt, args...) \
+	printk(lv"ata%u.%02u: "fmt, (dev)->ap->id, (dev)->devno , ##args)
 
+/*
+ * qc helpers
+ */
 static inline int
 ata_sg_is_last(struct scatterlist *sg, struct ata_queued_cmd *qc)
 {
-- 
1.2.4

Re: [PATCH 21/22] libata: implement ATA printk helpers

[Jeff Garzik]

Tejun Heo wrote:
> Implement ata_{port|dev}_printk() which prefixes the message with
> proper identification string.  This change is necessary for later PM
> support because devices and links should be identified differently
> depending on how they are attached.
> 
> This also helps unifying device id strings.  Currently, there are two
> forms in use (P is the port number D device number) - 'ataP(D):', and
> 'ataP: dev D '.  These macros also make it harder to forget proper ID
> string (e.g. printing only port number when a device is in question).
> 
> Debug message handling can be integrated into these printk macros by
> passing debug type and level via @lv.

FWIW:  The long term goal for libata printk'ing is to use the 
kernel/module command line, or blktool, to set the verbosity level for 
each ATA port.

The transition step to this is setting ap->msg_enable to a useful 
default bitmap, and (alas) touching each printk.

	Jeff

Re: [PATCH 21/22] libata: implement ATA printk helpers

[Albert Lee]

Tejun Heo wrote:
> Implement ata_{port|dev}_printk() which prefixes the message with
> proper identification string.  This change is necessary for later PM
> support because devices and links should be identified differently
> depending on how they are attached.
> 
> This also helps unifying device id strings.  Currently, there are two
> forms in use (P is the port number D device number) - 'ataP(D):', and
> 'ataP: dev D '.  These macros also make it harder to forget proper ID
> string (e.g. printing only port number when a device is in question).
> 
> Debug message handling can be integrated into these printk macros by
> passing debug type and level via @lv.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  include/linux/libata.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> 387b4ba08bdce6d729cf441e7c1173d9863f4c04
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index bd8cd3b..7b54d92 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -650,7 +650,18 @@ extern void ata_eng_timeout(struct ata_p
>  extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
>  extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
>  
> +/*
> + * printk helpers
> + */
> +#define ata_port_printk(ap, lv, fmt, args...) \
> +	printk(lv"ata%u: "fmt, (ap)->id , ##args)
> +
> +#define ata_dev_printk(dev, lv, fmt, args...) \
> +	printk(lv"ata%u.%02u: "fmt, (dev)->ap->id, (dev)->devno , ##args)
>  
> +/*
> + * qc helpers
> + */
>  static inline int
>  ata_sg_is_last(struct scatterlist *sg, struct ata_queued_cmd *qc)
>  {

A minor question:
Messages like "ata5.01" are seen in the log. Do we ever have more than 2 devices per port?
(with port multiplier?) Otherwise, maybe "ata5.1" is shorter?

--
albert

Re: [PATCH 21/22] libata: implement ATA printk helpers

[Tejun Heo]

Albert Lee wrote:
> Tejun Heo wrote:
>> Implement ata_{port|dev}_printk() which prefixes the message with
>> proper identification string.  This change is necessary for later PM
>> support because devices and links should be identified differently
>> depending on how they are attached.
>>
>> This also helps unifying device id strings.  Currently, there are two
>> forms in use (P is the port number D device number) - 'ataP(D):', and
>> 'ataP: dev D '.  These macros also make it harder to forget proper ID
>> string (e.g. printing only port number when a device is in question).
>>
>> Debug message handling can be integrated into these printk macros by
>> passing debug type and level via @lv.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> ---
>>
>>  include/linux/libata.h |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> 387b4ba08bdce6d729cf441e7c1173d9863f4c04
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index bd8cd3b..7b54d92 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -650,7 +650,18 @@ extern void ata_eng_timeout(struct ata_p
>>  extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
>>  extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
>>  
>> +/*
>> + * printk helpers
>> + */
>> +#define ata_port_printk(ap, lv, fmt, args...) \
>> +	printk(lv"ata%u: "fmt, (ap)->id , ##args)
>> +
>> +#define ata_dev_printk(dev, lv, fmt, args...) \
>> +	printk(lv"ata%u.%02u: "fmt, (dev)->ap->id, (dev)->devno , ##args)
>>  
>> +/*
>> + * qc helpers
>> + */
>>  static inline int
>>  ata_sg_is_last(struct scatterlist *sg, struct ata_queued_cmd *qc)
>>  {
> 
> A minor question:
> Messages like "ata5.01" are seen in the log. Do we ever have more than 2 devices per port?
> (with port multiplier?) Otherwise, maybe "ata5.1" is shorter?

Yes, the Port Multiplier control port is always device #15.  I thought 
it would be confusing to see messages where "ata5.3:" and "ata5.15:" are 
mixed, so the "%02d".

-- 
tejun

[PATCH 18/22] libata: kill old SCR functions and sata_dev_present()

[Tejun Heo]

Kill now unused scr_{read|write|write_flush}() and sata_dev_present().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 include/linux/libata.h |   22 ----------------------
 1 files changed, 0 insertions(+), 22 deletions(-)

d37cd2268afa9ac9fc3ef918c944646059cf9455
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7b3beea..b719b16 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -939,28 +939,6 @@ static inline u8 ata_irq_ack(struct ata_
 	return status;
 }
 
-static inline u32 scr_read(struct ata_port *ap, unsigned int reg)
-{
-	return ap->ops->scr_read(ap, reg);
-}
-
-static inline void scr_write(struct ata_port *ap, unsigned int reg, u32 val)
-{
-	ap->ops->scr_write(ap, reg, val);
-}
-
-static inline void scr_write_flush(struct ata_port *ap, unsigned int reg,
-				   u32 val)
-{
-	ap->ops->scr_write(ap, reg, val);
-	(void) ap->ops->scr_read(ap, reg);
-}
-
-static inline unsigned int sata_dev_present(struct ata_port *ap)
-{
-	return ((scr_read(ap, SCR_STATUS) & 0xf) == 0x3) ? 1 : 0;
-}
-
 static inline int ata_try_flush_cache(const struct ata_device *dev)
 {
 	return ata_id_wcache_enabled(dev->id) ||
-- 
1.2.4

[PATCH 14/22] sata_sil24: update TF image only when necessary

[Tejun Heo]

Update TF image (pp->tf) only when necessary.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/sata_sil24.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

9a79c598f88ccdb130416b67399194ac275af7b5
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index e9fd869..a9c8484 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -799,17 +799,9 @@ static inline void sil24_host_intr(struc
 		if (ap->flags & SIL24_FLAG_PCIX_IRQ_WOC)
 			writel(PORT_IRQ_COMPLETE, port + PORT_IRQ_STAT);
 
-		/*
-		 * !HOST_SSAT_ATTN guarantees successful completion,
-		 * so reading back tf registers is unnecessary for
-		 * most commands.  TODO: read tf registers for
-		 * commands which require these values on successful
-		 * completion (EXECUTE DEVICE DIAGNOSTIC, CHECK POWER,
-		 * DEVICE RESET and READ PORT MULTIPLIER (any more?).
-		 */
-		sil24_update_tf(ap);
-
 		if (qc) {
+			if (qc->flags & ATA_QCFLAG_RESULT_TF)
+				sil24_update_tf(ap);
 			qc->err_mask |= ac_err_mask(pp->tf.command);
 			ata_qc_complete(qc);
 		}
-- 
1.2.4

Re: [PATCH 14/22] sata_sil24: update TF image only when necessary

[Jeff Garzik]

Tejun Heo wrote:
> Update TF image (pp->tf) only when necessary.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 3 - 14.  Email on patches 1 and 2 (SCSI core patches) sent 
separatedly.

[PATCH 13/22] libata: implement qc->result_tf

[Tejun Heo]

Add qc->result_tf and ATA_QCFLAG_RESULT_TF.  This moves the
responsibility of loading result TF from post-compltion path to qc
execution path.  qc->result_tf is loaded if explicitly requested or
the qc failsa.  This allows more efficient completion implementation
and correct handling of result TF for controllers which don't have
global TF representation such as sil3124/32.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    4 ++--
 drivers/scsi/libata-scsi.c |   18 +++++++-----------
 include/linux/libata.h     |   16 ++++++++++++++--
 3 files changed, 23 insertions(+), 15 deletions(-)

46a93ec2cb1124f507e0cad7dcb65794a15af844
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 464fd46..4cb7828 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -955,7 +955,6 @@ void ata_qc_complete_internal(struct ata
 {
 	struct completion *waiting = qc->private_data;
 
-	qc->ap->ops->tf_read(qc->ap, &qc->tf);
 	complete(waiting);
 }
 
@@ -997,6 +996,7 @@ unsigned ata_exec_internal(struct ata_po
 	qc->tf = *tf;
 	if (cdb)
 		memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
+	qc->flags |= ATA_QCFLAG_RESULT_TF;
 	qc->dma_dir = dma_dir;
 	if (dma_dir != DMA_NONE) {
 		ata_sg_init_one(qc, buf, buflen);
@@ -1034,7 +1034,7 @@ unsigned ata_exec_internal(struct ata_po
 	/* finish up */
 	spin_lock_irqsave(&ap->host_set->lock, flags);
 
-	*tf = qc->tf;
+	*tf = qc->result_tf;
 	err_mask = qc->err_mask;
 
 	ata_qc_free(qc);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index b0c83c2..ce90b63 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -537,7 +537,7 @@ void ata_to_sense_error(unsigned id, u8 
 void ata_gen_ata_desc_sense(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
-	struct ata_taskfile *tf = &qc->tf;
+	struct ata_taskfile *tf = &qc->result_tf;
 	unsigned char *sb = cmd->sense_buffer;
 	unsigned char *desc = sb + 8;
 
@@ -608,7 +608,7 @@ void ata_gen_ata_desc_sense(struct ata_q
 void ata_gen_fixed_sense(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
-	struct ata_taskfile *tf = &qc->tf;
+	struct ata_taskfile *tf = &qc->result_tf;
 	unsigned char *sb = cmd->sense_buffer;
 
 	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
@@ -1199,14 +1199,11 @@ static void ata_scsi_qc_complete(struct 
 	 */
 	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
  	    ((cdb[2] & 0x20) || need_sense)) {
-		qc->ap->ops->tf_read(qc->ap, &qc->tf);
  		ata_gen_ata_desc_sense(qc);
 	} else {
 		if (!need_sense) {
 			cmd->result = SAM_STAT_GOOD;
 		} else {
-			qc->ap->ops->tf_read(qc->ap, &qc->tf);
-
 			/* TODO: decide which descriptor format to use
 			 * for 48b LBA devices and call that here
 			 * instead of the fixed desc, which is only
@@ -1217,10 +1214,8 @@ static void ata_scsi_qc_complete(struct 
 		}
 	}
 
-	if (need_sense) {
-		/* The ata_gen_..._sense routines fill in tf */
-		ata_dump_status(qc->ap->id, &qc->tf);
-	}
+	if (need_sense)
+		ata_dump_status(qc->ap->id, &qc->result_tf);
 
 	qc->scsidone(cmd);
 
@@ -2004,7 +1999,6 @@ static void atapi_sense_complete(struct 
 		 * a sense descriptors, since that's only
 		 * correct for ATA, not ATAPI
 		 */
-		qc->ap->ops->tf_read(qc->ap, &qc->tf);
 		ata_gen_ata_desc_sense(qc);
 	}
 
@@ -2080,7 +2074,6 @@ static void atapi_qc_complete(struct ata
 		 * a sense descriptors, since that's only
 		 * correct for ATA, not ATAPI
 		 */
-		qc->ap->ops->tf_read(qc->ap, &qc->tf);
 		ata_gen_ata_desc_sense(qc);
 	} else {
 		u8 *scsicmd = cmd->cmnd;
@@ -2361,6 +2354,9 @@ ata_scsi_pass_thru(struct ata_queued_cmd
 	 */
 	qc->nsect = cmd->bufflen / ATA_SECT_SIZE;
 
+	/* request result TF */
+	qc->flags |= ATA_QCFLAG_RESULT_TF;
+
 	return 0;
 
  invalid_fld:
diff --git a/include/linux/libata.h b/include/linux/libata.h
index a117ca2..3d15c00 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -162,7 +162,9 @@ enum {
 	ATA_QCFLAG_SINGLE	= (1 << 2), /* no s/g, just a single buffer */
 	ATA_QCFLAG_DMAMAP	= ATA_QCFLAG_SG | ATA_QCFLAG_SINGLE,
 	ATA_QCFLAG_IO		= (1 << 3), /* standard IO command */
-	ATA_QCFLAG_EH_SCHEDULED = (1 << 4), /* EH scheduled */
+	ATA_QCFLAG_RESULT_TF	= (1 << 4), /* result TF requested */
+
+	ATA_QCFLAG_EH_SCHEDULED = (1 << 16), /* EH scheduled */
 
 	/* host set flags */
 	ATA_HOST_SIMPLEX	= (1 << 0),	/* Host is simplex, one DMA channel per host_set only */
@@ -343,7 +345,7 @@ struct ata_queued_cmd {
 	struct scatterlist	*__sg;
 
 	unsigned int		err_mask;
-
+	struct ata_taskfile	result_tf;
 	ata_qc_cb_t		complete_fn;
 
 	void			*private_data;
@@ -824,6 +826,10 @@ static inline void ata_qc_reinit(struct 
 	qc->err_mask = 0;
 
 	ata_tf_init(qc->ap, &qc->tf, qc->dev->devno);
+
+	/* init result_tf such that it indicates normal completion */
+	qc->result_tf.command = ATA_DRDY;
+	qc->result_tf.feature = 0;
 }
 
 /**
@@ -839,9 +845,15 @@ static inline void ata_qc_reinit(struct 
  */
 static inline void ata_qc_complete(struct ata_queued_cmd *qc)
 {
+	struct ata_port *ap = qc->ap;
+
 	if (unlikely(qc->flags & ATA_QCFLAG_EH_SCHEDULED))
 		return;
 
+	/* read result TF if failed or requested */
+	if (qc->err_mask || qc->flags & ATA_QCFLAG_RESULT_TF)
+		ap->ops->tf_read(ap, &qc->result_tf);
+
 	__ata_qc_complete(qc);
 }
 
-- 
1.2.4

Re: [PATCH 13/22] libata: implement qc->result_tf

[Albert Lee]

Tejun Heo wrote:
> Add qc->result_tf and ATA_QCFLAG_RESULT_TF.  This moves the
> responsibility of loading result TF from post-compltion path to qc
> execution path.  qc->result_tf is loaded if explicitly requested or
> the qc failsa.  This allows more efficient completion implementation
> and correct handling of result TF for controllers which don't have
> global TF representation such as sil3124/32.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-core.c |    4 ++--
>  drivers/scsi/libata-scsi.c |   18 +++++++-----------
>  include/linux/libata.h     |   16 ++++++++++++++--
>  3 files changed, 23 insertions(+), 15 deletions(-)
> 
> 46a93ec2cb1124f507e0cad7dcb65794a15af844
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 464fd46..4cb7828 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -955,7 +955,6 @@ void ata_qc_complete_internal(struct ata
>  {
>  	struct completion *waiting = qc->private_data;
>  
> -	qc->ap->ops->tf_read(qc->ap, &qc->tf);
>  	complete(waiting);
>  }
>  
> @@ -997,6 +996,7 @@ unsigned ata_exec_internal(struct ata_po
>  	qc->tf = *tf;
>  	if (cdb)
>  		memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
> +	qc->flags |= ATA_QCFLAG_RESULT_TF;
>  	qc->dma_dir = dma_dir;
>  	if (dma_dir != DMA_NONE) {
>  		ata_sg_init_one(qc, buf, buflen);
> @@ -1034,7 +1034,7 @@ unsigned ata_exec_internal(struct ata_po
>  	/* finish up */
>  	spin_lock_irqsave(&ap->host_set->lock, flags);
>  
> -	*tf = qc->tf;
> +	*tf = qc->result_tf;
>  	err_mask = qc->err_mask;
>  
>  	ata_qc_free(qc);
> diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
> index b0c83c2..ce90b63 100644
> --- a/drivers/scsi/libata-scsi.c
> +++ b/drivers/scsi/libata-scsi.c
> @@ -537,7 +537,7 @@ void ata_to_sense_error(unsigned id, u8 
>  void ata_gen_ata_desc_sense(struct ata_queued_cmd *qc)
>  {
>  	struct scsi_cmnd *cmd = qc->scsicmd;
> -	struct ata_taskfile *tf = &qc->tf;
> +	struct ata_taskfile *tf = &qc->result_tf;
>  	unsigned char *sb = cmd->sense_buffer;
>  	unsigned char *desc = sb + 8;
>  
> @@ -608,7 +608,7 @@ void ata_gen_ata_desc_sense(struct ata_q
>  void ata_gen_fixed_sense(struct ata_queued_cmd *qc)
>  {
>  	struct scsi_cmnd *cmd = qc->scsicmd;
> -	struct ata_taskfile *tf = &qc->tf;
> +	struct ata_taskfile *tf = &qc->result_tf;
>  	unsigned char *sb = cmd->sense_buffer;
>  
>  	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> @@ -1199,14 +1199,11 @@ static void ata_scsi_qc_complete(struct 
>  	 */
>  	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
>   	    ((cdb[2] & 0x20) || need_sense)) {
> -		qc->ap->ops->tf_read(qc->ap, &qc->tf);
>   		ata_gen_ata_desc_sense(qc);
>  	} else {
>  		if (!need_sense) {
>  			cmd->result = SAM_STAT_GOOD;
>  		} else {
> -			qc->ap->ops->tf_read(qc->ap, &qc->tf);
> -
>  			/* TODO: decide which descriptor format to use
>  			 * for 48b LBA devices and call that here
>  			 * instead of the fixed desc, which is only
> @@ -1217,10 +1214,8 @@ static void ata_scsi_qc_complete(struct 
>  		}
>  	}
>  
> -	if (need_sense) {
> -		/* The ata_gen_..._sense routines fill in tf */
> -		ata_dump_status(qc->ap->id, &qc->tf);
> -	}
> +	if (need_sense)
> +		ata_dump_status(qc->ap->id, &qc->result_tf);
>  
>  	qc->scsidone(cmd);
>  
> @@ -2004,7 +1999,6 @@ static void atapi_sense_complete(struct 
>  		 * a sense descriptors, since that's only
>  		 * correct for ATA, not ATAPI
>  		 */
> -		qc->ap->ops->tf_read(qc->ap, &qc->tf);
>  		ata_gen_ata_desc_sense(qc);
>  	}
>  
> @@ -2080,7 +2074,6 @@ static void atapi_qc_complete(struct ata
>  		 * a sense descriptors, since that's only
>  		 * correct for ATA, not ATAPI
>  		 */
> -		qc->ap->ops->tf_read(qc->ap, &qc->tf);
>  		ata_gen_ata_desc_sense(qc);
>  	} else {
>  		u8 *scsicmd = cmd->cmnd;
> @@ -2361,6 +2354,9 @@ ata_scsi_pass_thru(struct ata_queued_cmd
>  	 */
>  	qc->nsect = cmd->bufflen / ATA_SECT_SIZE;
>  
> +	/* request result TF */
> +	qc->flags |= ATA_QCFLAG_RESULT_TF;
> +
>  	return 0;
>  
>   invalid_fld:
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index a117ca2..3d15c00 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -162,7 +162,9 @@ enum {
>  	ATA_QCFLAG_SINGLE	= (1 << 2), /* no s/g, just a single buffer */
>  	ATA_QCFLAG_DMAMAP	= ATA_QCFLAG_SG | ATA_QCFLAG_SINGLE,
>  	ATA_QCFLAG_IO		= (1 << 3), /* standard IO command */
> -	ATA_QCFLAG_EH_SCHEDULED = (1 << 4), /* EH scheduled */
> +	ATA_QCFLAG_RESULT_TF	= (1 << 4), /* result TF requested */
> +
> +	ATA_QCFLAG_EH_SCHEDULED = (1 << 16), /* EH scheduled */
>  
>  	/* host set flags */
>  	ATA_HOST_SIMPLEX	= (1 << 0),	/* Host is simplex, one DMA channel per host_set only */
> @@ -343,7 +345,7 @@ struct ata_queued_cmd {
>  	struct scatterlist	*__sg;
>  
>  	unsigned int		err_mask;
> -
> +	struct ata_taskfile	result_tf;
>  	ata_qc_cb_t		complete_fn;
>  
>  	void			*private_data;
> @@ -824,6 +826,10 @@ static inline void ata_qc_reinit(struct 
>  	qc->err_mask = 0;
>  
>  	ata_tf_init(qc->ap, &qc->tf, qc->dev->devno);
> +
> +	/* init result_tf such that it indicates normal completion */
> +	qc->result_tf.command = ATA_DRDY;

Normally 0x50 is seen with normal completion, not 0x40.
Do we really have to fake qc->result_tf here? Maybe just keep it zero or garbage
is good enough: if err_mask == 0 and ATA_QCFLAG_RESULT_TF is not set, qc->result_tf
shouldn't be used anyway.

--
albert

Re: [PATCH 13/22] libata: implement qc->result_tf

[Tejun Heo]

Albert Lee wrote:
> Normally 0x50 is seen with normal completion, not 0x40.

I'm okay either way.  One of my drives reports 0x40 for normal status, 
so it's where 0x40 came from.

> Do we really have to fake qc->result_tf here? Maybe just keep it zero or garbage
> is good enough: if err_mask == 0 and ATA_QCFLAG_RESULT_TF is not set, qc->result_tf
> shouldn't be used anyway.

Hmmm... Right.  I thought libata SCSI completion path checks TF status 
unconditionally and added the code.  Maybe I was confused.  I cannot 
find the offending part now.  Hmmm... I seem to recollect I met a 
spurious error due to garbage inside qc->result_tf and added the code to 
initialize it.  I'll keep looking.

-- 
tejun

Re: [PATCH 13/22] libata: implement qc->result_tf

[Albert Lee]

Tejun Heo wrote:
> Add qc->result_tf and ATA_QCFLAG_RESULT_TF.  This moves the
> responsibility of loading result TF from post-compltion path to qc
> execution path.  qc->result_tf is loaded if explicitly requested or
> the qc failsa.  This allows more efficient completion implementation
> and correct handling of result TF for controllers which don't have
> global TF representation such as sil3124/32.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-core.c |    4 ++--
>  drivers/scsi/libata-scsi.c |   18 +++++++-----------
>  include/linux/libata.h     |   16 ++++++++++++++--
>  3 files changed, 23 insertions(+), 15 deletions(-)
> 
> 46a93ec2cb1124f507e0cad7dcb65794a15af844
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 464fd46..4cb7828 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -955,7 +955,6 @@ void ata_qc_complete_internal(struct ata
>  {
>  	struct completion *waiting = qc->private_data;
>  
> -	qc->ap->ops->tf_read(qc->ap, &qc->tf);
>  	complete(waiting);
>  }
>  
> @@ -997,6 +996,7 @@ unsigned ata_exec_internal(struct ata_po
>  	qc->tf = *tf;
>  	if (cdb)
>  		memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
> +	qc->flags |= ATA_QCFLAG_RESULT_TF;
>  	qc->dma_dir = dma_dir;
>  	if (dma_dir != DMA_NONE) {
>  		ata_sg_init_one(qc, buf, buflen);
> @@ -1034,7 +1034,7 @@ unsigned ata_exec_internal(struct ata_po
>  	/* finish up */
>  	spin_lock_irqsave(&ap->host_set->lock, flags);
>  
> -	*tf = qc->tf;
> +	*tf = qc->result_tf;
>  	err_mask = qc->err_mask;
>  
>  	ata_qc_free(qc);
> diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
> index b0c83c2..ce90b63 100644
> --- a/drivers/scsi/libata-scsi.c
> +++ b/drivers/scsi/libata-scsi.c
> @@ -537,7 +537,7 @@ void ata_to_sense_error(unsigned id, u8 
>  void ata_gen_ata_desc_sense(struct ata_queued_cmd *qc)
>  {
>  	struct scsi_cmnd *cmd = qc->scsicmd;
> -	struct ata_taskfile *tf = &qc->tf;
> +	struct ata_taskfile *tf = &qc->result_tf;
>  	unsigned char *sb = cmd->sense_buffer;
>  	unsigned char *desc = sb + 8;
>  
> @@ -608,7 +608,7 @@ void ata_gen_ata_desc_sense(struct ata_q
>  void ata_gen_fixed_sense(struct ata_queued_cmd *qc)
>  {
>  	struct scsi_cmnd *cmd = qc->scsicmd;
> -	struct ata_taskfile *tf = &qc->tf;
> +	struct ata_taskfile *tf = &qc->result_tf;
>  	unsigned char *sb = cmd->sense_buffer;
>  
>  	memset(sb, 0, SCSI_SENSE_BUFFERSIZE);
> @@ -1199,14 +1199,11 @@ static void ata_scsi_qc_complete(struct 
>  	 */
>  	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
>   	    ((cdb[2] & 0x20) || need_sense)) {
> -		qc->ap->ops->tf_read(qc->ap, &qc->tf);
>   		ata_gen_ata_desc_sense(qc);
>  	} else {
>  		if (!need_sense) {
>  			cmd->result = SAM_STAT_GOOD;
>  		} else {
> -			qc->ap->ops->tf_read(qc->ap, &qc->tf);
> -
>  			/* TODO: decide which descriptor format to use
>  			 * for 48b LBA devices and call that here
>  			 * instead of the fixed desc, which is only
> @@ -1217,10 +1214,8 @@ static void ata_scsi_qc_complete(struct 
>  		}
>  	}
>  
> -	if (need_sense) {
> -		/* The ata_gen_..._sense routines fill in tf */
> -		ata_dump_status(qc->ap->id, &qc->tf);
> -	}
> +	if (need_sense)
> +		ata_dump_status(qc->ap->id, &qc->result_tf);
>  
>  	qc->scsidone(cmd);
>  
> @@ -2004,7 +1999,6 @@ static void atapi_sense_complete(struct 
>  		 * a sense descriptors, since that's only
>  		 * correct for ATA, not ATAPI
>  		 */
> -		qc->ap->ops->tf_read(qc->ap, &qc->tf);
>  		ata_gen_ata_desc_sense(qc);
>  	}
>  
> @@ -2080,7 +2074,6 @@ static void atapi_qc_complete(struct ata
>  		 * a sense descriptors, since that's only
>  		 * correct for ATA, not ATAPI
>  		 */
> -		qc->ap->ops->tf_read(qc->ap, &qc->tf);
>  		ata_gen_ata_desc_sense(qc);
>  	} else {
>  		u8 *scsicmd = cmd->cmnd;
> @@ -2361,6 +2354,9 @@ ata_scsi_pass_thru(struct ata_queued_cmd
>  	 */
>  	qc->nsect = cmd->bufflen / ATA_SECT_SIZE;
>  
> +	/* request result TF */
> +	qc->flags |= ATA_QCFLAG_RESULT_TF;
> +
>  	return 0;
>  
>   invalid_fld:
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index a117ca2..3d15c00 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -162,7 +162,9 @@ enum {
>  	ATA_QCFLAG_SINGLE	= (1 << 2), /* no s/g, just a single buffer */
>  	ATA_QCFLAG_DMAMAP	= ATA_QCFLAG_SG | ATA_QCFLAG_SINGLE,
>  	ATA_QCFLAG_IO		= (1 << 3), /* standard IO command */
> -	ATA_QCFLAG_EH_SCHEDULED = (1 << 4), /* EH scheduled */
> +	ATA_QCFLAG_RESULT_TF	= (1 << 4), /* result TF requested */
> +
> +	ATA_QCFLAG_EH_SCHEDULED = (1 << 16), /* EH scheduled */
>  
>  	/* host set flags */
>  	ATA_HOST_SIMPLEX	= (1 << 0),	/* Host is simplex, one DMA channel per host_set only */
> @@ -343,7 +345,7 @@ struct ata_queued_cmd {
>  	struct scatterlist	*__sg;
>  
>  	unsigned int		err_mask;
> -
> +	struct ata_taskfile	result_tf;
>  	ata_qc_cb_t		complete_fn;
>  
>  	void			*private_data;
> @@ -824,6 +826,10 @@ static inline void ata_qc_reinit(struct 
>  	qc->err_mask = 0;
>  
>  	ata_tf_init(qc->ap, &qc->tf, qc->dev->devno);
> +
> +	/* init result_tf such that it indicates normal completion */
> +	qc->result_tf.command = ATA_DRDY;
> +	qc->result_tf.feature = 0;
>  }
>  
>  /**
> @@ -839,9 +845,15 @@ static inline void ata_qc_reinit(struct 
>   */
>  static inline void ata_qc_complete(struct ata_queued_cmd *qc)
>  {
> +	struct ata_port *ap = qc->ap;
> +
>  	if (unlikely(qc->flags & ATA_QCFLAG_EH_SCHEDULED))
>  		return;
>  
> +	/* read result TF if failed or requested */
> +	if (qc->err_mask || qc->flags & ATA_QCFLAG_RESULT_TF)
> +		ap->ops->tf_read(ap, &qc->result_tf);
> +
>  	__ata_qc_complete(qc);
>  }
>  

This patch makes qc->tf well preserved, except we still have
qc->tf got overwritten by ata_pio_bytes().

Maybe we also need the attached follow-up patch?

--
albert

--- 02_atapi_pio_bytes/drivers/scsi/libata-core.c	2006-05-18 14:20:32.000000000 +0800
+++ 03_atapi_pio_bytes/drivers/scsi/libata-core.c	2006-05-18 14:25:41.000000000 +0800
@@ -3861,10 +3861,10 @@ static void atapi_pio_bytes(struct ata_q
 	unsigned int ireason, bc_lo, bc_hi, bytes;
 	int i_write, do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? 1 : 0;
 
-	ap->ops->tf_read(ap, &qc->tf);
-	ireason = qc->tf.nsect;
-	bc_lo = qc->tf.lbam;
-	bc_hi = qc->tf.lbah;
+	ap->ops->tf_read(ap, &qc->result_tf);
+	ireason = qc->result_tf.nsect;
+	bc_lo = qc->result_tf.lbam;
+	bc_hi = qc->result_tf.lbah;
 	bytes = (bc_hi << 8) | bc_lo;
 
 	/* shall be cleared to zero, indicating xfer of data */

Re: [PATCH 13/22] libata: implement qc->result_tf

[Tejun Heo]

Albert Lee wrote:
> This patch makes qc->tf well preserved, except we still have
> qc->tf got overwritten by ata_pio_bytes().
> 
> Maybe we also need the attached follow-up patch?
> 
> --
> albert
> 
> --- 02_atapi_pio_bytes/drivers/scsi/libata-core.c	2006-05-18 14:20:32.000000000 +0800
> +++ 03_atapi_pio_bytes/drivers/scsi/libata-core.c	2006-05-18 14:25:41.000000000 +0800
> @@ -3861,10 +3861,10 @@ static void atapi_pio_bytes(struct ata_q
>  	unsigned int ireason, bc_lo, bc_hi, bytes;
>  	int i_write, do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? 1 : 0;
>  
> -	ap->ops->tf_read(ap, &qc->tf);
> -	ireason = qc->tf.nsect;
> -	bc_lo = qc->tf.lbam;
> -	bc_hi = qc->tf.lbah;
> +	ap->ops->tf_read(ap, &qc->result_tf);
> +	ireason = qc->result_tf.nsect;
> +	bc_lo = qc->result_tf.lbam;
> +	bc_hi = qc->result_tf.lbah;
>  	bytes = (bc_hi << 8) | bc_lo;
>  
>  	/* shall be cleared to zero, indicating xfer of data */
> 

I actually think above should be done with local variable.  It's not 
comannd TF nor result TF.

-- 
tejun

Re: [PATCH 13/22] libata: implement qc->result_tf

[Albert Lee]

Tejun Heo wrote:
> Albert Lee wrote:
> 
>> This patch makes qc->tf well preserved, except we still have
>> qc->tf got overwritten by ata_pio_bytes().
>>
>> Maybe we also need the attached follow-up patch?
>>
>> -- 
>> albert
>>
>> --- 02_atapi_pio_bytes/drivers/scsi/libata-core.c    2006-05-18
>> 14:20:32.000000000 +0800
>> +++ 03_atapi_pio_bytes/drivers/scsi/libata-core.c    2006-05-18
>> 14:25:41.000000000 +0800
>> @@ -3861,10 +3861,10 @@ static void atapi_pio_bytes(struct ata_q
>>      unsigned int ireason, bc_lo, bc_hi, bytes;
>>      int i_write, do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? 1 : 0;
>>  
>> -    ap->ops->tf_read(ap, &qc->tf);
>> -    ireason = qc->tf.nsect;
>> -    bc_lo = qc->tf.lbam;
>> -    bc_hi = qc->tf.lbah;
>> +    ap->ops->tf_read(ap, &qc->result_tf);
>> +    ireason = qc->result_tf.nsect;
>> +    bc_lo = qc->result_tf.lbam;
>> +    bc_hi = qc->result_tf.lbah;
>>      bytes = (bc_hi << 8) | bc_lo;
>>  
>>      /* shall be cleared to zero, indicating xfer of data */
>>
> 
> I actually think above should be done with local variable.  It's not
> comannd TF nor result TF.
> 

You're right. But local variable adds 18+ extra bytes to the kernel stack.
And
- For normal completion, qc->result_tf is not relevant.
- For error, qc->result_tf is later overwritten by ata_qc_complete().
Maybe we can use qc->result_tf for temp storage of intermediate TF here
and save some stack usage?

--
albert

Re: [PATCH 13/22] libata: implement qc->result_tf

[Tejun Heo]

Albert Lee wrote:
> Tejun Heo wrote:
>> Albert Lee wrote:
>>
>>> This patch makes qc->tf well preserved, except we still have
>>> qc->tf got overwritten by ata_pio_bytes().
>>>
>>> Maybe we also need the attached follow-up patch?
>>>
>>> -- 
>>> albert
>>>
>>> --- 02_atapi_pio_bytes/drivers/scsi/libata-core.c    2006-05-18
>>> 14:20:32.000000000 +0800
>>> +++ 03_atapi_pio_bytes/drivers/scsi/libata-core.c    2006-05-18
>>> 14:25:41.000000000 +0800
>>> @@ -3861,10 +3861,10 @@ static void atapi_pio_bytes(struct ata_q
>>>      unsigned int ireason, bc_lo, bc_hi, bytes;
>>>      int i_write, do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? 1 : 0;
>>>  
>>> -    ap->ops->tf_read(ap, &qc->tf);
>>> -    ireason = qc->tf.nsect;
>>> -    bc_lo = qc->tf.lbam;
>>> -    bc_hi = qc->tf.lbah;
>>> +    ap->ops->tf_read(ap, &qc->result_tf);
>>> +    ireason = qc->result_tf.nsect;
>>> +    bc_lo = qc->result_tf.lbam;
>>> +    bc_hi = qc->result_tf.lbah;
>>>      bytes = (bc_hi << 8) | bc_lo;
>>>  
>>>      /* shall be cleared to zero, indicating xfer of data */
>>>
>> I actually think above should be done with local variable.  It's not
>> comannd TF nor result TF.
>>
> 
> You're right. But local variable adds 18+ extra bytes to the kernel stack.
> And
> - For normal completion, qc->result_tf is not relevant.
> - For error, qc->result_tf is later overwritten by ata_qc_complete().
> Maybe we can use qc->result_tf for temp storage of intermediate TF here
> and save some stack usage?

Yeah, maybe.  But, please note in the code that the use is not to record 
result.

-- 
tejun

[PATCH 1/1] libata: use qc->result_tf for temp taskfile storage

[Albert Lee]

Use qc->result_tf for temp taskfile storage.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Hopefully this can both preserve command TF and
doesn't increase kernel stack usage.

Patch against the upstream branch of libata-dev.

--- upstream0/drivers/scsi/libata-core.c	2006-05-16 11:08:49.000000000 +0800
+++ upstream3/drivers/scsi/libata-core.c	2006-05-18 17:42:49.000000000 +0800
@@ -3862,10 +3862,16 @@ static void atapi_pio_bytes(struct ata_q
 	unsigned int ireason, bc_lo, bc_hi, bytes;
 	int i_write, do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? 1 : 0;
 
-	ap->ops->tf_read(ap, &qc->tf);
-	ireason = qc->tf.nsect;
-	bc_lo = qc->tf.lbam;
-	bc_hi = qc->tf.lbah;
+	/* Abuse qc->result_tf for temp storage of intermediate TF
+	 * here to save some kernel stack usage.
+	 * For normal completion, qc->result_tf is not relevant. For
+	 * error, qc->result_tf is later overwritten by ata_qc_complete().
+	 * So, the correctness of qc->result_tf is not affected.
+	 */
+	ap->ops->tf_read(ap, &qc->result_tf);
+	ireason = qc->result_tf.nsect;
+	bc_lo = qc->result_tf.lbam;
+	bc_hi = qc->result_tf.lbah;
 	bytes = (bc_hi << 8) | bc_lo;
 
 	/* shall be cleared to zero, indicating xfer of data */

[PATCH 19/22] libata: add dev->ap

[Tejun Heo]

Add dev->ap which points back to the port the device belongs to.  This
makes it unnecessary to pass @ap for silly reasons (e.g. printks).
Also, this change is necessary to accomodate later PM support which
will introduce ATA link inbetween port and device.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |    1 +
 include/linux/libata.h     |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

d9d4fd3f482c7c5054b60ddde1d5b408b14ff288
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 128f84d..38a9833 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4748,6 +4748,7 @@ static void ata_host_init(struct ata_por
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
+		dev->ap = ap;
 		dev->devno = i;
 		dev->pio_mask = UINT_MAX;
 		dev->mwdma_mask = UINT_MAX;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index b719b16..8d7fbb3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -358,6 +358,7 @@ struct ata_host_stats {
 };
 
 struct ata_device {
+	struct ata_port		*ap;
 	u64			n_sectors;	/* size of device, if ATA */
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
 	unsigned int		class;		/* ATA_DEV_xxx */
-- 
1.2.4

Re: [PATCH 19/22] libata: add dev->ap

[Jeff Garzik]

Tejun Heo wrote:
> Add dev->ap which points back to the port the device belongs to.  This
> makes it unnecessary to pass @ap for silly reasons (e.g. printks).
> Also, this change is necessary to accomodate later PM support which
> will introduce ATA link inbetween port and device.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 17 - 19.

I'm happy to see dev->ap added, I've been thinking that should happen 
for a while now.

	Jeff

[PATCH 16/22] libata: implement new SCR handling and port on/offline functions

[Tejun Heo]

Implement ata_scr_{valid|read|write|write_flush}() and
ata_port_{online|offline}().  These functions replace
scr_{read|write}() and sata_dev_present().

Major difference between between the new SCR functions and the old
ones is that the new ones have a way to signal error to the caller.
This makes handling SCR-available and SCR-unavailable cases in the
same path easier.  Also, it eases later PM implementation where SCR
access can fail due to various reasons.

ata_port_{online|offline}() functions return 1 only when they are
affirmitive of the condition.  e.g.  if SCR is unaccessible or
presence cannot be determined for other reasons, these functions
return 0.  So, ata_port_online() != !ata_port_offline().  This
distinction is useful in many exception handling cases.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |  143 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/libata.h     |    6 ++
 2 files changed, 149 insertions(+), 0 deletions(-)

68b43f16a1d92da94b0c28cbecce0b801cbac4a6
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 3d44cff..e3c579e 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4365,6 +4365,143 @@ irqreturn_t ata_interrupt (int irq, void
 	return IRQ_RETVAL(handled);
 }
 
+/**
+ *	ata_scr_valid - test whether SCRs are accessible
+ *	@ap: ATA port to test SCR accessibility for
+ *
+ *	Test whether SCRs are accessible for @ap.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	1 if SCRs are accessible, 0 otherwise.
+ */
+int ata_scr_valid(struct ata_port *ap)
+{
+	return ap->cbl == ATA_CBL_SATA && ap->ops->scr_read;
+}
+
+/**
+ *	ata_scr_read - read SCR register of the specified port
+ *	@ap: ATA port to read SCR for
+ *	@reg: SCR to read
+ *	@val: Place to store read value
+ *
+ *	Read SCR register @reg of @ap into *@val.  This function is
+ *	guaranteed to succeed if the cable type of the port is SATA
+ *	and the port implements ->scr_read.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno on failure.
+ */
+int ata_scr_read(struct ata_port *ap, int reg, u32 *val)
+{
+	if (ata_scr_valid(ap)) {
+		*val = ap->ops->scr_read(ap, reg);
+		return 0;
+	}
+	return -EOPNOTSUPP;
+}
+
+/**
+ *	ata_scr_write - write SCR register of the specified port
+ *	@ap: ATA port to write SCR for
+ *	@reg: SCR to write
+ *	@val: value to write
+ *
+ *	Write @val to SCR register @reg of @ap.  This function is
+ *	guaranteed to succeed if the cable type of the port is SATA
+ *	and the port implements ->scr_read.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno on failure.
+ */
+int ata_scr_write(struct ata_port *ap, int reg, u32 val)
+{
+	if (ata_scr_valid(ap)) {
+		ap->ops->scr_write(ap, reg, val);
+		return 0;
+	}
+	return -EOPNOTSUPP;
+}
+
+/**
+ *	ata_scr_write_flush - write SCR register of the specified port and flush
+ *	@ap: ATA port to write SCR for
+ *	@reg: SCR to write
+ *	@val: value to write
+ *
+ *	This function is identical to ata_scr_write() except that this
+ *	function performs flush after writing to the register.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	0 on success, negative errno on failure.
+ */
+int ata_scr_write_flush(struct ata_port *ap, int reg, u32 val)
+{
+	if (ata_scr_valid(ap)) {
+		ap->ops->scr_write(ap, reg, val);
+		ap->ops->scr_read(ap, reg);
+		return 0;
+	}
+	return -EOPNOTSUPP;
+}
+
+/**
+ *	ata_port_online - test whether the given port is online
+ *	@ap: ATA port to test
+ *
+ *	Test whether @ap is online.  Note that this function returns 0
+ *	if online status of @ap cannot be obtained, so
+ *	ata_port_online(ap) != !ata_port_offline(ap).
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	1 if the port online status is available and online.
+ */
+int ata_port_online(struct ata_port *ap)
+{
+	u32 sstatus;
+
+	if (!ata_scr_read(ap, SCR_STATUS, &sstatus) && (sstatus & 0xf) == 0x3)
+		return 1;
+	return 0;
+}
+
+/**
+ *	ata_port_offline - test whether the given port is offline
+ *	@ap: ATA port to test
+ *
+ *	Test whether @ap is offline.  Note that this function returns
+ *	0 if offline status of @ap cannot be obtained, so
+ *	ata_port_online(ap) != !ata_port_offline(ap).
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	1 if the port offline status is available and offline.
+ */
+int ata_port_offline(struct ata_port *ap)
+{
+	u32 sstatus;
+
+	if (!ata_scr_read(ap, SCR_STATUS, &sstatus) && (sstatus & 0xf) != 0x3)
+		return 1;
+	return 0;
+}
 
 /*
  * Execute a 'simple' command, that only consists of the opcode 'cmd' itself,
@@ -5133,6 +5270,12 @@ EXPORT_SYMBOL_GPL(ata_scsi_queuecmd);
 EXPORT_SYMBOL_GPL(ata_scsi_slave_config);
 EXPORT_SYMBOL_GPL(ata_scsi_release);
 EXPORT_SYMBOL_GPL(ata_host_intr);
+EXPORT_SYMBOL_GPL(ata_scr_valid);
+EXPORT_SYMBOL_GPL(ata_scr_read);
+EXPORT_SYMBOL_GPL(ata_scr_write);
+EXPORT_SYMBOL_GPL(ata_scr_write_flush);
+EXPORT_SYMBOL_GPL(ata_port_online);
+EXPORT_SYMBOL_GPL(ata_port_offline);
 EXPORT_SYMBOL_GPL(ata_id_string);
 EXPORT_SYMBOL_GPL(ata_id_c_string);
 EXPORT_SYMBOL_GPL(ata_scsi_simulate);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3d15c00..7b3beea 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -536,6 +536,12 @@ extern int ata_scsi_ioctl(struct scsi_de
 extern int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(struct scsi_cmnd *));
 extern int ata_scsi_release(struct Scsi_Host *host);
 extern unsigned int ata_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc);
+extern int ata_scr_valid(struct ata_port *ap);
+extern int ata_scr_read(struct ata_port *ap, int reg, u32 *val);
+extern int ata_scr_write(struct ata_port *ap, int reg, u32 val);
+extern int ata_scr_write_flush(struct ata_port *ap, int reg, u32 val);
+extern int ata_port_online(struct ata_port *ap);
+extern int ata_port_offline(struct ata_port *ap);
 extern int ata_scsi_device_resume(struct scsi_device *);
 extern int ata_scsi_device_suspend(struct scsi_device *, pm_message_t state);
 extern int ata_device_resume(struct ata_port *, struct ata_device *);
-- 
1.2.4

Re: [PATCH 16/22] libata: implement new SCR handling and port on/offline functions

[Jeff Garzik]

Tejun Heo wrote:
> Implement ata_scr_{valid|read|write|write_flush}() and
> ata_port_{online|offline}().  These functions replace
> scr_{read|write}() and sata_dev_present().
> 
> Major difference between between the new SCR functions and the old
> ones is that the new ones have a way to signal error to the caller.
> This makes handling SCR-available and SCR-unavailable cases in the
> same path easier.  Also, it eases later PM implementation where SCR
> access can fail due to various reasons.
> 
> ata_port_{online|offline}() functions return 1 only when they are
> affirmitive of the condition.  e.g.  if SCR is unaccessible or
> presence cannot be determined for other reasons, these functions
> return 0.  So, ata_port_online() != !ata_port_offline().  This
> distinction is useful in many exception handling cases.

If its SATA-specific, it should have a "sata_" not "ata_" prefix.

Once a search-n-replace is applied, ACK.

	Jeff

Re: [PATCH 16/22] libata: implement new SCR handling and port on/offline functions

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Implement ata_scr_{valid|read|write|write_flush}() and
>> ata_port_{online|offline}().  These functions replace
>> scr_{read|write}() and sata_dev_present().
>>
>> Major difference between between the new SCR functions and the old
>> ones is that the new ones have a way to signal error to the caller.
>> This makes handling SCR-available and SCR-unavailable cases in the
>> same path easier.  Also, it eases later PM implementation where SCR
>> access can fail due to various reasons.
>>
>> ata_port_{online|offline}() functions return 1 only when they are
>> affirmitive of the condition.  e.g.  if SCR is unaccessible or
>> presence cannot be determined for other reasons, these functions
>> return 0.  So, ata_port_online() != !ata_port_offline().  This
>> distinction is useful in many exception handling cases.
> 
> If its SATA-specific, it should have a "sata_" not "ata_" prefix.
> 

I thought about it but the 'S' in SCR stands for Serial ATA and we also 
need to rename all PM functions to sata_pmp_xxx() - SCR and PMP are 
already SATA specific.  Do you think that's the way to go?

For ata_port_on/offline(), I think it's better to leave them alone for 
hotpluggable IDE hotbays with presence detection.

-- 
tejun

Re: [PATCH 16/22] libata: implement new SCR handling and port on/offline functions

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Implement ata_scr_{valid|read|write|write_flush}() and
>>> ata_port_{online|offline}().  These functions replace
>>> scr_{read|write}() and sata_dev_present().
>>>
>>> Major difference between between the new SCR functions and the old
>>> ones is that the new ones have a way to signal error to the caller.
>>> This makes handling SCR-available and SCR-unavailable cases in the
>>> same path easier.  Also, it eases later PM implementation where SCR
>>> access can fail due to various reasons.
>>>
>>> ata_port_{online|offline}() functions return 1 only when they are
>>> affirmitive of the condition.  e.g.  if SCR is unaccessible or
>>> presence cannot be determined for other reasons, these functions
>>> return 0.  So, ata_port_online() != !ata_port_offline().  This
>>> distinction is useful in many exception handling cases.
>>
>> If its SATA-specific, it should have a "sata_" not "ata_" prefix.
>>
> 
> I thought about it but the 'S' in SCR stands for Serial ATA and we also 
> need to rename all PM functions to sata_pmp_xxx() - SCR and PMP are 
> already SATA specific.  Do you think that's the way to go?
> 
> For ata_port_on/offline(), I think it's better to leave them alone for 
> hotpluggable IDE hotbays with presence detection.

Yeah, I feel pretty strongly about the sata_ prefix, even for 
port_on/offline.  The implementation is purely SATA specific.

Should that ever change for sata_port_on/offline, it then becomes 
trivial to create a ->port_on/offline hook, and update all SATA drivers 
to pass sata_port_on/offline to it.  IOW, I think its unlikely that 
these functions, as implemented and used, will morph in the future from

	sata-port-on

to

	if (sata)
		sata-port-on
	else if (hotpluggable PATA bays)
		bay-on

Its much more likely we will create two separate functions for the 
separate functionality.  Thus, your ata_port_on will always be SATA 
specific, and deserving of the spiffy sata_ prefix.  And hey, we want to 
emphasize its SATA because SATA is cooler than PATA, too.

	Jeff

Re: [PATCH 16/22] libata: implement new SCR handling and port on/offline functions

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Jeff Garzik wrote:
>>> Tejun Heo wrote:
>>>> Implement ata_scr_{valid|read|write|write_flush}() and
>>>> ata_port_{online|offline}().  These functions replace
>>>> scr_{read|write}() and sata_dev_present().
>>>>
>>>> Major difference between between the new SCR functions and the old
>>>> ones is that the new ones have a way to signal error to the caller.
>>>> This makes handling SCR-available and SCR-unavailable cases in the
>>>> same path easier.  Also, it eases later PM implementation where SCR
>>>> access can fail due to various reasons.
>>>>
>>>> ata_port_{online|offline}() functions return 1 only when they are
>>>> affirmitive of the condition.  e.g.  if SCR is unaccessible or
>>>> presence cannot be determined for other reasons, these functions
>>>> return 0.  So, ata_port_online() != !ata_port_offline().  This
>>>> distinction is useful in many exception handling cases.
>>>
>>> If its SATA-specific, it should have a "sata_" not "ata_" prefix.
>>>
>>
>> I thought about it but the 'S' in SCR stands for Serial ATA and we 
>> also need to rename all PM functions to sata_pmp_xxx() - SCR and PMP 
>> are already SATA specific.  Do you think that's the way to go?
>>
>> For ata_port_on/offline(), I think it's better to leave them alone for 
>> hotpluggable IDE hotbays with presence detection.
> 
> Yeah, I feel pretty strongly about the sata_ prefix, even for 
> port_on/offline.  The implementation is purely SATA specific.
> 
> Should that ever change for sata_port_on/offline, it then becomes 
> trivial to create a ->port_on/offline hook, and update all SATA drivers 
> to pass sata_port_on/offline to it.  IOW, I think its unlikely that 
> these functions, as implemented and used, will morph in the future from
> 
>     sata-port-on
> 
> to
> 
>     if (sata)
>         sata-port-on
>     else if (hotpluggable PATA bays)
>         bay-on

I'm more concerned with where it's used rather than how it's 
implemented.  The ata_port_on/offline(), which gets morphed into 
ata_link_on/offline() with ata_link introduction, is used throughout 
libata core layer which covers both PATA and SATA.  And I've been 
careful such that no on/off info cases are handled properly without ugly 
sata/pata test in each location.

So, my suggestion is to keep ata_port_on/offline() interface as they 
are.  If you're uncomfortable with putting SATA only implementation into 
ata_port_on/offline(), I can separate out sata_port_on/offline() and put 
sata/pata test into ata_port_on/offline().  But, if we simply rename 
them, we'll have to put sata/pata tests where those functions are used, 
which is ugly.

> Its much more likely we will create two separate functions for the 
> separate functionality.  Thus, your ata_port_on will always be SATA 
> specific, and deserving of the spiffy sata_ prefix.  And hey, we want to 
> emphasize its SATA because SATA is cooler than PATA, too.

So, the current ata_port_on/off functions can be considered as including 
both PATA and SATA implementation, where SATA implementation got 
collapsed into it because PATA implementation is trivial.

I agree with you about other functions.  SATA, cool, good.

-- 
tejun

Re: [PATCH 16/22] libata: implement new SCR handling and port on/offline functions

[Jeff Garzik]

Tejun Heo wrote:
> So, the current ata_port_on/off functions can be considered as including 
> both PATA and SATA implementation, where SATA implementation got 
> collapsed into it because PATA implementation is trivial.

ok, ACK this train of thought

[PATCH 11/22] libata: move ->set_mode() handling into ata_set_mode()

[Tejun Heo]

Move ->set_mode() handlng into ata_set_mode().

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/libata-core.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

9e0876cd42527ef2356a676edafc9d7f422e9a48
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 1806c7c..ec40c79 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1426,20 +1426,7 @@ static int ata_bus_probe(struct ata_port
 	}
 
 	/* configure transfer mode */
-	if (ap->ops->set_mode) {
-		/* FIXME: make ->set_mode handle no device case and
-		 * return error code and failing device on failure as
-		 * ata_set_mode() does.
-		 */
-		for (i = 0; i < ATA_MAX_DEVICES; i++)
-			if (ata_dev_enabled(&ap->device[i])) {
-				ap->ops->set_mode(ap);
-				break;
-			}
-		rc = 0;
-	} else
-		rc = ata_set_mode(ap, &dev);
-
+	rc = ata_set_mode(ap, &dev);
 	if (rc) {
 		down_xfermask = 1;
 		goto fail;
@@ -1997,6 +1984,20 @@ int ata_set_mode(struct ata_port *ap, st
 	struct ata_device *dev;
 	int i, rc = 0, used_dma = 0, found = 0;
 
+	/* has private set_mode? */
+	if (ap->ops->set_mode) {
+		/* FIXME: make ->set_mode handle no device case and
+		 * return error code and failing device on failure.
+		 */
+		for (i = 0; i < ATA_MAX_DEVICES; i++) {
+			if (ata_dev_enabled(&ap->device[i])) {
+				ap->ops->set_mode(ap);
+				break;
+			}
+		}
+		return 0;
+	}
+
 	/* step 1: calculate xfer_mask */
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		unsigned int pio_mask, dma_mask;
-- 
1.2.4

[PATCH 22/22] libata: use ATA printk helpers

[Tejun Heo]

Use ATA printk helpers.

Signed-off-by: Tejun Heo <htejun@gmail.com>

---

 drivers/scsi/ahci.c         |    8 +-
 drivers/scsi/ata_piix.c     |    4 -
 drivers/scsi/libata-core.c  |  199 ++++++++++++++++++++-----------------------
 drivers/scsi/libata-eh.c    |    5 +
 drivers/scsi/libata-scsi.c  |    9 +-
 drivers/scsi/sata_mv.c      |    8 +-
 drivers/scsi/sata_promise.c |    7 +-
 drivers/scsi/sata_sil.c     |    8 +-
 drivers/scsi/sata_sil24.c   |    6 +
 drivers/scsi/sata_sx4.c     |    7 +-
 10 files changed, 124 insertions(+), 137 deletions(-)

9fbba69c142464ffe56e6682d2d96cffef1aea8d
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index f6e4c8e..a4fb8d0 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -655,8 +655,7 @@ static int ahci_softreset(struct ata_por
  fail_restart:
 	ahci_start_engine(ap);
  fail:
-	printk(KERN_ERR "ata%u: softreset failed (%s)\n",
-	       ap->id, reason);
+	ata_port_printk(ap, KERN_ERR, "softreset failed (%s)\n", reason);
 	return rc;
 }
 
@@ -798,9 +797,8 @@ static void ahci_restart_port(struct ata
 
 	if ((ap->device[0].class != ATA_DEV_ATAPI) ||
 	    ((irq_stat & PORT_IRQ_TF_ERR) == 0))
-		printk(KERN_WARNING "ata%u: port reset, "
+		ata_port_printk(ap, KERN_WARNING, "port reset, "
 		       "p_is %x is %x pis %x cmd %x tf %x ss %x se %x\n",
-			ap->id,
 			irq_stat,
 			readl(mmio + HOST_IRQ_STAT),
 			readl(port_mmio + PORT_IRQ_STAT),
@@ -840,7 +838,7 @@ static void ahci_eng_timeout(struct ata_
 	struct ata_queued_cmd *qc;
 	unsigned long flags;
 
-	printk(KERN_WARNING "ata%u: handling error/timeout\n", ap->id);
+	ata_port_printk(ap, KERN_WARNING, "handling error/timeout\n");
 
 	spin_lock_irqsave(&host_set->lock, flags);
 
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
index 62dabf7..af1d46e 100644
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -484,7 +484,7 @@ static int piix_pata_probe_reset(struct 
 	struct pci_dev *pdev = to_pci_dev(ap->host_set->dev);
 
 	if (!pci_test_config_bits(pdev, &piix_enable_bits[ap->hard_port_no])) {
-		printk(KERN_INFO "ata%u: port disabled. ignoring.\n", ap->id);
+		ata_port_printk(ap, KERN_INFO, "port disabled. ignoring.\n");
 		return 0;
 	}
 
@@ -565,7 +565,7 @@ static unsigned int piix_sata_probe (str
 static int piix_sata_probe_reset(struct ata_port *ap, unsigned int *classes)
 {
 	if (!piix_sata_probe(ap)) {
-		printk(KERN_INFO "ata%u: SATA port has no device.\n", ap->id);
+		ata_port_printk(ap, KERN_INFO, "SATA port has no device.\n");
 		return 0;
 	}
 
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 8eee5af..126adef 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -412,8 +412,7 @@ static const char *sata_spd_string(unsig
 void ata_dev_disable(struct ata_device *dev)
 {
 	if (ata_dev_enabled(dev)) {
-		printk(KERN_WARNING "ata%u: dev %u disabled\n",
-		       dev->ap->id, dev->devno);
+		ata_dev_printk(dev, KERN_WARNING, "disabled\n");
 		dev->class++;
 	}
 }
@@ -1021,8 +1020,9 @@ unsigned ata_exec_internal(struct ata_de
 		if (qc->flags & ATA_QCFLAG_ACTIVE) {
 			qc->err_mask = AC_ERR_TIMEOUT;
 			ata_qc_complete(qc);
-			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
-			       ap->id, command);
+
+			ata_dev_printk(dev, KERN_WARNING,
+				       "qc timeout (cmd 0x%x)\n", command);
 		}
 
 		spin_unlock_irqrestore(&ap->host_set->lock, flags);
@@ -1187,8 +1187,8 @@ static int ata_dev_read_id(struct ata_de
 	return 0;
 
  err_out:
-	printk(KERN_WARNING "ata%u: dev %u failed to IDENTIFY (%s)\n",
-	       ap->id, dev->devno, reason);
+	ata_dev_printk(dev, KERN_WARNING, "failed to IDENTIFY "
+		       "(%s, err_mask=0x%x)\n", reason, err_mask);
 	return rc;
 }
 
@@ -1228,10 +1228,10 @@ static int ata_dev_configure(struct ata_
 
 	/* print device capabilities */
 	if (print_info)
-		printk(KERN_DEBUG "ata%u: dev %u cfg 49:%04x 82:%04x 83:%04x "
-		       "84:%04x 85:%04x 86:%04x 87:%04x 88:%04x\n",
-		       ap->id, dev->devno, id[49], id[82], id[83],
-		       id[84], id[85], id[86], id[87], id[88]);
+		ata_dev_printk(dev, KERN_DEBUG, "cfg 49:%04x 82:%04x 83:%04x "
+			       "84:%04x 85:%04x 86:%04x 87:%04x 88:%04x\n",
+			       id[49], id[82], id[83], id[84],
+			       id[85], id[86], id[87], id[88]);
 
 	/* initialize to-be-configured parameters */
 	dev->flags &= ~ATA_DFLAG_CFG_MASK;
@@ -1267,13 +1267,12 @@ static int ata_dev_configure(struct ata_
 
 			/* print device info to dmesg */
 			if (print_info)
-				printk(KERN_INFO "ata%u: dev %u ATA-%d, "
-				       "max %s, %Lu sectors: %s\n",
-				       ap->id, dev->devno,
-				       ata_id_major_version(id),
-				       ata_mode_string(xfer_mask),
-				       (unsigned long long)dev->n_sectors,
-				       lba_desc);
+				ata_dev_printk(dev, KERN_INFO, "ATA-%d, "
+					"max %s, %Lu sectors: %s\n",
+					ata_id_major_version(id),
+					ata_mode_string(xfer_mask),
+					(unsigned long long)dev->n_sectors,
+					lba_desc);
 		} else {
 			/* CHS */
 
@@ -1291,13 +1290,12 @@ static int ata_dev_configure(struct ata_
 
 			/* print device info to dmesg */
 			if (print_info)
-				printk(KERN_INFO "ata%u: dev %u ATA-%d, "
-				       "max %s, %Lu sectors: CHS %u/%u/%u\n",
-				       ap->id, dev->devno,
-				       ata_id_major_version(id),
-				       ata_mode_string(xfer_mask),
-				       (unsigned long long)dev->n_sectors,
-				       dev->cylinders, dev->heads, dev->sectors);
+				ata_dev_printk(dev, KERN_INFO, "ATA-%d, "
+					"max %s, %Lu sectors: CHS %u/%u/%u\n",
+					ata_id_major_version(id),
+					ata_mode_string(xfer_mask),
+					(unsigned long long)dev->n_sectors,
+					dev->cylinders, dev->heads, dev->sectors);
 		}
 
 		dev->cdb_len = 16;
@@ -1307,7 +1305,8 @@ static int ata_dev_configure(struct ata_
 	else if (dev->class == ATA_DEV_ATAPI) {
 		rc = atapi_cdb_len(id);
 		if ((rc < 12) || (rc > ATAPI_CDB_LEN)) {
-			printk(KERN_WARNING "ata%u: unsupported CDB len\n", ap->id);
+			ata_dev_printk(dev, KERN_WARNING,
+				       "unsupported CDB len\n");
 			rc = -EINVAL;
 			goto err_out_nosup;
 		}
@@ -1315,8 +1314,8 @@ static int ata_dev_configure(struct ata_
 
 		/* print device info to dmesg */
 		if (print_info)
-			printk(KERN_INFO "ata%u: dev %u ATAPI, max %s\n",
-			       ap->id, dev->devno, ata_mode_string(xfer_mask));
+			ata_dev_printk(dev, KERN_INFO, "ATAPI, max %s\n",
+				       ata_mode_string(xfer_mask));
 	}
 
 	ap->host->max_cmd_len = 0;
@@ -1328,8 +1327,8 @@ static int ata_dev_configure(struct ata_
 	/* limit bridge transfers to udma5, 200 sectors */
 	if (ata_dev_knobble(dev)) {
 		if (print_info)
-			printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
-			       ap->id, dev->devno);
+			ata_dev_printk(dev, KERN_INFO,
+				       "applying bridge limits\n");
 		dev->udma_mask &= ATA_UDMA5;
 		dev->max_sectors = ATA_MAX_SECTORS;
 	}
@@ -1382,7 +1381,8 @@ static int ata_bus_probe(struct ata_port
 	if (ap->ops->probe_reset) {
 		rc = ap->ops->probe_reset(ap, classes);
 		if (rc) {
-			printk("ata%u: reset failed (errno=%d)\n", ap->id, rc);
+			ata_port_printk(ap, KERN_ERR,
+					"reset failed (errno=%d)\n", rc);
 			return rc;
 		}
 	} else {
@@ -1495,13 +1495,13 @@ static void sata_print_link_status(struc
 
 	if (ata_port_online(ap)) {
 		tmp = (sstatus >> 4) & 0xf;
-		printk(KERN_INFO
-		       "ata%u: SATA link up %s (SStatus %X SControl %X)\n",
-		       ap->id, sata_spd_string(tmp), sstatus, scontrol);
+		ata_port_printk(ap, KERN_INFO,
+				"SATA link up %s (SStatus %X SControl %X)\n",
+				sata_spd_string(tmp), sstatus, scontrol);
 	} else {
-		printk(KERN_INFO
-		       "ata%u: SATA link down (SStatus %X SControl %X)\n",
-		       ap->id, sstatus, scontrol);
+		ata_port_printk(ap, KERN_INFO,
+				"SATA link down (SStatus %X SControl %X)\n",
+				sstatus, scontrol);
 	}
 }
 
@@ -1655,8 +1655,8 @@ int ata_down_sata_spd_limit(struct ata_p
 
 	ap->sata_spd_limit = mask;
 
-	printk(KERN_WARNING "ata%u: limiting SATA link speed to %s\n",
-	       ap->id, sata_spd_string(fls(mask)));
+	ata_port_printk(ap, KERN_WARNING, "limiting SATA link speed to %s\n",
+			sata_spd_string(fls(mask)));
 
 	return 0;
 }
@@ -1895,7 +1895,6 @@ int ata_timing_compute(struct ata_device
  */
 int ata_down_xfermask_limit(struct ata_device *dev, int force_pio0)
 {
-	struct ata_port *ap = dev->ap;
 	unsigned long xfer_mask;
 	int highbit;
 
@@ -1918,8 +1917,8 @@ int ata_down_xfermask_limit(struct ata_d
 	ata_unpack_xfermask(xfer_mask, &dev->pio_mask, &dev->mwdma_mask,
 			    &dev->udma_mask);
 
-	printk(KERN_WARNING "ata%u: dev %u limiting speed to %s\n",
-	       ap->id, dev->devno, ata_mode_string(xfer_mask));
+	ata_dev_printk(dev, KERN_WARNING, "limiting speed to %s\n",
+		       ata_mode_string(xfer_mask));
 
 	return 0;
 
@@ -1929,7 +1928,6 @@ int ata_down_xfermask_limit(struct ata_d
 
 static int ata_dev_set_mode(struct ata_device *dev)
 {
-	struct ata_port *ap = dev->ap;
 	unsigned int err_mask;
 	int rc;
 
@@ -1939,9 +1937,8 @@ static int ata_dev_set_mode(struct ata_d
 
 	err_mask = ata_dev_set_xfermode(dev);
 	if (err_mask) {
-		printk(KERN_ERR
-		       "ata%u: failed to set xfermode (err_mask=0x%x)\n",
-		       ap->id, err_mask);
+		ata_dev_printk(dev, KERN_ERR, "failed to set xfermode "
+			       "(err_mask=0x%x)\n", err_mask);
 		return -EIO;
 	}
 
@@ -1952,9 +1949,8 @@ static int ata_dev_set_mode(struct ata_d
 	DPRINTK("xfer_shift=%u, xfer_mode=0x%x\n",
 		dev->xfer_shift, (int)dev->xfer_mode);
 
-	printk(KERN_INFO "ata%u: dev %u configured for %s\n",
-	       ap->id, dev->devno,
-	       ata_mode_string(ata_xfer_mode2mask(dev->xfer_mode)));
+	ata_dev_printk(dev, KERN_INFO, "configured for %s\n",
+		       ata_mode_string(ata_xfer_mode2mask(dev->xfer_mode)));
 	return 0;
 }
 
@@ -2022,8 +2018,7 @@ int ata_set_mode(struct ata_port *ap, st
 			continue;
 
 		if (!dev->pio_mode) {
-			printk(KERN_WARNING "ata%u: dev %u no PIO support\n",
-			       ap->id, dev->devno);
+			ata_dev_printk(dev, KERN_WARNING, "no PIO support\n");
 			rc = -EINVAL;
 			goto out;
 		}
@@ -2122,8 +2117,8 @@ unsigned int ata_busy_sleep (struct ata_
 	}
 
 	if (status & ATA_BUSY)
-		printk(KERN_WARNING "ata%u is slow to respond, "
-		       "please be patient\n", ap->id);
+		ata_port_printk(ap, KERN_WARNING,
+				"port is slow to respond, please be patient\n");
 
 	timeout = timer_start + tmout;
 	while ((status & ATA_BUSY) && (time_before(jiffies, timeout))) {
@@ -2132,8 +2127,8 @@ unsigned int ata_busy_sleep (struct ata_
 	}
 
 	if (status & ATA_BUSY) {
-		printk(KERN_ERR "ata%u failed to respond (%lu secs)\n",
-		       ap->id, tmout / HZ);
+		ata_port_printk(ap, KERN_ERR, "port failed to respond "
+				"(%lu secs)\n", tmout / HZ);
 		return 1;
 	}
 
@@ -2226,7 +2221,7 @@ static unsigned int ata_bus_softreset(st
 	 * pulldown resistor.
 	 */
 	if (ata_check_status(ap) == 0xFF) {
-		printk(KERN_ERR "ata%u: SRST failed (status 0xFF)\n", ap->id);
+		ata_port_printk(ap, KERN_ERR, "SRST failed (status 0xFF)\n");
 		return AC_ERR_OTHER;
 	}
 
@@ -2320,7 +2315,7 @@ void ata_bus_reset(struct ata_port *ap)
 	return;
 
 err_out:
-	printk(KERN_ERR "ata%u: disabling port\n", ap->id);
+	ata_port_printk(ap, KERN_ERR, "disabling port\n");
 	ap->ops->port_disable(ap);
 
 	DPRINTK("EXIT\n");
@@ -2423,8 +2418,8 @@ int ata_std_softreset(struct ata_port *a
 	DPRINTK("about to softreset, devmask=%x\n", devmask);
 	err_mask = ata_bus_softreset(ap, devmask);
 	if (err_mask) {
-		printk(KERN_ERR "ata%u: SRST failed (err_mask=0x%x)\n",
-		       ap->id, err_mask);
+		ata_port_printk(ap, KERN_ERR, "SRST failed (err_mask=0x%x)\n",
+				err_mask);
 		return -EIO;
 	}
 
@@ -2502,8 +2497,8 @@ int sata_std_hardreset(struct ata_port *
 	}
 
 	if (ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT)) {
-		printk(KERN_ERR
-		       "ata%u: COMRESET failed (device not ready)\n", ap->id);
+		ata_port_printk(ap, KERN_ERR,
+				"COMRESET failed (device not ready)\n");
 		return -EIO;
 	}
 
@@ -2661,8 +2656,8 @@ int ata_drive_probe_reset(struct ata_por
 		rc = ata_do_reset(ap, softreset, classes);
 		if (rc == 0 && classes[0] != ATA_DEV_UNKNOWN)
 			goto done;
-		printk(KERN_INFO "ata%u: softreset failed, will try "
-		       "hardreset in 5 secs\n", ap->id);
+		ata_port_printk(ap, KERN_INFO, "softreset failed, "
+				"will try hardreset in 5 secs\n");
 		ssleep(5);
 	}
 
@@ -2680,15 +2675,15 @@ int ata_drive_probe_reset(struct ata_por
 		if (ata_down_sata_spd_limit(ap))
 			goto done;
 
-		printk(KERN_INFO "ata%u: hardreset failed, will retry "
-		       "in 5 secs\n", ap->id);
+		ata_port_printk(ap, KERN_INFO, "hardreset failed, "
+				"will retry in 5 secs\n");
 		ssleep(5);
 	}
 
 	if (softreset) {
-		printk(KERN_INFO "ata%u: hardreset succeeded without "
-		       "classification, will retry softreset in 5 secs\n",
-		       ap->id);
+		ata_port_printk(ap, KERN_INFO,
+				"hardreset succeeded without classification, "
+				"will retry softreset in 5 secs\n");
 		ssleep(5);
 
 		rc = ata_do_reset(ap, softreset, classes);
@@ -2723,15 +2718,13 @@ int ata_drive_probe_reset(struct ata_por
 static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,
 			       const u16 *new_id)
 {
-	struct ata_port *ap = dev->ap;
 	const u16 *old_id = dev->id;
 	unsigned char model[2][41], serial[2][21];
 	u64 new_n_sectors;
 
 	if (dev->class != new_class) {
-		printk(KERN_INFO
-		       "ata%u: dev %u class mismatch %d != %d\n",
-		       ap->id, dev->devno, dev->class, new_class);
+		ata_dev_printk(dev, KERN_INFO, "class mismatch %d != %d\n",
+			       dev->class, new_class);
 		return 0;
 	}
 
@@ -2742,24 +2735,22 @@ static int ata_dev_same_device(struct at
 	new_n_sectors = ata_id_n_sectors(new_id);
 
 	if (strcmp(model[0], model[1])) {
-		printk(KERN_INFO
-		       "ata%u: dev %u model number mismatch '%s' != '%s'\n",
-		       ap->id, dev->devno, model[0], model[1]);
+		ata_dev_printk(dev, KERN_INFO, "model number mismatch "
+			       "'%s' != '%s'\n", model[0], model[1]);
 		return 0;
 	}
 
 	if (strcmp(serial[0], serial[1])) {
-		printk(KERN_INFO
-		       "ata%u: dev %u serial number mismatch '%s' != '%s'\n",
-		       ap->id, dev->devno, serial[0], serial[1]);
+		ata_dev_printk(dev, KERN_INFO, "serial number mismatch "
+			       "'%s' != '%s'\n", serial[0], serial[1]);
 		return 0;
 	}
 
 	if (dev->class == ATA_DEV_ATA && dev->n_sectors != new_n_sectors) {
-		printk(KERN_INFO
-		       "ata%u: dev %u n_sectors mismatch %llu != %llu\n",
-		       ap->id, dev->devno, (unsigned long long)dev->n_sectors,
-		       (unsigned long long)new_n_sectors);
+		ata_dev_printk(dev, KERN_INFO, "n_sectors mismatch "
+			       "%llu != %llu\n",
+			       (unsigned long long)dev->n_sectors,
+			       (unsigned long long)new_n_sectors);
 		return 0;
 	}
 
@@ -2782,9 +2773,8 @@ static int ata_dev_same_device(struct at
  */
 int ata_dev_revalidate(struct ata_device *dev, int post_reset)
 {
-	struct ata_port *ap = dev->ap;
 	unsigned int class = dev->class;
-	u16 *id = (void *)ap->sector_buf;
+	u16 *id = (void *)dev->ap->sector_buf;
 	int rc;
 
 	if (!ata_dev_enabled(dev)) {
@@ -2811,8 +2801,7 @@ int ata_dev_revalidate(struct ata_device
 		return 0;
 
  fail:
-	printk(KERN_ERR "ata%u: dev %u revalidation failed (errno=%d)\n",
-	       ap->id, dev->devno, rc);
+	ata_dev_printk(dev, KERN_ERR, "revalidation failed (errno=%d)\n", rc);
 	return rc;
 }
 
@@ -2940,8 +2929,8 @@ static void ata_dev_xfermask(struct ata_
 	}
 
 	if (ata_dma_blacklisted(dev))
-		printk(KERN_WARNING "ata%u: dev %u is on DMA blacklist, "
-		       "disabling DMA\n", ap->id, dev->devno);
+		ata_dev_printk(dev, KERN_WARNING,
+			       "device is on DMA blacklist, disabling DMA\n");
 
 	if (hs->flags & ATA_HOST_SIMPLEX) {
 		if (hs->simplex_claimed)
@@ -3732,8 +3721,8 @@ next_sg:
 		unsigned int i;
 
 		if (words) /* warning if bytes > 1 */
-			printk(KERN_WARNING "ata%u: %u bytes trailing data\n",
-			       ap->id, bytes);
+			ata_dev_printk(qc->dev, KERN_WARNING,
+				       "%u bytes trailing data\n", bytes);
 
 		for (i = 0; i < words; i++)
 			ata_data_xfer(ap, (unsigned char*)pad_buf, 2, do_write);
@@ -3816,8 +3805,7 @@ static void atapi_pio_bytes(struct ata_q
 	return;
 
 err_out:
-	printk(KERN_INFO "ata%u: dev %u: ATAPI check failed\n",
-	      ap->id, dev->devno);
+	ata_dev_printk(dev, KERN_INFO, "ATAPI check failed\n");
 	qc->err_mask |= AC_ERR_HSM;
 	ap->hsm_task_state = HSM_ST_ERR;
 }
@@ -3886,8 +3874,7 @@ static void ata_pio_error(struct ata_que
 	struct ata_port *ap = qc->ap;
 
 	if (qc->tf.command != ATA_CMD_PACKET)
-		printk(KERN_WARNING "ata%u: dev %u PIO error\n",
-		       ap->id, qc->dev->devno);
+		ata_dev_printk(qc->dev, KERN_WARNING, "PIO error\n");
 
 	/* make sure qc->err_mask is available to
 	 * know what's wrong and recover
@@ -4313,7 +4300,7 @@ idle_irq:
 #ifdef ATA_IRQ_TRAP
 	if ((ap->stats.idle_irq % 1000) == 0) {
 		ata_irq_ack(ap, 0); /* debug trap */
-		printk(KERN_WARNING "ata%d: irq trap\n", ap->id);
+		ata_port_printk(ap, KERN_WARNING, "irq trap\n");
 		return 1;
 	}
 #endif
@@ -4521,8 +4508,8 @@ static int ata_do_simple_cmd(struct ata_
 
 	err = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0);
 	if (err)
-		printk(KERN_ERR "%s: ata command failed: %d\n",
-				__FUNCTION__, err);
+		ata_dev_printk(dev, KERN_ERR, "%s: ata command failed: %d\n",
+			       __FUNCTION__, err);
 
 	return err;
 }
@@ -4862,15 +4849,14 @@ int ata_device_add(const struct ata_prob
 				(ap->pio_mask << ATA_SHIFT_PIO);
 
 		/* print per-port info to dmesg */
-		printk(KERN_INFO "ata%u: %cATA max %s cmd 0x%lX ctl 0x%lX "
-				 "bmdma 0x%lX irq %lu\n",
-			ap->id,
-			ap->flags & ATA_FLAG_SATA ? 'S' : 'P',
-			ata_mode_string(xfer_mode_mask),
-	       		ap->ioaddr.cmd_addr,
-	       		ap->ioaddr.ctl_addr,
-	       		ap->ioaddr.bmdma_addr,
-	       		ent->irq);
+		ata_port_printk(ap, KERN_INFO, "%cATA max %s cmd 0x%lX "
+				"ctl 0x%lX bmdma 0x%lX irq %lu\n",
+				ap->flags & ATA_FLAG_SATA ? 'S' : 'P',
+				ata_mode_string(xfer_mode_mask),
+				ap->ioaddr.cmd_addr,
+				ap->ioaddr.ctl_addr,
+				ap->ioaddr.bmdma_addr,
+				ent->irq);
 
 		ata_chk_status(ap);
 		host_set->ops->irq_clear(ap);
@@ -4908,8 +4894,7 @@ int ata_device_add(const struct ata_prob
 
 		rc = scsi_add_host(ap->host, dev);
 		if (rc) {
-			printk(KERN_ERR "ata%u: scsi_add_host failed\n",
-			       ap->id);
+			ata_port_printk(ap, KERN_ERR, "scsi_add_host failed\n");
 			/* FIXME: do something useful here */
 			/* FIXME: handle unconditional calls to
 			 * scsi_scan_host and ata_host_remove, below,
diff --git a/drivers/scsi/libata-eh.c b/drivers/scsi/libata-eh.c
index c31b13f..959a1cd 100644
--- a/drivers/scsi/libata-eh.c
+++ b/drivers/scsi/libata-eh.c
@@ -167,8 +167,9 @@ static void ata_qc_timeout(struct ata_qu
 		/* ack bmdma irq events */
 		ap->ops->irq_clear(ap);
 
-		printk(KERN_ERR "ata%u: command 0x%x timeout, stat 0x%x host_stat 0x%x\n",
-		       ap->id, qc->tf.command, drv_stat, host_stat);
+		ata_dev_printk(qc->dev, KERN_ERR, "command 0x%x timeout, "
+			       "stat 0x%x host_stat 0x%x\n",
+			       qc->tf.command, drv_stat, host_stat);
 
 		/* complete taskfile transaction */
 		qc->err_mask |= ac_err_mask(drv_stat);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index fcbf64e..a9b4083 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -1261,8 +1261,8 @@ static void ata_scsi_translate(struct at
 	if (cmd->sc_data_direction == DMA_FROM_DEVICE ||
 	    cmd->sc_data_direction == DMA_TO_DEVICE) {
 		if (unlikely(cmd->request_bufflen < 1)) {
-			printk(KERN_WARNING "ata%u(%u): WARNING: zero len r/w req\n",
-			       dev->ap->id, dev->devno);
+			ata_dev_printk(dev, KERN_WARNING,
+				       "WARNING: zero len r/w req\n");
 			goto err_did;
 		}
 
@@ -2200,8 +2200,9 @@ ata_scsi_find_dev(struct ata_port *ap, c
 
 	if (!atapi_enabled || (ap->flags & ATA_FLAG_NO_ATAPI)) {
 		if (unlikely(dev->class == ATA_DEV_ATAPI)) {
-			printk(KERN_WARNING "ata%u(%u): WARNING: ATAPI is %s, device ignored.\n",
-			       ap->id, dev->devno, atapi_enabled ? "not supported with this driver" : "disabled");
+			ata_dev_printk(dev, KERN_WARNING,
+				"WARNING: ATAPI is %s, device ignored.\n",
+				atapi_enabled ? "not supported with this driver" : "disabled");
 			return NULL;
 		}
 	}
diff --git a/drivers/scsi/sata_mv.c b/drivers/scsi/sata_mv.c
index 97aad57..5b7fea5 100644
--- a/drivers/scsi/sata_mv.c
+++ b/drivers/scsi/sata_mv.c
@@ -680,7 +680,7 @@ static void mv_stop_dma(struct ata_port 
 	}
 
 	if (EDMA_EN & reg) {
-		printk(KERN_ERR "ata%u: Unable to stop eDMA\n", ap->id);
+		ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
 		/* FIXME: Consider doing a reset here to recover */
 	}
 }
@@ -1964,8 +1964,8 @@ comreset_retry:
 		ata_port_probe(ap);
 	} else {
 		ata_scr_read(ap, SCR_STATUS, &sstatus);
-		printk(KERN_INFO "ata%u: no device found (phy stat %08x)\n",
-		       ap->id, sstatus);
+		ata_port_printk(ap, KERN_INFO,
+				"no device found (phy stat %08x)\n", sstatus);
 		ata_port_disable(ap);
 		return;
 	}
@@ -2023,7 +2023,7 @@ static void mv_eng_timeout(struct ata_po
 {
 	struct ata_queued_cmd *qc;
 
-	printk(KERN_ERR "ata%u: Entering mv_eng_timeout\n",ap->id);
+	ata_port_printk(ap, KERN_ERR, "Entering mv_eng_timeout\n");
 	DPRINTK("All regs @ start of eng_timeout\n");
 	mv_dump_all_regs(ap->host_set->mmio_base, ap->port_no,
 			 to_pci_dev(ap->host_set->dev));
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
index ddbc0c6..e9d61bc 100644
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -435,7 +435,7 @@ static void pdc_eng_timeout(struct ata_p
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
-		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
+		ata_port_printk(ap, KERN_ERR, "command timeout\n");
 		drv_stat = ata_wait_idle(ap);
 		qc->err_mask |= __ac_err_mask(drv_stat);
 		break;
@@ -443,8 +443,9 @@ static void pdc_eng_timeout(struct ata_p
 	default:
 		drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 
-		printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n",
-		       ap->id, qc->tf.command, drv_stat);
+		ata_port_printk(ap, KERN_ERR,
+				"unknown timeout, cmd 0x%x stat 0x%x\n",
+				qc->tf.command, drv_stat);
 
 		qc->err_mask |= ac_err_mask(drv_stat);
 		break;
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
index c933357..bfcece1 100644
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -360,16 +360,16 @@ static void sil_dev_config(struct ata_po
 	if (slow_down ||
 	    ((ap->flags & SIL_FLAG_MOD15WRITE) &&
 	     (quirks & SIL_QUIRK_MOD15WRITE))) {
-		printk(KERN_INFO "ata%u(%u): applying Seagate errata fix (mod15write workaround)\n",
-		       ap->id, dev->devno);
+		ata_dev_printk(dev, KERN_INFO, "applying Seagate errata fix "
+			       "(mod15write workaround)\n");
 		dev->max_sectors = 15;
 		return;
 	}
 
 	/* limit to udma5 */
 	if (quirks & SIL_QUIRK_UDMA5MAX) {
-		printk(KERN_INFO "ata%u(%u): applying Maxtor errata fix %s\n",
-		       ap->id, dev->devno, model_num);
+		ata_dev_printk(dev, KERN_INFO,
+			       "applying Maxtor errata fix %s\n", model_num);
 		dev->udma_mask &= ATA_UDMA5;
 		return;
 	}
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 664b138..6144f7c 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -516,7 +516,7 @@ static int sil24_softreset(struct ata_po
 	return 0;
 
  err:
-	printk(KERN_ERR "ata%u: softreset failed (%s)\n", ap->id, reason);
+	ata_port_printk(ap, KERN_ERR, "softreset failed (%s)\n", reason);
 	return -EIO;
 }
 
@@ -561,7 +561,7 @@ static int sil24_hardreset(struct ata_po
 	return 0;
 
  err:
-	printk(KERN_ERR "ata%u: hardreset failed (%s)\n", ap->id, reason);
+	ata_port_printk(ap, KERN_ERR, "hardreset failed (%s)\n", reason);
 	return -EIO;
 }
 
@@ -721,7 +721,7 @@ static void sil24_eng_timeout(struct ata
 
 	qc = ata_qc_from_tag(ap, ap->active_tag);
 
-	printk(KERN_ERR "ata%u: command timeout\n", ap->id);
+	ata_port_printk(ap, KERN_ERR, "command timeout\n");
 	qc->err_mask |= AC_ERR_TIMEOUT;
 	ata_eh_qc_complete(qc);
 
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
index a669d05..96d7b73 100644
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -868,15 +868,16 @@ static void pdc_eng_timeout(struct ata_p
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
 	case ATA_PROT_NODATA:
-		printk(KERN_ERR "ata%u: command timeout\n", ap->id);
+		ata_port_printk(ap, KERN_ERR, "command timeout\n");
 		qc->err_mask |= __ac_err_mask(ata_wait_idle(ap));
 		break;
 
 	default:
 		drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 
-		printk(KERN_ERR "ata%u: unknown timeout, cmd 0x%x stat 0x%x\n",
-		       ap->id, qc->tf.command, drv_stat);
+		ata_port_printk(ap, KERN_ERR,
+				"unknown timeout, cmd 0x%x stat 0x%x\n",
+				qc->tf.command, drv_stat);
 
 		qc->err_mask |= ac_err_mask(drv_stat);
 		break;
-- 
1.2.4

Re: [PATCH 22/22] libata: use ATA printk helpers

[Jeff Garzik]

Tejun Heo wrote:
> Use ATA printk helpers.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 20 - 22

Re: [PATCHSET 01/11] prep for new EH

[Jeff Garzik]

Tejun Heo wrote:
> Hello, all.
> 
> This is part of patchset series described in [T].
> 
> This is the first take of prep-for-new-EH patchset.  This patchset
> contains 22 patches and can be grouped as follows.
> 
> #01-02: SCSI support for libata EH
> #03-09: various fixes
> #10-22: various new features

After some minor changes, it sounds like I can pull this patchset.

	Jeff