Re: [PATCH] scsi: ufs: read door bell register after clearing interruptaggregation

[Subhash Jadavani]

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 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-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/