| Message ID | 1537523064-13478-7-git-send-email-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: Add ufs bsg endpoint | expand |
> + case UPIU_TRANSACTION_QUERY_REQ: > + qr = &bsg_request->upiu_req.qr; > + if (qr->opcode == UPIU_QUERY_OPCODE_READ_DESC) > + goto not_supported; > + > + if (ufs_bsg_get_query_desc_size(hba, qr, &desc_len)) > + goto null_desc_buff; > + > + if (qr->opcode == UPIU_QUERY_OPCODE_WRITE_DESC) { > + rw = WRITE; > + desc_buff = (uint8_t *)(bsg_request + 1); > + } > + > +null_desc_buff: > + /* fall through */ > + case UPIU_TRANSACTION_NOP_OUT: > + case UPIU_TRANSACTION_TASK_REQ: > + /* Now that we know if its a read or write, verify again */ > + if (rw != UFS_BSG_NOP || desc_len) { > + ret = ufs_bsg_verify_query_size(request_len, reply_len, > + rw, desc_len); > + if (ret) { > + dev_err(job->dev, > + "not enough space assigned\n"); > + goto out; > + } > + } I think this check should be moved into the above switch case as it can only be hit for UPIU_TRANSACTION_QUERY_REQ requests. In fact I think the code would be a lot cleaner if you could factor the UPIU_TRANSACTION_QUERY_REQ case into a little helper function (ufs_bsg_get_query_desc_size should probably be merged into that helper also). > + case UPIU_TRANSACTION_UIC_CMD: > + /* later */ > + case UPIU_TRANSACTION_COMMAND: > + case UPIU_TRANSACTION_DATA_OUT: > +not_supported: > + /* for the time being, we do not support data transfer upiu */ > + ret = -ENOTSUPP; > + dev_err(job->dev, "unsupported msgcode 0x%x\n", msgcode); > + > + break; > + default: > + ret = -EINVAL; > + > + break; > + } This difference between known but not supported and not recognized is a bit odd. I think there should be just the generic not supported case, and you can decide if you want to log it or not. Also please try to avoid goto labels jumping into switch statements, code is generlaly a lot more readable if you keep the goto targets outside the switch statement.
> > > + case UPIU_TRANSACTION_QUERY_REQ: > > + qr = &bsg_request->upiu_req.qr; > > + if (qr->opcode == UPIU_QUERY_OPCODE_READ_DESC) > > + goto not_supported; > > + > > + if (ufs_bsg_get_query_desc_size(hba, qr, &desc_len)) > > + goto null_desc_buff; > > + > > + if (qr->opcode == UPIU_QUERY_OPCODE_WRITE_DESC) { > > + rw = WRITE; > > + desc_buff = (uint8_t *)(bsg_request + 1); > > + } > > + > > +null_desc_buff: > > + /* fall through */ > > + case UPIU_TRANSACTION_NOP_OUT: > > + case UPIU_TRANSACTION_TASK_REQ: > > + /* Now that we know if its a read or write, verify again */ > > + if (rw != UFS_BSG_NOP || desc_len) { > > + ret = ufs_bsg_verify_query_size(request_len, > reply_len, > > + rw, desc_len); > > + if (ret) { > > + dev_err(job->dev, > > + "not enough space assigned\n"); > > + goto out; > > + } > > + } > > I think this check should be moved into the above switch case > as it can only be hit for UPIU_TRANSACTION_QUERY_REQ requests. In > fact I think the code would be a lot cleaner if you could factor > the UPIU_TRANSACTION_QUERY_REQ case into a little helper function > (ufs_bsg_get_query_desc_size should probably be merged into that > helper also). Done. > > > + case UPIU_TRANSACTION_UIC_CMD: This should introduced only in the next patch > > + /* later */ > > + case UPIU_TRANSACTION_COMMAND: > > + case UPIU_TRANSACTION_DATA_OUT: > > +not_supported: > > + /* for the time being, we do not support data transfer upiu */ > > + ret = -ENOTSUPP; > > + dev_err(job->dev, "unsupported msgcode 0x%x\n", msgcode); > > + > > + break; > > + default: > > + ret = -EINVAL; > > + > > + break; > > + } > > This difference between known but not supported and not recognized is > a bit odd. I think there should be just the generic not supported > case, and you can decide if you want to log it or not. Done. > > Also please try to avoid goto labels jumping into switch statements, > code is generlaly a lot more readable if you keep the goto targets > outside the switch statement. Done. Thanks, Avri
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c index 1036c52..c1797e2 100644 --- a/drivers/scsi/ufs/ufs_bsg.c +++ b/drivers/scsi/ufs/ufs_bsg.c @@ -7,18 +7,122 @@ #include "ufs_bsg.h" +static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, + struct utp_upiu_query *qr, + int *desc_len) +{ + int desc_size = be16_to_cpu(qr->length); + int desc_id = qr->idn; + int ret = 0; + + if (qr->opcode != UPIU_QUERY_OPCODE_WRITE_DESC || desc_size <= 0) + return -EINVAL; + + ret = ufshcd_map_desc_id_to_length(hba, desc_id, desc_len); + + if (ret || !*desc_len) + return -EINVAL; + + *desc_len = min_t(int, *desc_len, desc_size); + + return ret; +} + +static int ufs_bsg_verify_query_size(unsigned int request_len, + unsigned int reply_len, + int rw, int desc_len) +{ + int min_req_len = sizeof(struct ufs_bsg_request); + int min_rsp_len = sizeof(struct ufs_bsg_reply); + + if (rw == WRITE) + min_req_len += desc_len; + + if (min_req_len > request_len) + return -EINVAL; + + if (min_rsp_len > reply_len) + return -EINVAL; + + return 0; +} + static int ufs_bsg_request(struct bsg_job *job) { struct ufs_bsg_request *bsg_request = job->request; struct ufs_bsg_reply *bsg_reply = job->reply; - int ret = -ENOTSUPP; + struct ufs_hba *hba = shost_priv(dev_to_shost(job->dev->parent)); + unsigned int request_len = job->request_len; + unsigned int reply_len = job->reply_len; + struct utp_upiu_query *qr; + int msgcode; + uint8_t *desc_buff = NULL; + int desc_len = 0; + int rw = UFS_BSG_NOP; + int ret; + ret = ufs_bsg_verify_query_size(request_len, reply_len, rw, desc_len); + if (ret) { + dev_err(job->dev, "not enough space assigned\n"); + goto out; + } bsg_reply->reply_payload_rcv_len = 0; - /* Do Nothing for now */ - dev_err(job->dev, "unsupported message_code 0x%x\n", - bsg_request->msgcode); + msgcode = bsg_request->msgcode; + switch (msgcode) { + case UPIU_TRANSACTION_QUERY_REQ: + qr = &bsg_request->upiu_req.qr; + if (qr->opcode == UPIU_QUERY_OPCODE_READ_DESC) + goto not_supported; + + if (ufs_bsg_get_query_desc_size(hba, qr, &desc_len)) + goto null_desc_buff; + + if (qr->opcode == UPIU_QUERY_OPCODE_WRITE_DESC) { + rw = WRITE; + desc_buff = (uint8_t *)(bsg_request + 1); + } + +null_desc_buff: + /* fall through */ + case UPIU_TRANSACTION_NOP_OUT: + case UPIU_TRANSACTION_TASK_REQ: + /* Now that we know if its a read or write, verify again */ + if (rw != UFS_BSG_NOP || desc_len) { + ret = ufs_bsg_verify_query_size(request_len, reply_len, + rw, desc_len); + if (ret) { + dev_err(job->dev, + "not enough space assigned\n"); + goto out; + } + } + + ret = ufshcd_exec_raw_upiu_cmd(hba, &bsg_request->upiu_req, + &bsg_reply->upiu_rsp, msgcode, + desc_buff, &desc_len, rw); + if (ret) + dev_dbg(hba->dev, + "exe raw upiu: error code %d\n", ret); + + break; + case UPIU_TRANSACTION_UIC_CMD: + /* later */ + case UPIU_TRANSACTION_COMMAND: + case UPIU_TRANSACTION_DATA_OUT: +not_supported: + /* for the time being, we do not support data transfer upiu */ + ret = -ENOTSUPP; + dev_err(job->dev, "unsupported msgcode 0x%x\n", msgcode); + + break; + default: + ret = -EINVAL; + + break; + } +out: bsg_reply->result = ret; job->reply_len = sizeof(struct ufs_bsg_reply) + bsg_reply->reply_payload_rcv_len; diff --git a/drivers/scsi/ufs/ufs_bsg.h b/drivers/scsi/ufs/ufs_bsg.h index d099187..b628583 100644 --- a/drivers/scsi/ufs/ufs_bsg.h +++ b/drivers/scsi/ufs/ufs_bsg.h @@ -12,6 +12,9 @@ #include "ufshcd.h" #include "ufs.h" + +#define UFS_BSG_NOP (-1) + #ifdef CONFIG_SCSI_UFS_BSG void ufs_bsg_remove(struct ufs_hba *hba); int ufs_bsg_probe(struct ufs_hba *hba);
Do that for the currently supported UPIUs: query, nop out, and task management. We do not support UPIU of type scsi command yet, while we are using the job's request and reply pointers to hold the payload. We will look into it in later patches. We might need to elaborate the raw upiu api for that. We also still not supporting uic commands: For first phase, we plan to use the existing api, and send only uic commands that are already supported. Anyway, all that will come in the next patch. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufs_bsg.c | 112 +++++++++++++++++++++++++++++++++++++++++++-- drivers/scsi/ufs/ufs_bsg.h | 3 ++ 2 files changed, 111 insertions(+), 4 deletions(-)