RE: [PATCH V5 3/4] scsi: ufs: Fix device and host reset methods

2013-08-13 Thread Yaniv Gardi 
Reviewed-by: Yaniv Gardi yga...@codeaurora.org

QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


=  -Original Message-
=  From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
=  ow...@vger.kernel.org] On Behalf Of Dolev Raviv
=  Sent: Monday, August 12, 2013 4:02 PM
=  To: Sujit Reddy Thumma
=  Cc: Vinayak Holikatti; Santosh Y; James E.J. Bottomley; linux-
=  s...@vger.kernel.org; Sujit Reddy Thumma; linux-arm-
=  m...@vger.kernel.org
=  Subject: Re: [PATCH V5 3/4] scsi: ufs: Fix device and host reset methods
=  
=  Tested-by: Dolev Raviv dra...@codeaurora.org
=  
=   As of now SCSI initiated error handling is broken because, the reset
=   APIs don't try to bring back the device initialized and ready for
=   further transfers.
=  
=   In case of timeouts, the scsi error handler takes care of handling
=   aborts and resets. Improve the error handling in such scenario by
=   resetting the device and host and re-initializing them in proper
manner.
=  
=   Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org
=   ---
=drivers/scsi/ufs/ufshcd.c |  240
=   +++--
=drivers/scsi/ufs/ufshcd.h |2 +
=2 files changed, 189 insertions(+), 53 deletions(-)
=  
=   diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
=   index d4ee48d..c577e6e 100644
=   --- a/drivers/scsi/ufs/ufshcd.c
=   +++ b/drivers/scsi/ufs/ufshcd.c
=   @@ -69,9 +69,14 @@ enum {
=  
=/* UFSHCD states */
=enum {
=   - UFSHCD_STATE_OPERATIONAL,
= UFSHCD_STATE_RESET,
= UFSHCD_STATE_ERROR,
=   + UFSHCD_STATE_OPERATIONAL,
=   +};
=   +
=   +/* UFSHCD error handling flags */
=   +enum {
=   + UFSHCD_EH_IN_PROGRESS = (1  0),
=};
=  
=/* Interrupt configuration options */ @@ -87,6 +92,16 @@ enum {
= INT_AGGR_CONFIG,
=};
=  
=   +#define ufshcd_set_eh_in_progress(h) \
=   + (h-eh_flags |= UFSHCD_EH_IN_PROGRESS) #define
=   +ufshcd_eh_in_progress(h) \
=   + (h-eh_flags  UFSHCD_EH_IN_PROGRESS) #define
=   +ufshcd_clear_eh_in_progress(h) \
=   + (h-eh_flags = ~UFSHCD_EH_IN_PROGRESS)
=   +
=   +static void ufshcd_tmc_handler(struct ufs_hba *hba); static void
=   +ufshcd_async_scan(void *data, async_cookie_t cookie);
=   +
=/*
= * ufshcd_wait_for_register - wait for register value to change
= * @hba - per-adapter interface
=   @@ -840,10 +855,25 @@ static int ufshcd_queuecommand(struct
=  Scsi_Host
=   *host, struct scsi_cmnd *cmd)
=  
= tag = cmd-request-tag;
=  
=   - if (hba-ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
=   + spin_lock_irqsave(hba-host-host_lock, flags);
=   + switch (hba-ufshcd_state) {
=   + case UFSHCD_STATE_OPERATIONAL:
=   + break;
=   + case UFSHCD_STATE_RESET:
= err = SCSI_MLQUEUE_HOST_BUSY;
=   - goto out;
=   + goto out_unlock;
=   + case UFSHCD_STATE_ERROR:
=   + set_host_byte(cmd, DID_ERROR);
=   + cmd-scsi_done(cmd);
=   + goto out_unlock;
=   + default:
=   + dev_WARN_ONCE(hba-dev, 1, %s: invalid state %d\n,
=   + __func__, hba-ufshcd_state);
=   + set_host_byte(cmd, DID_BAD_TARGET);
=   + cmd-scsi_done(cmd);
=   + goto out_unlock;
= }
=   + spin_unlock_irqrestore(hba-host-host_lock, flags);
=  
= /* acquire the tag to make sure device cmds don't use it */
= if (test_and_set_bit_lock(tag, hba-lrb_in_use)) { @@ -880,6
=  +910,7
=   @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct
=   scsi_cmnd *cmd)
= /* issue command to the controller */
= spin_lock_irqsave(hba-host-host_lock, flags);
= ufshcd_send_command(hba, tag);
=   +out_unlock:
= spin_unlock_irqrestore(hba-host-host_lock, flags);
=out:
= return err;
=   @@ -1495,8 +1526,6 @@ static int
=  ufshcd_make_hba_operational(struct
=   ufs_hba *hba)
= if (hba-ufshcd_state == UFSHCD_STATE_RESET)
= scsi_unblock_requests(hba-host);
=  
=   - hba-ufshcd_state = UFSHCD_STATE_OPERATIONAL;
=   -
=out:
= return err;
=}
=   @@ -2245,8 +2274,12 @@ static void ufshcd_err_handler(struct ufs_hba
=  *hba)
= }
= return;
=fatal_eh:
=   - hba-ufshcd_state = UFSHCD_STATE_ERROR;
=   - schedule_work(hba-feh_workq);
=   + /* handle fatal errors only when link is functional */
=   + if (hba-ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
=   + /* block commands at driver layer until error is handled */
=   + hba-ufshcd_state = UFSHCD_STATE_ERROR;
=   + schedule_work(hba-feh_workq);
=   + }
=}
=  
=/**
=   @@ -2411,12 +2444,13 @@ static int ufshcd_issue_tm_cmd(struct
=  ufs_hba
=   *hba, int lun_id, int task_id,  }
=  
=/**
=   - * ufshcd_device_reset - reset device and abort all the pending
=   commands
=   + * ufshcd_eh_device_reset_handler - device reset handler registered
to
=   + *scsi layer.
= * @cmd: SCSI command

Re: [PATCH V5 3/4] scsi: ufs: Fix device and host reset methods

2013-08-12 Thread Dolev Raviv 
Tested-by: Dolev Raviv dra...@codeaurora.org

 As of now SCSI initiated error handling is broken because,
 the reset APIs don't try to bring back the device initialized and
 ready for further transfers.

 In case of timeouts, the scsi error handler takes care of handling aborts
 and resets. Improve the error handling in such scenario by resetting the
 device and host and re-initializing them in proper manner.

 Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org
 ---
  drivers/scsi/ufs/ufshcd.c |  240
 +++--
  drivers/scsi/ufs/ufshcd.h |2 +
  2 files changed, 189 insertions(+), 53 deletions(-)

 diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
 index d4ee48d..c577e6e 100644
 --- a/drivers/scsi/ufs/ufshcd.c
 +++ b/drivers/scsi/ufs/ufshcd.c
 @@ -69,9 +69,14 @@ enum {

  /* UFSHCD states */
  enum {
 - UFSHCD_STATE_OPERATIONAL,
   UFSHCD_STATE_RESET,
   UFSHCD_STATE_ERROR,
 + UFSHCD_STATE_OPERATIONAL,
 +};
 +
 +/* UFSHCD error handling flags */
 +enum {
 + UFSHCD_EH_IN_PROGRESS = (1  0),
  };

  /* Interrupt configuration options */
 @@ -87,6 +92,16 @@ enum {
   INT_AGGR_CONFIG,
  };

 +#define ufshcd_set_eh_in_progress(h) \
 + (h-eh_flags |= UFSHCD_EH_IN_PROGRESS)
 +#define ufshcd_eh_in_progress(h) \
 + (h-eh_flags  UFSHCD_EH_IN_PROGRESS)
 +#define ufshcd_clear_eh_in_progress(h) \
 + (h-eh_flags = ~UFSHCD_EH_IN_PROGRESS)
 +
 +static void ufshcd_tmc_handler(struct ufs_hba *hba);
 +static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 +
  /*
   * ufshcd_wait_for_register - wait for register value to change
   * @hba - per-adapter interface
 @@ -840,10 +855,25 @@ static int ufshcd_queuecommand(struct Scsi_Host
 *host, struct scsi_cmnd *cmd)

   tag = cmd-request-tag;

 - if (hba-ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
 + spin_lock_irqsave(hba-host-host_lock, flags);
 + switch (hba-ufshcd_state) {
 + case UFSHCD_STATE_OPERATIONAL:
 + break;
 + case UFSHCD_STATE_RESET:
   err = SCSI_MLQUEUE_HOST_BUSY;
 - goto out;
 + goto out_unlock;
 + case UFSHCD_STATE_ERROR:
 + set_host_byte(cmd, DID_ERROR);
 + cmd-scsi_done(cmd);
 + goto out_unlock;
 + default:
 + dev_WARN_ONCE(hba-dev, 1, %s: invalid state %d\n,
 + __func__, hba-ufshcd_state);
 + set_host_byte(cmd, DID_BAD_TARGET);
 + cmd-scsi_done(cmd);
 + goto out_unlock;
   }
 + spin_unlock_irqrestore(hba-host-host_lock, flags);

   /* acquire the tag to make sure device cmds don't use it */
   if (test_and_set_bit_lock(tag, hba-lrb_in_use)) {
 @@ -880,6 +910,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host,
 struct scsi_cmnd *cmd)
   /* issue command to the controller */
   spin_lock_irqsave(hba-host-host_lock, flags);
   ufshcd_send_command(hba, tag);
 +out_unlock:
   spin_unlock_irqrestore(hba-host-host_lock, flags);
  out:
   return err;
 @@ -1495,8 +1526,6 @@ static int ufshcd_make_hba_operational(struct
 ufs_hba *hba)
   if (hba-ufshcd_state == UFSHCD_STATE_RESET)
   scsi_unblock_requests(hba-host);

 - hba-ufshcd_state = UFSHCD_STATE_OPERATIONAL;
 -
  out:
   return err;
  }
 @@ -2245,8 +2274,12 @@ static void ufshcd_err_handler(struct ufs_hba *hba)
   }
   return;
  fatal_eh:
 - hba-ufshcd_state = UFSHCD_STATE_ERROR;
 - schedule_work(hba-feh_workq);
 + /* handle fatal errors only when link is functional */
 + if (hba-ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
 + /* block commands at driver layer until error is handled */
 + hba-ufshcd_state = UFSHCD_STATE_ERROR;
 + schedule_work(hba-feh_workq);
 + }
  }

  /**
 @@ -2411,12 +2444,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
 *hba, int lun_id, int task_id,
  }

  /**
 - * ufshcd_device_reset - reset device and abort all the pending commands
 + * ufshcd_eh_device_reset_handler - device reset handler registered to
 + *scsi layer.
   * @cmd: SCSI command pointer
   *
   * Returns SUCCESS/FAILED
   */
 -static int ufshcd_device_reset(struct scsi_cmnd *cmd)
 +static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
  {
   struct Scsi_Host *host;
   struct ufs_hba *hba;
 @@ -2425,6 +2459,7 @@ static int ufshcd_device_reset(struct scsi_cmnd
 *cmd)
   int err;
   u8 resp = 0xF;
   struct ufshcd_lrb *lrbp;
 + unsigned long flags;

   host = cmd-device-host;
   hba = shost_priv(host);
 @@ -2433,55 +2468,33 @@ static int ufshcd_device_reset(struct scsi_cmnd
 *cmd)
   lrbp = hba-lrb[tag];
   err = ufshcd_issue_tm_cmd(hba, lrbp-lun, 0, UFS_LOGICAL_RESET, resp);
   if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
 - err = FAILED;
 + if (!err)
 + err

[PATCH V5 3/4] scsi: ufs: Fix device and host reset methods

2013-07-29 Thread Sujit Reddy Thumma 
As of now SCSI initiated error handling is broken because,
the reset APIs don't try to bring back the device initialized and
ready for further transfers.

In case of timeouts, the scsi error handler takes care of handling aborts
and resets. Improve the error handling in such scenario by resetting the
device and host and re-initializing them in proper manner.

Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org
---
 drivers/scsi/ufs/ufshcd.c |  240 +++--
 drivers/scsi/ufs/ufshcd.h |2 +
 2 files changed, 189 insertions(+), 53 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d4ee48d..c577e6e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -69,9 +69,14 @@ enum {
 
 /* UFSHCD states */
 enum {
-   UFSHCD_STATE_OPERATIONAL,
UFSHCD_STATE_RESET,
UFSHCD_STATE_ERROR,
+   UFSHCD_STATE_OPERATIONAL,
+};
+
+/* UFSHCD error handling flags */
+enum {
+   UFSHCD_EH_IN_PROGRESS = (1  0),
 };
 
 /* Interrupt configuration options */
@@ -87,6 +92,16 @@ enum {
INT_AGGR_CONFIG,
 };
 
+#define ufshcd_set_eh_in_progress(h) \
+   (h-eh_flags |= UFSHCD_EH_IN_PROGRESS)
+#define ufshcd_eh_in_progress(h) \
+   (h-eh_flags  UFSHCD_EH_IN_PROGRESS)
+#define ufshcd_clear_eh_in_progress(h) \
+   (h-eh_flags = ~UFSHCD_EH_IN_PROGRESS)
+
+static void ufshcd_tmc_handler(struct ufs_hba *hba);
+static void ufshcd_async_scan(void *data, async_cookie_t cookie);
+
 /*
  * ufshcd_wait_for_register - wait for register value to change
  * @hba - per-adapter interface
@@ -840,10 +855,25 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, 
struct scsi_cmnd *cmd)
 
tag = cmd-request-tag;
 
-   if (hba-ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
+   spin_lock_irqsave(hba-host-host_lock, flags);
+   switch (hba-ufshcd_state) {
+   case UFSHCD_STATE_OPERATIONAL:
+   break;
+   case UFSHCD_STATE_RESET:
err = SCSI_MLQUEUE_HOST_BUSY;
-   goto out;
+   goto out_unlock;
+   case UFSHCD_STATE_ERROR:
+   set_host_byte(cmd, DID_ERROR);
+   cmd-scsi_done(cmd);
+   goto out_unlock;
+   default:
+   dev_WARN_ONCE(hba-dev, 1, %s: invalid state %d\n,
+   __func__, hba-ufshcd_state);
+   set_host_byte(cmd, DID_BAD_TARGET);
+   cmd-scsi_done(cmd);
+   goto out_unlock;
}
+   spin_unlock_irqrestore(hba-host-host_lock, flags);
 
/* acquire the tag to make sure device cmds don't use it */
if (test_and_set_bit_lock(tag, hba-lrb_in_use)) {
@@ -880,6 +910,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, 
struct scsi_cmnd *cmd)
/* issue command to the controller */
spin_lock_irqsave(hba-host-host_lock, flags);
ufshcd_send_command(hba, tag);
+out_unlock:
spin_unlock_irqrestore(hba-host-host_lock, flags);
 out:
return err;
@@ -1495,8 +1526,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba 
*hba)
if (hba-ufshcd_state == UFSHCD_STATE_RESET)
scsi_unblock_requests(hba-host);
 
-   hba-ufshcd_state = UFSHCD_STATE_OPERATIONAL;
-
 out:
return err;
 }
@@ -2245,8 +2274,12 @@ static void ufshcd_err_handler(struct ufs_hba *hba)
}
return;
 fatal_eh:
-   hba-ufshcd_state = UFSHCD_STATE_ERROR;
-   schedule_work(hba-feh_workq);
+   /* handle fatal errors only when link is functional */
+   if (hba-ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
+   /* block commands at driver layer until error is handled */
+   hba-ufshcd_state = UFSHCD_STATE_ERROR;
+   schedule_work(hba-feh_workq);
+   }
 }
 
 /**
@@ -2411,12 +2444,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int 
lun_id, int task_id,
 }
 
 /**
- * ufshcd_device_reset - reset device and abort all the pending commands
+ * ufshcd_eh_device_reset_handler - device reset handler registered to
+ *scsi layer.
  * @cmd: SCSI command pointer
  *
  * Returns SUCCESS/FAILED
  */
-static int ufshcd_device_reset(struct scsi_cmnd *cmd)
+static int ufshcd_eh_device_reset_handler(struct scsi_cmnd *cmd)
 {
struct Scsi_Host *host;
struct ufs_hba *hba;
@@ -2425,6 +2459,7 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
int err;
u8 resp = 0xF;
struct ufshcd_lrb *lrbp;
+   unsigned long flags;
 
host = cmd-device-host;
hba = shost_priv(host);
@@ -2433,55 +2468,33 @@ static int ufshcd_device_reset(struct scsi_cmnd *cmd)
lrbp = hba-lrb[tag];
err = ufshcd_issue_tm_cmd(hba, lrbp-lun, 0, UFS_LOGICAL_RESET, resp);
if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
-   err = FAILED;
+   if (!err)
+   err = resp;
goto out;
-