scsi: ufs: add support for query requests
Subsystems (2)
Santosh, thank you very much for your comments. Most where adopted, others where replied inline.
Please note the comment on removing ufs_coe.c file. I will appreciated your opinion on that.
Quoted 3 lines
[email protected] is for UFS filesystem.
Quoted 1 line
The API for submitting a query request is
ufs_query_request()in
ufs_core.c. This function is responsible for:
Quoted 2 lines
This can be part of
ufshcd.cwhich is actually the core file, no need to
create a new core file for the function.
You are right and I moved it for now. But, I think that a good design that will allow easy extension and feature development, will be to separate the hci and features from the rest.
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+ +#define UFS_QUERY_RESERVED_SCSI_CMD_SIZE 10 +Move this to
ufs.h, The name seems too big, it can be changed to
UFS_QUERY_CMD_SIZE.
Quoted 3 lines
Quoted 3 lines
+ if (!hba || !query || !response) {
+ pr_err("%s: NULL pointer hba = %p, query = %p response
= %p",
Quoted 1 line
Quoted 1 line
+
__func__, hba, query, response); +
return -EINVAL;
Quoted 3 lines
Quoted 1 line
+ }
The function will be called withing the driver, So, no need for these
checks. The caller will/must pass the correct parameters. I disagree. Why not play it safe?
Anyone can make this function accessible for IOCTLs or EXPORT it in future. In case they forget (most do), let's not leave holes.
Quoted 8 lines
Quoted 7 lines
+ /*
+ * A SCSI command structure is composed from opcode at the
+ * begining and 0 at the end.
+ */
+memset(cmd, 0, UFS_QUERY_RESERVED_SCSI_CMD_SIZE);
+ cmd[0] =UFS_QUERY_RESERVED_SCSI_CMD;
Quoted 7 lines
Remove memset. Anyway only the first byte is being checked in
ufshcd_is_query_req();Quoted 4 lines
+ *
ufshcd_is_valid_query_rsp- checks if controller TR response is valid
+ * @ucd_rsp_ptr: pointer to response UPIU
+ *
+ * This function checks the response UPIU for valid transaction type
in + * response field
Quoted 6 lines
Review
Quoted 4 lines
+ * Returns 0 on success, non-zero on failure
+ */(quoted diff) +2
-0+static inline int +ufshcd_is_valid_query_rsp(struct utp_upiu_rsp *ucd_rsp_ptr)Combine this with
ufshcd_is_valid_req_rsp(), accordingly handle it in
ufshcd_transfer_rsp_status().
Quoted 2 lines
Quoted 2 lines
+static inline void ufshcd_copy_query_response(struct
ufs_hba*hba, +
struct ufshcd_lrb *lrbp)
Quoted 10 lines
Quoted 3 lines
+{
+ structufs_query_res*query_res=hba->query.response;
+ u8 *descp = (u8 *)&lrbp->ucd_rsp_ptr +GENERAL_UPIU_REQUEST_SIZE;This can be done inside the following if condition i.e. if (
hba->query.descriptor!= NULL).
and change the condition to if (!hba->query.descriptor).Quoted 2 lines
+ /* Get the descriptor */
+ if (hba->query.descriptor!= NULL &&lrbp->ucd_rsp_ptr->qr.opcode==
+ UPIU_QUERY_OPCODE_READ_DESC) {
Quoted 3 lines
Quoted 3 lines
+static int ufshcd_compose_upiu(struct
ufs_hba*hba, structufshcd_lrb
*lrbp)
Quoted 7 lines
Quoted 2 lines
+{
+ structutp_upiu_req*ucd_req_ptr;Remove
ucd_req_ptr, it is not doing anything.Quoted 2 lines
+ if (!lrbp || !lrbp->cmd || !lrbp->ucd_req_ptr ||
+ !lrbp->utr_descriptor_ptr) { +
if (!lrbp)
Quoted 1 line
Quoted 1 line
+
pr_err("%s: lrbp can not be NULL", __func__); +
if (!lrbp->cmd)
Quoted 4 lines
Review
Quoted 4 lines
+ pr_err("%s:
lrbp->cmdcan not be NULL",__func__);(quoted diff) +2
-0+ if (!lrbp->ucd_req_ptr) + pr_err("%s: ucd_req_ptr can not be NULL",
__func__);
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!lrbp->utr_descriptor_ptr) + pr_err("%s: utr_descriptor_ptr can not be NULL",
+ __func__);
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ ret = -EINVAL; + goto exit; + }
I rather play it safe and not remove it.
I rewrote it to make it cleaner.
Quoted 15 lines
These are redundant, remove them.
Quoted 11 lines
+ if (
ufshcd_is_query_req(lrbp))
+lrbp->command_type=UTP_CMD_TYPE_DEV_MANAGE;
+ else
+lrbp->command_type=UTP_CMD_TYPE_SCSI;/* form UPIU before issuing the command */
-ufshcd_compose_upiu(lrbp);
+ufshcd_compose_upiu(hba, lrbp);
err =ufshcd_map_sg(lrbp);
if (err)goto out;
Quoted 3 lines
Why call
ufshcd_map_sg()andscsi_dma_unmap()inufshcd_transfer_req_compl()for requests of typeUTP_CMD_TYPE_DEV_MANAGE?
We are doing this since there is no harm calling it when the buffer is null, as well as this flow is responsible for setting the length parameter in the transfer request descriptor.
Bottom line there is no need to split the original flow.
Quoted 32 lines
Quoted 8 lines
ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)+ if (
ufshcd_is_query_req(lrbp))
+ result =ufshcd_is_valid_query_rsp(lrbp->ucd_rsp_ptr);
+ else
+ result =ufshcd_is_valid_req_rsp(lrbp->ucd_rsp_ptr); +As already mentioned you can combine these.
Quoted 4 lines
(quoted diff) +4
-0+/* Query response result code */ +enum { + QUERY_RESULT_SUCCESS = 0x00, + QUERY_RESULT_NOT_READABLE = 0xF6,Review
Quoted 10 lines
(quoted diff) +10
-0+ 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, +};Move this to
ufs.h.--
~Santosh
Signature · 4 lines
Dolev -- QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation