[PATCHSET 01/03] prep for hotplug support, take 3

[PATCHSET 01/03] prep for hotplug support, take 3

[Tejun Heo]

Hello,

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

This is the third take of prep-for-hotplug-support patchset.  Changes
from the last take[L] are

* sata_phy_debounce() now takes const array of three ulongs for timing
  parameters.  This makes it easy to define and export different
  predefined timing parameters and to pass timing parameter through
  other functions.  sata_phy_resume() is changed to take timing
  parameters and pass it to sata_phy_debounce() intead of crude
  boolean @quick.

* Three predefined parameters are provided - sata_deb_timing_boot,
  sata_deb_timing_eh and sata_deb_timing_before_fsrst.  These
  predefined parameters are used in the new hotplug framework and
  don't have to be long enough to cover all the cases.  The goal is to
  cover most cases in reasonable amount of time.  If timeout occurs,
  libata will give it more time and then retry.

* prereset() framework has been updated such that prereset() can tell
  EH what reset mechanism to use or to skip reset completely.  This
  way, HRST_TO_RESUME handling is confined into prereset() and device
  detection from prereset() is handled cleanly.

* ata_std_prereset() has been updated to handle different behaviors
  controllers show on hotplug.  Some controllers can wait for the
  first FIS34, some can only after COMRESET, others just can't.  These
  are controlled by two flags - ATA_FLAG_HRST_TO_RESUME and
  ATA_FLAG_CANT_WAIT_FIS34.  Note that those two flags describe PHY
  property.  They will move out to ata_link later.

  If necessary, ata_std_prereset() waits for spinup until hotplug
  event timestamp + spinup wait time (currently 8s).  This mechanism
  will allow later PMP support to avoid excessive amount of waiting.

This patchset is against

  upstream (8d4ee71ff6de5255ebfdf44fb83419d27bd06368)
  + enforce-default-EH-actions patch [1]
  + scsi_implement_eh-patch [2]

Thanks.

--
tejun

[T] http://article.gmane.org/gmane.linux.ide/10530
[L] http://article.gmane.org/gmane.linux.ide/10073
[1] http://article.gmane.org/gmane.linux.ide/10317
[2] http://article.gmane.org/gmane.linux.ide/10529

[PATCH 03/14] libata-hp-prep: make some ata_device fields persistent

[Tejun Heo]

Lifetimes of some fields span over device plugging/unplugging.  This
patch moves such persistent fields to the top of ata_device and
separate them with ATA_DEVICE_CLEAR_OFFSET.  Fields above the offset
are initialized once during host initializatino while all other fields
are cleared before hotplugging.  Currently ->ap, devno and part of
flags are persistent.

Note that flags is partially cleared while holding host_set lock.
This is to synchronize with later warm plug implementation which will
record hotplug request in dev->flags.

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

---

 drivers/scsi/libata-core.c |   14 ++++++++++++--
 include/linux/libata.h     |    8 ++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

2a6927015b1d7a4c1de409c527331ebf9755d613
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index f239324..ccdb867 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5120,9 +5120,18 @@ static void ata_host_remove(struct ata_p
 void ata_dev_init(struct ata_device *dev)
 {
 	struct ata_port *ap = dev->ap;
+	unsigned long flags;
+
+	/* High bits of dev->flags are used to record warm plug
+	 * requests which occur asynchronously.  Synchronize using
+	 * host_set lock.
+	 */
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	dev->flags &= ~ATA_DFLAG_INIT_MASK;
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
-	memset((void *)dev, 0, sizeof(*dev));
-	dev->devno = dev - ap->device;
+	memset((void *)dev + ATA_DEVICE_CLEAR_OFFSET, 0,
+	       sizeof(*dev) - ATA_DEVICE_CLEAR_OFFSET);
 	dev->pio_mask = UINT_MAX;
 	dev->mwdma_mask = UINT_MAX;
 	dev->udma_mask = UINT_MAX;
@@ -5183,6 +5192,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;
 		ata_dev_init(dev);
 	}
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3fb6dc3..99808c5 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -130,6 +130,7 @@ enum {
 	ATA_DFLAG_CFG_MASK	= (1 << 8) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device currently in PIO mode */
+	ATA_DFLAG_INIT_MASK	= (1 << 16) - 1,
 
 	ATA_DFLAG_DETACH	= (1 << 16),
 	ATA_DFLAG_DETACHED	= (1 << 17),
@@ -410,10 +411,11 @@ struct ata_ering {
 
 struct ata_device {
 	struct ata_port		*ap;
-	u64			n_sectors;	/* size of device, if ATA */
+	unsigned int		devno;		/* 0 or 1 */
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
+	/* fields above n_sectors are not cleared across device init */
+	u64			n_sectors;	/* size of device, if ATA */
 	unsigned int		class;		/* ATA_DEV_xxx */
-	unsigned int		devno;		/* 0 or 1 */
 	u16			id[ATA_ID_WORDS]; /* IDENTIFY xxx DEVICE data */
 	u8			pio_mode;
 	u8			dma_mode;
@@ -439,6 +441,8 @@ struct ata_device {
 	struct ata_ering	ering;
 };
 
+#define ATA_DEVICE_CLEAR_OFFSET		offsetof(struct ata_device, n_sectors)
+
 struct ata_eh_info {
 	struct ata_device	*dev;		/* offending device */
 	u32			serror;		/* SError from LLDD */
-- 
1.3.2

Re: [PATCH 03/14] libata-hp-prep: make some ata_device fields persistent

[Jeff Garzik]

Tejun Heo wrote:
> @@ -439,6 +441,8 @@ struct ata_device {
>  	struct ata_ering	ering;
>  };
>  
> +#define ATA_DEVICE_CLEAR_OFFSET		offsetof(struct ata_device, n_sectors)

add a code comment, explaining what this is used for.

ACK everything else in patch.

ACK patch #2, also.

	Jeff

[PATCH 01/14] libata-hp-prep: add flags and eh_info/context fields for hotplug

[Tejun Heo]

Add hotplug related flags and eh_info/context fields.

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

---

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

95c2d43950709c541509bbedd692e984dc67b474
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2803ab8..3fb6dc3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -131,6 +131,9 @@ enum {
 
 	ATA_DFLAG_PIO		= (1 << 8), /* device currently in PIO mode */
 
+	ATA_DFLAG_DETACH	= (1 << 16),
+	ATA_DFLAG_DETACHED	= (1 << 17),
+
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
 	ATA_DEV_ATA_UNSUP	= 2,	/* ATA device (unsupported) */
@@ -153,6 +156,9 @@ enum {
 	ATA_FLAG_PIO_POLLING	= (1 << 10), /* use polling PIO if LLD
 					      * doesn't handle PIO interrupts */
 	ATA_FLAG_NCQ		= (1 << 11), /* host supports NCQ */
+	ATA_FLAG_HRST_TO_RESUME	= (1 << 12), /* hardreset to resume phy */
+	ATA_FLAG_CANT_WAIT_FIS34 = (1 << 13), /* can't wait for the first D2H
+					       * Register FIS clearing BSY */
 
 	ATA_FLAG_DEBUGMSG	= (1 << 14),
 	ATA_FLAG_FLUSH_PORT_TASK = (1 << 15), /* flush port task */
@@ -160,6 +166,9 @@ enum {
 	ATA_FLAG_EH_PENDING	= (1 << 16), /* EH pending */
 	ATA_FLAG_FROZEN		= (1 << 17), /* port is frozen */
 	ATA_FLAG_RECOVERED	= (1 << 18), /* recovery action performed */
+	ATA_FLAG_LOADING	= (1 << 19), /* boot/loading probe */
+	ATA_FLAG_UNLOADING	= (1 << 20), /* module is unloading */
+	ATA_FLAG_SCSI_HOTPLUG	= (1 << 21), /* SCSI hotplug scheduled */
 
 	ATA_FLAG_DISABLED	= (1 << 22), /* port is disabled, ignore it */
 	ATA_FLAG_SUSPENDED	= (1 << 23), /* port is suspended (power) */
@@ -241,7 +250,9 @@ enum {
 	ATA_EH_RESET_MASK	= ATA_EH_SOFTRESET | ATA_EH_HARDRESET,
 
 	/* ata_eh_info->flags */
-	ATA_EHI_DID_RESET	= (1 << 0), /* already reset this port */
+	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
+
+	ATA_EHI_DID_RESET	= (1 << 16), /* already reset this port */
 
 	/* max repeat if error condition is still set after ->error_handler */
 	ATA_EH_MAX_REPEAT	= 5,
@@ -434,6 +445,10 @@ struct ata_eh_info {
 	unsigned int		err_mask;	/* port-wide err_mask */
 	unsigned int		action;		/* ATA_EH_* action mask */
 	unsigned int		flags;		/* ATA_EHI_* flags */
+
+	unsigned long		hotplug_timestamp;
+	unsigned int		probe_mask;
+
 	char			desc[ATA_EH_DESC_LEN];
 	int			desc_len;
 };
@@ -441,6 +456,8 @@ struct ata_eh_info {
 struct ata_eh_context {
 	struct ata_eh_info	i;
 	int			tries[ATA_MAX_DEVICES];
+	unsigned int		classes[ATA_MAX_DEVICES];
+	unsigned int		did_probe_mask;
 };
 
 struct ata_port {
-- 
1.3.2

Re: [PATCH 01/14] libata-hp-prep: add flags and eh_info/context fields for hotplug

[Jeff Garzik]

Tejun Heo wrote:
> Add hotplug related flags and eh_info/context fields.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  include/linux/libata.h |   19 ++++++++++++++++++-
>  1 files changed, 18 insertions(+), 1 deletions(-)
> 
> 95c2d43950709c541509bbedd692e984dc67b474
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 2803ab8..3fb6dc3 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -131,6 +131,9 @@ enum {
>  
>  	ATA_DFLAG_PIO		= (1 << 8), /* device currently in PIO mode */
>  
> +	ATA_DFLAG_DETACH	= (1 << 16),
> +	ATA_DFLAG_DETACHED	= (1 << 17),
> +
>  	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
>  	ATA_DEV_ATA		= 1,	/* ATA device */
>  	ATA_DEV_ATA_UNSUP	= 2,	/* ATA device (unsupported) */
> @@ -153,6 +156,9 @@ enum {
>  	ATA_FLAG_PIO_POLLING	= (1 << 10), /* use polling PIO if LLD
>  					      * doesn't handle PIO interrupts */
>  	ATA_FLAG_NCQ		= (1 << 11), /* host supports NCQ */
> +	ATA_FLAG_HRST_TO_RESUME	= (1 << 12), /* hardreset to resume phy */
> +	ATA_FLAG_CANT_WAIT_FIS34 = (1 << 13), /* can't wait for the first D2H
> +					       * Register FIS clearing BSY */

FIS34 is an awful name.  I hereby ban it from libata :)

Also those names are getting long.  Consider shortening CANT_WAIT, 
perhaps to NOWAIT.  And perhaps s/HRST_TO_RESUME/HRST_RESUMES/.

ACK everything else in this patch.

	Jeff

Re: [PATCH 01/14] libata-hp-prep: add flags and eh_info/context fields for hotplug

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Add hotplug related flags and eh_info/context fields.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> ---
>>
>>  include/linux/libata.h |   19 ++++++++++++++++++-
>>  1 files changed, 18 insertions(+), 1 deletions(-)
>>
>> 95c2d43950709c541509bbedd692e984dc67b474
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 2803ab8..3fb6dc3 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -131,6 +131,9 @@ enum {
>>  
>>      ATA_DFLAG_PIO        = (1 << 8), /* device currently in PIO mode */
>>  
>> +    ATA_DFLAG_DETACH    = (1 << 16),
>> +    ATA_DFLAG_DETACHED    = (1 << 17),
>> +
>>      ATA_DEV_UNKNOWN        = 0,    /* unknown device */
>>      ATA_DEV_ATA        = 1,    /* ATA device */
>>      ATA_DEV_ATA_UNSUP    = 2,    /* ATA device (unsupported) */
>> @@ -153,6 +156,9 @@ enum {
>>      ATA_FLAG_PIO_POLLING    = (1 << 10), /* use polling PIO if LLD
>>                            * doesn't handle PIO interrupts */
>>      ATA_FLAG_NCQ        = (1 << 11), /* host supports NCQ */
>> +    ATA_FLAG_HRST_TO_RESUME    = (1 << 12), /* hardreset to resume 
>> phy */
>> +    ATA_FLAG_CANT_WAIT_FIS34 = (1 << 13), /* can't wait for the first 
>> D2H
>> +                           * Register FIS clearing BSY */
> 
> FIS34 is an awful name.  I hereby ban it from libata :)
> 
> Also those names are getting long.  Consider shortening CANT_WAIT, 
> perhaps to NOWAIT.  And perhaps s/HRST_TO_RESUME/HRST_RESUMES/.
> 
> ACK everything else in this patch.

Yeah I hate FIS34 too.  Can you suggest a name which can also signify 
that the wait is for hotplug probing?  ATA_FLAG_CANT_WAIT_HP?

-- 
tejun

Re: [PATCH 01/14] libata-hp-prep: add flags and eh_info/context fields for hotplug

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Add hotplug related flags and eh_info/context fields.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>>
>>> ---
>>>
>>>  include/linux/libata.h |   19 ++++++++++++++++++-
>>>  1 files changed, 18 insertions(+), 1 deletions(-)
>>>
>>> 95c2d43950709c541509bbedd692e984dc67b474
>>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>>> index 2803ab8..3fb6dc3 100644
>>> --- a/include/linux/libata.h
>>> +++ b/include/linux/libata.h
>>> @@ -131,6 +131,9 @@ enum {
>>>  
>>>      ATA_DFLAG_PIO        = (1 << 8), /* device currently in PIO mode */
>>>  
>>> +    ATA_DFLAG_DETACH    = (1 << 16),
>>> +    ATA_DFLAG_DETACHED    = (1 << 17),
>>> +
>>>      ATA_DEV_UNKNOWN        = 0,    /* unknown device */
>>>      ATA_DEV_ATA        = 1,    /* ATA device */
>>>      ATA_DEV_ATA_UNSUP    = 2,    /* ATA device (unsupported) */
>>> @@ -153,6 +156,9 @@ enum {
>>>      ATA_FLAG_PIO_POLLING    = (1 << 10), /* use polling PIO if LLD
>>>                            * doesn't handle PIO interrupts */
>>>      ATA_FLAG_NCQ        = (1 << 11), /* host supports NCQ */
>>> +    ATA_FLAG_HRST_TO_RESUME    = (1 << 12), /* hardreset to resume 
>>> phy */
>>> +    ATA_FLAG_CANT_WAIT_FIS34 = (1 << 13), /* can't wait for the 
>>> first D2H
>>> +                           * Register FIS clearing BSY */
>>
>> FIS34 is an awful name.  I hereby ban it from libata :)
>>
>> Also those names are getting long.  Consider shortening CANT_WAIT, 
>> perhaps to NOWAIT.  And perhaps s/HRST_TO_RESUME/HRST_RESUMES/.
>>
>> ACK everything else in this patch.
> 
> Yeah I hate FIS34 too.  Can you suggest a name which can also signify 
> that the wait is for hotplug probing?  ATA_FLAG_CANT_WAIT_HP?

I wouldn't mention hotplug probing, because feels more like an 
implementation detail/requirement, rather than low-level ATA.  Something 
that would be more clear to a libata newbie might be ATA_FLAG_SKIP_D2H_BSY.

For general 'FIS34' substitutions, I would suggestion 'D2H' or 'D2HREG'.

	Jeff

[PATCH 04/14] libata-hp-prep: update ata_scsi_find_dev() and friends

[Tejun Heo]

Separate out ata_find_dev() and __ata_scsi_find_dev() from
ata_scsi_find_dev().  ata_find_dev() checks ATA_FLAG_SLAVE_POSS for
id==1 case, so all three functions return NULL if id==1 is specified
for !SLAVE_POSS port.  These will be used by later hotplug
implementation.

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

---

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

9677c34d81dee7f9564be0a7e2b037c7e0d6ca52
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 2007b4b..1b2ac23 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -52,8 +52,12 @@ #include "libata.h"
 #define SECTOR_SIZE	512
 
 typedef unsigned int (*ata_xlat_func_t)(struct ata_queued_cmd *qc, const u8 *scsicmd);
-static struct ata_device *
-ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device *scsidev);
+
+static struct ata_device * __ata_scsi_find_dev(struct ata_port *ap,
+					const struct scsi_device *scsidev);
+static struct ata_device * ata_scsi_find_dev(struct ata_port *ap,
+					    const struct scsi_device *scsidev);
+
 
 #define RW_RECOVERY_MPAGE 0x1
 #define RW_RECOVERY_MPAGE_LEN 12
@@ -2308,6 +2312,23 @@ static unsigned int atapi_xlat(struct at
 	return 0;
 }
 
+static struct ata_device * ata_find_dev(struct ata_port *ap, int id)
+{
+	if (likely(id == 0 || (id == 1 && ap->flags & ATA_FLAG_SLAVE_POSS)))
+		return &ap->device[id];
+	return NULL;
+}
+
+static struct ata_device * __ata_scsi_find_dev(struct ata_port *ap,
+					const struct scsi_device *scsidev)
+{
+	/* skip commands not addressed to targets we simulate */
+	if (unlikely(scsidev->channel || scsidev->lun))
+		return NULL;
+
+	return ata_find_dev(ap, scsidev->id);
+}
+
 /**
  *	ata_scsi_find_dev - lookup ata_device from scsi_cmnd
  *	@ap: ATA port to which the device is attached
@@ -2324,23 +2345,12 @@ static unsigned int atapi_xlat(struct at
  *	RETURNS:
  *	Associated ATA device, or %NULL if not found.
  */
-
 static struct ata_device *
 ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device *scsidev)
 {
-	struct ata_device *dev;
-
-	/* skip commands not addressed to targets we simulate */
-	if (likely(scsidev->id < ATA_MAX_DEVICES))
-		dev = &ap->device[scsidev->id];
-	else
-		return NULL;
-
-	if (unlikely((scsidev->channel != 0) ||
-		     (scsidev->lun != 0)))
-		return NULL;
+	struct ata_device *dev = __ata_scsi_find_dev(ap, scsidev);
 
-	if (unlikely(!ata_dev_enabled(dev)))
+	if (unlikely(!dev || !ata_dev_enabled(dev)))
 		return NULL;
 
 	if (!atapi_enabled || (ap->flags & ATA_FLAG_NO_ATAPI)) {
-- 
1.3.2

Re: [PATCH 04/14] libata-hp-prep: update ata_scsi_find_dev() and friends

[Jeff Garzik]

Tejun Heo wrote:
> Separate out ata_find_dev() and __ata_scsi_find_dev() from
> ata_scsi_find_dev().  ata_find_dev() checks ATA_FLAG_SLAVE_POSS for
> id==1 case, so all three functions return NULL if id==1 is specified
> for !SLAVE_POSS port.  These will be used by later hotplug
> implementation.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK with future comment:

Once we have a better way to sequence simplex + master/slave + normal 
SATA, libata-scsi should move to exporting one Scsi_Host per controller, 
with each physical ATA bus being a scsi channel.

libata abuses Scsi_Host currently, to make master/slave + normal SATA 
sequencing work.  (note to self: need to review that area WRT NCQ, too)

	Jeff

Re: [PATCH 04/14] libata-hp-prep: update ata_scsi_find_dev() and friends

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Separate out ata_find_dev() and __ata_scsi_find_dev() from
>> ata_scsi_find_dev().  ata_find_dev() checks ATA_FLAG_SLAVE_POSS for
>> id==1 case, so all three functions return NULL if id==1 is specified
>> for !SLAVE_POSS port.  These will be used by later hotplug
>> implementation.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ACK with future comment:
> 
> Once we have a better way to sequence simplex + master/slave + normal 
> SATA, libata-scsi should move to exporting one Scsi_Host per controller, 
> with each physical ATA bus being a scsi channel.

I think the 'better way' would be implemented after we move to new block 
infrastructure (libstorage).  libstroage will probably deal with the 
mentioned command scheduling with queue grouping and associated 
scheduling mechanism, and SCSI mapping will be done by SATL.  Although 
mapping port id to channel fits better, I'm not sure whether the 
conversion would be worthwhile at that point. It wouldn't make any 
functional difference but the change would be visible to userspace.

If we're gonna do that, how about piggybacking ataX -> ataX-1 change 
with/around it?  It also is a user-visible change and the magnitude of 
affect on user space would be similar.

-- 
tejun

Re: [PATCH 04/14] libata-hp-prep: update ata_scsi_find_dev() and friends

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Separate out ata_find_dev() and __ata_scsi_find_dev() from
>>> ata_scsi_find_dev().  ata_find_dev() checks ATA_FLAG_SLAVE_POSS for
>>> id==1 case, so all three functions return NULL if id==1 is specified
>>> for !SLAVE_POSS port.  These will be used by later hotplug
>>> implementation.
>>>
>>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>>
>> ACK with future comment:
>>
>> Once we have a better way to sequence simplex + master/slave + normal 
>> SATA, libata-scsi should move to exporting one Scsi_Host per 
>> controller, with each physical ATA bus being a scsi channel.
> 
> I think the 'better way' would be implemented after we move to new block 
> infrastructure (libstorage).  libstroage will probably deal with the 
> mentioned command scheduling with queue grouping and associated 
> scheduling mechanism, and SCSI mapping will be done by SATL.  Although 
> mapping port id to channel fits better, I'm not sure whether the 
> conversion would be worthwhile at that point. It wouldn't make any 
> functional difference but the change would be visible to userspace.
> 
> If we're gonna do that, how about piggybacking ataX -> ataX-1 change 
> with/around it?  It also is a user-visible change and the magnitude of 
> affect on user space would be similar.

Sure...

	Jeff

[PATCH 02/14] libata-hp-prep: implement ata_dev_init()

[Tejun Heo]

Move initialization of struct ata_device into ata_dev_init() in
preparation for hotplug.  This patch calls ata_dev_init() from
ata_host_init() and thus makes no functional difference.

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

---

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

1b2521bc48b11f2b347d5c3b7cf7ee26c9bdc380
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 9051b68..f239324 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -5109,6 +5109,26 @@ static void ata_host_remove(struct ata_p
 }
 
 /**
+ *	ata_dev_init - Initialize an ata_device structure
+ *	@dev: Device structure to initialize
+ *
+ *	Initialize @dev in preparation for probing.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+void ata_dev_init(struct ata_device *dev)
+{
+	struct ata_port *ap = dev->ap;
+
+	memset((void *)dev, 0, sizeof(*dev));
+	dev->devno = dev - ap->device;
+	dev->pio_mask = UINT_MAX;
+	dev->mwdma_mask = UINT_MAX;
+	dev->udma_mask = UINT_MAX;
+}
+
+/**
  *	ata_host_init - Initialize an ata_port structure
  *	@ap: Structure to initialize
  *	@host: associated SCSI mid-layer structure
@@ -5122,7 +5142,6 @@ static void ata_host_remove(struct ata_p
  *	LOCKING:
  *	Inherited from caller.
  */
-
 static void ata_host_init(struct ata_port *ap, struct Scsi_Host *host,
 			  struct ata_host_set *host_set,
 			  const struct ata_probe_ent *ent, unsigned int port_no)
@@ -5164,10 +5183,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;
-		dev->udma_mask = UINT_MAX;
+		ata_dev_init(dev);
 	}
 
 #ifdef ATA_IRQ_TRAP
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index b76ad7d..d46c552 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -62,6 +62,7 @@ extern int ata_check_atapi_dma(struct at
 extern void ata_dev_select(struct ata_port *ap, unsigned int device,
                            unsigned int wait, unsigned int can_sleep);
 extern void swap_buf_le16(u16 *buf, unsigned int buf_words);
+extern void ata_dev_init(struct ata_device *dev);
 extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
 
-- 
1.3.2

[PATCH 05/14] libata-hp-prep: use __ata_scsi_find_dev()

[Tejun Heo]

Convert direct sdev -> dev lookup to __ata_scsi_find_dev().

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

---

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

0671154af98c1bd1dadc37359dea4162b0d16b10
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index 1b2ac23..b52591e 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -399,7 +399,7 @@ void ata_dump_status(unsigned id, struct
 int ata_scsi_device_resume(struct scsi_device *sdev)
 {
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
-	struct ata_device *dev = &ap->device[sdev->id];
+	struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
 
 	return ata_device_resume(dev);
 }
@@ -407,7 +407,7 @@ int ata_scsi_device_resume(struct scsi_d
 int ata_scsi_device_suspend(struct scsi_device *sdev, pm_message_t state)
 {
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
-	struct ata_device *dev = &ap->device[sdev->id];
+	struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
 
 	return ata_device_suspend(dev, state);
 }
@@ -713,19 +713,15 @@ static void ata_scsi_dev_config(struct s
 
 int ata_scsi_slave_config(struct scsi_device *sdev)
 {
+	struct ata_port *ap = ata_shost_to_port(sdev->host);
+	struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
+
 	ata_scsi_sdev_config(sdev);
 
 	blk_queue_max_phys_segments(sdev->request_queue, LIBATA_MAX_PRD);
 
-	if (sdev->id < ATA_MAX_DEVICES) {
-		struct ata_port *ap;
-		struct ata_device *dev;
-
-		ap = ata_shost_to_port(sdev->host);
-		dev = &ap->device[sdev->id];
-
+	if (dev)
 		ata_scsi_dev_config(sdev, dev);
-	}
 
 	return 0;	/* scsi layer doesn't check return value, sigh */
 }
-- 
1.3.2

[PATCH 09/14] libata-hp-prep: make probing related functions global

[Tejun Heo]

Hotplug will be implemented in libata-eh.c.  Make ata_dev_read_id()
and ata_dev_configure() global.

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

---

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

03952258155acd6310ace4caaeb320df06cd7874
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index fba5514..8b25cb5 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1164,8 +1164,8 @@ unsigned int ata_pio_need_iordy(const st
  *	RETURNS:
  *	0 on success, -errno otherwise.
  */
-static int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
-			   int post_reset, u16 *id)
+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;
@@ -1289,7 +1289,7 @@ static void ata_dev_config_ncq(struct at
  *	RETURNS:
  *	0 on success, -errno otherwise
  */
-static int ata_dev_configure(struct ata_device *dev, int print_info)
+int ata_dev_configure(struct ata_device *dev, int print_info)
 {
 	struct ata_port *ap = dev->ap;
 	const u16 *id = dev->id;
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index 4a67675..d728fd5 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -50,6 +50,9 @@ extern void ata_port_flush_task(struct a
 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_dev_read_id(struct ata_device *dev, unsigned int *p_class,
+			   int post_reset, u16 *id);
+extern int ata_dev_configure(struct ata_device *dev, int print_info);
 extern int sata_down_spd_limit(struct ata_port *ap);
 extern int sata_set_spd_needed(struct ata_port *ap);
 extern int ata_down_xfermask_limit(struct ata_device *dev, int force_pio0);
-- 
1.3.2

[PATCH 06/14] libata-hp-prep: implement ap->hw_sata_spd_limit

[Tejun Heo]

Add ap->hw_sata_spd_limit and initialize it once during the boot
initialization (or driver load initialization).  ap->sata_spd_limit is
reset to ap->hw_sata_spd_limit on hotplug.  This prevents spd limits
introduced by earlier devices from affecting new devices.

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

---

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

b2fd37ae97c04c7d19e5ae17dd50ad8d3ad8c725
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index ccdb867..9e10b79 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2457,17 +2457,9 @@ static int sata_phy_resume(struct ata_po
  */
 void ata_std_probeinit(struct ata_port *ap)
 {
-	u32 scontrol;
-
 	/* resume link */
 	sata_phy_resume(ap);
 
-	/* init sata_spd_limit to the current value */
-	if (sata_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);
@@ -5122,6 +5114,9 @@ void ata_dev_init(struct ata_device *dev
 	struct ata_port *ap = dev->ap;
 	unsigned long flags;
 
+	/* SATA spd limit is bound to the first device */
+	ap->sata_spd_limit = ap->hw_sata_spd_limit;
+
 	/* High bits of dev->flags are used to record warm plug
 	 * requests which occur asynchronously.  Synchronize using
 	 * host_set lock.
@@ -5177,7 +5172,7 @@ 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->sata_spd_limit = UINT_MAX;
+	ap->hw_sata_spd_limit = UINT_MAX;
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
 
@@ -5340,10 +5335,18 @@ int ata_device_add(const struct ata_prob
 	DPRINTK("probe begin\n");
 	for (i = 0; i < count; i++) {
 		struct ata_port *ap;
+		u32 scontrol;
 		int rc;
 
 		ap = host_set->ports[i];
 
+		/* init sata_spd_limit to the current value */
+		if (sata_scr_read(ap, SCR_CONTROL, &scontrol) == 0) {
+			int spd = (scontrol >> 4) & 0xf;
+			ap->hw_sata_spd_limit &= (1 << spd) - 1;
+		}
+		ap->sata_spd_limit = ap->hw_sata_spd_limit;
+
 		DPRINTK("ata%u: bus probe begin\n", ap->id);
 		rc = ata_bus_probe(ap);
 		DPRINTK("ata%u: bus probe end\n", ap->id);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 99808c5..07c98c2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -486,6 +486,7 @@ struct ata_port {
 	unsigned int		mwdma_mask;
 	unsigned int		udma_mask;
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
+	unsigned int		hw_sata_spd_limit;
 	unsigned int		sata_spd_limit;	/* SATA PHY speed limit */
 
 	/* record runtime error info, protected by host_set lock */
-- 
1.3.2

[PATCH 07/14] libata-hp-prep: store attached SCSI device

[Tejun Heo]

Add device persistent field dev->sdev and store the attached SCSI
device.  With hotplug, libata needs to know the attached SCSI device
to offline and detach it, but scsi_device_lookup() cannot be used
because libata will reuse SCSI ID numbers - dead but not gone devices
(due to zombie opens, etc...) interfere with the lookup.

dev->sdev doesn't hold reference to the SCSI device.  It's cleared
when the SCSI device goes away.

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

---

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

54beb5597d09c17687cc2a400c04a9ed1a28210f
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index b52591e..d24e091 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -2738,16 +2738,22 @@ void ata_scsi_simulate(struct ata_device
 
 void ata_scsi_scan_host(struct ata_port *ap)
 {
-	struct ata_device *dev;
 	unsigned int i;
 
 	if (ap->flags & ATA_FLAG_DISABLED)
 		return;
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
-		dev = &ap->device[i];
+		struct ata_device *dev = &ap->device[i];
+		struct scsi_device *sdev;
+
+		if (!ata_dev_enabled(dev) || dev->sdev)
+			continue;
 
-		if (ata_dev_enabled(dev))
-			scsi_scan_target(&ap->host->shost_gendev, 0, i, 0, 0);
+		sdev = __scsi_add_device(ap->host, 0, i, 0, NULL);
+		if (!IS_ERR(sdev)) {
+			dev->sdev = sdev;
+			scsi_device_put(sdev);
+		}
 	}
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 07c98c2..dbc9602 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -413,6 +413,7 @@ struct ata_device {
 	struct ata_port		*ap;
 	unsigned int		devno;		/* 0 or 1 */
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
+	struct scsi_device	*sdev;		/* attached SCSI device */
 	/* fields above n_sectors are not cleared across device init */
 	u64			n_sectors;	/* size of device, if ATA */
 	unsigned int		class;		/* ATA_DEV_xxx */
-- 
1.3.2

Re: [PATCH 07/14] libata-hp-prep: store attached SCSI device

[Jeff Garzik]

Tejun Heo wrote:
> Add device persistent field dev->sdev and store the attached SCSI
> device.  With hotplug, libata needs to know the attached SCSI device
> to offline and detach it, but scsi_device_lookup() cannot be used
> because libata will reuse SCSI ID numbers - dead but not gone devices
> (due to zombie opens, etc...) interfere with the lookup.
> 
> dev->sdev doesn't hold reference to the SCSI device.  It's cleared
> when the SCSI device goes away.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK, though begin to think about when SCSI simulation is made optional...

	Jeff

[PATCH 12/14] libata-hp-prep: implement ata_noop_check_status()

[Tejun Heo]

As with ops->tf_read, ops->check_status() and ops->check_altstatus()
don't make sense for controllers without single TF image.  As
->check_status() and ->check_altstatus() are deeply integrated into
libata core layer, implement ata_noop_check_status() as a temporary
measure.  This function always returns ATA_DRDY without doing
anything.

In the long term, these IO ops should be made optional and removed
from top level ata_port_operations such that driver implementing
high-level behavior don't have to bother with these.

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

---

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

89c675e09ffd6c3cdf7cc656152f6a1ea03533f0
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 9a4124e..72eef00 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -707,6 +707,24 @@ static u64 ata_id_n_sectors(const u16 *i
 }
 
 /**
+ *	ata_noop_check_status - Fake device status reg
+ *	@ap: target port
+ *
+ *	This function performs no actual function and always returns
+ *	ATA_DRDY.
+ *
+ *	May be used as the check_status/altstatus() entry in
+ *	ata_port_operations.
+ *
+ *	LOCKING:
+ *	caller.
+ */
+u8 ata_noop_check_status(struct ata_port *ap)
+{
+	return ATA_DRDY;
+}
+
+/**
  *	ata_noop_dev_select - Select device 0/1 on ATA bus
  *	@ap: ATA channel to manipulate
  *	@device: ATA device (numbered from zero) to select
@@ -5769,6 +5787,7 @@ EXPORT_SYMBOL_GPL(ata_qc_issue_prot);
 EXPORT_SYMBOL_GPL(ata_tf_load);
 EXPORT_SYMBOL_GPL(ata_tf_read);
 EXPORT_SYMBOL_GPL(ata_noop_dev_select);
+EXPORT_SYMBOL_GPL(ata_noop_check_status);
 EXPORT_SYMBOL_GPL(ata_std_dev_select);
 EXPORT_SYMBOL_GPL(ata_tf_to_fis);
 EXPORT_SYMBOL_GPL(ata_tf_from_fis);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 3d7c3e3..35f6d3b 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -665,6 +665,7 @@ extern void ata_tf_load(struct ata_port 
 extern void ata_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
 extern void ata_tf_to_fis(const struct ata_taskfile *tf, u8 *fis, u8 pmp);
 extern void ata_tf_from_fis(const u8 *fis, struct ata_taskfile *tf);
+extern u8 ata_noop_check_status(struct ata_port *ap);
 extern void ata_noop_dev_select (struct ata_port *ap, unsigned int device);
 extern void ata_std_dev_select (struct ata_port *ap, unsigned int device);
 extern u8 ata_check_status(struct ata_port *ap);
-- 
1.3.2

Re: [PATCH 12/14] libata-hp-prep: implement ata_noop_check_status()

[Jeff Garzik]

Tejun Heo wrote:
> As with ops->tf_read, ops->check_status() and ops->check_altstatus()
> don't make sense for controllers without single TF image.  As
> ->check_status() and ->check_altstatus() are deeply integrated into
> libata core layer, implement ata_noop_check_status() as a temporary
> measure.  This function always returns ATA_DRDY without doing
> anything.

NAK, read previous message for rationale

[PATCH 11/14] libata-hp-prep: make ops->tf_read() optional

[Tejun Heo]

Not all controllers have single TF image and ops->tf_read() is
inappropriate for them.  Newly implemented hotplug probing will allow
drivers for such controllers to get rid of ops->tf_read() completely.
Make ops->tf_read() optional in core layer.

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

---

 drivers/scsi/libata-core.c |    8 +++++---
 drivers/scsi/libata-eh.c   |    3 ++-
 2 files changed, 7 insertions(+), 4 deletions(-)

b27b1cc97dad12a344d60a184e94dff40c4ed15b
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 8f18ced..9a4124e 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -4428,14 +4428,15 @@ void ata_qc_complete(struct ata_queued_c
 		if (unlikely(qc->flags & ATA_QCFLAG_FAILED)) {
 			if (!ata_tag_internal(qc->tag)) {
 				/* always fill result TF for failed qc */
-				ap->ops->tf_read(ap, &qc->result_tf);
+				if (ap->ops->tf_read)
+					ap->ops->tf_read(ap, &qc->result_tf);
 				ata_qc_schedule_eh(qc);
 				return;
 			}
 		}
 
 		/* read result TF if requested */
-		if (qc->flags & ATA_QCFLAG_RESULT_TF)
+		if (ap->ops->tf_read && qc->flags & ATA_QCFLAG_RESULT_TF)
 			ap->ops->tf_read(ap, &qc->result_tf);
 
 		__ata_qc_complete(qc);
@@ -4444,7 +4445,8 @@ void ata_qc_complete(struct ata_queued_c
 			return;
 
 		/* read result TF if failed or requested */
-		if (qc->err_mask || qc->flags & ATA_QCFLAG_RESULT_TF)
+		if (ap->ops->tf_read &&
+		    (qc->err_mask || qc->flags & ATA_QCFLAG_RESULT_TF))
 			ap->ops->tf_read(ap, &qc->result_tf);
 
 		__ata_qc_complete(qc);
diff --git a/drivers/scsi/libata-eh.c b/drivers/scsi/libata-eh.c
index 71b45ad..7cbb311 100644
--- a/drivers/scsi/libata-eh.c
+++ b/drivers/scsi/libata-eh.c
@@ -806,7 +806,8 @@ static unsigned int atapi_eh_request_sen
 	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
 
 	/* XXX: why tf_read here? */
-	ap->ops->tf_read(ap, &tf);
+	if (ap->ops->tf_read)
+		ap->ops->tf_read(ap, &tf);
 
 	/* fill these in, for the case where they are -not- overwritten */
 	sense_buf[0] = 0x70;
-- 
1.3.2

Re: [PATCH 11/14] libata-hp-prep: make ops->tf_read() optional

[Jeff Garzik]

Tejun Heo wrote:
> Not all controllers have single TF image and ops->tf_read() is
> inappropriate for them.  Newly implemented hotplug probing will allow
> drivers for such controllers to get rid of ops->tf_read() completely.
> Make ops->tf_read() optional in core layer.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

NAK.

At the present time, the controller model expects to be able to obtain 
correct values from the shadow registers.  For FIS-based controllers, 
this may imply that the driver needs to maintain an accurate shadow 
register image, if the chip doesn't automatically supply one.

I think this is a quite reasonable expectation, given that ATA continues 
to have specifically defined shadow register outputs, and even in the 
face of NCQ, we have defined states where the shadow registers are 
available.

On this basis, attempts to make ->check_status() optional are also NAK'd.

	Jeff

Re: [PATCH 11/14] libata-hp-prep: make ops->tf_read() optional

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Not all controllers have single TF image and ops->tf_read() is
>> inappropriate for them.  Newly implemented hotplug probing will allow
>> drivers for such controllers to get rid of ops->tf_read() completely.
>> Make ops->tf_read() optional in core layer.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> NAK.
> 
> At the present time, the controller model expects to be able to obtain 
> correct values from the shadow registers.  For FIS-based controllers, 
> this may imply that the driver needs to maintain an accurate shadow 
> register image, if the chip doesn't automatically supply one.
> 
> I think this is a quite reasonable expectation, given that ATA continues 
> to have specifically defined shadow register outputs, and even in the 
> face of NCQ, we have defined states where the shadow registers are 
> available.
> 
> On this basis, attempts to make ->check_status() optional are also NAK'd.

The reasons why I did this are...

* For controllers which don't maintain shadow register image, the cached 
value gets out of sync easily and cause malfunction.  In some cases, the 
only way is to fake safe values.

* With generic TF access, LLDs cannot determine when TF will be accessed 
and why.  It must try to keep TF as up-to-date as possible.  e.g. sil24 
skips TF loading after a qc completion, this makes the cached TF go out 
of sync, which can easily cause problems depending on the previous 
cached value and how it will be used later.

* With PM, on both AHCI and sil24, each port's TF image must be 
cached/faked.  I think this would be true for many controllers.

* libata is heading toward high-level ops.  At least, that's the plan. 
With high-level qc_issue/complete interface, there's no reason for 
libata-core layer to access TF directly.

Currently, for FIS-based controllers, the only places where libata-core 
layer accesses TF are...

* waiting for !BSY.  This is often broken (cache TF gets out of sync, 
uncleared previous FIS value...)

* loading qc->result_tf when ATA_QCFLAG_RESULT_TF is set or the qc 
failed.  As noted above, I think it's best done in LLD completion routine.

I'll drop this and related patches from hotplug series, but I really 
don't want to cache/fake TFs for all ports of PM.  The cached values 
wouldn't be accurate, sometimes values will have to be faked to bypass 
TF accesses, and all those should be implemented in LLDs.

Thanks.

-- 
tejun

[PATCH 08/14] libata-hp-prep: add ata_hotplug_wq

[Tejun Heo]

It's best to run ATA hotplug from EH but attaching SCSI devices needs
working EH.  ata_hotplug_wq is used to give SCSI hotplug operations a
separate context.

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

---

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

5b942048c7f14c12bd0807da3cfbb2fa09925e1f
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 9e10b79..fba5514 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -69,6 +69,8 @@ static void ata_dev_xfermask(struct ata_
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
 
+struct workqueue_struct *ata_hotplug_wq;
+
 int atapi_enabled = 1;
 module_param(atapi_enabled, int, 0444);
 MODULE_PARM_DESC(atapi_enabled, "Enable discovery of ATAPI devices (0=off, 1=on)");
@@ -5588,6 +5590,12 @@ static int __init ata_init(void)
 	if (!ata_wq)
 		return -ENOMEM;
 
+	ata_hotplug_wq = create_singlethread_workqueue("ata_hotplug");
+	if (!ata_hotplug_wq) {
+		destroy_workqueue(ata_wq);
+		return -ENOMEM;
+	}
+
 	printk(KERN_DEBUG "libata version " DRV_VERSION " loaded.\n");
 	return 0;
 }
@@ -5595,6 +5603,7 @@ static int __init ata_init(void)
 static void __exit ata_exit(void)
 {
 	destroy_workqueue(ata_wq);
+	destroy_workqueue(ata_hotplug_wq);
 }
 
 module_init(ata_init);
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index d46c552..4a67675 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -39,6 +39,7 @@ struct ata_scsi_args {
 };
 
 /* libata-core.c */
+extern struct workqueue_struct *ata_hotplug_wq;
 extern int atapi_enabled;
 extern int atapi_dmadir;
 extern int libata_fua;
-- 
1.3.2

[PATCH 10/14] libata-hp-prep: implement sata_phy_debounce()

[Tejun Heo]

With hotplug, PHY always needs to be debounced before a reset as any
reset might find new devices.  Extract PHY waiting code from
sata_phy_resume() and extend it to include SStatus debouncing.  Note
that sata_phy_debounce() is superset of what used to be done inside
sata_phy_resume().

Three default debounce timing parameters are defined to be used by
hot/boot plug.  As resume failure during probing will be properly
handled as errors, timeout doesn't have to be long as before.
probeinit() uses the same timeout to retain the original behavior.

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

---

 drivers/scsi/libata-core.c |  104 ++++++++++++++++++++++++++++++++++++++------
 include/linux/libata.h     |    6 +++
 2 files changed, 95 insertions(+), 15 deletions(-)

fab1e57199f37d383773923278bc9ada10a3c60b
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 8b25cb5..8f18ced 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -61,6 +61,11 @@ #include <asm/byteorder.h>
 
 #include "libata.h"
 
+/* debounce timing parameters in msecs { interval, duration, timeout } */
+const unsigned long sata_deb_timing_boot[]		= {   5,  100, 2000 };
+const unsigned long sata_deb_timing_eh[]		= {  25,  500, 2000 };
+const unsigned long sata_deb_timing_before_fsrst[]	= { 100, 2000, 5000 };
+
 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);
@@ -2418,10 +2423,81 @@ err_out:
 	DPRINTK("EXIT\n");
 }
 
-static int sata_phy_resume(struct ata_port *ap)
+/**
+ *	sata_phy_debounce - debounce SATA phy status
+ *	@ap: ATA port to debounce SATA phy status for
+ *	@params: timing parameters { interval, duratinon, timeout } in msec
+ *
+ *	Make sure SStatus of @ap reaches stable state, determined by
+ *	holding the same value where DET is not 1 for @duration polled
+ *	every @interval, before @timeout.  Timeout constraints the
+ *	beginning of the stable state.  Because, after hot unplugging,
+ *	DET gets stuck at 1 on some controllers, this functions waits
+ *	until timeout then returns 0 if DET is stable at 1.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 on success, -errno on failure.
+ */
+int sata_phy_debounce(struct ata_port *ap, const unsigned long *params)
 {
-	unsigned long timeout = jiffies + (HZ * 5);
-	u32 scontrol, sstatus;
+	unsigned long interval_msec = params[0];
+	unsigned long duration = params[1] * HZ / 1000;
+	unsigned long timeout = jiffies + params[2] * HZ / 1000;
+	unsigned long last_jiffies;
+	u32 last, cur;
+	int rc;
+
+	if ((rc = sata_scr_read(ap, SCR_STATUS, &cur)))
+		return rc;
+	cur &= 0xf;
+
+	last = cur;
+	last_jiffies = jiffies;
+
+	while (1) {
+		msleep(interval_msec);
+		if ((rc = sata_scr_read(ap, SCR_STATUS, &cur)))
+			return rc;
+		cur &= 0xf;
+
+		/* DET stable? */
+		if (cur == last) {
+			if (cur == 1 && time_before(jiffies, timeout))
+				continue;
+			if (time_after(jiffies, last_jiffies + duration))
+				return 0;
+			continue;
+		}
+
+		/* unstable, start over */
+		last = cur;
+		last_jiffies = jiffies;
+
+		/* check timeout */
+		if (time_after(jiffies, timeout))
+			return -EBUSY;
+	}
+}
+
+/**
+ *	sata_phy_resume - resume SATA phy
+ *	@ap: ATA port to resume SATA phy for
+ *	@params: timing parameters { interval, duratinon, timeout } in msec
+ *
+ *	Resume SATA phy of @ap and debounce it.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 on success, -errno on failure.
+ */
+int sata_phy_resume(struct ata_port *ap, const unsigned long *params)
+{
+	u32 scontrol;
 	int rc;
 
 	if ((rc = sata_scr_read(ap, SCR_CONTROL, &scontrol)))
@@ -2432,16 +2508,7 @@ static int sata_phy_resume(struct ata_po
 	if ((rc = sata_scr_write(ap, SCR_CONTROL, scontrol)))
 		return rc;
 
-	/* Wait for phy to become ready, if necessary. */
-	do {
-		msleep(200);
-		if ((rc = sata_scr_read(ap, SCR_STATUS, &sstatus)))
-			return rc;
-		if ((sstatus & 0xf) != 1)
-			return 0;
-	} while (time_before(jiffies, timeout));
-
-	return -EBUSY;
+	return sata_phy_debounce(ap, params);
 }
 
 /**
@@ -2459,8 +2526,10 @@ static int sata_phy_resume(struct ata_po
  */
 void ata_std_probeinit(struct ata_port *ap)
 {
+	static const unsigned long deb_timing[] = { 5, 100, 5000 };
+
 	/* resume link */
-	sata_phy_resume(ap);
+	sata_phy_resume(ap, deb_timing);
 
 	/* wait for device */
 	if (ata_port_online(ap))
@@ -2576,7 +2645,7 @@ int sata_std_hardreset(struct ata_port *
 	msleep(1);
 
 	/* bring phy back */
-	sata_phy_resume(ap);
+	sata_phy_resume(ap, sata_deb_timing_eh);
 
 	/* TODO: phy layer with polling, timeouts, etc. */
 	if (ata_port_offline(ap)) {
@@ -5683,6 +5752,9 @@ u32 ata_wait_register(void __iomem *reg,
  * Do not depend on ABI/API stability.
  */
 
+EXPORT_SYMBOL_GPL(sata_deb_timing_boot);
+EXPORT_SYMBOL_GPL(sata_deb_timing_eh);
+EXPORT_SYMBOL_GPL(sata_deb_timing_before_fsrst);
 EXPORT_SYMBOL_GPL(ata_std_bios_param);
 EXPORT_SYMBOL_GPL(ata_std_ports);
 EXPORT_SYMBOL_GPL(ata_device_add);
@@ -5719,6 +5791,8 @@ EXPORT_SYMBOL_GPL(ata_bmdma_error_handle
 EXPORT_SYMBOL_GPL(ata_bmdma_post_internal_cmd);
 EXPORT_SYMBOL_GPL(ata_port_probe);
 EXPORT_SYMBOL_GPL(sata_set_spd);
+EXPORT_SYMBOL_GPL(sata_phy_debounce);
+EXPORT_SYMBOL_GPL(sata_phy_resume);
 EXPORT_SYMBOL_GPL(sata_phy_reset);
 EXPORT_SYMBOL_GPL(__sata_phy_reset);
 EXPORT_SYMBOL_GPL(ata_bus_reset);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index dbc9602..3d7c3e3 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -601,11 +601,17 @@ struct ata_timing {
 
 #define FIT(v,vmin,vmax)	max_t(short,min_t(short,v,vmax),vmin)
 
+extern const unsigned long sata_deb_timing_boot[];
+extern const unsigned long sata_deb_timing_eh[];
+extern const unsigned long sata_deb_timing_before_fsrst[];
+
 extern void ata_port_probe(struct ata_port *);
 extern void __sata_phy_reset(struct ata_port *ap);
 extern void sata_phy_reset(struct ata_port *ap);
 extern void ata_bus_reset(struct ata_port *ap);
 extern int sata_set_spd(struct ata_port *ap);
+extern int sata_phy_debounce(struct ata_port *ap, const unsigned long *param);
+extern int sata_phy_resume(struct ata_port *ap, const unsigned long *param);
 extern int ata_drive_probe_reset(struct ata_port *ap,
 			ata_probeinit_fn_t probeinit,
 			ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
-- 
1.3.2

Re: [PATCH 10/14] libata-hp-prep: implement sata_phy_debounce()

[Jeff Garzik]

Tejun Heo wrote:
> With hotplug, PHY always needs to be debounced before a reset as any
> reset might find new devices.  Extract PHY waiting code from
> sata_phy_resume() and extend it to include SStatus debouncing.  Note
> that sata_phy_debounce() is superset of what used to be done inside
> sata_phy_resume().
> 
> Three default debounce timing parameters are defined to be used by
> hot/boot plug.  As resume failure during probing will be properly
> handled as errors, timeout doesn't have to be long as before.
> probeinit() uses the same timeout to retain the original behavior.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK patches 8-10

[PATCH 14/14] libata-hp-prep: implement followup softreset handling

[Tejun Heo]

In some cases, hardreset must be followed by SRST.

* some controllers can't classify with hardreset
* some controllers can't wait for !BSY after hardreset (LLDD should
  explicitly request followup softreset by returning -EAGAIN)
* (later) PM needs SRST w/ PMP==15 to operate after hardreset

To handle above cases, this patch implements follow-up softreset.
After a hardreset, ata_eh_reset() checks whether any of above
conditions are met and do a follow-up softreset if necessary.

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

---

 drivers/scsi/libata-eh.c |   58 +++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 52 insertions(+), 6 deletions(-)

ce8c79e15ae26381f0e32b532a594c2f86037065
diff --git a/drivers/scsi/libata-eh.c b/drivers/scsi/libata-eh.c
index 4ec50d4..c76fab0 100644
--- a/drivers/scsi/libata-eh.c
+++ b/drivers/scsi/libata-eh.c
@@ -1281,16 +1281,28 @@ static void ata_eh_report(struct ata_por
 	}
 }
 
-static int ata_eh_reset(struct ata_port *ap,
+static int ata_eh_followup_srst_needed(int rc, int classify,
+				       const unsigned int *classes)
+{
+	if (rc == -EAGAIN)
+		return 1;
+	if (rc != 0)
+		return 0;
+	if (classify && classes[0] == ATA_DEV_UNKNOWN)
+		return 1;
+	return 0;
+}
+
+static int ata_eh_reset(struct ata_port *ap, int classify,
 			ata_prereset_fn_t prereset, ata_reset_fn_t softreset,
 			ata_reset_fn_t hardreset, ata_postreset_fn_t postreset)
 {
 	struct ata_eh_context *ehc = &ap->eh_context;
-	unsigned int classes[ATA_MAX_DEVICES];
+	unsigned int *classes = ehc->classes;
 	int tries = ATA_EH_RESET_TRIES;
 	unsigned int action;
 	ata_reset_fn_t reset;
-	int i, rc;
+	int i, did_followup_srst, rc;
 
 	/* Determine which reset to use and record in ehc->i.action.
 	 * prereset() may examine and modify it.
@@ -1344,10 +1356,44 @@ static int ata_eh_reset(struct ata_port 
 
 	rc = ata_do_reset(ap, reset, classes);
 
+	did_followup_srst = 0;
+	if (reset == hardreset &&
+	    ata_eh_followup_srst_needed(rc, classify, classes)) {
+		/* okay, let's do follow-up softreset */
+		did_followup_srst = 1;
+		reset = softreset;
+
+		if (!reset) {
+			ata_port_printk(ap, KERN_ERR,
+					"follow-up softreset required "
+					"but no softreset avaliable\n");
+			return -EINVAL;
+		}
+
+		ata_eh_about_to_do(ap, ATA_EH_RESET_MASK);
+		rc = ata_do_reset(ap, reset, classes);
+
+		if (rc == 0 && classify &&
+		    classes[0] == ATA_DEV_UNKNOWN) {
+			ata_port_printk(ap, KERN_ERR,
+					"classification failed\n");
+			return -EINVAL;
+		}
+	}
+
 	if (rc && --tries) {
+		const char *type;
+
+		if (reset == softreset) {
+			if (did_followup_srst)
+				type = "follow-up soft";
+			else
+				type = "soft";
+		} else
+			type = "hard";
+
 		ata_port_printk(ap, KERN_WARNING,
-				"%sreset failed, retrying in 5 secs\n",
-				reset == softreset ? "soft" : "hard");
+				"%sreset failed, retrying in 5 secs\n", type);
 		ssleep(5);
 
 		if (reset == hardreset)
@@ -1465,7 +1511,7 @@ static int ata_eh_recover(struct ata_por
 	if (ehc->i.action & ATA_EH_RESET_MASK) {
 		ata_eh_freeze_port(ap);
 
-		rc = ata_eh_reset(ap, prereset, softreset, hardreset,
+		rc = ata_eh_reset(ap, 0, prereset, softreset, hardreset,
 				  postreset);
 		if (rc) {
 			ata_port_printk(ap, KERN_ERR,
-- 
1.3.2

Re: [PATCH 14/14] libata-hp-prep: implement followup softreset handling

[Jeff Garzik]

Tejun Heo wrote:
> In some cases, hardreset must be followed by SRST.
> 
> * some controllers can't classify with hardreset
> * some controllers can't wait for !BSY after hardreset (LLDD should
>   explicitly request followup softreset by returning -EAGAIN)
> * (later) PM needs SRST w/ PMP==15 to operate after hardreset
> 
> To handle above cases, this patch implements follow-up softreset.
> After a hardreset, ata_eh_reset() checks whether any of above
> conditions are met and do a follow-up softreset if necessary.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

ACK

[PATCH 13/14] libata-hp-prep: add prereset() method and implement ata_std_prereset()

[Tejun Heo]

With hotplug, every reset might be a probing reset and thus something
similar to probe_init() is needed.  prereset() method is called before
a series of resets to a port and is the counterpart of postreset().
prereset() can tell EH to use different type of reset or skip reset by
modifying ehc->i.action.

This patch also implements ata_std_prereset().  Most controllers
should be able to use this function directly or with some wrapping.
After hotplug, different controllers need different actions to resume
the PHY and detect the newly attached device.  Controllers can be
categorized as follows.

* Controllers which can wait for the first FIS34 after hotplug.  Note
  that if the waiting is implemented by polling TF status, there needs
  to be a way to set BSY on PHY status change.  It can be implemented
  by hardware or with the help of the driver.

* Controllers which can wait for the first FIS34 after sending
  COMRESET.  These controllers need to issue COMRESET to wait for the
  first FIS.  Note that the received FIS34 could be the first FIS34
  after POR (power-on-reset) or FIS34 in response to the COMRESET.
  Some controllers use COMRESET as TF status synchronization point and
  clear TF automatically (sata_sil).

* Controllers which cannot wait for the first FIS34 reliably.  Blindly
  issuing SRST to spinning-up device often results in command issue
  failure or timeout, causing extended delay.  For these controllers,
  ata_std_prereset() explicitly waits ATA_SPINUP_WAIT (currently 8s)
  to give newly attached device time to spin up, then issues reset.
  Note that failing to getting ready in ATA_SPINUP_WAIT is not
  critical.  libata will retry.  So, the timeout needs to be long
  enough to spin up most devices.

LLDDs can tell ata_std_prereset() which of above action is needed with
ATA_FLAG_HRST_TO_RESUME and ATA_FLAG_CANT_WAIT_FIS34 flags.  These
flags are PHY-specific property and will be moved to ata_link later.

While at it, this patch unifies function typedef's such that they all
have named arguments.

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

---

 drivers/scsi/ahci.c         |    3 +-
 drivers/scsi/libata-bmdma.c |   11 ++++--
 drivers/scsi/libata-core.c  |   81 +++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/libata-eh.c    |   62 ++++++++++++++++++++++++++++-----
 drivers/scsi/sata_sil24.c   |    3 +-
 include/linux/libata.h      |   24 ++++++++++---
 6 files changed, 162 insertions(+), 22 deletions(-)

3d2d7d0fb0501519e0940f77303bdde24955aa79
diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
index 45fd71d..8493b02 100644
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -1026,7 +1026,8 @@ static void ahci_error_handler(struct at
 	}
 
 	/* perform recovery */
-	ata_do_eh(ap, ahci_softreset, ahci_hardreset, ahci_postreset);
+	ata_do_eh(ap, ata_std_prereset, ahci_softreset, ahci_hardreset,
+		  ahci_postreset);
 }
 
 static void ahci_post_internal_cmd(struct ata_queued_cmd *qc)
diff --git a/drivers/scsi/libata-bmdma.c b/drivers/scsi/libata-bmdma.c
index 6d30d2c..4bc0537 100644
--- a/drivers/scsi/libata-bmdma.c
+++ b/drivers/scsi/libata-bmdma.c
@@ -695,6 +695,7 @@ void ata_bmdma_thaw(struct ata_port *ap)
 /**
  *	ata_bmdma_drive_eh - Perform EH with given methods for BMDMA controller
  *	@ap: port to handle error for
+ *	@prereset: prereset method (can be NULL)
  *	@softreset: softreset method (can be NULL)
  *	@hardreset: hardreset method (can be NULL)
  *	@postreset: postreset method (can be NULL)
@@ -710,8 +711,9 @@ void ata_bmdma_thaw(struct ata_port *ap)
  *	LOCKING:
  *	Kernel thread context (may sleep)
  */
-void ata_bmdma_drive_eh(struct ata_port *ap, ata_reset_fn_t softreset,
-			ata_reset_fn_t hardreset, ata_postreset_fn_t postreset)
+void ata_bmdma_drive_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
+			ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
+			ata_postreset_fn_t postreset)
 {
 	struct ata_host_set *host_set = ap->host_set;
 	struct ata_eh_context *ehc = &ap->eh_context;
@@ -759,7 +761,7 @@ void ata_bmdma_drive_eh(struct ata_port 
 		ata_eh_thaw_port(ap);
 
 	/* PIO and DMA engines have been stopped, perform recovery */
-	ata_do_eh(ap, softreset, hardreset, postreset);
+	ata_do_eh(ap, prereset, softreset, hardreset, postreset);
 }
 
 /**
@@ -779,7 +781,8 @@ void ata_bmdma_error_handler(struct ata_
 	if (sata_scr_valid(ap))
 		hardreset = sata_std_hardreset;
 
-	ata_bmdma_drive_eh(ap, ata_std_softreset, hardreset, ata_std_postreset);
+	ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, hardreset,
+			   ata_std_postreset);
 }
 
 /**
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 72eef00..b2197b0 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -2529,6 +2529,86 @@ int sata_phy_resume(struct ata_port *ap,
 	return sata_phy_debounce(ap, params);
 }
 
+static void ata_wait_spinup(struct ata_port *ap)
+{
+	struct ata_eh_context *ehc = &ap->eh_context;
+	unsigned long end, secs;
+	int rc;
+
+	/* first, debounce */
+	rc = sata_phy_debounce(ap, sata_deb_timing_eh);
+
+	/* if debounced successfully and offline, no need to wait */
+	if (rc == 0 && ata_port_offline(ap))
+		return;
+
+	/* okay, let's give the drive time to spin up */
+	end = ehc->i.hotplug_timestamp + ATA_SPINUP_WAIT * HZ / 1000;
+	secs = ((end - jiffies) + HZ - 1) / HZ;
+
+	if (time_after(jiffies, end))
+		return;
+
+	if (secs > 5)
+		ata_port_printk(ap, KERN_INFO, "waiting for device to spin up "
+				"(%lu secs)\n", secs);
+
+	schedule_timeout_uninterruptible(end - jiffies);
+}
+
+/**
+ *	ata_std_prereset - prepare for reset
+ *	@ap: ATA port to be reset
+ *
+ *	@ap is about to be reset.  Initialize it.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ *
+ *	RETURNS:
+ *	0 on success, -errno otherwise.
+ */
+int ata_std_prereset(struct ata_port *ap)
+{
+	struct ata_eh_context *ehc = &ap->eh_context;
+	const unsigned long *timing;
+	int rc;
+
+	/* hotplug? */
+	if (ehc->i.flags & ATA_EHI_HOTPLUGGED) {
+		if (ap->flags & ATA_FLAG_HRST_TO_RESUME)
+			ehc->i.action |= ATA_EH_HARDRESET;
+		if (ap->flags & ATA_FLAG_CANT_WAIT_FIS34)
+			ata_wait_spinup(ap);
+	}
+
+	/* if we're about to do hardreset, nothing more to do */
+	if (ehc->i.action & ATA_EH_HARDRESET)
+		return 0;
+
+	/* resume port */
+	if (ap->flags & ATA_FLAG_LOADING)
+		timing = sata_deb_timing_boot;
+	else
+		timing = sata_deb_timing_eh;
+
+	rc = sata_phy_resume(ap, timing);
+	if (rc && rc != -EOPNOTSUPP) {
+		/* phy resume failed */
+		ata_port_printk(ap, KERN_WARNING, "failed to resume link "
+				"for reset (errno=%d)\n", rc);
+		return rc;
+	}
+
+	/* Wait for !BSY if the controller can wait for the first
+	 * FIS34 and we don't know that no device is attached.
+	 */
+	if (!(ap->flags & ATA_FLAG_CANT_WAIT_FIS34) && !ata_port_offline(ap))
+		ata_busy_sleep(ap, ATA_TMOUT_BOOT_QUICK, ATA_TMOUT_BOOT);
+
+	return 0;
+}
+
 /**
  *	ata_std_probeinit - initialize probing
  *	@ap: port to be probed
@@ -5818,6 +5898,7 @@ EXPORT_SYMBOL_GPL(sata_phy_reset);
 EXPORT_SYMBOL_GPL(__sata_phy_reset);
 EXPORT_SYMBOL_GPL(ata_bus_reset);
 EXPORT_SYMBOL_GPL(ata_std_probeinit);
+EXPORT_SYMBOL_GPL(ata_std_prereset);
 EXPORT_SYMBOL_GPL(ata_std_softreset);
 EXPORT_SYMBOL_GPL(sata_std_hardreset);
 EXPORT_SYMBOL_GPL(ata_std_postreset);
diff --git a/drivers/scsi/libata-eh.c b/drivers/scsi/libata-eh.c
index 7cbb311..4ec50d4 100644
--- a/drivers/scsi/libata-eh.c
+++ b/drivers/scsi/libata-eh.c
@@ -1281,20 +1281,58 @@ static void ata_eh_report(struct ata_por
 	}
 }
 
-static int ata_eh_reset(struct ata_port *ap, ata_reset_fn_t softreset,
+static int ata_eh_reset(struct ata_port *ap,
+			ata_prereset_fn_t prereset, ata_reset_fn_t softreset,
 			ata_reset_fn_t hardreset, ata_postreset_fn_t postreset)
 {
 	struct ata_eh_context *ehc = &ap->eh_context;
 	unsigned int classes[ATA_MAX_DEVICES];
 	int tries = ATA_EH_RESET_TRIES;
+	unsigned int action;
 	ata_reset_fn_t reset;
-	int rc;
+	int i, rc;
 
+	/* Determine which reset to use and record in ehc->i.action.
+	 * prereset() may examine and modify it.
+	 */
+	action = ehc->i.action;
+	ehc->i.action &= ~ATA_EH_RESET_MASK;
 	if (softreset && (!hardreset || (!sata_set_spd_needed(ap) &&
-					 !(ehc->i.action & ATA_EH_HARDRESET))))
-		reset = softreset;
+					 !(action & ATA_EH_HARDRESET))))
+		ehc->i.action |= ATA_EH_SOFTRESET;
 	else
+		ehc->i.action |= ATA_EH_HARDRESET;
+
+	if (prereset) {
+		rc = prereset(ap);
+		if (rc) {
+			ata_port_printk(ap, KERN_ERR,
+					"prereset failed (errno=%d)\n", rc);
+			return rc;
+		}
+	}
+
+	/* prereset() might have modified ehc->i.action */
+	if (ehc->i.action & ATA_EH_HARDRESET)
 		reset = hardreset;
+	else if (ehc->i.action & ATA_EH_SOFTRESET)
+		reset = softreset;
+	else {
+		/* prereset told us not to reset, bang classes and return */
+		for (i = 0; i < ATA_MAX_DEVICES; i++)
+			classes[i] = ATA_DEV_NONE;
+		return 0;
+	}
+
+	/* did prereset() screw up?  if so, fix up to avoid oopsing */
+	if (!reset) {
+		ata_port_printk(ap, KERN_ERR, "BUG: prereset() requested "
+				"invalid reset type\n");
+		if (softreset)
+			reset = softreset;
+		else
+			reset = hardreset;
+	}
 
  retry:
 	ata_port_printk(ap, KERN_INFO, "%s resetting port\n",
@@ -1381,6 +1419,7 @@ static int ata_port_nr_enabled(struct at
 /**
  *	ata_eh_recover - recover host port after error
  *	@ap: host port to recover
+ *	@prereset: prereset method (can be NULL)
  *	@softreset: softreset method (can be NULL)
  *	@hardreset: hardreset method (can be NULL)
  *	@postreset: postreset method (can be NULL)
@@ -1397,8 +1436,8 @@ static int ata_port_nr_enabled(struct at
  *	RETURNS:
  *	0 on success, -errno on failure.
  */
-static int ata_eh_recover(struct ata_port *ap, ata_reset_fn_t softreset,
-			  ata_reset_fn_t hardreset,
+static int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
+			  ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
 			  ata_postreset_fn_t postreset)
 {
 	struct ata_eh_context *ehc = &ap->eh_context;
@@ -1426,7 +1465,8 @@ static int ata_eh_recover(struct ata_por
 	if (ehc->i.action & ATA_EH_RESET_MASK) {
 		ata_eh_freeze_port(ap);
 
-		rc = ata_eh_reset(ap, softreset, hardreset, postreset);
+		rc = ata_eh_reset(ap, prereset, softreset, hardreset,
+				  postreset);
 		if (rc) {
 			ata_port_printk(ap, KERN_ERR,
 					"reset failed, giving up\n");
@@ -1543,6 +1583,7 @@ static void ata_eh_finish(struct ata_por
 /**
  *	ata_do_eh - do standard error handling
  *	@ap: host port to handle error for
+ *	@prereset: prereset method (can be NULL)
  *	@softreset: softreset method (can be NULL)
  *	@hardreset: hardreset method (can be NULL)
  *	@postreset: postreset method (can be NULL)
@@ -1552,11 +1593,12 @@ static void ata_eh_finish(struct ata_por
  *	LOCKING:
  *	Kernel thread context (may sleep).
  */
-void ata_do_eh(struct ata_port *ap, ata_reset_fn_t softreset,
-	       ata_reset_fn_t hardreset, ata_postreset_fn_t postreset)
+void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
+	       ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
+	       ata_postreset_fn_t postreset)
 {
 	ata_eh_autopsy(ap);
 	ata_eh_report(ap);
-	ata_eh_recover(ap, softreset, hardreset, postreset);
+	ata_eh_recover(ap, prereset, softreset, hardreset, postreset);
 	ata_eh_finish(ap);
 }
diff --git a/drivers/scsi/sata_sil24.c b/drivers/scsi/sata_sil24.c
index 4c76f05..26d7c54 100644
--- a/drivers/scsi/sata_sil24.c
+++ b/drivers/scsi/sata_sil24.c
@@ -912,7 +912,8 @@ static void sil24_error_handler(struct a
 	}
 
 	/* perform recovery */
-	ata_do_eh(ap, sil24_softreset, sil24_hardreset, ata_std_postreset);
+	ata_do_eh(ap, ata_std_prereset, sil24_softreset, sil24_hardreset,
+		  ata_std_postreset);
 }
 
 static void sil24_post_internal_cmd(struct ata_queued_cmd *qc)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 35f6d3b..f078c4c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -262,6 +262,15 @@ enum {
 	ATA_PROBE_MAX_TRIES	= 3,
 	ATA_EH_RESET_TRIES	= 3,
 	ATA_EH_DEV_TRIES	= 3,
+
+	/* Drive spinup time (time from power-on to the first FIS34)
+	 * in msecs - 8s currently.  Failing to get ready in this time
+	 * isn't critical.  It will result in reset failure for
+	 * controllers which can't wait for the first FIS34.  libata
+	 * will retry, so it just has to be long enough to spin up
+	 * most devices.
+	 */
+	ATA_SPINUP_WAIT		= 8000,
 };
 
 enum hsm_task_states {
@@ -294,9 +303,10 @@ struct ata_queued_cmd;
 
 /* typedefs */
 typedef void (*ata_qc_cb_t) (struct ata_queued_cmd *qc);
-typedef void (*ata_probeinit_fn_t)(struct ata_port *);
-typedef int (*ata_reset_fn_t)(struct ata_port *, unsigned int *);
-typedef void (*ata_postreset_fn_t)(struct ata_port *ap, unsigned int *);
+typedef void (*ata_probeinit_fn_t)(struct ata_port *ap);
+typedef int (*ata_prereset_fn_t)(struct ata_port *ap);
+typedef int (*ata_reset_fn_t)(struct ata_port *ap, unsigned int *classes);
+typedef void (*ata_postreset_fn_t)(struct ata_port *ap, unsigned int *classes);
 
 struct ata_ioports {
 	unsigned long		cmd_addr;
@@ -617,6 +627,7 @@ extern int ata_drive_probe_reset(struct 
 			ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
 			ata_postreset_fn_t postreset, unsigned int *classes);
 extern void ata_std_probeinit(struct ata_port *ap);
+extern int ata_std_prereset(struct ata_port *ap);
 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);
@@ -695,7 +706,7 @@ extern u8   ata_bmdma_status(struct ata_
 extern void ata_bmdma_irq_clear(struct ata_port *ap);
 extern void ata_bmdma_freeze(struct ata_port *ap);
 extern void ata_bmdma_thaw(struct ata_port *ap);
-extern void ata_bmdma_drive_eh(struct ata_port *ap,
+extern void ata_bmdma_drive_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
 			       ata_reset_fn_t softreset,
 			       ata_reset_fn_t hardreset,
 			       ata_postreset_fn_t postreset);
@@ -773,8 +784,9 @@ extern void ata_eh_thaw_port(struct ata_
 extern void ata_eh_qc_complete(struct ata_queued_cmd *qc);
 extern void ata_eh_qc_retry(struct ata_queued_cmd *qc);
 
-extern void ata_do_eh(struct ata_port *ap, ata_reset_fn_t softreset,
-		      ata_reset_fn_t hardreset, ata_postreset_fn_t postreset);
+extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
+		      ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
+		      ata_postreset_fn_t postreset);
 
 /*
  * printk helpers
-- 
1.3.2

Re: [PATCH 13/14] libata-hp-prep: add prereset() method and implement ata_std_prereset()

[Jeff Garzik]

Tejun Heo wrote:
> With hotplug, every reset might be a probing reset and thus something
> similar to probe_init() is needed.  prereset() method is called before
> a series of resets to a port and is the counterpart of postreset().
> prereset() can tell EH to use different type of reset or skip reset by
> modifying ehc->i.action.
> 
> This patch also implements ata_std_prereset().  Most controllers
> should be able to use this function directly or with some wrapping.
> After hotplug, different controllers need different actions to resume
> the PHY and detect the newly attached device.  Controllers can be
> categorized as follows.
> 
> * Controllers which can wait for the first FIS34 after hotplug.  Note
>   that if the waiting is implemented by polling TF status, there needs
>   to be a way to set BSY on PHY status change.  It can be implemented
>   by hardware or with the help of the driver.
> 
> * Controllers which can wait for the first FIS34 after sending
>   COMRESET.  These controllers need to issue COMRESET to wait for the
>   first FIS.  Note that the received FIS34 could be the first FIS34
>   after POR (power-on-reset) or FIS34 in response to the COMRESET.
>   Some controllers use COMRESET as TF status synchronization point and
>   clear TF automatically (sata_sil).
> 
> * Controllers which cannot wait for the first FIS34 reliably.  Blindly
>   issuing SRST to spinning-up device often results in command issue
>   failure or timeout, causing extended delay.  For these controllers,
>   ata_std_prereset() explicitly waits ATA_SPINUP_WAIT (currently 8s)
>   to give newly attached device time to spin up, then issues reset.
>   Note that failing to getting ready in ATA_SPINUP_WAIT is not
>   critical.  libata will retry.  So, the timeout needs to be long
>   enough to spin up most devices.
> 
> LLDDs can tell ata_std_prereset() which of above action is needed with
> ATA_FLAG_HRST_TO_RESUME and ATA_FLAG_CANT_WAIT_FIS34 flags.  These
> flags are PHY-specific property and will be moved to ata_link later.
> 
> While at it, this patch unifies function typedef's such that they all
> have named arguments.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/ahci.c         |    3 +-
>  drivers/scsi/libata-bmdma.c |   11 ++++--
>  drivers/scsi/libata-core.c  |   81 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/libata-eh.c    |   62 ++++++++++++++++++++++++++++-----
>  drivers/scsi/sata_sil24.c   |    3 +-
>  include/linux/libata.h      |   24 ++++++++++---
>  6 files changed, 162 insertions(+), 22 deletions(-)
> 
> 3d2d7d0fb0501519e0940f77303bdde24955aa79
> diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
> index 45fd71d..8493b02 100644
> --- a/drivers/scsi/ahci.c
> +++ b/drivers/scsi/ahci.c
> @@ -1026,7 +1026,8 @@ static void ahci_error_handler(struct at
>  	}
>  
>  	/* perform recovery */
> -	ata_do_eh(ap, ahci_softreset, ahci_hardreset, ahci_postreset);
> +	ata_do_eh(ap, ata_std_prereset, ahci_softreset, ahci_hardreset,
> +		  ahci_postreset);
>  }
>  
>  static void ahci_post_internal_cmd(struct ata_queued_cmd *qc)
> diff --git a/drivers/scsi/libata-bmdma.c b/drivers/scsi/libata-bmdma.c
> index 6d30d2c..4bc0537 100644
> --- a/drivers/scsi/libata-bmdma.c
> +++ b/drivers/scsi/libata-bmdma.c
> @@ -695,6 +695,7 @@ void ata_bmdma_thaw(struct ata_port *ap)
>  /**
>   *	ata_bmdma_drive_eh - Perform EH with given methods for BMDMA controller
>   *	@ap: port to handle error for
> + *	@prereset: prereset method (can be NULL)
>   *	@softreset: softreset method (can be NULL)
>   *	@hardreset: hardreset method (can be NULL)
>   *	@postreset: postreset method (can be NULL)
> @@ -710,8 +711,9 @@ void ata_bmdma_thaw(struct ata_port *ap)
>   *	LOCKING:
>   *	Kernel thread context (may sleep)
>   */
> -void ata_bmdma_drive_eh(struct ata_port *ap, ata_reset_fn_t softreset,
> -			ata_reset_fn_t hardreset, ata_postreset_fn_t postreset)
> +void ata_bmdma_drive_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
> +			ata_reset_fn_t softreset, ata_reset_fn_t hardreset,
> +			ata_postreset_fn_t postreset)
>  {
>  	struct ata_host_set *host_set = ap->host_set;
>  	struct ata_eh_context *ehc = &ap->eh_context;
> @@ -759,7 +761,7 @@ void ata_bmdma_drive_eh(struct ata_port 
>  		ata_eh_thaw_port(ap);
>  
>  	/* PIO and DMA engines have been stopped, perform recovery */
> -	ata_do_eh(ap, softreset, hardreset, postreset);
> +	ata_do_eh(ap, prereset, softreset, hardreset, postreset);
>  }
>  
>  /**
> @@ -779,7 +781,8 @@ void ata_bmdma_error_handler(struct ata_
>  	if (sata_scr_valid(ap))
>  		hardreset = sata_std_hardreset;
>  
> -	ata_bmdma_drive_eh(ap, ata_std_softreset, hardreset, ata_std_postreset);
> +	ata_bmdma_drive_eh(ap, ata_std_prereset, ata_std_softreset, hardreset,
> +			   ata_std_postreset);
>  }
>  
>  /**
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index 72eef00..b2197b0 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -2529,6 +2529,86 @@ int sata_phy_resume(struct ata_port *ap,
>  	return sata_phy_debounce(ap, params);
>  }
>  
> +static void ata_wait_spinup(struct ata_port *ap)
> +{
> +	struct ata_eh_context *ehc = &ap->eh_context;
> +	unsigned long end, secs;
> +	int rc;
> +
> +	/* first, debounce */
> +	rc = sata_phy_debounce(ap, sata_deb_timing_eh);

unconditionally calls a sata_xxx function, and so "sata_wait_spinup" 
naming is preferred.


> +	/* if debounced successfully and offline, no need to wait */
> +	if (rc == 0 && ata_port_offline(ap))
> +		return;

ISTR ata_port_offline being SATA-specific too, but anyway...  :)


> +	/* okay, let's give the drive time to spin up */
> +	end = ehc->i.hotplug_timestamp + ATA_SPINUP_WAIT * HZ / 1000;
> +	secs = ((end - jiffies) + HZ - 1) / HZ;
> +
> +	if (time_after(jiffies, end))
> +		return;
> +
> +	if (secs > 5)
> +		ata_port_printk(ap, KERN_INFO, "waiting for device to spin up "
> +				"(%lu secs)\n", secs);
> +
> +	schedule_timeout_uninterruptible(end - jiffies);

BTW:  Medium-term, re-read Hale Landis's ATADRVR probe code 
(http://www.ata-atapi.com/) to make sure the PATA probe path is still 
intact.

ACK everything else in this patch.

	Jeff

Re: [PATCH 13/14] libata-hp-prep: add prereset() method and implement ata_std_prereset()

[Tejun Heo]

Jeff Garzik wrote:
>> +static void ata_wait_spinup(struct ata_port *ap)
>> +{
>> +    struct ata_eh_context *ehc = &ap->eh_context;
>> +    unsigned long end, secs;
>> +    int rc;
>> +
>> +    /* first, debounce */
>> +    rc = sata_phy_debounce(ap, sata_deb_timing_eh);
> 
> unconditionally calls a sata_xxx function, and so "sata_wait_spinup" 
> naming is preferred.

Waiting for spinup is common operation for both PATA and SATA although I 
don't know how many PATA controllers support hotplug.  The following 
code pattern is used a lot to avoid explicit support testing in the 
caller.  ie. The function which uses optional controller feature is 
responsible for testing whether it is implemented by the controller.

rc = sata_xxx(args);
if (rc && rc != -EOPNOTSUPP) {
	/* handle error */
}

Also, if we rename this function to sata_wait_spinup() because it calls 
sata_*() unconditionally then ata_std_prereset() should be renamed to 
sata_std_prereset() as it calls sata_wait_spinup() unconditionally.

>> +    /* if debounced successfully and offline, no need to wait */
>> +    if (rc == 0 && ata_port_offline(ap))
>> +        return;
> 
> ISTR ata_port_offline being SATA-specific too, but anyway...  :)

ata_port_offline() applies to both PATA and SATA.  It's just that the 
current implmentation always returns 0 for PATA.  Well, I don't know. 
Maybe we should rename all those to sata_*() and rename them again when 
actual PATA support is implemented.

>> +    /* okay, let's give the drive time to spin up */
>> +    end = ehc->i.hotplug_timestamp + ATA_SPINUP_WAIT * HZ / 1000;
>> +    secs = ((end - jiffies) + HZ - 1) / HZ;
>> +
>> +    if (time_after(jiffies, end))
>> +        return;
>> +
>> +    if (secs > 5)
>> +        ata_port_printk(ap, KERN_INFO, "waiting for device to spin up "
>> +                "(%lu secs)\n", secs);
>> +
>> +    schedule_timeout_uninterruptible(end - jiffies);
> 
> BTW:  Medium-term, re-read Hale Landis's ATADRVR probe code 
> (http://www.ata-atapi.com/) to make sure the PATA probe path is still 
> intact.

Will do.

-- 
tejun

Re: [PATCH 13/14] libata-hp-prep: add prereset() method and implement ata_std_prereset()

[Alan Cox]

On Maw, 2006-05-23 at 07:44 -0700, Tejun Heo wrote:
> Waiting for spinup is common operation for both PATA and SATA although I 
> don't know how many PATA controllers support hotplug.

Most do, but few vendors ship the required extra buffers for many of the
chips. Unfortunately it means we have to add code for lots of chips each
with few hot/warmplug users 8(


> ata_port_offline() applies to both PATA and SATA.  It's just that the 
> current implmentation always returns 0 for PATA.  Well, I don't know. 
> Maybe we should rename all those to sata_*() and rename them again when 
> actual PATA support is implemented.
> 

What is ata_port_offline() intended to do ?

Re: [PATCH 13/14] libata-hp-prep: add prereset() method and implement ata_std_prereset()

[Tejun Heo]

Alan Cox wrote:
> On Maw, 2006-05-23 at 07:44 -0700, Tejun Heo wrote:
>> Waiting for spinup is common operation for both PATA and SATA although I 
>> don't know how many PATA controllers support hotplug.
> 
> Most do, but few vendors ship the required extra buffers for many of the
> chips. Unfortunately it means we have to add code for lots of chips each
> with few hot/warmplug users 8(
> 
> 
>> ata_port_offline() applies to both PATA and SATA.  It's just that the 
>> current implmentation always returns 0 for PATA.  Well, I don't know. 
>> Maybe we should rename all those to sata_*() and rename them again when 
>> actual PATA support is implemented.
>>
> 
> What is ata_port_offline() intended to do ?
> 

ata_port_online(@ap) returns 1 if @ap knows that one or more devices are
attached to it; otherwise, 0

ata_port_offline(@ap) returns 1 if @ap knows that no device is attached
to it; otherwise, 0

With later ata_link addition, above functions are changed to
ata_link_on/offline(@link) respectively.

-- 
tejun