RE: [PATCH] scsi: ufs: read door bell register after clearing interrupt aggregation
From: Yaniv Gardi
Date: Fri Aug 30 2013 - 17:26:00 EST
Tested-by: Yaniv Gardi <ygardi@xxxxxxxxxxxxxx>
-----Original Message-----
From: linux-scsi-owner@xxxxxxxxxxxxxxx
[mailto:linux-scsi-owner@xxxxxxxxxxxxxxx] On Behalf Of Subhash Jadavani
Sent: Tuesday, August 27, 2013 11:59 AM
To: Dolev Raviv
Cc: linux-scsi@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; open list
Subject: Re: [PATCH] scsi: ufs: read door bell register after clearing
interrupt aggregation
On 8/27/2013 1:50 PM, Subhash Jadavani wrote:
>
>
> Looks good to me.
> Reviewed-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
>
> On 8/25/2013 4:06 PM, Dolev Raviv wrote:
>> In interrupt context, after reading and comparing the UTRLDBR to
>> hba->outstanding_request and before resetting the interrupt
>> hba->aggregation,
>> there might be completion of another transfer request (TR). Such TRs
>> might get stuck, pending, until the next interrupt is generated.
>>
>> The fix reads the UTRLDBR after resetting the interrupt aggregation
>> and handles completed TRs.
>>
>> Signed-off-by: Dolev Raviv <draviv@xxxxxxxxxxxxxx>
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 4dca9b4..30c84d8 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -1915,6 +1915,13 @@ static void ufshcd_uic_cmd_compl(struct
>> ufs_hba *hba)
>> /**
>> * ufshcd_transfer_req_compl - handle SCSI and query command
>> completion
>> * @hba: per adapter instance
>> + *
>> + * Resetting interrupt aggregation counters first and reading the
>> DOOR_BELL
>> + * afterward , allows us to handle all the completed requests.
>> + * In order to prevent other interrupts starvation the DB is read
>> + once
>> + * after reset. The down side of this solution is the possibility of
>> false
>> + * interrupt if device completes another request after resetting
>> aggregation
>> + * and before reading the DB.
>> */
>> static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
>> {
>> @@ -1924,47 +1931,36 @@ static void ufshcd_transfer_req_compl(struct
>> ufs_hba *hba)
>> u32 tr_doorbell;
>> int result;
>> int index;
>> - bool int_aggr_reset = false;
>> +
>> + /* Reset interrupt aggregation counters */
>> + ufshcd_config_int_aggr(hba, INT_AGGR_RESET);
You may have to rebase your change on top of "[PATCH v3 2/6] scsi: ufs:
fix the setting interrupt aggregation counter" patch. Once you do that, you
will be calling ufshcd_reset_intr_aggr() (instead of
ufshscd_config_int_aggr()) to reset the interrupt aggregation counters.
>> tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
>> - for (index = 0; index < hba->nutrs; index++) {
>> - if (test_bit(index, &completed_reqs)) {
>> - lrbp = &hba->lrb[index];
>> - cmd = lrbp->cmd;
>> - /*
>> - * Don't skip resetting interrupt aggregation counters
>> - * if a regular command is present.
>> - */
>> - int_aggr_reset |= !lrbp->intr_cmd;
>> -
>> - if (cmd) {
>> - result = ufshcd_transfer_rsp_status(hba, lrbp);
>> - scsi_dma_unmap(cmd);
>> - cmd->result = result;
>> - /* Mark completed command as NULL in LRB */
>> - lrbp->cmd = NULL;
>> - clear_bit_unlock(index, &hba->lrb_in_use);
>> - /* Do not touch lrbp after scsi done */
>> - cmd->scsi_done(cmd);
>> - } else if (lrbp->command_type ==
>> - UTP_CMD_TYPE_DEV_MANAGE) {
>> - if (hba->dev_cmd.complete)
>> - complete(hba->dev_cmd.complete);
>> - }
>> - } /* end of if */
>> - } /* end of for */
>> + for_each_set_bit(index, &completed_reqs, hba->nutrs) {
>> + lrbp = &hba->lrb[index];
>> + cmd = lrbp->cmd;
>> + if (cmd) {
>> + result = ufshcd_transfer_rsp_status(hba, lrbp);
>> + scsi_dma_unmap(cmd);
>> + cmd->result = result;
>> + /* Mark completed command as NULL in LRB */
>> + lrbp->cmd = NULL;
>> + clear_bit_unlock(index, &hba->lrb_in_use);
>> + /* Do not touch lrbp after scsi done */
>> + cmd->scsi_done(cmd);
>> + } else if (lrbp->command_type == UTP_CMD_TYPE_DEV_MANAGE) {
>> + if (hba->dev_cmd.complete)
>> + complete(hba->dev_cmd.complete);
>> + }
>> + } /* end of for_each_set_bit */
>> /* clear corresponding bits of completed commands */
>> hba->outstanding_reqs ^= completed_reqs;
>> /* we might have free'd some tags above */
>> wake_up(&hba->dev_cmd.tag_wq);
>> -
>> - /* Reset interrupt aggregation counters */
>> - if (int_aggr_reset)
>> - ufshcd_config_int_aggr(hba, INT_AGGR_RESET);
>> }
>> /**
>> @@ -2569,6 +2565,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>> int poll_cnt;
>> u8 resp = 0xF;
>> struct ufshcd_lrb *lrbp;
>> + u32 reg;
>> host = cmd->device->host;
>> hba = shost_priv(host);
>> @@ -2578,6 +2575,13 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>> if (!(test_bit(tag, &hba->outstanding_reqs)))
>> goto out;
>> + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>> + if (!(reg & (1 << tag))) {
>> + dev_err(hba->dev,
>> + "%s: cmd was completed, but without a notifying intr, tag =
>> %d",
>> + __func__, tag);
>> + }
>> +
>> lrbp = &hba->lrb[tag];
>> for (poll_cnt = 100; poll_cnt; poll_cnt--) {
>> err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag,
>> @@ -2586,8 +2590,6 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>> /* cmd pending in the device */
>> break;
>> } else if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) {
>> - u32 reg;
>> -
>> /*
>> * cmd not pending in the device, check if it is
>> * in transition.
>
> --
> 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
--
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/