lkml.org 
[lkml]   [2012]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 2/5] [SCSI] ufshcd: UFS UTP Transfer requests handling
    >>  #include "ufs.h"
    >> @@ -75,6 +76,8 @@ enum {
    >>       UFSHCD_MAX_CHANNEL      = 0,
    >>       UFSHCD_MAX_ID           = 1,
    >>       UFSHCD_MAX_LUNS         = 8,
    >> +     UFSHCD_CMD_PER_LUN      = 32,
    >> +     UFSHCD_CAN_QUEUE        = 32,
    >
    >
    > Is the can queue right, or are you working around a issue in the shared
    > tag map code, or is this due to how you are tracking outstanding reqs
    > (just a unsigned long so limited by that)?
    >
    > Can you support multiple luns per host? If so, this seems low for normal
    > HBAs. Is this low due to the hw or something? If you have multiple luns
    > then you are going to get a burst of 32 IOs and the host will work on
    > them, then when those are done we will send IO to the next device, then
    > repeat. For normal HBAs we would have can_queue = cmd_per_lun * X, so we
    > can do IO on X devices at the same time.
    >

    The host can support multiple LUNS, but the UFS host controller can
    support only 32 outstanding commands.
    So can queue is set to 32.

    >> + *
    >> + * Returns 0 for success, non-zero in case of failure
    >> + */
    >> +static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
    >> +{
    >> +     struct ufshcd_lrb *lrbp;
    >> +     struct ufs_hba *hba;
    >> +     unsigned long flags;
    >> +     int tag;
    >> +     int err = 0;
    >> +
    >> +     hba = (struct ufs_hba *) &host->hostdata[0];
    >
    >
    > Use shost_priv instead of accessing hostdata. Check the rest of the
    > driver for this.
    >

    ok, will use shost_priv.

    >> +
    >> +/**
    >> + * ufshcd_adjust_lun_qdepth - Update LUN queue depth if device responds with
    >> + *                         SAM_STAT_TASK_SET_FULL SCSI command status.
    >> + * @cmd: pointer to SCSI command
    >> + */
    >> +static void ufshcd_adjust_lun_qdepth(struct scsi_cmnd *cmd)
    >> +{
    >> +     struct ufs_hba *hba;
    >> +     int i;
    >> +     int lun_qdepth = 0;
    >> +
    >> +     hba = (struct ufs_hba *) cmd->device->host->hostdata;
    >> +
    >> +     /*
    >> +      * LUN queue depth can be obtained by counting outstanding commands
    >> +      * on the LUN.
    >> +      */
    >> +     for (i = 0; i < hba->nutrs; i++) {
    >> +             if (test_bit(i, &hba->outstanding_reqs)) {
    >> +
    >> +                     /*
    >> +                      * Check if the outstanding command belongs
    >> +                      * to the LUN which reported SAM_STAT_TASK_SET_FULL.
    >> +                      */
    >> +                     if (cmd->device->lun == hba->lrb[i].lun)
    >> +                             lun_qdepth++;
    >> +             }
    >> +     }
    >> +
    >> +     /*
    >> +      * LUN queue depth will be total outstanding commands, except the
    >> +      * command for which the LUN reported SAM_STAT_TASK_SET_FULL.
    >> +      */
    >> +     scsi_adjust_queue_depth(cmd->device, MSG_SIMPLE_TAG, lun_qdepth - 1);
    >> +}
    >> +
    >> +/**
    >> + * ufshcd_scsi_cmd_status - Update SCSI command result based on SCSI status
    >> + * @lrb: pointer to local reference block of completed command
    >> + * @scsi_status: SCSI command status
    >> + *
    >> + * Returns value base on SCSI command status
    >> + */
    >> +static inline int
    >> +ufshcd_scsi_cmd_status(struct ufshcd_lrb *lrbp, int scsi_status)
    >> +{
    >> +     int result = 0;
    >> +
    >> +     switch (scsi_status) {
    >> +     case SAM_STAT_GOOD:
    >> +             result |= DID_OK << 16 |
    >> +                       COMMAND_COMPLETE << 8 |
    >> +                       SAM_STAT_GOOD;
    >> +             break;
    >> +     case SAM_STAT_CHECK_CONDITION:
    >> +             result |= DRIVER_SENSE << 24 |
    >
    > scsi ml will set the driver sense bit for you when it completes the
    > command in scsi_finish_command. No need for the driver to touch this.
    >

    ok, i'll change it.

    >
    >> +                       DRIVER_OK << 16 |
    >> +                       COMMAND_COMPLETE << 8 |
    >> +                       SAM_STAT_CHECK_CONDITION;
    >> +             ufshcd_copy_sense_data(lrbp);
    >> +             break;
    >> +     case SAM_STAT_BUSY:
    >> +             result |= DID_BUS_BUSY << 16 |
    >> +                       SAM_STAT_BUSY;
    >
    > If you got sam stat busy just set that. You do not need to set the host
    > byte and you do not want to, because the scsi_error.c handling will be
    > incorrect. Instead of retrying until sam stat busy is no longer returned
    > (or until cmd->retries * cmd->timeout) you only get 5 retries.
    >

    Ok, I'll change it.

    >
    >> +             break;
    >> +     case SAM_STAT_TASK_SET_FULL:
    >> +
    >> +             /*
    >> +              * If a LUN reports SAM_STAT_TASK_SET_FULL, then the LUN queue
    >> +              * depth needs to be adjusted to the exact number of
    >> +              * outstanding commands the LUN can handle at any given time.
    >> +              */
    >> +             ufshcd_adjust_lun_qdepth(lrbp->cmd);
    >> +             result |= DID_SOFT_ERROR << 16 |
    >> +                       SAM_STAT_TASK_SET_FULL;
    >
    >
    > Same here. Just set the sam stat part.
    >
    >
    >> +             break;
    >> +     case SAM_STAT_TASK_ABORTED:
    >> +             result |=  DID_ABORT << 16 |
    >> +                        ABORT_TASK << 8 |
    >> +                        SAM_STAT_TASK_ABORTED;
    >
    > Same.
    >

    ok.

    >> +             break;
    >> +     default:
    >> +             result |= DID_ERROR << 16;
    >> +             break;
    >> +     } /* end of switch */
    >> +
    >> +     return result;
    >> +}
    >> +
    >> +/**
    >>  /**
    >> @@ -809,7 +1295,13 @@ static struct scsi_host_template ufshcd_driver_template = {
    >>       .module                 = THIS_MODULE,
    >>       .name                   = UFSHCD,
    >>       .proc_name              = UFSHCD,
    >> +     .queuecommand           = ufshcd_queuecommand,
    >> +     .slave_alloc            = ufshcd_slave_alloc,
    >> +     .slave_destroy          = ufshcd_slave_destroy,
    >>       .this_id                = -1,
    >> +     .sg_tablesize           = SG_ALL,
    >> +     .cmd_per_lun            = UFSHCD_CMD_PER_LUN,
    >> +     .can_queue              = UFSHCD_CAN_QUEUE,
    >
    > Did you want a  change_queue_depth callout?
    >
    >>  };
    >>

    No, command per LUN is set to maximum outstanding commands UFS
    controller can support.
    In case of queue full, the queue depth for a LUN will be calculated
    and set in ufshcd_adjust_lun_qdepth.
    So I don't think change_queue_depth will be necessary.

    Thanks for reviewing, please let me know if you find any issues in
    other patches as well.

    --
    ~Santosh
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-02-27 07:23    [W:3.733 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site