prep for hotplug support, take 4

PATCH New Tejun Heo 3 participants 32 messages May 29, 2006 – May 30, 2006

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Tejun Heo

author

2006-05-29 06:26 UTC #0 lore raw

PATCH 1/3 prep for hotplug support, take 4

Hello,

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

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

* ATA_FLAG_CANT_WAIT_FIS34 renamed to ATA_FLAG_SKIP_D2H_BSY

* make-ops->tf_read()-optional patch killed

* implement-ata_noop_check_status() patch killed

* better comments for ATA_DEVICE_CLEAR_OFFSET

* ata_hotplug_wq renamed to ata_scsi_wq

* wrap calls to sata_*() from ata_*() under 'if (ap->cbl == SATA)' condition

This patchset is against

upstream (ef2824073fba9def3cf122e89cc485f66dd71f70)
+ [1] set-PIO-0-after-successful-EH-reset

Thanks.

--
tejun

[T] http://article.gmane.org/gmane.linux.ide/10891
[L] http://article.gmane.org/gmane.linux.ide/10546
[1] http://article.gmane.org/gmane.linux.ide/10890

Tejun Heo

author

2006-05-29 06:26 UTC #1 lore raw

PATCH 1/12 libata-hp-prep: add flags and eh_info/context fields for hotplug

Add hotplug related flags and eh_info/context fields.

Signed-off-by Tejun Heo <[email protected]>

---

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

5577bff1743157fb00b7499d832dd55c47245e3e

include/linux/libata.h +18 -1

index b0ee1c1..4f67c43 100644
@@ -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) */

@@ -152,6 +155,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_SKIP_D2H_BSY	= (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 */

@@ -159,6 +165,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) */

@@ -240,7 +249,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,

@@ -433,6 +444,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;
 };

@@ -440,6 +455,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 {

Signature · 1 line

1.3.2

Tejun Heo

author

2006-05-29 06:26 UTC #2 lore raw

PATCH 2/12 libata-hp-prep: implement ata_dev_init()

In reply to 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 <[email protected]>

---

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

1b63513233c45c44f359191b2e854e090f404e69

drivers/scsi/libata-core.c +21 -5

index 11df827..84afb59 100644
@@ -5142,6 +5142,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

@@ -5155,7 +5175,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)

@@ -5198,10 +5217,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

drivers/scsi/libata.h +1 -0

index b76ad7d..d46c552 100644
@@ -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);

Signature · 1 line

1.3.2

Tejun Heo

author

2006-05-29 06:26 UTC #3 lore raw

PATCH 3/12 libata-hp-prep: make some ata_device fields persistent

In reply to 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 <[email protected]>

---

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

a615dbf0ff4deac11925b7be6916caed19a66f3e

drivers/scsi/libata-core.c +12 -2

index 84afb59..fc9989d 100644
@@ -5153,9 +5153,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;

@@ -5217,6 +5226,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);
 	}

include/linux/libata.h +9 -2

index 4f67c43..20c6aa1 100644
@@ -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),

@@ -409,10 +410,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 */
+	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
+	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;

@@ -438,6 +440,11 @@ struct ata_device {
 	struct ata_ering	ering;
 };
 
+/* Offset into struct ata_device.  Fields above it are maintained
+ * acress device init.  Fields below are zeroed.
+ */
+#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 */

Signature · 1 line

1.3.2

Jeff Garzik

2006-05-30 04:00 UTC #4 lore raw

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

Quoted 7 lines · 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.

Quoted 5 lines

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 <[email protected]>

ACK 1-3

Tejun Heo

author

2006-05-29 06:26 UTC #5 lore raw

PATCH 4/12 libata-hp-prep: update ata_scsi_find_dev() and friends

In reply to 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 <[email protected]>

---

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

113b8fe60b97a6ce664d6098f23b56b75247de87

drivers/scsi/libata-scsi.c +25 ~~-15~~

index 9e5cb9f..9adc5c1 100644
@@ -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)) {

Signature · 1 line

1.3.2

zhao, forrest

2006-05-29 08:52 UTC #6 lore raw

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

Quoted 32 lines · Tejun Heo

113b8fe60b97a6ce664d6098f23b56b75247de87

drivers/scsi/libata-scsi.c +13 -2

index 9e5cb9f..9adc5c1 100644
@@ -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;
+}
+

Quoted 1 line

From my understanding, the definition of function ata_find_dev() should

be in libata-core.c instead of libata-scsi.c. Does it make sense to you?

Thanks,
Forrest

Tejun Heo

author

2006-05-29 09:09 UTC #7 lore raw

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

Quoted 10 lines

Quoted 9 lines
drivers/scsi/libata-scsi.c +7 -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;
+}
+
From my understanding, the definition of function ata_find_dev() should
be in libata-core.c instead of libata-scsi.c. Does it make sense to you?

Could be. However, as it's currently used only inside libata-scsi.c, I think we can leave it as static function for the time being.

Signature · 1 line

tejun

Jeff Garzik

2006-05-30 04:38 UTC #8 lore raw

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

In reply to zhao, forrest

Quoted 33 lines · forrest

Quoted 31 lines · Tejun Heo

113b8fe60b97a6ce664d6098f23b56b75247de87

drivers/scsi/libata-scsi.c +13 -2

index 9e5cb9f..9adc5c1 100644
@@ -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;
+}
+

Quoted 2 lines

Quoted 1 line
From my understanding, the definition of function ata_find_dev() should
be in libata-core.c instead of libata-scsi.c. Does it make sense to you?

You are correct from a modularity standpoint, but doing so will eliminate the possibility of the compiler noticing that this function -- a fast path function -- is small enough to inline. That would completely eliminate the function call overhead.

Jeff

Jeff Garzik

2006-05-30 04:02 UTC #9 lore raw

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

In reply to Tejun Heo

Quoted 6 lines · Tejun Heo

drivers/scsi/libata-scsi.c +5 -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;

drivers/scsi/libata-scsi.c static unsigned int atapi_xlat(struct at · line 2337

Quoted 10 lines

drivers/scsi/libata-scsi.c +0 -7

 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;

You replace a '<' test with a more complex test, which must then be modified when we start supporting Port Multipliers.

It is more simple, and easier, to keep the 'id < ATA_MAX_DEVICES' test AFAICS.

Jeff

Tejun Heo

author

2006-05-30 04:26 UTC #10 lore raw

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

Quoted 20 lines · Jeff Garzik

Quoted 10 lines · Tejun Heo

drivers/scsi/libata-scsi.c +5 -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)
{

drivers/scsi/libata-scsi.c static unsigned int atapi_xlat(struct at · line 2337

Quoted 7 lines

drivers/scsi/libata-scsi.c +0 -7

-    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;

Quoted 3 lines

You replace a '<' test with a more complex test, which must then be modified when we start supporting Port Multipliers.
It is more simple, and easier, to keep the 'id < ATA_MAX_DEVICES' test AFAICS.

The change is to catch user warm plug request for impossible devices. I think it's worth to change it two times as it makes user visible interface consistent.

Signature · 1 line

tejun

Jeff Garzik

2006-05-30 04:37 UTC #11 lore raw

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

Quoted 22 lines · Tejun Heo

Quoted 21 lines · Jeff Garzik

Quoted 18 lines · Tejun Heo

drivers/scsi/libata-scsi.c +3 -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)))

(quoted diff) +2 -0

+        return &ap->device[id];
+    return NULL;
 
 static struct ata_device *
 ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device

*scsidev)
{

drivers/scsi/libata-scsi.c +0 -7

-    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;

Quoted 5 lines

Quoted 3 lines
You replace a '<' test with a more complex test, which must then be modified when we start supporting Port Multipliers.
It is more simple, and easier, to keep the 'id < ATA_MAX_DEVICES' test AFAICS.
The change is to catch user warm plug request for impossible devices. I think it's worth to change it two times as it makes user visible interface consistent.

Warm plug isn't a fast path, but this is... I would rather filter out bad IDs elsewhere; and the code itself will catch bad IDs anyway, eventually.

Jeff

Tejun Heo

author

2006-05-30 04:43 UTC #12 lore raw

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

Quoted 23 lines · Jeff Garzik

Quoted 22 lines · Tejun Heo

Quoted 21 lines · Jeff Garzik

Quoted 18 lines · Tejun Heo

drivers/scsi/libata-scsi.c +3 -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)))

(quoted diff) +2 -0

+        return &ap->device[id];
+    return NULL;
 
 static struct ata_device *
 ata_scsi_find_dev(struct ata_port *ap, const struct scsi_device

*scsidev)
{

drivers/scsi/libata-scsi.c +0 -7

-    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;

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
You replace a '<' test with a more complex test, which must then be modified when we start supporting Port Multipliers.
It is more simple, and easier, to keep the 'id < ATA_MAX_DEVICES' test AFAICS.
The change is to catch user warm plug request for impossible devices. I think it's worth to change it two times as it makes user visible interface consistent.
Warm plug isn't a fast path, but this is... I would rather filter out bad IDs elsewhere; and the code itself will catch bad IDs anyway, eventually.

Okay, I'll drop the change.

Signature · 1 line

tejun

Tejun Heo

author

2006-05-29 06:26 UTC #13 lore raw

PATCH 5/12 libata-hp-prep: use __ata_scsi_find_dev()

In reply to Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>

---

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

bea7ebdc48efc94a29ada2a32a648d8e5e545427

drivers/scsi/libata-scsi.c +6 ~~-10~~

index 9adc5c1..95e5c23 100644
@@ -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 */
 }

Signature · 1 line

1.3.2

Tejun Heo

author

2006-05-29 06:26 UTC #14 lore raw

PATCH 6/12 libata-hp-prep: implement ap->hw_sata_spd_limit

In reply to 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 <[email protected]>

---

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

0329251e5f08a8110541cbd2e64adb29c340c5d7

drivers/scsi/libata-core.c +12 -9

index fc9989d..83fb9dd 100644
@@ -2466,17 +2466,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);

@@ -5155,6 +5147,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.

@@ -5210,7 +5205,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;
 	ap->msg_enable = ATA_MSG_DRV;

@@ -5374,10 +5369,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);

include/linux/libata.h +1 -0

index 20c6aa1..c42ea93 100644
@@ -488,6 +488,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 */

Signature · 1 line

1.3.2

Tejun Heo

author

2006-05-29 06:26 UTC #15 lore raw

PATCH 7/12 libata-hp-prep: store attached SCSI device

In reply to 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 <[email protected]>

---

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

16098cb84a40faee6726ff51371e9aa17e018a5f

drivers/scsi/libata-scsi.c +10 -4

index 95e5c23..a1caf3f 100644
@@ -2743,16 +2743,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);
+		}
 	}
 }

include/linux/libata.h +1 -0

index c42ea93..54b2259 100644
@@ -412,6 +412,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 */
 	/* n_sector is used as CLEAR_OFFSET, read comment above CLEAR_OFFSET */
 	u64			n_sectors;	/* size of device, if ATA */
 	unsigned int		class;		/* ATA_DEV_xxx */

Signature · 1 line

1.3.2

zhao, forrest

2006-05-29 08:57 UTC #16 lore raw

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

Quoted 11 lines · Tejun Heo

drivers/scsi/libata-scsi.c +0 -1

@@ -2743,16 +2743,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++) {

drivers/scsi/libata-scsi.c void ata_scsi_simulate(struct ata_device · line 2758

Quoted 10 lines

drivers/scsi/libata-scsi.c +6 -3

-		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);

Is it better to use macro scsi_add_device() than invoking __scsi_add_device() directly here? I know this is only a trivial change.

drivers/scsi/libata-scsi.c void ata_scsi_simulate(struct ata_device · line 2764

Quoted 6 lines

drivers/scsi/libata-scsi.c +4 -0

+		if (!IS_ERR(sdev)) {
+			dev->sdev = sdev;
+			scsi_device_put(sdev);
+		}
 	}
 }

Thanks,
Forrest

Tejun Heo

author

2006-05-29 09:14 UTC #17 lore raw

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

Quoted 19 lines · forrest

Quoted 18 lines · Tejun Heo

drivers/scsi/libata-scsi.c +5 -2

@@ -2743,16 +2743,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;

Quoted 5 lines

drivers/scsi/libata-scsi.c void ata_scsi_simulate(struct ata_device · line 2758
Quoted 3 lines
drivers/scsi/libata-scsi.c +1 -2
-		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);
Is it better to use macro scsi_add_device() than invoking __scsi_add_device() directly here? I know this is only a trivial change.

Again, could be. The change is mostly cosmetic. I like the current form because it explicitly signifies that libata doesn't hold reference to sdev.

Signature · 1 line

tejun

Jeff Garzik

2006-05-30 04:04 UTC #18 lore raw

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

In reply to Tejun Heo

Quoted 2 lines · Tejun Heo

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

ACK patches 5-7, though I consider the above quoted statement somewhat questionable... _why_ not hold a reference?

Jeff

Tejun Heo

author

2006-05-30 04:55 UTC #19 lore raw

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

Quoted 5 lines · Jeff Garzik

Quoted 2 lines · Tejun Heo
dev->sdev doesn't hold reference to the SCSI device. It's cleared when the SCSI device goes away.
ACK patches 5-7, though I consider the above quoted statement somewhat questionable... _why_ not hold a reference?

libata shouldn't affect the lifetime of the scsi device. When the sdev disappears, dev->sdev should go too. It's easy to carry an extra reference but it wouldn't mean anything - the reference will be increased on attach and should be decreased with the device lifetime reference, so I thought it would be better to make it explicit that libata doesn't affect sdev's lifetime by not holding any extra reference to it.

Signature · 1 line

tejun

Tejun Heo

author

2006-05-29 06:26 UTC #20 lore raw

PATCH 8/12 libata-hp-prep: add ata_scsi_wq

In reply to Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>

---

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

84fc45de6b3fc32e8310a27dd54fa455a1038e9b

drivers/scsi/libata-core.c +9 -0

index 83fb9dd..aa1e0a1 100644
@@ -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_scsi_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)");

@@ -5622,6 +5624,12 @@ static int __init ata_init(void)
 	if (!ata_wq)
 		return -ENOMEM;
 
+	ata_scsi_wq = create_singlethread_workqueue("ata_scsi");
+	if (!ata_scsi_wq) {
+		destroy_workqueue(ata_wq);
+		return -ENOMEM;
+	}
+
 	printk(KERN_DEBUG "libata version " DRV_VERSION " loaded.\n");
 	return 0;
 }

@@ -5629,6 +5637,7 @@ static int __init ata_init(void)
 static void __exit ata_exit(void)
 {
 	destroy_workqueue(ata_wq);
+	destroy_workqueue(ata_scsi_wq);
 }
 
 module_init(ata_init);

drivers/scsi/libata.h +1 -0

index d46c552..d4bb2e4 100644
@@ -39,6 +39,7 @@ struct ata_scsi_args {
 };
 
 /* libata-core.c */
+extern struct workqueue_struct *ata_scsi_wq;
 extern int atapi_enabled;
 extern int atapi_dmadir;
 extern int libata_fua;

Signature · 1 line

1.3.2

Tejun Heo

author

2006-05-29 06:26 UTC #21 lore raw

PATCH 9/12 libata-hp-prep: make probing related functions global

In reply to 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 <[email protected]>

---

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

a0fe7eb0b738cb74416f362a956e8593bfd3a0c0

drivers/scsi/libata-core.c +3 -3

index aa1e0a1..009db45 100644
@@ -1167,8 +1167,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;

@@ -1292,7 +1292,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;

drivers/scsi/libata.h +3 -0

index d4bb2e4..e3f7406 100644
@@ -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);

Signature · 1 line

1.3.2

Jeff Garzik

2006-05-30 04:06 UTC #22 lore raw

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

Quoted 4 lines · 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 <[email protected]>

ACK 8-9

Tejun Heo

author

2006-05-29 06:26 UTC #23 lore raw

PATCH 10/12 libata-hp-prep: implement sata_phy_debounce()

In reply to 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 <[email protected]>

---

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

c9aae0f91110060a799f70706bd06ec0dab80cb0

drivers/scsi/libata-core.c +89 ~~-15~~

index 009db45..9763395 100644
@@ -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);

@@ -2427,10 +2432,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)))

@@ -2441,16 +2517,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);
 }
 
 /**

@@ -2468,8 +2535,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))

@@ -2585,7 +2654,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)) {

@@ -5717,6 +5786,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);

@@ -5756,6 +5828,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);

include/linux/libata.h +6 -0

index 54b2259..838b11a 100644
@@ -605,11 +605,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,

Signature · 1 line

1.3.2

Jeff Garzik

NAK

2006-05-30 04:10 UTC #24 lore raw

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

Quoted 81 lines · Tejun Heo

Signed-off-by Tejun Heo <[email protected]>

---

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

c9aae0f91110060a799f70706bd06ec0dab80cb0

drivers/scsi/libata-core.c +42 -3

index 009db45..9763395 100644
@@ -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);

@@ -2427,10 +2432,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;

drivers/scsi/libata-core.c err_out: · line 2482

Quoted 10 lines

drivers/scsi/libata-core.c +10 -0

+		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;
+		}

NAK, changes behavior. We want to preserve the old order of operations because some PHYs (sata_via and sata_mv come to mind) are a bit sensitive to being pounded immediately after wakeup.

Jeff

Tejun Heo

author

2006-05-30 05:08 UTC #25 lore raw

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

Quoted 7 lines · Jeff Garzik

Quoted 6 lines · 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().

Quoted 8 lines

Quoted 6 lines
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 <[email protected]>
NAK, changes behavior. We want to preserve the old order of operations because some PHYs (sata_via and sata_mv come to mind) are a bit sensitive to being pounded immediately after wakeup.

The following is what the original sata_phy_resume() did

1. read SControl
2. write SControl (wake)
3. msleep(200)
4. read SStatus
5. if condition not met, goto #3

The new code does...

1. read SControl
2. write SControl (wake)
3. read SStatus
4. msleep(interval_msec)
5. read SStatus
6. if condition not met, goto #4

How about putting msleep(200) before calling sata_phy_debounce() from sata_phy_resume(). This will add msleep(200) between #2 and #3 and the only difference left would be the duration of interval_msec in #4 which is 5ms during boot, 25ms during EH. The wait is in the millisecs range and it's difficult to imagine the change would cause any problem.

Signature · 1 line

tejun

Jeff Garzik

2006-05-30 05:23 UTC #26 lore raw

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

Quoted 27 lines · Tejun Heo

Quoted 7 lines · Jeff Garzik
Quoted 6 lines · 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().
Quoted 8 lines
Quoted 6 lines
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 <[email protected]>
NAK, changes behavior. We want to preserve the old order of operations because some PHYs (sata_via and sata_mv come to mind) are a bit sensitive to being pounded immediately after wakeup.
The following is what the original sata_phy_resume() did
1. read SControl
2. write SControl (wake)
3. msleep(200)
4. read SStatus
5. if condition not met, goto #3
The new code does...

Quoted 8 lines

1. read SControl
2. write SControl (wake)
3. read SStatus
4. msleep(interval_msec)
5. read SStatus
6. if condition not met, goto #4
How about putting msleep(200) before calling sata_phy_debounce() from sata_phy_resume(). This will add msleep(200) between #2 and #3 and the only difference left would be the duration of interval_msec in #4 which is 5ms during boot, 25ms during EH. The wait is in the millisecs range and it's difficult to imagine the change would cause any problem.

As long as the delay following #2 remains, I'm happy...

Jeff

Tejun Heo

author

2006-05-29 06:26 UTC #27 lore raw

PATCH 11/12 libata-hp-prep: add prereset() method and implement ata_std_prereset()

In reply to 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 <[email protected]>

---

 drivers/scsi/ahci.c         |    3 +-
 drivers/scsi/libata-bmdma.c |   11 ++++--
 drivers/scsi/libata-core.c  |   85 +++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/libata-eh.c    |   60 ++++++++++++++++++++++++++----
 drivers/scsi/sata_sil24.c   |    3 +-
 include/linux/libata.h      |   24 +++++++++---
 6 files changed, 165 insertions(+), 21 deletions(-)

232f49110f635dc8d95966eaa556d37a87cf7992

drivers/scsi/ahci.c +2 -1

index 45fd71d..8493b02 100644
@@ -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)

drivers/scsi/libata-bmdma.c +7 -4

index 6d30d2c..4bc0537 100644
@@ -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);
 }
 
 /**

drivers/scsi/libata-core.c +85 -0

97 lines not on this page · raw message

drivers/scsi/libata-eh.c +51 -9

109 lines not on this page · raw message

drivers/scsi/sata_sil24.c +2 -1

index 4c76f05..26d7c54 100644
@@ -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)

include/linux/libata.h +18 -6

index 838b11a..8822340 100644
@@ -261,6 +261,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 {

@@ -293,9 +302,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;

@@ -621,6 +631,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);

@@ -704,7 +715,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);

@@ -782,8 +793,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

Signature · 1 line

1.3.2

Tejun Heo

author

2006-05-29 11:12 UTC #28 lore raw

Re: [PATCH 11/12] (updated) libata-hp-prep: add prereset() method and implement ata_std_prereset()

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

Signed-off-by Tejun Heo <[email protected]>

---

Last minute change broke error condition check in ata_std_prereset() and ata_wait_spinup(). This is the updated patch. git tree has been updated accordingly. No other patch is affected.

 drivers/scsi/ahci.c         |    3 +-
 drivers/scsi/libata-bmdma.c |   11 ++++--
 drivers/scsi/libata-core.c  |   85 +++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/libata-eh.c    |   60 ++++++++++++++++++++++++++----
 drivers/scsi/sata_sil24.c   |    3 +-
 include/linux/libata.h      |   24 +++++++++---
 6 files changed, 165 insertions(+), 21 deletions(-)

44c3cba6ef79db2eb102894cfd73cd49cff37e05

drivers/scsi/ahci.c +2 -1

index 45fd71d..8493b02 100644
@@ -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)

drivers/scsi/libata-bmdma.c +7 -4

index 6d30d2c..4bc0537 100644
@@ -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);
 }
 
 /**

drivers/scsi/libata-core.c +85 -0

97 lines not on this page · raw message

drivers/scsi/libata-eh.c +51 -9

109 lines not on this page · raw message

drivers/scsi/sata_sil24.c +2 -1

index 4c76f05..26d7c54 100644
@@ -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)

include/linux/libata.h +18 -6

index 838b11a..8822340 100644
@@ -261,6 +261,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 {

@@ -293,9 +302,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;

@@ -621,6 +631,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);

@@ -704,7 +715,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);

@@ -782,8 +793,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

Signature · 1 line

1.3.2

Jeff Garzik

2006-05-30 04:12 UTC #29 lore raw

Re: [PATCH 11/12] (updated) libata-hp-prep: add prereset() method and implement ata_std_prereset()

Quoted 33 lines · 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.

Quoted 8 lines

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 <[email protected]>

ACK, provided that you banish all mentions of "FIS34" from the patch description.

"D2H FIS" is far more understandable to the average human.

Jeff

Tejun Heo

author

2006-05-29 06:26 UTC #30 lore raw

PATCH 12/12 libata-hp-prep: implement followup softreset handling

In reply to 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 <[email protected]>

---

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

a92b5a9913a41dbf5ef816da38b015a4319b54ea

drivers/scsi/libata-eh.c +52 -6

index ab2c640..4189bb4 100644
@@ -1280,16 +1280,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.

@@ -1343,10 +1355,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)

@@ -1470,7 +1516,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,

Signature · 1 line

1.3.2

Jeff Garzik

2006-05-30 04:12 UTC #31 lore raw

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

Quoted 7 lines · 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

Quoted 5 lines

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 <[email protected]>

ACK