On 7/24/2013 7:09 PM, Seungwon Jeon wrote:
> On Wed, July 24, 2013, Sujit Reddy Thumma wrote:
>> On 7/23/2013 2:04 PM, Seungwon Jeon wrote:
>>> 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.
>>>
>>
>> You can refer to section 8.3 of HCI spec.
>> On fatal errors the controller h/w will have to update the OCS field of
>> the command that caused error and then raise an fatal error interrupt.
>> The s/w reads the OCS value and determine commands that are in error
>> and then carry out reset.
> I don't think so.
> OCS field can be updated regardless of fatal error.
> As mentioned previously, your implementations are gathering all errors into 'ufshcd_fatal_err_handler'.
> It means that non-fatal error is also handled and if any OCS value, host reset will be reserved.
>
Okay. Will remove the OCS dependency.
>>
>>>>
>>>>>
>>>>>> + 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.
>>
>> You are right, this should return with scsi_unblock_requests()
>> called and there is no need to complete the processed requests as we
>> might be in middle of something else while the RESET is in progress.
>>
>>
>>>>
>>>>>
>>>>>> + }
>>>>>> +
>>>>>> + 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.
>>>
>>
>> I am doing the same in this patch -
>> 1) Return to SCSI the successful commands.
>> 2) Clear the pending (but not cause of error) commands by writing into
>> UTMRLCLR/UTRCLR registers. So scsi_host_result = DID_REQUEUE
>> 3) Reset and return the commands that "caused error" to SCSI with
>> DID_ERROR.
>>
>> Am I doing anything extra than what you have suggested?
> If some are cleared, let me review more.
>
>>
>>>>
>>>>> 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.]
>>>
>>
>> I meant "8.3.6: When this condition occurs, host software shall follow
>> the same procedure for UIC error handling as described in 8.2.2,". There
>> is an error in the spec. it was not 8.2.2 but 8.3.2 for UIC error
>> handling. So going by 8.3.2 HCE needs to be toggled.
> I feel like 8.3.2 of spec. makes it difficult to identifying 'device fatal error' with a fatal UIC error.
> It needs to clarify these.
Yes, we can post this to Jedec to clarify with more details in error
handling section.
>
> Anyway, I found some descriptions related to host' reset.
> 5.3.1 Device Fatal Error Status (DFES):
> ...
> If the error occurs, host SW should reset the host controller.
> I's explicit. If spec. saying is right, we would reset host.
For now, I guess it is safe have following implementation until spec.
clarifies -
1) For any non-fatal errors (but still require s/w attention), clear
the door-bell register and retry the command.
2) For any fatal errors (UIC fatal, device fatal, host fatal, system
bus fatal), reset the controller and device and re-establish the link.
Given that these fatal errors are very rare the delay involved in reset
and re-establish link should not be much concern unless the device is
really bad and failing even after reset.
>
>>
>>
>>
>>>>
>>>>>
>>>>>> + } 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.
>>>
>>
>> Yeah, ufshcd_err_handler is apt but it is already consumed. Probably,
>> ufshcd_err_handler -> ufshcd_check_errors
>> ufshcd_fatal_err_handler -> ufshcd_err_handler
>> rename would be fine?
> I like it.
>
> Thanks,
> Seungwon Jeon
>
Thanks a lot for reviewing the patches :)
I will update the patchset shortly.
--
Regards,
Sujit
--
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
