* [PATCH 1/3] libsas: modify SATA error handler
@ 2014-04-24 13:27 Xiangliang Yu
[not found] ` <CAP0P+NWxxxnQH1e3tXcqwud7EaV4kRRMW46Ad-PWoMWFzgy1eg@mail.gmail.com>
2014-06-04 1:41 ` Dan Williams
0 siblings, 2 replies; 13+ messages in thread
From: Xiangliang Yu @ 2014-04-24 13:27 UTC (permalink / raw)
To: tj, JBottomley
Cc: dan.j.williams, todd.e.brandt, lukasz.dorau, linux-ide,
linux-kernel, linux-scsi, Xiangliang Yu
Add support for SATA port softreset and port multiplier error
handling.
Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
---
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))
+{
+ 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);
+ }
+
+ 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);
+ 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);
+ 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;
+fail:
+ 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);
+
+ 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);
+
+ 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;
+
+fail:
+ 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);
+ 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;
+
+error:
+ 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 *);
+ void (*lldd_dev_thaw)(struct domain_device *);
+ int (*lldd_wait_task_done)(struct sas_task *);
+ int (*lldd_dev_classify)(struct domain_device *);
};
extern int sas_register_ha(struct sas_ha_struct *);
--
1.7.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] libsas: modify SATA error handler
[not found] ` <CAP0P+NWxxxnQH1e3tXcqwud7EaV4kRRMW46Ad-PWoMWFzgy1eg@mail.gmail.com>
@ 2014-06-03 7:13 ` Xiangliang Yu
2014-06-03 16:05 ` Dan Williams
0 siblings, 1 reply; 13+ messages in thread
From: Xiangliang Yu @ 2014-06-03 7:13 UTC (permalink / raw)
To: tj, JBottomley
Cc: dan.j.williams, todd.e.brandt, lukasz.dorau, linux-ide,
linux-kernel, linux-scsi
Hi,
How about the patch?
>From: Xiangliang Yu <yxlraid@gmail.com>
>Date: 2014-04-24 21:27 GMT+08:00
>Subject: [PATCH 1/3] libsas: modify SATA error handler
>To: tj@kernel.org, JBottomley@parallels.com
>Cc: dan.j.williams@intel.com, todd.e.brandt@intel.com, >lukasz.dorau@intel.com, linux-ide@vger.kernel.org, linux->kernel@vger.kernel.org, linux-scsi@vger.kernel.org, Xiangliang Yu ><yxlraid@gmail.com>
>
>Add support for SATA port softreset and port multiplier error
>handling.
>
>Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
---
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] libsas: modify SATA error handler
2014-06-03 7:13 ` Xiangliang Yu
@ 2014-06-03 16:05 ` Dan Williams
2014-08-06 10:50 ` Xiangliang Yu
0 siblings, 1 reply; 13+ messages in thread
From: Dan Williams @ 2014-06-03 16:05 UTC (permalink / raw)
To: Xiangliang Yu
Cc: tj, JBottomley, todd.e.brandt, lukasz.dorau, linux-ide,
linux-kernel, linux-scsi
On Tue, Jun 3, 2014 at 12:13 AM, Xiangliang Yu <yuxiangl@marvell.com> wrote:
> Hi,
>
> How about the patch?
>
I'll take a look today. Sorry for the latency.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] libsas: modify SATA error handler
2014-04-24 13:27 [PATCH 1/3] libsas: modify SATA error handler Xiangliang Yu
[not found] ` <CAP0P+NWxxxnQH1e3tXcqwud7EaV4kRRMW46Ad-PWoMWFzgy1eg@mail.gmail.com>
@ 2014-06-04 1:41 ` Dan Williams
2014-08-07 23:54 ` Dan Williams
1 sibling, 1 reply; 13+ messages in thread
From: Dan Williams @ 2014-06-04 1:41 UTC (permalink / raw)
To: Xiangliang Yu
Cc: Tejun Heo, James Bottomley, Todd E Brandt, Lukasz Dorau,
IDE/ATA development list, linux-kernel, linux-scsi
Hi, some notes below:
On Thu, Apr 24, 2014 at 6:27 AM, Xiangliang Yu <yxlraid@gmail.com> wrote:
> Add support for SATA port softreset and port multiplier error
> handling.
Some more detailed notes about the approach and any caveats would be
appreciated.
>
> Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
> ---
> 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().
> + 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)
> + }
> +
> + 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.
> + 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.
> + 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;
> +fail:
> + 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.
> +
> + 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.
> +
> + 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;
> +
> +fail:
> + 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.
> + 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;
> +
> +error:
> + 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.
> + void (*lldd_dev_thaw)(struct domain_device *);
Same note as lldd_dev_freeze
> + int (*lldd_wait_task_done)(struct sas_task *);
Should not be needed.
> + 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] libsas: modify SATA error handler
2014-06-03 16:05 ` Dan Williams
@ 2014-08-06 10:50 ` Xiangliang Yu
0 siblings, 0 replies; 13+ messages in thread
From: Xiangliang Yu @ 2014-08-06 10:50 UTC (permalink / raw)
To: Dan Williams
Cc: tj, JBottomley, todd.e.brandt, lukasz.dorau, linux-ide,
linux-kernel, linux-scsi
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:dan.j.williams@intel.com]
Sent: 2014年6月4日 0:05
To: Xiangliang Yu
Cc: tj@kernel.org; JBottomley@parallels.com; todd.e.brandt@intel.com; lukasz.dorau@intel.com; linux-ide@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
Subject: Re: [PATCH 1/3] libsas: modify SATA error handler
On Tue, Jun 3, 2014 at 12:13 AM, Xiangliang Yu <yuxiangl@marvell.com> wrote:
> Hi,
>
> How about the patch?
>
I'll take a look today. Sorry for the latency.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] libsas: modify SATA error handler
@ 2014-08-06 10:50 ` Xiangliang Yu
0 siblings, 0 replies; 13+ messages in thread
From: Xiangliang Yu @ 2014-08-06 10:50 UTC (permalink / raw)
To: Dan Williams, JBottomley
Cc: tj, JBottomley, todd.e.brandt, lukasz.dorau, linux-ide,
linux-kernel, linux-scsi
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 807 bytes --]
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:dan.j.williams@intel.com]
Sent: 2014å¹´6æ4æ¥ 0:05
To: Xiangliang Yu
Cc: tj@kernel.org; JBottomley@parallels.com; todd.e.brandt@intel.com; lukasz.dorau@intel.com; linux-ide@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
Subject: Re: [PATCH 1/3] libsas: modify SATA error handler
On Tue, Jun 3, 2014 at 12:13 AM, Xiangliang Yu <yuxiangl@marvell.com> wrote:
> Hi,
>
> How about the patch?
>
I'll take a look today. Sorry for the latency.
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] libsas: modify SATA error handler
2014-08-06 10:50 ` Xiangliang Yu
(?)
@ 2014-08-06 17:21 ` Dan Williams
2014-08-07 1:50 ` Xiangliang Yu
-1 siblings, 1 reply; 13+ messages in thread
From: Dan Williams @ 2014-08-06 17:21 UTC (permalink / raw)
To: Xiangliang Yu
Cc: JBottomley, tj, todd.e.brandt, lukasz.dorau, linux-ide,
linux-kernel, linux-scsi
On Wed, Aug 6, 2014 at 3:50 AM, Xiangliang Yu <yuxiangl@marvell.com> wrote:
> 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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] libsas: modify SATA error handler
2014-08-06 17:21 ` Dan Williams
@ 2014-08-07 1:50 ` Xiangliang Yu
0 siblings, 0 replies; 13+ messages in thread
From: Xiangliang Yu @ 2014-08-07 1:50 UTC (permalink / raw)
To: Dan Williams
Cc: JBottomley, tj, todd.e.brandt, lukasz.dorau, linux-ide,
linux-kernel, linux-scsi
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:dan.j.williams@intel.com]
Sent: 2014年8月7日 1:22
To: Xiangliang Yu
Cc: JBottomley@parallels.com; tj@kernel.org; todd.e.brandt@intel.com; lukasz.dorau@intel.com; linux-ide@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
Subject: Re: [PATCH 1/3] libsas: modify SATA error handler
On Wed, Aug 6, 2014 at 3:50 AM, Xiangliang Yu <yuxiangl@marvell.com> wrote:
> 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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] libsas: modify SATA error handler
@ 2014-08-07 1:50 ` Xiangliang Yu
0 siblings, 0 replies; 13+ messages in thread
From: Xiangliang Yu @ 2014-08-07 1:50 UTC (permalink / raw)
To: Dan Williams
Cc: JBottomley, tj, todd.e.brandt, lukasz.dorau, linux-ide,
linux-kernel, linux-scsi
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 920 bytes --]
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:dan.j.williams@intel.com]
Sent: 2014å¹´8æ7æ¥ 1:22
To: Xiangliang Yu
Cc: JBottomley@parallels.com; tj@kernel.org; todd.e.brandt@intel.com; lukasz.dorau@intel.com; linux-ide@vger.kernel.org; linux-kernel@vger.kernel.org; linux-scsi@vger.kernel.org
Subject: Re: [PATCH 1/3] libsas: modify SATA error handler
On Wed, Aug 6, 2014 at 3:50 AM, Xiangliang Yu <yuxiangl@marvell.com> wrote:
> 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Ç+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] libsas: modify SATA error handler
2014-06-04 1:41 ` Dan Williams
@ 2014-08-07 23:54 ` Dan Williams
2014-08-08 9:46 ` Xiangliang Yu
0 siblings, 1 reply; 13+ messages in thread
From: Dan Williams @ 2014-08-07 23:54 UTC (permalink / raw)
To: Xiangliang Yu
Cc: Tejun Heo, James Bottomley, Todd E Brandt, Lukasz Dorau,
IDE/ATA development list, linux-kernel, linux-scsi,
Xiangliang Yu
[ adding yuxiangl@marvell.com ]
On Tue, Jun 3, 2014 at 6:41 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> Hi, some notes below:
>
> On Thu, Apr 24, 2014 at 6:27 AM, Xiangliang Yu <yxlraid@gmail.com> wrote:
>> Add support for SATA port softreset and port multiplier error
>> handling.
>
> Some more detailed notes about the approach and any caveats would be
> appreciated.
>
>>
>> Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
>> ---
>> 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().
>
>> + 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)
>
>> + }
>> +
>> + 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.
>
>> + 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.
>
>> + 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;
>> +fail:
>> + 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.
>
>> +
>> + 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.
>
>> +
>> + 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;
>> +
>> +fail:
>> + 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.
>
>> + 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;
>> +
>> +error:
>> + 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.
>
>> + void (*lldd_dev_thaw)(struct domain_device *);
>
> Same note as lldd_dev_freeze
>
>> + int (*lldd_wait_task_done)(struct sas_task *);
>
> Should not be needed.
>
>> + 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] libsas: modify SATA error handler
2014-08-07 23:54 ` Dan Williams
@ 2014-08-08 9:46 ` Xiangliang Yu
0 siblings, 0 replies; 13+ messages in thread
From: Xiangliang Yu @ 2014-08-08 9:46 UTC (permalink / raw)
To: Dan Williams, Xiangliang Yu
Cc: Tejun Heo, James Bottomley, Todd E Brandt, Lukasz Dorau,
IDE/ATA development list, linux-kernel, linux-scsi
Hi, Dan
> Hi, some notes below:
>
> On Thu, Apr 24, 2014 at 6:27 AM, Xiangliang Yu <yxlraid@gmail.com> wrote:
>> Add support for SATA port softreset and port multiplier error
>> handling.
>
> Some more detailed notes about the approach and any caveats would be
> appreciated.
>
>>
>> Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
>> ---
>> 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
>
>> + 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)
Yes
>
>> + }
>> +
>> + 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
>
>> + 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.
>
>> + 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;
>> +fail:
>> + 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;
>
>> +
>> + 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.
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.
>
>> +
>> + 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;
>> +
>> +fail:
>> + 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.
>> + 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;
>> +
>> +error:
>> + 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.
>> + void (*lldd_dev_thaw)(struct domain_device *);
>
> Same note as lldd_dev_freeze
>
>> + int (*lldd_wait_task_done)(struct sas_task *);
>
> Should not be needed.
>
>> + 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!
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/3] libsas: modify SATA error handler
@ 2014-08-08 9:46 ` Xiangliang Yu
0 siblings, 0 replies; 13+ messages in thread
From: Xiangliang Yu @ 2014-08-08 9:46 UTC (permalink / raw)
To: Dan Williams, Xiangliang Yu
Cc: Tejun Heo, James Bottomley, Todd E Brandt, Lukasz Dorau,
IDE/ATA development list, linux-kernel, linux-scsi
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11559 bytes --]
Hi, Dan
> Hi, some notes below:
>
> On Thu, Apr 24, 2014 at 6:27 AM, Xiangliang Yu <yxlraid@gmail.com> wrote:
>> Add support for SATA port softreset and port multiplier error
>> handling.
>
> Some more detailed notes about the approach and any caveats would be
> appreciated.
>
>>
>> Signed-off-by: Xiangliang Yu <yxlraid@gmail.com>
>> ---
>> 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
>
>> + 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)
Yes
>
>> + }
>> +
>> + 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
>
>> + 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.
>
>> + 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;
>> +fail:
>> + 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;
>
>> +
>> + 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.
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.
>
>> +
>> + 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;
>> +
>> +fail:
>> + 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.
>> + 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;
>> +
>> +error:
>> + 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.
>> + void (*lldd_dev_thaw)(struct domain_device *);
>
> Same note as lldd_dev_freeze
>
>> + int (*lldd_wait_task_done)(struct sas_task *);
>
> Should not be needed.
>
>> + 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!
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] libsas: modify SATA error handler
2014-08-08 9:46 ` Xiangliang Yu
(?)
@ 2014-08-25 16:02 ` Dan Williams
-1 siblings, 0 replies; 13+ messages in thread
From: Dan Williams @ 2014-08-25 16:02 UTC (permalink / raw)
To: Xiangliang Yu
Cc: Xiangliang Yu, Tejun Heo, James Bottomley, Todd E Brandt,
Lukasz Dorau, IDE/ATA development list, linux-kernel, linux-scsi
Some more comments below.
[..]
>>> +
>>> + 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.
>
> 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.
[..]
>>> +/* 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.
>
>>> + 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;
>>> +
>>> +error:
>>> + 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.
>>> + void (*lldd_dev_thaw)(struct domain_device *);
>>
>> Same note as lldd_dev_freeze
>>
>>> + int (*lldd_wait_task_done)(struct sas_task *);
>>
>> Should not be needed.
>>
>>> + 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!
See sas_ata_hard_reset(). It currently does not perform device
re-classification in the same manner as libata. If you want to
improve the "re-classify after reset" implementation then it should be
done in a separate patch in my opinion. In other words, just doing it
for the SRST case is incomplete.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-08-25 16:02 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-24 13:27 [PATCH 1/3] libsas: modify SATA error handler Xiangliang Yu
[not found] ` <CAP0P+NWxxxnQH1e3tXcqwud7EaV4kRRMW46Ad-PWoMWFzgy1eg@mail.gmail.com>
2014-06-03 7:13 ` Xiangliang Yu
2014-06-03 16:05 ` Dan Williams
2014-08-06 10:50 ` Xiangliang Yu
2014-08-06 10:50 ` Xiangliang Yu
2014-08-06 17:21 ` Dan Williams
2014-08-07 1:50 ` Xiangliang Yu
2014-08-07 1:50 ` Xiangliang Yu
2014-06-04 1:41 ` Dan Williams
2014-08-07 23:54 ` Dan Williams
2014-08-08 9:46 ` Xiangliang Yu
2014-08-08 9:46 ` Xiangliang Yu
2014-08-25 16:02 ` Dan Williams
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.