RE: [PATCH V5 3/4] scsi: ufs: Fix device and host reset methods
Reviewed-by: Yaniv Gardi 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 = > = > > 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 = > > --- = > > 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
Re: [PATCH V5 3/4] scsi: ufs: Fix device and host reset methods
Tested-by: Dolev Raviv > 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 > --- > 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
[PATCH V5 3/4] scsi: ufs: Fix device and host reset methods
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 --- 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;