On Thursday, July 18, 2013, Dolev Raviv wrote: > > Hi, > > > > Sorry for the late response. I had a few days off. > > > > On Thu, July 11, 2013, Dolev Raviv wrote: > >> > On Tuesday, July 09, 2013, Sujit Reddy Thumma wrote: > >> >> From: Dolev Raviv <dra...@codeaurora.org> > >> >> Allow UFS device to complete its initialization and accept > >> >> SCSI commands by setting fDeviceInit flag. The device may take > >> >> time for this operation and hence the host should poll until > >> >> fDeviceInit flag is toggled to zero. This step is mandated by > >> >> UFS device specification for device initialization completion. > >> >> Signed-off-by: Dolev Raviv <dra...@codeaurora.org> > >> >> Signed-off-by: Sujit Reddy Thumma <sthu...@codeaurora.org> > >> >> --- > >> >> drivers/scsi/ufs/ufs.h | 88 +++++++++++++- > >> >> drivers/scsi/ufs/ufshcd.c | 292 > >> >> ++++++++++++++++++++++++++++++++++++++++++++- > >> >> drivers/scsi/ufs/ufshcd.h | 14 ++ > >> >> drivers/scsi/ufs/ufshci.h | 2 +- > >> >> 4 files changed, 390 insertions(+), 6 deletions(-) > >> >> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h > >> >> index 14c0a4e..db5bde4 100644 > >> >> --- a/drivers/scsi/ufs/ufs.h > >> >> +++ b/drivers/scsi/ufs/ufs.h > >> >> @@ -43,6 +43,8 @@ > >> >> #define GENERAL_UPIU_REQUEST_SIZE 32 > >> >> #define UPIU_HEADER_DATA_SEGMENT_MAX_SIZE ((ALIGNED_UPIU_SIZE) - \ > >> >> (GENERAL_UPIU_REQUEST_SIZE)) > >> >> +#define QUERY_OSF_SIZE ((GENERAL_UPIU_REQUEST_SIZE) - \ > >> >> + (sizeof(struct > >> >> utp_upiu_header))) > >> >> #define UPIU_HEADER_DWORD(byte3, byte2, byte1, byte0)\ > >> >> cpu_to_be32((byte3 << 24) | (byte2 << 16) |\ > >> >> @@ -68,7 +70,7 @@ enum { > >> >> UPIU_TRANSACTION_COMMAND = 0x01, > >> >> UPIU_TRANSACTION_DATA_OUT = 0x02, > >> >> UPIU_TRANSACTION_TASK_REQ = 0x04, > >> >> - UPIU_TRANSACTION_QUERY_REQ = 0x26, > >> >> + UPIU_TRANSACTION_QUERY_REQ = 0x16, > >> >> }; > >> >> /* UTP UPIU Transaction Codes Target to Initiator */ > >> >> @@ -97,8 +99,19 @@ enum { > >> >> UPIU_TASK_ATTR_ACA = 0x03, > >> >> }; > >> >> -/* UTP QUERY Transaction Specific Fields OpCode */ > >> >> +/* UPIU Query request function */ > >> >> enum { > >> >> + UPIU_QUERY_FUNC_STANDARD_READ_REQUEST = 0x01, > >> >> + UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST = 0x81, > >> >> +}; > >> >> + > >> >> +/* Flag idn for Query Requests*/ > >> >> +enum flag_idn { > >> >> + QUERY_FLAG_IDN_FDEVICEINIT = 0x01, > >> >> +}; > >> >> + > >> >> +/* UTP QUERY Transaction Specific Fields OpCode */ > >> >> +enum query_opcode { > >> >> UPIU_QUERY_OPCODE_NOP = 0x0, > >> >> UPIU_QUERY_OPCODE_READ_DESC = 0x1, > >> >> UPIU_QUERY_OPCODE_WRITE_DESC = 0x2, > >> >> @@ -110,6 +123,21 @@ enum { > >> >> UPIU_QUERY_OPCODE_TOGGLE_FLAG = 0x8, > >> >> }; > >> >> +/* Query response result code */ > >> >> +enum { > >> >> + QUERY_RESULT_SUCCESS = 0x00, > >> >> + QUERY_RESULT_NOT_READABLE = 0xF6, > >> >> + QUERY_RESULT_NOT_WRITEABLE = 0xF7, > >> >> + QUERY_RESULT_ALREADY_WRITTEN = 0xF8, > >> >> + QUERY_RESULT_INVALID_LENGTH = 0xF9, > >> >> + QUERY_RESULT_INVALID_VALUE = 0xFA, > >> >> + QUERY_RESULT_INVALID_SELECTOR = 0xFB, > >> >> + QUERY_RESULT_INVALID_INDEX = 0xFC, > >> >> + QUERY_RESULT_INVALID_IDN = 0xFD, > >> >> + QUERY_RESULT_INVALID_OPCODE = 0xFE, > >> >> + QUERY_RESULT_GENERAL_FAILURE = 0xFF, > >> >> +}; > >> >> + > >> >> /* UTP Transfer Request Command Type (CT) */ > >> >> enum { > >> >> UPIU_COMMAND_SET_TYPE_SCSI = 0x0, > >> >> @@ -127,6 +155,7 @@ enum { > >> >> MASK_SCSI_STATUS = 0xFF, > >> >> MASK_TASK_RESPONSE = 0xFF00, > >> >> MASK_RSP_UPIU_RESULT = 0xFFFF, > >> >> + MASK_QUERY_DATA_SEG_LEN = 0xFFFF, > >> >> }; > >> >> /* Task management service response */ > >> >> @@ -160,13 +189,40 @@ struct utp_upiu_cmd { > >> >> }; > >> >> /** > >> >> + * struct utp_upiu_query - upiu request buffer structure for > >> >> + * query request. > >> >> + * @opcode: command to perform B-0 > >> >> + * @idn: a value that indicates the particular type of data B-1 + * > >> @index: Index to further identify data B-2 > >> >> + * @selector: Index to further identify data B-3 > >> >> + * @reserved_osf: spec reserved field B-4,5 > >> >> + * @length: number of descriptor bytes to read/write B-6,7 > >> >> + * @value: Attribute value to be written DW-5 > >> >> + * @reserved: spec reserved DW-6,7 > >> >> + */ > >> >> +struct utp_upiu_query { > >> >> + u8 opcode; > >> >> + u8 idn; > >> >> + u8 index; > >> >> + u8 selector; > >> >> + u16 reserved_osf; > >> >> + u16 length; > >> >> + u32 value; > >> >> + u32 reserved[2]; > >> >> +}; > >> >> + > >> >> +/** > >> >> * struct utp_upiu_req - general upiu request structure > >> >> * @header:UPIU header structure DW-0 to DW-2 > >> >> * @sc: fields structure for scsi command DW-3 to DW-7 > >> >> + * @qr: fields structure for query request DW-3 to DW-7 > >> >> */ > >> >> struct utp_upiu_req { > >> >> struct utp_upiu_header header; > >> >> - struct utp_upiu_cmd sc; > >> >> + union { > >> >> + struct utp_upiu_cmd sc; > >> >> + struct utp_upiu_query qr; > >> >> + }; > >> >> }; > >> >> /** > >> >> @@ -187,10 +243,14 @@ struct utp_cmd_rsp { > >> >> * struct utp_upiu_rsp - general upiu response structure > >> >> * @header: UPIU header structure DW-0 to DW-2 > >> >> * @sr: fields structure for scsi command DW-3 to DW-12 > >> >> + * @qr: fields structure for query request DW-3 to DW-7 > >> >> */ > >> >> struct utp_upiu_rsp { > >> >> struct utp_upiu_header header; > >> >> - struct utp_cmd_rsp sr; > >> >> + union { > >> >> + struct utp_cmd_rsp sr; > >> >> + struct utp_upiu_query qr; > >> >> + }; > >> >> }; > >> >> /** > >> >> @@ -223,4 +283,24 @@ struct utp_upiu_task_rsp { > >> >> u32 reserved[3]; > >> >> }; > >> >> +/** > >> >> + * struct ufs_query_req - parameters for building a query request + > >> * > >> @query_func: UPIU header query function > >> >> + * @upiu_req: the query request data > >> >> + */ > >> >> +struct ufs_query_req { > >> >> + u8 query_func; > >> >> + struct utp_upiu_query upiu_req; > >> >> +}; > >> >> + > >> >> +/** > >> >> + * struct ufs_query_resp - UPIU QUERY > >> >> + * @response: device response code > >> >> + * @upiu_res: query response data > >> >> + */ > >> >> +struct ufs_query_res { > >> >> + u8 response; > >> >> + struct utp_upiu_query upiu_res; > >> >> +}; > >> >> + > >> >> #endif /* End of Header */ > >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > >> index 3f482b6..96ccb28 100644 > >> >> --- a/drivers/scsi/ufs/ufshcd.c > >> >> +++ b/drivers/scsi/ufs/ufshcd.c > >> >> @@ -48,6 +48,14 @@ > >> >> /* Timeout after 30 msecs if NOP OUT hangs without response */ > >> #define > >> NOP_OUT_TIMEOUT 30 /* msecs */ > >> >> +/* Query request retries */ > >> >> +#define QUERY_REQ_RETRIES 10 > >> >> +/* Query request timeout */ > >> >> +#define QUERY_REQ_TIMEOUT 30 /* msec */ > >> >> + > >> >> +/* Expose the flag value from utp_upiu_query.value */ > >> >> +#define MASK_QUERY_UPIU_FLAG_LOC 0xFF > >> >> + > >> >> enum { > >> >> UFSHCD_MAX_CHANNEL = 0, > >> >> UFSHCD_MAX_ID = 1, > >> >> @@ -348,6 +356,63 @@ static inline void ufshcd_copy_sense_data(struct > >> ufshcd_lrb *lrbp) > >> >> } > >> >> /** > >> >> + * ufshcd_query_to_cpu() - formats the buffer to native cpu endian + > >> * > >> @response: upiu query response to convert > >> >> + */ > >> >> +static inline void ufshcd_query_to_cpu(struct utp_upiu_query > >> *response) > >> >> +{ > >> >> + response->length = be16_to_cpu(response->length); > >> >> + response->value = be32_to_cpu(response->value); > >> >> +} > >> >> + > >> >> +/** > >> >> + * ufshcd_query_to_be() - formats the buffer to big endian > >> >> + * @request: upiu query request to convert > >> >> + */ > >> >> +static inline void ufshcd_query_to_be(struct utp_upiu_query > >> *request) +{ > >> >> + request->length = cpu_to_be16(request->length); > >> >> + request->value = cpu_to_be32(request->value); > >> >> +} > >> >> + > >> >> +/** > >> >> + * ufshcd_copy_query_response() - Copy the Query Response and the > >> data > >> + * descriptor > >> >> + * @hba: per adapter instance > >> >> + * @lrb - pointer to local reference block > >> >> + */ > >> >> +static > >> >> +void ufshcd_copy_query_response(struct ufs_hba *hba, struct > >> ufshcd_lrb > >> *lrbp) > >> >> +{ > >> >> + struct ufs_query_res *query_res = hba->dev_cmd.query.response; + > >> >> + /* Get the UPIU response */ > >> >> + if (query_res) { > >> >> + query_res->response = ufshcd_get_rsp_upiu_result( > >> >> + lrbp->ucd_rsp_ptr) >> UPIU_RSP_CODE_OFFSET; > >> >> + > >> >> + memcpy(&query_res->upiu_res, &lrbp->ucd_rsp_ptr->qr, > >> >> + QUERY_OSF_SIZE); > >> >> + ufshcd_query_to_cpu(&query_res->upiu_res); > >> >> + } > >> >> + > >> >> + /* Get the descriptor */ > >> >> + if (hba->dev_cmd.query.descriptor && > >> >> lrbp->ucd_rsp_ptr->qr.opcode > >> == > >> + UPIU_QUERY_OPCODE_READ_DESC) { > >> >> + u8 *descp = (u8 *)&lrbp->ucd_rsp_ptr + > >> >> + GENERAL_UPIU_REQUEST_SIZE; > >> >> + u16 len; > >> >> + > >> >> + /* data segment length */ > >> >> + len = be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_2) & > >> >> + MASK_QUERY_DATA_SEG_LEN; > >> >> + > >> >> + memcpy(hba->dev_cmd.query.descriptor, descp, > >> >> + min_t(u16, len, > >> >> UPIU_HEADER_DATA_SEGMENT_MAX_SIZE)); > >> >> + } > >> >> +} > >> >> + > >> >> +/** > >> >> * ufshcd_hba_capabilities - Read controller capabilities > >> >> * @hba: per adapter instance > >> >> */ > >> >> @@ -629,6 +694,46 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct > >> ufshcd_lrb *lrbp, u32 upiu_flags) > >> >> (min_t(unsigned short, lrbp->cmd->cmd_len, > >> >> MAX_CDB_SIZE))); > >> >> } > >> >> +/** > >> >> + * ufshcd_prepare_utp_query_req_upiu() - fills the > >> >> utp_transfer_req_desc, > >> >> + * for query requsts > >> >> + * @hba: UFS hba > >> >> + * @lrbp: local reference block pointer > >> >> + * @upiu_flags: flags > >> >> + */ > >> >> +static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba, > >> + struct ufshcd_lrb *lrbp, > >> >> + u32 upiu_flags) > >> >> +{ > >> >> + struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr; > >> >> + u16 len = hba->dev_cmd.query.request->upiu_req.length; > >> >> + u8 *descp = (u8 *)lrbp->ucd_req_ptr + > >> >> GENERAL_UPIU_REQUEST_SIZE; + > >> >> + /* Query request header */ > >> >> + ucd_req_ptr->header.dword_0 = UPIU_HEADER_DWORD( > >> >> + UPIU_TRANSACTION_QUERY_REQ, upiu_flags, > >> >> + lrbp->lun, lrbp->task_tag); > >> >> + ucd_req_ptr->header.dword_1 = UPIU_HEADER_DWORD( > >> >> + 0, hba->dev_cmd.query.request->query_func, 0, > >> >> 0); > >> >> + > >> >> + /* Data segment length */ > >> >> + ucd_req_ptr->header.dword_2 = UPIU_HEADER_DWORD( > >> >> + 0, 0, len >> 8, (u8)len); > >> >> + > >> >> + /* Copy the Query Request buffer as is */ > >> >> + memcpy(&lrbp->ucd_req_ptr->qr, > >> &hba->dev_cmd.query.request->upiu_req, > >> + QUERY_OSF_SIZE); > >> >> + ufshcd_query_to_be(&lrbp->ucd_req_ptr->qr); > >> >> + > >> >> + /* Copy the Descriptor */ > >> >> + if ((hba->dev_cmd.query.descriptor != NULL) && (len > 0) && > >> >> + (hba->dev_cmd.query.request->upiu_req.opcode == > >> >> + UPIU_QUERY_OPCODE_WRITE_DESC)) { > >> >> + memcpy(descp, hba->dev_cmd.query.descriptor, > >> >> + min_t(u16, len, > >> >> UPIU_HEADER_DATA_SEGMENT_MAX_SIZE)); > >> >> + } > >> >> +} > >> >> + > >> >> static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb > >> *lrbp) > >> >> { > >> >> struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr; > >> >> @@ -663,7 +768,10 @@ static int ufshcd_compose_upiu(struct ufs_hba > >> *hba, > >> >> struct ufshcd_lrb *lrbp) > >> >> break; > >> >> case UTP_CMD_TYPE_DEV_MANAGE: > >> >> ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, > >> >> DMA_NONE); > >> >> - if (hba->dev_cmd.type == DEV_CMD_TYPE_NOP) > >> >> + if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY) > >> >> + ufshcd_prepare_utp_query_req_upiu( > >> >> + hba, lrbp, upiu_flags); > >> >> + else if (hba->dev_cmd.type == DEV_CMD_TYPE_NOP) > >> >> ufshcd_prepare_utp_nop_upiu(lrbp); > >> >> else > >> >> ret = -EINVAL; > >> >> @@ -831,6 +939,9 @@ ufshcd_dev_cmd_completion(struct ufs_hba *hba, > >> struct ufshcd_lrb *lrbp) > >> >> __func__, resp); > >> >> } > >> >> break; > >> >> + case UPIU_TRANSACTION_QUERY_RSP: > >> >> + ufshcd_copy_query_response(hba, lrbp); > >> >> + break; > >> >> case UPIU_TRANSACTION_REJECT_UPIU: > >> >> /* TODO: handle Reject UPIU Response */ > >> >> err = -EPERM; > >> >> @@ -941,6 +1052,128 @@ out_put_tag: > >> >> } > >> >> /** > >> >> + * ufshcd_query_request() - API for issuing query request to the > >> device. > >> >> + * @hba: ufs driver context > >> >> + * @query: params for query request > >> >> + * @descriptor: buffer for sending/receiving descriptor > >> >> + * @retries: number of times to try executing the command > >> >> + * > >> >> + * All necessary fields for issuing a query and receiving its > >> response > >> + * are stored in the UFS hba struct. We can use this method since we > >> know > >> >> + * there is only one active query request or any device management > >> command > >> >> + * at all times. > >> >> + */ > >> >> +static int ufshcd_send_query_request(struct ufs_hba *hba, > >> >> + struct ufs_query_req *query, > >> >> + u8 *descriptor, > >> >> + struct ufs_query_res *response) > >> >> +{ > >> >> + int ret; > >> >> + > >> >> + BUG_ON(!hba); > >> >> + if (!query || !response) { > >> >> + dev_err(hba->dev, > >> >> + "%s: NULL pointer query = %p, response = %p\n", > >> >> + __func__, query, response); > >> >> + return -EINVAL; > >> >> + } > >> >> + > >> >> + mutex_lock(&hba->dev_cmd.lock); > >> >> + hba->dev_cmd.query.request = query; > >> >> + hba->dev_cmd.query.response = response; > >> >> + hba->dev_cmd.query.descriptor = descriptor; > >> >> + > >> >> + ret = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_QUERY, > >> >> + QUERY_REQ_TIMEOUT); > >> >> + > >> >> + hba->dev_cmd.query.request = NULL; > >> >> + hba->dev_cmd.query.response = NULL; > >> >> + hba->dev_cmd.query.descriptor = NULL; > >> >> + mutex_unlock(&hba->dev_cmd.lock); > >> >> + > >> >> + return ret; > >> >> +} > >> >> + > >> >> +/** > >> >> + * ufshcd_query_flag() - Helper function for composing flag query > >> requests > >> >> + * hba: per-adapter instance > >> >> + * query_opcode: flag query to perform > >> >> + * idn: flag idn to access > >> >> + * flag_res: the flag value after the query request completes > >> >> + * > >> >> + * Returns 0 for success, non-zero in case of failure > >> >> + */ > >> >> +static int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode > >> opcode, > >> >> + enum flag_idn idn, bool *flag_res) > >> >> +{ > >> >> + struct ufs_query_req *query; > >> >> + struct ufs_query_res *response; > >> >> + int err = -ENOMEM; > >> >> + > >> >> + query = kzalloc(sizeof(struct ufs_query_req), GFP_KERNEL); > >> >> + if (!query) { > >> >> + dev_err(hba->dev, > >> >> + "%s: Failed allocating ufs_query_req > >> >> instance\n", > >> >> + __func__); > >> >> + goto out_no_mem; > >> >> + } > >> >> + response = kzalloc(sizeof(struct ufs_query_res), GFP_KERNEL); + > >> >> if > >> (!response) { > >> >> + dev_err(hba->dev, > >> >> + "%s: Failed allocating ufs_query_res > >> >> instance\n", > >> >> + __func__); > >> >> + goto out_free_query; > >> >> + } > >> > Can't stack local variable be permitted for query and response instead > >> of > >> > dynamic allocation? > >> It can, though I think it is safer this way. In addition, this code is > >> not > >> a bottle neck. > > Yes, once may not be a problem. But I am seeing this function is called > > from some loop routine. > > Could you explain the reason dynamic allocation is needed? > I don't like the idea of allocating it on stack, I suggest to statically > allocate it on the query struct. What do you think? Yes.
> > > >> >> + > >> >> + switch (opcode) { > >> >> + case UPIU_QUERY_OPCODE_SET_FLAG: > >> >> + case UPIU_QUERY_OPCODE_CLEAR_FLAG: > >> >> + case UPIU_QUERY_OPCODE_TOGGLE_FLAG: > >> >> + query->query_func = > >> >> UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST; > >> >> + break; > >> >> + case UPIU_QUERY_OPCODE_READ_FLAG: > >> >> + query->query_func = > >> >> UPIU_QUERY_FUNC_STANDARD_READ_REQUEST; > >> >> + if (!flag_res) { > >> >> + /* No dummy reads */ > >> >> + dev_err(hba->dev, "%s: Invalid argument for > >> >> read request\n", > >> + __func__); > >> >> + err = -EINVAL; > >> >> + goto out; > >> >> + } > >> >> + break; > >> >> + default: > >> >> + dev_err(hba->dev, > >> >> + "%s: Expected query flag opcode but got = %d\n", > >> >> + __func__, opcode); > >> >> + err = -EINVAL; > >> >> + goto out; > >> >> + } > >> >> + query->upiu_req.opcode = opcode; > >> >> + query->upiu_req.idn = idn; > >> >> + > >> >> + /* Send query request */ > >> >> + err = ufshcd_send_query_request(hba, query, NULL, response); > >> >> + > >> >> + if (err) { > >> >> + dev_err(hba->dev, > >> >> + "%s: Sending flag query for idn %d failed, err > >> >> = %d\n", > >> >> + __func__, idn, err); > >> >> + goto out; > >> >> + } > >> >> + > >> >> + if (flag_res) > >> >> + *flag_res = (response->upiu_res.value & > >> >> + MASK_QUERY_UPIU_FLAG_LOC) & 0x1; > >> >> + > >> >> +out: > >> >> + kfree(response); > >> >> +out_free_query: > >> >> + kfree(query); > >> >> +out_no_mem: > >> >> + return err; > >> >> +} > >> >> + > >> >> +/** > >> >> * ufshcd_memory_alloc - allocate memory for host memory space data > >> >> structures > >> >> * @hba: per adapter instance > >> >> * > >> >> @@ -1110,6 +1343,59 @@ static int ufshcd_dme_link_startup(struct > >> ufs_hba > >> >> *hba) > >> >> } > >> >> /** > >> >> + * ufshcd_validate_device_init() - checks device readiness > >> >> + * hba: per-adapter instance > >> >> + * > >> >> + * Set fDeviceInit flag, than, query the flag until the device > >> clears the > >> >> + * flag. > >> >> + */ > >> >> +static int ufshcd_validate_device_init(struct ufs_hba *hba) > >> > As standard description, this function is for initialization > >> completion. > >> How about ufshcd_complete_dev_init for function name? > >> You have a point. > >> >> +{ > >> >> + int i, retries, err = 0; > >> >> + bool flag_res = 0; > >> >> + > >> >> + for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) { > >> >> + /* Set the fDeviceIntit flag */ > >> >> + err = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_SET_FLAG, > >> >> + QUERY_FLAG_IDN_FDEVICEINIT, > >> >> &flag_res); > >> > There is no need to get flag_res in case set flag. > >> > Just passing NULL is good. > >> Consider the following scenario: a device is already fully initialized > >> when we init the driver, in this case setting the flag will return 0 > >> since > >> no further initialization is required. This means we don't have to query > >> again or to poll for the fDeviceInit because we already now the > >> initialization is completed. > > It's not clear. > > I think it may be a broad interpretation. > > What's the meaning of returned flag_res? > > 1. original value > > 2. value which is applied by setting the fDeviceInit flag > > 3. value which is applied by setting the fDeviceInit flag and reset > > > > In this step, query is not for read-flag but for set-flag. > > Above all, spec. mentions polling the fDeviceInit using read-flag > > definitively. > After reading the spec, I agree, the statement is a little fuzzy. I will > change it to null and fix the code according. > > > >> >> + if (!err || err == -ETIMEDOUT) > >> >> + break; > >> >> + dev_dbg(hba->dev, "%s: error %d retrying\n", __func__, > >> >> err); + } > >> >> + if (err) { > >> >> + dev_err(hba->dev, > >> >> + "%s setting fDeviceInit flag failed with error > >> >> %d\n", > >> >> + __func__, err); > >> >> + goto out; > >> >> + } > >> >> + > >> >> + /* poll for max. 100 iterations for fDeviceInit flag to clear */ > >> + for (i = 0; i < 100 && !err && flag_res; i++) { > >> > In first ufshcd_query_flag, if flag_res is updated with '0', this loop > >> can't be executed. > >> > Of course, we expect that flag_res has '1'. > >> In the described case above, this condition allows us to skip the > >> polling > >> process. > >> > >> >> + retries = QUERY_REQ_RETRIES; > >> >> + for (retries = QUERY_REQ_RETRIES; retries > 0; > >> >> retries--) { > >> >> + err = ufshcd_query_flag(hba, > >> >> + UPIU_QUERY_OPCODE_READ_FLAG, > >> >> + QUERY_FLAG_IDN_FDEVICEINIT, > >> >> &flag_res); > >> >> + if (!err || err == -ETIMEDOUT) > >> >> + break; > >> > Normally, ufshcd_query_flag returns '0' to err while flag is not being > >> cleared. > >> > If these routine is repeated, inner for-loop is just executed and go > >> to > >> outer loop. > >> > What is difference between two for-loop? > >> Your analysis was correct, each loop has a different responsibility. The > >> outer loop counts the poling retries, while the inner loop is > >> responsible > >> for passing the query request to the device without errors. So the inner > >> loop will execute more then once iff there was an error and the request > >> was not executed properly. > > I wonder that valid result comes after error is happened and retry is > > taken. > > Should we allow the retry in case errors? > Well, currently all other components, SCSI included, are retrying in case > of errors. I don't see a reason why this case should be different. > > > > Thanks, > > Seungwon Jeon > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > > the body of a message to majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > Thanks, > Dolev > -- > QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html