[PATCH 1/3] libsas: modify SATA error handler

Hi,

How about the patch?

Date: 2014-04-24 21:27 GMT+08:00
Subject: [PATCH 1/3] libsas: modify SATA error handler
Add support for SATA port softreset and port multiplier error
handling.

---
=A0
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Williams

2014-06-03 16:05:03 UTC

Post by Xiangliang Yu
Hi,
How about the patch?

I'll take a look today. Sorry for the latency.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Xiangliang Yu

2014-08-06 10:50:07 UTC

Hi, Dan & James
How about the patches about support for PM?
Two months had passed since I submitted the patches.
Thanks!

-----Original Message-----
From: Dan Williams [mailto:***@intel.com]
Sent: 2014年6月4日 0:05
To: Xiangliang Yu
Cc: ***@kernel.org; ***@parallels.com; ***@intel.com; ***@intel.com; linux-***@vger.kernel.org; linux-***@vger.kernel.org; linux-***@vger.kernel.org
Subject: Re: [PATCH 1/3] libsas: modify SATA error handler

Post by Xiangliang Yu
Hi,
How about the patch?

I'll take

Dan Williams

2014-08-06 17:21:50 UTC

Post by Xiangliang Yu
Hi, Dan & James
How about the patches about support for PM?
Two months had passed since I submitted the patches.
Thanks!

Did you address my review comments?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Xiangliang Yu

2014-08-07 01:50:48 UTC

Hi, Dan
I haven't receive your review comments, could you send it to me again, thanks!
PS: I can't login my gmail, so please send mail to this count.

-----Original Message-----
From: Dan Williams [mailto:***@intel.com]
Sent: 2014年8月7日 1:22
To: Xiangliang Yu
Cc: ***@parallels.com; ***@kernel.org; ***@intel.com; ***@intel.com; linux-***@vger.kernel.org; linux-***@vger.kernel.org; linux-***@vger.kernel.org
Subject: Re: [PATCH 1/3] libsas: modify SATA error handler

Post by Xiangliang Yu
Hi, Dan & James
How about the patches about support for PM?
Two months had passed since I submitted the patches.
Thanks!

Did you address my review comments?
N��r��y��b�X��ǧv�^�)޺{.n�+��{��ע��^n�r��z��h��&��G��h�

Dan Williams

2014-06-04 01:41:10 UTC

Post by Xiangliang Yu
Add support for SATA port softreset and port multiplier error
handling.

Some more detailed notes about the approach and any caveats would be
appreciated.

Post by Xiangliang Yu
---
drivers/scsi/libsas/sas_ata.c | 226 ++++++++++++++++++++++++++++++++++++++++-
include/scsi/libsas.h | 6 +
2 files changed, 231 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 766098a..29a19fd 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -442,6 +442,226 @@ static int sas_ata_hard_reset(struct ata_link *link, unsigned int *class,
return ret;
}
+static void sas_ata_freeze(struct ata_port *ap)
+{
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+
+ if (i->dft->lldd_dev_freeze)
+ i->dft->lldd_dev_freeze(dev);
+}
+
+static void sas_ata_thaw(struct ata_port *ap)
+{
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+
+ if (i->dft->lldd_dev_thaw)
+ i->dft->lldd_dev_thaw(dev);
+}
+
+static int sas_ata_wait_task_done(struct sas_task *task, unsigned long timeout,
+ int (*check_done)(struct sas_task *task))
+{

Why do we need a custom check_done() routine? Since we have a
sas_task we can use the normal completion infrastructure. See
smp_execute_task().

Post by Xiangliang Yu
+ struct ata_port *ap = task->uldd_task;
+ unsigned long deadline;
+ int done;
+
+ if (!check_done) {
+ SAS_DPRINTK("check function is null.\n");
+ return -1;
+ }
+
+ deadline = ata_deadline(jiffies, timeout);
+ done = check_done(task);
+
+ while (done && time_before(jiffies, deadline)) {
+ ata_msleep(ap, 1);
+
+ done = check_done(task);

This can simply be:

completion_done(&task->slow_task->completion)

Post by Xiangliang Yu
+ }
+
+ return done;
+}
+
+static int sas_ata_exec_polled_cmd(struct ata_port *ap, struct ata_taskfile *tf,
+ int pmp, unsigned long timeout)
+{
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+ struct sas_task *task = NULL;
+ int ret = -1;
+
+ if (!i->dft->lldd_execute_task) {
+ SAS_DPRINTK("execute function is null.\n");
+ return ret;
+ }
+
+ task = sas_alloc_task(GFP_ATOMIC);

I think this can be downgraded to GFP_NOIO. We're in a sleepable context.

Post by Xiangliang Yu
+ if (!task) {
+ SAS_DPRINTK("failed to alloc sas task.\n");
+ goto fail;
+ }
+
+ task->dev = dev;
+ task->task_proto = SAS_PROTOCOL_SATA;
+ task->uldd_task = ap;
+
+ ata_tf_to_fis(tf, pmp, 0, (u8 *)&task->ata_task.fis);
+ task->ata_task.retry_count = 1;
+ task->task_state_flags = SAS_TASK_STATE_PENDING;
+ task->task_state_flags |= SAS_TASK_NEED_DEV_RESET;
+
+ ret = i->dft->lldd_execute_task(task, 1, GFP_ATOMIC);

Same here.

Post by Xiangliang Yu
+ if (ret) {
+ SAS_DPRINTK("failed to send internal task.\n");
+ goto fail;
+ }
+
+ if (timeout) {
+ ret = sas_ata_wait_task_done(task, timeout,
+ i->dft->lldd_wait_task_done);
+ if (ret) {
+ SAS_DPRINTK("get wrong status.\n");
+ goto fail;
+ }
+ }
+ list_del_init(&task->list);
+ sas_free_task(task);
+
+ return 0;
+ if (task) {
+ list_del_init(&task->list);
+ sas_free_task(task);
+ }
+
+ return ret;
+}
+
+static int sas_ata_soft_reset(struct ata_link *link, unsigned int *class,
+ unsigned long deadline)
+{
+ struct ata_taskfile tf;
+ struct ata_port *ap = link->ap;
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+ struct sas_phy *phy;
+ unsigned long now, msecs;
+ unsigned int pmp;
+ int ret = -1;
+ int (*check_ready)(struct ata_link *link);
+
+ phy = sas_get_local_phy(dev);
+ if (scsi_is_sas_phy_local(phy))
+ check_ready = local_ata_check_ready;
+ else
+ check_ready = smp_ata_check_ready;
+ sas_put_local_phy(phy);

Does this attempt to support expander attached port multiplier's?
Last I checked expanders do not support this so we should just fail
this for non-local phys.

Post by Xiangliang Yu
+
+ pmp = sata_srst_pmp(link);
+
+ msecs = 0;
+ now = jiffies;
+ if (time_after(deadline, now))
+ msecs = jiffies_to_msecs(deadline - now);
+
+ memset(&tf, 0, sizeof(struct ata_taskfile));
+ tf.ctl = ATA_SRST;
+ tf.device = ATA_DEVICE_OBS;
+
+ if (sas_ata_exec_polled_cmd(ap, &tf, pmp, msecs)) {
+ ret = -EIO;
+ goto fail;
+ }
+
+ tf.ctl &= ~ATA_SRST;
+ sas_ata_exec_polled_cmd(ap, &tf, pmp, msecs);

What about lldd's that do not know how to handle ATA_SRST? I think we
need preparation patches to make SRST support opt-in, or teach all
lldds how to handle these SRST sas_tasks.

Post by Xiangliang Yu
+
+ ret = ata_wait_after_reset(link, deadline, check_ready);
+ if (ret) {
+ SAS_DPRINTK("device is not ready.\n");
+ ret = -EIO;
+ goto fail;
+ } else {
+ if (i->dft->lldd_dev_classify)
+ *class = i->dft->lldd_dev_classify(dev);
+ }
+
+ return 0;
+
+ SAS_DPRINTK("failed to reset.\n");
+ return ret;
+}
+
+/* According sata 3.0 spec 13.15.4.2, enable the device port */
+static int sas_ata_pmp_hard_reset(struct ata_link *link, unsigned int *class,
+ unsigned long deadline)
+{
+ struct ata_port *ap = link->ap;
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+ int ret = -1;
+ u32 scontrol = 0;
+
+ set_bit(SAS_DEV_RESET, &dev->state);
+
+ ret = sata_scr_read(link, SCR_CONTROL, &scontrol);

I think a comment is needed clarifying that these reads generate
sas_tasks to a pmp and are not trying to read/write local SCR
registers that do not exist on a SAS hba.

Post by Xiangliang Yu
+ if (ret)
+ goto error;
+
+ /* enable device port */
+ scontrol = 0x1;
+ ret = sata_scr_write(link, SCR_CONTROL, scontrol);
+ if (ret)
+ goto error;
+
+ ata_msleep(ap, 1);
+
+ scontrol = 0x0;
+ ret = sata_scr_write(link, SCR_CONTROL, scontrol);
+ if (ret)
+ goto error;
+
+ ata_msleep(ap, 10);
+
+ /* check device link status */
+ if (ata_link_offline(link)) {
+ SAS_DPRINTK("link is offline.\n");
+ goto error;
+ }
+
+ /* clear X bit */
+ scontrol = 0xFFFFFFFF;
+ ret = sata_scr_write(link, SCR_ERROR, scontrol);
+ if (ret)
+ goto error;
+
+ /* may be need to wait for device sig */
+ ata_msleep(ap, 3);
+
+ /* check device class */
+ if (i->dft->lldd_dev_classify)
+ *class = i->dft->lldd_dev_classify(dev);
+
+ return 0;
+
+ SAS_DPRINTK("failed to hard reset.\n");
+ return ret;
+}
+
/*
* notify the lldd to forget the sas_task for this internal ata command
* that bypasses scsi-eh
@@ -551,8 +771,12 @@ void sas_ata_end_eh(struct ata_port *ap)
static struct ata_port_operations sas_sata_ops = {
.prereset = ata_std_prereset,
.hardreset = sas_ata_hard_reset,
+ .softreset = sas_ata_soft_reset,
+ .pmp_hardreset = sas_ata_pmp_hard_reset,
+ .freeze = sas_ata_freeze,
+ .thaw = sas_ata_thaw,
.postreset = ata_std_postreset,
- .error_handler = ata_std_error_handler,
+ .error_handler = sata_pmp_error_handler,
.post_internal_cmd = sas_ata_post_internal,
.qc_defer = ata_std_qc_defer,
.qc_prep = ata_noop_qc_prep,
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index ef7872c..a26466a 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -689,6 +689,12 @@ struct sas_domain_function_template {
/* GPIO support */
int (*lldd_write_gpio)(struct sas_ha_struct *, u8 reg_type,
u8 reg_index, u8 reg_count, u8 *write_data);
+
+ /* ATA EH functions */
+ void (*lldd_dev_freeze)(struct domain_device *);

Why do we need to pass this all the way through to the lldd? Can we
get away with emulating this in sas_ata.c.

Post by Xiangliang Yu
+ void (*lldd_dev_thaw)(struct domain_device *);

Same note as lldd_dev_freeze

Post by Xiangliang Yu
+ int (*lldd_wait_task_done)(struct sas_task *);

Should not be needed.

Post by Xiangliang Yu
+ int (*lldd_dev_classify)(struct domain_device *);

I think this belongs in a different patch set. If we solve device
re-classification for soft reset we need to do the same for the
sas_ata_hard_reset path.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Williams

2014-08-07 23:54:01 UTC

Post by Xiangliang Yu
Add support for SATA port softreset and port multiplier error
handling.

Some more detailed notes about the approach and any caveats would be
appreciated.

Why do we need a custom check_done() routine? Since we have a
sas_task we can use the normal completion infrastructure. See
smp_execute_task().

completion_done(&task->slow_task->completion)

I think this can be downgraded to GFP_NOIO. We're in a sleepable context.

Same here.

Does this attempt to support expander attached port multiplier's?
Last I checked expanders do not support this so we should just fail
this for non-local phys.

What about lldd's that do not know how to handle ATA_SRST? I think we
need preparation patches to make SRST support opt-in, or teach all
lldds how to handle these SRST sas_tasks.

I think a comment is needed clarifying that these reads generate
sas_tasks to a pmp and are not trying to read/write local SCR
registers that do not exist on a SAS hba.

Why do we need to pass this all the way through to the lldd? Can we
get away with emulating this in sas_ata.c.

Post by Xiangliang Yu
+ void (*lldd_dev_thaw)(struct domain_device *);

Same note as lldd_dev_freeze

Post by Xiangliang Yu
+ int (*lldd_wait_task_done)(struct sas_task *);

Should not be needed.

Post by Xiangliang Yu
+ int (*lldd_dev_classify)(struct domain_device *);

I think this belongs in a different patch set. If we solve device
re-classification for soft reset we need to do the same for the
sas_ata_hard_reset path.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Xiangliang Yu

2014-08-08 09:46:17 UTC

Hi, Dan

Post by Xiangliang Yu
Add support for SATA port softreset and port multiplier error
handling.

Some more detailed notes about the approach and any caveats would be
appreciated.

Post by Xiangliang Yu
---
drivers/scsi/libsas/sas_ata.c | 226 ++++++++++++++++++++++++++++++++++++++++-
include/scsi/libsas.h | 6 +
2 files changed, 231 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c
b/drivers/scsi/libsas/sas_ata.c index 766098a..29a19fd 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -442,6 +442,226 @@ static int sas_ata_hard_reset(struct ata_link *link, unsigned int *class,
return ret;
}
+static void sas_ata_freeze(struct ata_port *ap) {
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+
+ if (i->dft->lldd_dev_freeze)
+ i->dft->lldd_dev_freeze(dev); }
+
+static void sas_ata_thaw(struct ata_port *ap) {
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+
+ if (i->dft->lldd_dev_thaw)
+ i->dft->lldd_dev_thaw(dev); }
+
+static int sas_ata_wait_task_done(struct sas_task *task, unsigned long timeout,
+ int (*check_done)(struct sas_task *task)) {

Why do we need a custom check_done() routine? Since we have a
sas_task we can use the normal completion infrastructure. See
smp_execute_task().

You are right, I'll change it

completion_done(&task->slow_task->completion)

Yes

Post by Xiangliang Yu
+ }
+
+ return done;
+}
+
+static int sas_ata_exec_polled_cmd(struct ata_port *ap, struct ata_taskfile *tf,
+ int pmp, unsigned long timeout) {
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+ struct sas_task *task = NULL;
+ int ret = -1;
+
+ if (!i->dft->lldd_execute_task) {
+ SAS_DPRINTK("execute function is null.\n");
+ return ret;
+ }
+
+ task = sas_alloc_task(GFP_ATOMIC);

I think this can be downgraded to GFP_NOIO. We're in a sleepable context.

Yes, got it

Same here.

Post by Xiangliang Yu
+ if (ret) {
+ SAS_DPRINTK("failed to send internal task.\n");
+ goto fail;
+ }
+
+ if (timeout) {
+ ret = sas_ata_wait_task_done(task, timeout,
+ i->dft->lldd_wait_task_done);
+ if (ret) {
+ SAS_DPRINTK("get wrong status.\n");
+ goto fail;
+ }
+ }
+ list_del_init(&task->list);
+ sas_free_task(task);
+
+ return 0;
+ if (task) {
+ list_del_init(&task->list);
+ sas_free_task(task);
+ }
+
+ return ret;
+}
+
+static int sas_ata_soft_reset(struct ata_link *link, unsigned int *class,
+ unsigned long deadline) {
+ struct ata_taskfile tf;
+ struct ata_port *ap = link->ap;
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+ struct sas_phy *phy;
+ unsigned long now, msecs;
+ unsigned int pmp;
+ int ret = -1;
+ int (*check_ready)(struct ata_link *link);
+
+ phy = sas_get_local_phy(dev);
+ if (scsi_is_sas_phy_local(phy))
+ check_ready = local_ata_check_ready;
+ else
+ check_ready = smp_ata_check_ready;
+ sas_put_local_phy(phy);

Does this attempt to support expander attached port multiplier's?
Last I checked expanders do not support this so we should just fail
this for non-local phys.

Sorry, I copy it from hard_reset function, and I'll clear the code;

What about lldd's that do not know how to handle ATA_SRST? I think we
need preparation patches to make SRST support opt-in, or teach all
lldds how to handle these SRST sas_tasks.

I think lldds have different actions to handle SRST because there is no unified standard.
But it should be easy to support it.
Later, I'll submit a mvsas patch to show how to support it.

Post by Xiangliang Yu
+
+ ret = ata_wait_after_reset(link, deadline, check_ready);
+ if (ret) {
+ SAS_DPRINTK("device is not ready.\n");
+ ret = -EIO;
+ goto fail;
+ } else {
+ if (i->dft->lldd_dev_classify)
+ *class = i->dft->lldd_dev_classify(dev);
+ }
+
+ return 0;
+
+ SAS_DPRINTK("failed to reset.\n");
+ return ret;
+}
+
+/* According sata 3.0 spec 13.15.4.2, enable the device port */
+static int sas_ata_pmp_hard_reset(struct ata_link *link, unsigned int *class,
+ unsigned long deadline) {
+ struct ata_port *ap = link->ap;
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+ int ret = -1;
+ u32 scontrol = 0;
+
+ set_bit(SAS_DEV_RESET, &dev->state);
+
+ ret = sata_scr_read(link, SCR_CONTROL, &scontrol);

I think a comment is needed clarifying that these reads generate
sas_tasks to a pmp and are not trying to read/write local SCR
registers that do not exist on a SAS hba.

I think the situation can't happen here.

Post by Xiangliang Yu
+ if (ret)
+ goto error;
+
+ /* enable device port */
+ scontrol = 0x1;
+ ret = sata_scr_write(link, SCR_CONTROL, scontrol);
+ if (ret)
+ goto error;
+
+ ata_msleep(ap, 1);
+
+ scontrol = 0x0;
+ ret = sata_scr_write(link, SCR_CONTROL, scontrol);
+ if (ret)
+ goto error;
+
+ ata_msleep(ap, 10);
+
+ /* check device link status */
+ if (ata_link_offline(link)) {
+ SAS_DPRINTK("link is offline.\n");
+ goto error;
+ }
+
+ /* clear X bit */
+ scontrol = 0xFFFFFFFF;
+ ret = sata_scr_write(link, SCR_ERROR, scontrol);
+ if (ret)
+ goto error;
+
+ /* may be need to wait for device sig */
+ ata_msleep(ap, 3);
+
+ /* check device class */
+ if (i->dft->lldd_dev_classify)
+ *class = i->dft->lldd_dev_classify(dev);
+
+ return 0;
+
+ SAS_DPRINTK("failed to hard reset.\n");
+ return ret;
+}
+
/*
* notify the lldd to forget the sas_task for this internal ata command
* that bypasses scsi-eh
@@ -551,8 +771,12 @@ void sas_ata_end_eh(struct ata_port *ap) static
struct ata_port_operations sas_sata_ops = {
.prereset = ata_std_prereset,
.hardreset = sas_ata_hard_reset,
+ .softreset = sas_ata_soft_reset,
+ .pmp_hardreset = sas_ata_pmp_hard_reset,
+ .freeze = sas_ata_freeze,
+ .thaw = sas_ata_thaw,
.postreset = ata_std_postreset,
- .error_handler = ata_std_error_handler,
+ .error_handler = sata_pmp_error_handler,
.post_internal_cmd = sas_ata_post_internal,
.qc_defer = ata_std_qc_defer,
.qc_prep = ata_noop_qc_prep,
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index
ef7872c..a26466a 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -689,6 +689,12 @@ struct sas_domain_function_template {
/* GPIO support */
int (*lldd_write_gpio)(struct sas_ha_struct *, u8 reg_type,
u8 reg_index, u8 reg_count, u8
*write_data);
+
+ /* ATA EH functions */
+ void (*lldd_dev_freeze)(struct domain_device *);

Why do we need to pass this all the way through to the lldd? Can we
get away with emulating this in sas_ata.c.

Because SAS HBAs spec haven't a unified standard, not like AHCI.
But I think we can delete the interface because we don't disable interrupt
during EH now.

Post by Xiangliang Yu
+ void (*lldd_dev_thaw)(struct domain_device *);

Same note as lldd_dev_freeze

Post by Xiangliang Yu
+ int (*lldd_wait_task_done)(struct sas_task *);

Should not be needed.

Post by Xiangliang Yu
+ int (*lldd_dev_classify)(struct domain_device *);

I think this belongs in a different patch set. If we solve device
re-classification for soft reset we need to do the same for the
sas_ata_hard_reset path.

Could you explain more details? Thanks!
��칻�&�~�&��+-��ݶ��w��˛��m�b��bu觶��ܨ}��Ơz�&j:+v��zZ+

Dan Williams

2014-08-25 16:02:27 UTC

Some more comments below.

[..]

What about lldd's that do not know how to handle ATA_SRST? I think we
need preparation patches to make SRST support opt-in, or teach all
lldds how to handle these SRST sas_tasks.

I think lldds have different actions to handle SRST because there is no unified standard.
But it should be easy to support it.
Later, I'll submit a mvsas patch to show how to support it.

Right, but what about the other lldd's? Libsas needs to check whether
the lldd supports SRST before attempting to submit.

[..]

Post by Xiangliang Yu
+/* According sata 3.0 spec 13.15.4.2, enable the device port */
+static int sas_ata_pmp_hard_reset(struct ata_link *link, unsigned int *class,
+ unsigned long deadline) {
+ struct ata_port *ap = link->ap;
+ struct domain_device *dev = ap->private_data;
+ struct sas_ha_struct *sas_ha = dev->port->ha;
+ struct Scsi_Host *host = sas_ha->core.shost;
+ struct sas_internal *i = to_sas_internal(host->transportt);
+ int ret = -1;
+ u32 scontrol = 0;
+
+ set_bit(SAS_DEV_RESET, &dev->state);
+
+ ret = sata_scr_read(link, SCR_CONTROL, &scontrol);

I think a comment is needed clarifying that these reads generate
sas_tasks to a pmp and are not trying to read/write local SCR
registers that do not exist on a SAS hba.

I think the situation can't happen here.

Right, that's why we need a comment, because by normally libsas lldd's
do not support scr-register accesses.

Post by Xiangliang Yu
+ if (ret)
+ goto error;
+
+ /* enable device port */
+ scontrol = 0x1;
+ ret = sata_scr_write(link, SCR_CONTROL, scontrol);
+ if (ret)
+ goto error;
+
+ ata_msleep(ap, 1);
+
+ scontrol = 0x0;
+ ret = sata_scr_write(link, SCR_CONTROL, scontrol);
+ if (ret)
+ goto error;
+
+ ata_msleep(ap, 10);
+
+ /* check device link status */
+ if (ata_link_offline(link)) {
+ SAS_DPRINTK("link is offline.\n");
+ goto error;
+ }
+
+ /* clear X bit */
+ scontrol = 0xFFFFFFFF;
+ ret = sata_scr_write(link, SCR_ERROR, scontrol);
+ if (ret)
+ goto error;
+
+ /* may be need to wait for device sig */
+ ata_msleep(ap, 3);
+
+ /* check device class */
+ if (i->dft->lldd_dev_classify)
+ *class = i->dft->lldd_dev_classify(dev);
+
+ return 0;
+
+ SAS_DPRINTK("failed to hard reset.\n");
+ return ret;
+}
+
/*
* notify the lldd to forget the sas_task for this internal ata command
* that bypasses scsi-eh
@@ -551,8 +771,12 @@ void sas_ata_end_eh(struct ata_port *ap) static
struct ata_port_operations sas_sata_ops = {
.prereset = ata_std_prereset,
.hardreset = sas_ata_hard_reset,
+ .softreset = sas_ata_soft_reset,
+ .pmp_hardreset = sas_ata_pmp_hard_reset,
+ .freeze = sas_ata_freeze,
+ .thaw = sas_ata_thaw,
.postreset = ata_std_postreset,
- .error_handler = ata_std_error_handler,
+ .error_handler = sata_pmp_error_handler,
.post_internal_cmd = sas_ata_post_internal,
.qc_defer = ata_std_qc_defer,
.qc_prep = ata_noop_qc_prep,
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index
ef7872c..a26466a 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -689,6 +689,12 @@ struct sas_domain_function_template {
/* GPIO support */
int (*lldd_write_gpio)(struct sas_ha_struct *, u8 reg_type,
u8 reg_index, u8 reg_count, u8 *write_data);
+
+ /* ATA EH functions */
+ void (*lldd_dev_freeze)(struct domain_device *);

Why do we need to pass this all the way through to the lldd? Can we
get away with emulating this in sas_ata.c.

Because SAS HBAs spec haven't a unified standard, not like AHCI.
But I think we can delete the interface because we don't disable interrupt
during EH now.

Ok.