On Sat, July 20, 2013, Sujit Reddy Thumma wrote:
> On 7/19/2013 7:28 PM, Seungwon Jeon wrote:
> > On Tue, July 09, 2013, Sujit Reddy Thumma wrote:
> >> Error handling in UFS driver is broken and resets the host controller
> >> for fatal errors without re-initialization. Correct the fatal error
> >> handling sequence according to UFS Host Controller Interface (HCI)
> >> v1.1 specification.
> >>
> >> o Upon determining fatal error condition the host controller may hang
> >> forever until a reset is applied, so just retrying the command doesn't
> >> work without a reset. So, the reset is applied in the driver context
> >> in a separate work and SCSI mid-layer isn't informed until reset is
> >> applied.
> >>
> >> o Processed requests which are completed without error are reported to
> >> SCSI layer as successful and any pending commands that are not started
> >> yet or are not cause of the error are re-queued into scsi midlayer queue.
> >> For the command that caused error, host controller or device is reset
> >> and DID_ERROR is returned for command retry after applying reset.
> >>
> >> o SCSI is informed about the expected Unit-Attentioni exception from the
> > Attention'i', typo.
> Okay.
>
> >
> >> device for the immediate command after a reset so that the SCSI layer
> >> take necessary steps to establish communication with the device.
> >>
> >> Signed-off-by: Sujit Reddy Thumma <sthumma@xxxxxxxxxxxxxx>
> >> ---
> >> drivers/scsi/ufs/ufshcd.c | 349 +++++++++++++++++++++++++++++++++++---------
> >> drivers/scsi/ufs/ufshcd.h | 2 +
> >> drivers/scsi/ufs/ufshci.h | 19 ++-
> >> 3 files changed, 295 insertions(+), 75 deletions(-)
> >>
> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >> index b4c9910..2a3874f 100644
> >> --- a/drivers/scsi/ufs/ufshcd.c
> >> +++ b/drivers/scsi/ufs/ufshcd.c
> >> @@ -80,6 +80,14 @@ enum {
> >> UFSHCD_EH_DEVICE_RESET_PENDING = (1 << 1),
> >> };
> >>
> >> +/* UFSHCD UIC layer error flags */
> >> +enum {
> >> + UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */
> >> + UFSHCD_UIC_NL_ERROR = (1 << 1), /* Network layer error */
> >> + UFSHCD_UIC_TL_ERROR = (1 << 2), /* Transport Layer error */
> >> + UFSHCD_UIC_DME_ERROR = (1 << 3), /* DME error */
> >> +};
> >> +
> >> /* Interrupt configuration options */
> >> enum {
> >> UFSHCD_INT_DISABLE,
> >> @@ -108,6 +116,7 @@ enum {
> >>
> >> static void ufshcd_tmc_handler(struct ufs_hba *hba);
> >> static void ufshcd_async_scan(void *data, async_cookie_t cookie);
> >> +static int ufshcd_reset_and_restore(struct ufs_hba *hba);
> >>
> >> /*
> >> * ufshcd_wait_for_register - wait for register value to change
> >> @@ -1605,9 +1614,6 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
> >> goto out;
> >> }
> >>
> >> - if (hba->ufshcd_state == UFSHCD_STATE_RESET)
> >> - scsi_unblock_requests(hba->host);
> >> -
> >> out:
> >> return err;
> >> }
> >> @@ -1733,66 +1739,6 @@ static int ufshcd_validate_dev_connection(struct ufs_hba *hba)
> >> }
> >>
> >> /**
> >> - * ufshcd_do_reset - reset the host controller
> >> - * @hba: per adapter instance
> >> - *
> >> - * Returns SUCCESS/FAILED
> >> - */
> >> -static int ufshcd_do_reset(struct ufs_hba *hba)
> >> -{
> >> - struct ufshcd_lrb *lrbp;
> >> - unsigned long flags;
> >> - int tag;
> >> -
> >> - /* block commands from midlayer */
> >> - scsi_block_requests(hba->host);
> >> -
> >> - spin_lock_irqsave(hba->host->host_lock, flags);
> >> - hba->ufshcd_state = UFSHCD_STATE_RESET;
> >> -
> >> - /* send controller to reset state */
> >> - ufshcd_hba_stop(hba);
> >> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> >> -
> >> - /* abort outstanding commands */
> >> - for (tag = 0; tag < hba->nutrs; tag++) {
> >> - if (test_bit(tag, &hba->outstanding_reqs)) {
> >> - lrbp = &hba->lrb[tag];
> >> - if (lrbp->cmd) {
> >> - scsi_dma_unmap(lrbp->cmd);
> >> - lrbp->cmd->result = DID_RESET << 16;
> >> - lrbp->cmd->scsi_done(lrbp->cmd);
> >> - lrbp->cmd = NULL;
> >> - clear_bit_unlock(tag, &hba->lrb_in_use);
> >> - }
> >> - }
> >> - }
> >> -
> >> - /* complete device management command */
> >> - if (hba->dev_cmd.complete)
> >> - complete(hba->dev_cmd.complete);
> >> -
> >> - /* clear outstanding request/task bit maps */
> >> - hba->outstanding_reqs = 0;
> >> - hba->outstanding_tasks = 0;
> >> -
> >> - /* Host controller enable */
> >> - if (ufshcd_hba_enable(hba)) {
> >> - dev_err(hba->dev,
> >> - "Reset: Controller initialization failed\n");
> >> - return FAILED;
> >> - }
> >> -
> >> - if (ufshcd_link_startup(hba)) {
> >> - dev_err(hba->dev,
> >> - "Reset: Link start-up failed\n");
> >> - return FAILED;
> >> - }
> >> -
> >> - return SUCCESS;
> >> -}
> >> -
> >> -/**
> >> * ufshcd_slave_alloc - handle initial SCSI device configurations
> >> * @sdev: pointer to SCSI device
> >> *
> >> @@ -1809,6 +1755,9 @@ static int ufshcd_slave_alloc(struct scsi_device *sdev)
> >> sdev->use_10_for_ms = 1;
> >> scsi_set_tag_type(sdev, MSG_SIMPLE_TAG);
> >>
> >> + /* allow SCSI layer to restart the device in case of errors */
> >> + sdev->allow_restart = 1;
> >> +
> >> /*
> >> * Inform SCSI Midlayer that the LUN queue depth is same as the
> >> * controller queue depth. If a LUN queue depth is less than the
> >> @@ -2013,6 +1962,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> >> case OCS_ABORTED:
> >> result |= DID_ABORT << 16;
> >> break;
> >> + case OCS_INVALID_COMMAND_STATUS:
> >> + result |= DID_REQUEUE << 16;
> >> + break;
> >> case OCS_INVALID_CMD_TABLE_ATTR:
> >> case OCS_INVALID_PRDT_ATTR:
> >> case OCS_MISMATCH_DATA_BUF_SIZE:
> >> @@ -2405,42 +2357,295 @@ out:
> >> return err;
> >> }
> >>
> >> +static void ufshcd_decide_eh_xfer_req(struct ufs_hba *hba, u32 ocs)
> >> +{
> >> + switch (ocs) {
> >> + case OCS_SUCCESS:
> >> + case OCS_INVALID_COMMAND_STATUS:
> >> + break;
> >> + case OCS_MISMATCH_DATA_BUF_SIZE:
> >> + case OCS_MISMATCH_RESP_UPIU_SIZE:
> >> + case OCS_PEER_COMM_FAILURE:
> >> + case OCS_FATAL_ERROR:
> >> + case OCS_ABORTED:
> >> + case OCS_INVALID_CMD_TABLE_ATTR:
> >> + case OCS_INVALID_PRDT_ATTR:
> >> + ufshcd_set_host_reset_pending(hba);
> > Should host be reset on ocs error, including below ufshcd_decide_eh_task_req?
> > It's just overall command status.
>
> Yes, the error handling section in the UFS 1.1 spec. mentions so.
If host's reset is required, it should be allowed in fatal situation.
Deciding with OCS field seems not proper. There is no mentions for that in spec.
If I have a wrong information, please let it clear.
>
> >
> >> + break;
> >> + default:
> >> + dev_err(hba->dev, "%s: unknown OCS 0x%x\n",
> >> + __func__, ocs);
> >> + BUG();
> >> + }
> >> +}
> >> +
> >> +static void ufshcd_decide_eh_task_req(struct ufs_hba *hba, u32 ocs)
> >> +{
> >> + switch (ocs) {
> >> + case OCS_TMR_SUCCESS:
> >> + case OCS_TMR_INVALID_COMMAND_STATUS:
> >> + break;
> >> + case OCS_TMR_MISMATCH_REQ_SIZE:
> >> + case OCS_TMR_MISMATCH_RESP_SIZE:
> >> + case OCS_TMR_PEER_COMM_FAILURE:
> >> + case OCS_TMR_INVALID_ATTR:
> >> + case OCS_TMR_ABORTED:
> >> + case OCS_TMR_FATAL_ERROR:
> >> + ufshcd_set_host_reset_pending(hba);
> >> + break;
> >> + default:
> >> + dev_err(hba->dev, "%s: uknown TMR OCS 0x%x\n",
> >> + __func__, ocs);
> >> + BUG();
> >> + }
> >> +}
> >> +
> >> /**
> >> - * ufshcd_fatal_err_handler - handle fatal errors
> >> + * ufshcd_error_autopsy_transfer_req() - reads OCS field of failed command and
> >> + * decide error handling
> >> * @hba: per adapter instance
> >> + * @err_xfer: bit mask for transfer request errors
> >> + *
> >> + * Iterate over completed transfer requests and
> >> + * set error handling flags.
> >> + */
> >> +static void
> >> +ufshcd_error_autopsy_transfer_req(struct ufs_hba *hba, u32 *err_xfer)
> >> +{
> >> + unsigned long completed;
> >> + u32 doorbell;
> >> + int index;
> >> + int ocs;
> >> +
> >> + if (!err_xfer)
> >> + goto out;
> >> +
> >> + doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> >> + completed = doorbell ^ (u32)hba->outstanding_reqs;
> >> +
> >> + for (index = 0; index < hba->nutrs; index++) {
> >> + if (test_bit(index, &completed)) {
> >> + ocs = ufshcd_get_tr_ocs(&hba->lrb[index]);
> >> + if ((ocs == OCS_SUCCESS) ||
> >> + (ocs == OCS_INVALID_COMMAND_STATUS))
> >> + continue;
> >> +
> >> + *err_xfer |= (1 << index);
> >> + ufshcd_decide_eh_xfer_req(hba, ocs);
> >> + }
> >> + }
> >> +out:
> >> + return;
> >> +}
> >> +
> >> +/**
> >> + * ufshcd_error_autopsy_task_req() - reads OCS field of failed command and
> >> + * decide error handling
> >> + * @hba: per adapter instance
> >> + * @err_tm: bit mask for task management errors
> >> + *
> >> + * Iterate over completed task management requests and
> >> + * set error handling flags.
> >> + */
> >> +static void
> >> +ufshcd_error_autopsy_task_req(struct ufs_hba *hba, u32 *err_tm)
> >> +{
> >> + unsigned long completed;
> >> + u32 doorbell;
> >> + int index;
> >> + int ocs;
> >> +
> >> + if (!err_tm)
> >> + goto out;
> >> +
> >> + doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> >> + completed = doorbell ^ (u32)hba->outstanding_tasks;
> >> +
> >> + for (index = 0; index < hba->nutmrs; index++) {
> >> + if (test_bit(index, &completed)) {
> >> + struct utp_task_req_desc *tm_descp;
> >> +
> >> + tm_descp = hba->utmrdl_base_addr;
> >> + ocs = ufshcd_get_tmr_ocs(&tm_descp[index]);
> >> + if ((ocs == OCS_TMR_SUCCESS) ||
> >> + (ocs == OCS_TMR_INVALID_COMMAND_STATUS))
> >> + continue;
> >> +
> >> + *err_tm |= (1 << index);
> >> + ufshcd_decide_eh_task_req(hba, ocs);
> >> + }
> >> + }
> >> +
> >> +out:
> >> + return;
> >> +}
> >> +
> >> +/**
> >> + * ufshcd_fatal_err_handler - handle fatal errors
> >> + * @work: pointer to work structure
> >> */
> >> static void ufshcd_fatal_err_handler(struct work_struct *work)
> >> {
> >> struct ufs_hba *hba;
> >> + unsigned long flags;
> >> + u32 err_xfer = 0;
> >> + u32 err_tm = 0;
> >> + int err;
> >> +
> >> hba = container_of(work, struct ufs_hba, feh_workq);
> >>
> >> pm_runtime_get_sync(hba->dev);
> >> - /* check if reset is already in progress */
> >> - if (hba->ufshcd_state != UFSHCD_STATE_RESET)
> >> - ufshcd_do_reset(hba);
> >> + spin_lock_irqsave(hba->host->host_lock, flags);
> >> + if (hba->ufshcd_state == UFSHCD_STATE_RESET) {
> >> + /* complete processed requests and exit */
> >> + ufshcd_transfer_req_compl(hba);
> >> + ufshcd_tmc_handler(hba);
> >> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> >> + pm_runtime_put_sync(hba->dev);
> >> + return;
> > Host driver is here with finishing 'scsi_block_requests'.
> > 'scsi_unblock_requests' can be called somewhere?
>
> No, but it can be possible that SCSI command timeout which triggers
> device/host reset and fatal error handler race each other.
Sorry, I didn't get your meaning exactly.
I saw that scsi_block_requests is done before ufshcd_fatal_err_handler is scheduled.
If device or host was requested from scsi mid-layer just before ufshcd_fatal_err_handler,
ufshcd_fatal_err_handler will be out through if statement. Then, there is nowhere to call scsi_unblock_requests
though device/host reset is done successfully.
>
> >
> >> + }
> >> +
> >> + hba->ufshcd_state = UFSHCD_STATE_RESET;
> >> + ufshcd_error_autopsy_transfer_req(hba, &err_xfer);
> >> + ufshcd_error_autopsy_task_req(hba, &err_tm);
> >> +
> >> + /*
> >> + * Complete successful and pending transfer requests.
> >> + * DID_REQUEUE is returned for pending requests as they have
> >> + * nothing to do with error'ed request and SCSI layer should
> >> + * not treat them as errors and decrement retry count.
> >> + */
> >> + hba->outstanding_reqs &= ~err_xfer;
> >> + ufshcd_transfer_req_compl(hba);
> >> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> >> + ufshcd_complete_pending_reqs(hba);
> >> + spin_lock_irqsave(hba->host->host_lock, flags);
> >> + hba->outstanding_reqs |= err_xfer;
> > Hmm... error handling seems so complicated.
> > To simplify it, how about below?
> >
> > 1. If requests(transfer or task management) are completed, finish them with success/failure.
> This is what we are trying to do above.
>
> > 2. If there are pending requests, abort them.
> No, if a fatal error is occurred it is possible that host controller is
> freez'ed we are not sure if it can take task management commands and
> execute them.
I meant that aborting the request by clearing corresponding UTMRLCLR/UTMRLCLR.
>
> > 3. If fatal error, reset.
> >
>
>
> >> +
> >> + /* Complete successful and pending task requests */
> >> + hba->outstanding_tasks &= ~err_tm;
> >> + ufshcd_tmc_handler(hba);
> >> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> >> + ufshcd_complete_pending_tasks(hba);
> >> + spin_lock_irqsave(hba->host->host_lock, flags);
> >> +
> >> + hba->outstanding_tasks |= err_tm;
> >> +
> >> + /*
> >> + * Controller may generate multiple fatal errors, handle
> >> + * errors based on severity.
> >> + * 1) DEVICE_FATAL_ERROR
> >> + * 2) SYSTEM_BUS/CONTROLLER_FATAL_ERROR
> >> + * 3) UIC_ERROR
> >> + */
> >> + if (hba->errors & DEVICE_FATAL_ERROR) {
> >> + /*
> >> + * Some HBAs may not clear UTRLDBR/UTMRLDBR or update
> >> + * OCS field on device fatal error.
> >> + */
> >> + ufshcd_set_host_reset_pending(hba);
> > In DEVICE_FATAL_ERROR, ufshcd_device_reset_pending is right?
>
> It looks so, but the spec. mentions to reset the host as well (8.3.6).
Do you pointing below?
[8.3.6. Device Errors are fatal errors. ...the host software shall reset the device too.]
>
> >
> >> + } else if (hba->errors & (SYSTEM_BUS_FATAL_ERROR |
> >> + CONTROLLER_FATAL_ERROR)) {
> >> + /* eh flags should be set in err autopsy based on OCS values */
> >> + if (!hba->eh_flags)
> >> + WARN(1, "%s: fatal error without error handling\n",
> >> + dev_name(hba->dev));
> >> + } else if (hba->errors & UIC_ERROR) {
> >> + if (hba->uic_error & UFSHCD_UIC_DL_PA_INIT_ERROR) {
> >> + /* fatal error - reset controller */
> >> + ufshcd_set_host_reset_pending(hba);
> >> + } else if (hba->uic_error & (UFSHCD_UIC_NL_ERROR |
> >> + UFSHCD_UIC_TL_ERROR |
> >> + UFSHCD_UIC_DME_ERROR)) {
> >> + /* non-fatal, report error to SCSI layer */
> >> + if (!hba->eh_flags) {
> >> + spin_unlock_irqrestore(
> >> + hba->host->host_lock, flags);
> >> + ufshcd_complete_pending_reqs(hba);
> >> + ufshcd_complete_pending_tasks(hba);
> >> + spin_lock_irqsave(hba->host->host_lock, flags);
> >> + }
> >> + }
> >> + }
> >> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> >> +
> >> + if (hba->eh_flags) {
> >> + err = ufshcd_reset_and_restore(hba);
> >> + if (err) {
> >> + ufshcd_clear_host_reset_pending(hba);
> >> + ufshcd_clear_device_reset_pending(hba);
> >> + dev_err(hba->dev, "%s: reset and restore failed\n",
> >> + __func__);
> >> + hba->ufshcd_state = UFSHCD_STATE_ERROR;
> >> + }
> >> + /*
> >> + * Inform scsi mid-layer that we did reset and allow to handle
> >> + * Unit Attention properly.
> >> + */
> >> + scsi_report_bus_reset(hba->host, 0);
> >> + hba->errors = 0;
> >> + hba->uic_error = 0;
> >> + }
> >> + scsi_unblock_requests(hba->host);
> >> pm_runtime_put_sync(hba->dev);
> >> }
> >>
> >> /**
> >> - * ufshcd_err_handler - Check for fatal errors
> >> - * @work: pointer to a work queue structure
> >> + * ufshcd_update_uic_error - check and set fatal UIC error flags.
> >> + * @hba: per-adapter instance
> >> */
> >> -static void ufshcd_err_handler(struct ufs_hba *hba)
> >> +static void ufshcd_update_uic_error(struct ufs_hba *hba)
> >> {
> >> u32 reg;
> >>
> >> + /* PA_INIT_ERROR is fatal and needs UIC reset */
> >> + reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
> >> + if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> >> + hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
> >> +
> >> + /* UIC NL/TL/DME errors needs software retry */
> >> + reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_NETWORK_LAYER);
> >> + if (reg)
> >> + hba->uic_error |= UFSHCD_UIC_NL_ERROR;
> >> +
> >> + reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_TRANSPORT_LAYER);
> >> + if (reg)
> >> + hba->uic_error |= UFSHCD_UIC_TL_ERROR;
> >> +
> >> + reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DME);
> >> + if (reg)
> >> + hba->uic_error |= UFSHCD_UIC_DME_ERROR;
> > REG_UIC_ERROR_CODE_PHY_ADAPTER_LAYER is not handled.
>
> UFS spec. mentions that it is non-fatal error and UIC recovers
> by itself and doesn't need software intervention.
Ok.
>
> >
> >> +
> >> + dev_dbg(hba->dev, "%s: UIC error flags = 0x%08x\n",
> >> + __func__, hba->uic_error);
> >> +}
> >> +
> >> +/**
> >> + * ufshcd_err_handler - Check for fatal errors
> >> + * @hba: per-adapter instance
> >> + */
> >> +static void ufshcd_err_handler(struct ufs_hba *hba)
> >> +{
> >> if (hba->errors & INT_FATAL_ERRORS)
> >> goto fatal_eh;
> >>
> >> if (hba->errors & UIC_ERROR) {
> >> - reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
> >> - if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> >> + hba->uic_error = 0;
> >> + ufshcd_update_uic_error(hba);
> >> + if (hba->uic_error)
> > Except UFSHCD_UIC_DL_PA_INIT_ERROR, it's not fatal. Should it go to fatal_eh?
>
> Please see the UIC error handling in ufshcd_fatal_err_handler(), others
> need software intervention so I combined it with fatal_eh to complete
> the requests and report to SCSI.
As gathering all error(fatal, non-fatal)handling into origin one, it makes confused.
Then, I would be better to rename ufshcd_fatal_err_handler.
Thanks,
Seungwon Jeon
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
