| Message ID | 20111217023420.15036.51486.stgit@localhost6.localdomain6 |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 17-12-2011 6:34, Dan Williams wrote: > We invoke task->task_done() to free the task in the eh case, but at this > point we are prepared for scsi_eh_flush_done_q() to finish off the scmd. > Introduce sas_end_task() to capture the final response status from the > lldd and free the task. > Also take the opportunity to kill this warning. > drivers/scsi/libsas/sas_scsi_host.c: In function ‘sas_end_task’: > drivers/scsi/libsas/sas_scsi_host.c:102:3: warning: case value ‘2’ not in enumerated type ‘enum exec_status’ [-Wswitch] > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/scsi/libsas/sas_scsi_host.c | 61 +++++++++++++++++++---------------- > include/scsi/libsas.h | 5 ++- > 2 files changed, 37 insertions(+), 29 deletions(-) > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index f32c628..13aee61 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -49,27 +49,12 @@ > #include<linux/scatterlist.h> > #include<linux/libata.h> > > -/* ---------- SCSI Host glue ---------- */ > - > -static void sas_scsi_task_done(struct sas_task *task) > +/* record final status and free the task */ > +static void sas_end_task(struct scsi_cmnd *sc, struct sas_task *task) > { > struct task_status_struct *ts =&task->task_status; > - struct scsi_cmnd *sc = task->uldd_task; > int hs = 0, stat = 0; [...] > @@ -124,10 +109,32 @@ static void sas_scsi_task_done(struct sas_task *task) > break; > } > } > - ASSIGN_SAS_TASK(sc, NULL); > + > sc->result = (hs<< 16) | stat; > + ASSIGN_SAS_TASK(sc, NULL); > list_del_init(&task->list); > sas_free_task(task); > +} > + > +static void sas_scsi_task_done(struct sas_task *task) > +{ > + struct scsi_cmnd *sc = task->uldd_task; > + > + if (unlikely(task->task_state_flags& SAS_TASK_STATE_ABORTED)) { > + /* Aborted tasks will be completed by the error handler */ > + SAS_DPRINTK("task done but aborted\n"); > + return; > + } > + > + if (unlikely(!sc)) { > + SAS_DPRINTK("task_done called with non existing SCSI cmnd!\n"); > + list_del_init(&task->list); > + sas_free_task(task); > + return; > + } > + > + ASSIGN_SAS_TASK(sc, NULL); Why do it twice -- once here and then in sas_end_task()? > + sas_end_task(sc, task); > sc->scsi_done(sc); > } MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17-12-2011 6:34, Dan Williams wrote: > We invoke task->task_done() to free the task in the eh case, but at this > point we are prepared for scsi_eh_flush_done_q() to finish off the scmd. > Introduce sas_end_task() to capture the final response status from the > lldd and free the task. > Also take the opportunity to kill this warning. > drivers/scsi/libsas/sas_scsi_host.c: In function ‘sas_end_task’: > drivers/scsi/libsas/sas_scsi_host.c:102:3: warning: case value ‘2’ not in enumerated type ‘enum exec_status’ [-Wswitch] Perhaps this a material for a separate patch... > Signed-off-by: Dan Williams<dan.j.williams@intel.com> > --- > drivers/scsi/libsas/sas_scsi_host.c | 61 +++++++++++++++++++---------------- > include/scsi/libsas.h | 5 ++- > 2 files changed, 37 insertions(+), 29 deletions(-) > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index 6064f82..b6b0b99 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -443,7 +443,10 @@ enum service_response { > }; > > enum exec_status { > - /* The SAM_STAT_.. codes fit in the lower 6 bits */ > + /* The SAM_STAT_.. codes fit in the lower 6 bits, alias some of > + * them here to silence 'case value not in enumerated type' warnings > + */ > + __SAM_STAT_CHECK_CONDITION = SAM_STAT_CHECK_CONDITION, Looks like you forgot to change the problematic *case* itself... MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 17, 2011 at 5:08 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote: >> + ASSIGN_SAS_TASK(sc, NULL); > > > Why do it twice -- once here and then in sas_end_task()? ...for the sas_eh_finish_cmd() case where eh has taken responsibility for cleanup and the sas_scsi_task_done() path has been disabled. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 17, 2011 at 5:13 AM, Sergei Shtylyov <sshtylyov@mvista.com> wrote: > Perhaps this a material for a separate patch... If it changed behavior maybe we'd want it as a separate bisection point, or if this approach for silencing this type of warnings gets nak'd. [..] >> enum exec_status { >> - /* The SAM_STAT_.. codes fit in the lower 6 bits */ >> + /* The SAM_STAT_.. codes fit in the lower 6 bits, alias some of >> + * them here to silence 'case value not in enumerated type' >> warnings >> + */ >> + __SAM_STAT_CHECK_CONDITION = SAM_STAT_CHECK_CONDITION, > > > Looks like you forgot to change the problematic *case* itself... It was deliberate. This is just an alias to get the 'value' in the enum, but I think the real definition should be used in all cases. -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index f32c628..13aee61 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -49,27 +49,12 @@ #include <linux/scatterlist.h> #include <linux/libata.h> -/* ---------- SCSI Host glue ---------- */ - -static void sas_scsi_task_done(struct sas_task *task) +/* record final status and free the task */ +static void sas_end_task(struct scsi_cmnd *sc, struct sas_task *task) { struct task_status_struct *ts = &task->task_status; - struct scsi_cmnd *sc = task->uldd_task; int hs = 0, stat = 0; - if (unlikely(task->task_state_flags & SAS_TASK_STATE_ABORTED)) { - /* Aborted tasks will be completed by the error handler */ - SAS_DPRINTK("task done but aborted\n"); - return; - } - - if (unlikely(!sc)) { - SAS_DPRINTK("task_done called with non existing SCSI cmnd!\n"); - list_del_init(&task->list); - sas_free_task(task); - return; - } - if (ts->resp == SAS_TASK_UNDELIVERED) { /* transport error */ hs = DID_NO_CONNECT; @@ -124,10 +109,32 @@ static void sas_scsi_task_done(struct sas_task *task) break; } } - ASSIGN_SAS_TASK(sc, NULL); + sc->result = (hs << 16) | stat; + ASSIGN_SAS_TASK(sc, NULL); list_del_init(&task->list); sas_free_task(task); +} + +static void sas_scsi_task_done(struct sas_task *task) +{ + struct scsi_cmnd *sc = task->uldd_task; + + if (unlikely(task->task_state_flags & SAS_TASK_STATE_ABORTED)) { + /* Aborted tasks will be completed by the error handler */ + SAS_DPRINTK("task done but aborted\n"); + return; + } + + if (unlikely(!sc)) { + SAS_DPRINTK("task_done called with non existing SCSI cmnd!\n"); + list_del_init(&task->list); + sas_free_task(task); + return; + } + + ASSIGN_SAS_TASK(sc, NULL); + sas_end_task(sc, task); sc->scsi_done(sc); } @@ -236,18 +243,16 @@ static void sas_eh_finish_cmd(struct scsi_cmnd *cmd) struct sas_task *task = TO_SAS_TASK(cmd); struct sas_ha_struct *sas_ha = SHOST_TO_SAS_HA(cmd->device->host); - /* remove the aborted task flag to allow the task to be - * completed now. At this point, we only get called following - * an actual abort of the task, so we should be guaranteed not - * to be racing with any completions from the LLD (hence we - * don't need the task state lock to clear the flag) */ - task->task_state_flags &= ~SAS_TASK_STATE_ABORTED; - /* Now call task_done. However, task will be free'd after - * this */ - task->task_done(task); + /* At this point, we only get called following an actual abort + * of the task, so we should be guaranteed not to be racing with + * any completions from the LLD. Task is freed after this. + */ + sas_end_task(cmd, task); + /* now finish the command and move it on to the error * handler done list, this also takes it off the - * error handler pending list */ + * error handler pending list. + */ scsi_eh_finish_cmd(cmd, &sas_ha->eh_done_q); } diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index 6064f82..b6b0b99 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -443,7 +443,10 @@ enum service_response { }; enum exec_status { - /* The SAM_STAT_.. codes fit in the lower 6 bits */ + /* The SAM_STAT_.. codes fit in the lower 6 bits, alias some of + * them here to silence 'case value not in enumerated type' warnings + */ + __SAM_STAT_CHECK_CONDITION = SAM_STAT_CHECK_CONDITION, SAS_DEV_NO_RESPONSE = 0x80, SAS_DATA_UNDERRUN,
We invoke task->task_done() to free the task in the eh case, but at this point we are prepared for scsi_eh_flush_done_q() to finish off the scmd. Introduce sas_end_task() to capture the final response status from the lldd and free the task. Also take the opportunity to kill this warning. drivers/scsi/libsas/sas_scsi_host.c: In function ‘sas_end_task’: drivers/scsi/libsas/sas_scsi_host.c:102:3: warning: case value ‘2’ not in enumerated type ‘enum exec_status’ [-Wswitch] Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/scsi/libsas/sas_scsi_host.c | 61 +++++++++++++++++++---------------- include/scsi/libsas.h | 5 ++- 2 files changed, 37 insertions(+), 29 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html