[v2,2/2] scsi: ufs-bsg: Allow reading descriptors

Commit Message

Avri Altman Jan. 10, 2019, 3:53 p.m. UTC

Add this functionality, placing the descriptor being read in the actual
data buffer in the bio. Consequently, do that for write descriptor as
well.

Signed-off-by: Avri Altman <[e-mail]>
---
 Documentation/scsi/ufs.txt | 11 ++++++++
 drivers/scsi/ufs/ufs_bsg.c | 63 +++++++++++++++++++++++++---------------------
 2 files changed, 46 insertions(+), 28 deletions(-)

Comments

Christoph Hellwig Jan. 21, 2019, 4:56 p.m. UTC | #1

>  static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
>  				     unsigned int request_len,
> -				     unsigned int reply_len,
> -				     int desc_len, enum query_opcode desc_op)
> +				     unsigned int reply_len)
>  {
>  	int min_req_len = sizeof(struct ufs_bsg_request);
>  	int min_rsp_len = sizeof(struct ufs_bsg_reply);
>  
> -	if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
> -		min_req_len += desc_len;
> -

I think this calling convention cleanup should go into a ѕeparate
prep patch with its own changelog.

> +	descp = kzalloc(*desc_len, GFP_KERNEL);
> +	if (!descp)
> +		return -ENOMEM;
> +
> +	if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
> +		sg_copy_to_buffer(job->request_payload.sg_list,
> +				  job->request_payload.sg_cnt, descp,
> +				  *desc_len);

So we always need to bounce buffer here?  Is there any good reason
we can't pass through the sglist to the low-level functions? and
share the low-level code used in ->queuecommand?

Also isn't this an ABI change for the write side?  Are we sure there
are not existing users relying on it?   Is there an API document
that needs to be updated?

Last but not least the commit log needs to be a lot more detailed.

Avri Altman Jan. 21, 2019, 7:26 p.m. UTC | #2

> 
> >  static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
> >  				     unsigned int request_len,
> > -				     unsigned int reply_len,
> > -				     int desc_len, enum query_opcode desc_op)
> > +				     unsigned int reply_len)
> >  {
> >  	int min_req_len = sizeof(struct ufs_bsg_request);
> >  	int min_rsp_len = sizeof(struct ufs_bsg_reply);
> >
> > -	if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
> > -		min_req_len += desc_len;
> > -
> 
> I think this calling convention cleanup should go into a ѕeparate
> prep patch with its own changelog.
Done

> 
> > +	descp = kzalloc(*desc_len, GFP_KERNEL);
> > +	if (!descp)
> > +		return -ENOMEM;
> > +
> > +	if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
> > +		sg_copy_to_buffer(job->request_payload.sg_list,
> > +				  job->request_payload.sg_cnt, descp,
> > +				  *desc_len);
> 
> So we always need to bounce buffer here?  Is there any good reason
> we can't pass through the sglist to the low-level functions? and
> share the low-level code used in ->queuecommand?
This is a device management command - there isn't really a data phase here,
And we are not using any component of queuecommand, 
Specifically not the scsi command sg list.

> 
> Also isn't this an ABI change for the write side?  Are we sure there
> are not existing users relying on it?   Is there an API document
> that needs to be updated?
Right. I did updated Documentation/scsi/ufs.txt.
It's in the beginning of the patch, you might have missed it.
I am counting that ufs-utils will be the first utility to use ufs-bsg, 
and it's waiting for this series to get accepted first.

> 
> Last but not least the commit log needs to be a lot more detailed.
Done.

Patch

diff --git a/Documentation/scsi/ufs.txt b/Documentation/scsi/ufs.txt
index 520b5b0..1769f71 100644
--- a/Documentation/scsi/ufs.txt
+++ b/Documentation/scsi/ufs.txt
@@ -147,6 +147,17 @@  send SG_IO with the applicable sg_io_v4:
 	io_hdr_v4.max_response_len = reply_len;
 	io_hdr_v4.request_len = request_len;
 	io_hdr_v4.request = (__u64)request_upiu;
+	if (dir == SG_DXFER_TO_DEV) {
+		io_hdr_v4.dout_xfer_len = (uint32_t)byte_cnt;
+		io_hdr_v4.dout_xferp = (uintptr_t)(__u64)buff;
+	} else {
+		io_hdr_v4.din_xfer_len = (uint32_t)byte_cnt;
+		io_hdr_v4.din_xferp = (uintptr_t)(__u64)buff;
+	}
+
+If you wish to read or write a descriptor, use the appropriate xferp of
+sg_io_v4.
+
 
 UFS Specifications can be found at,
 UFS - http://www.jedec.org/sites/default/files/docs/JESD220.pdf
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 775bb4e..35c782a 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -27,15 +27,11 @@  static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
 
 static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
 				     unsigned int request_len,
-				     unsigned int reply_len,
-				     int desc_len, enum query_opcode desc_op)
+				     unsigned int reply_len)
 {
 	int min_req_len = sizeof(struct ufs_bsg_request);
 	int min_rsp_len = sizeof(struct ufs_bsg_reply);
 
-	if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
-		min_req_len += desc_len;
-
 	if (min_req_len > request_len || min_rsp_len > reply_len) {
 		dev_err(hba->dev, "not enough space assigned\n");
 		return -EINVAL;
@@ -44,21 +40,16 @@  static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
 	return 0;
 }
 
-static int ufs_bsg_verify_query_params(struct ufs_hba *hba,
-				       struct ufs_bsg_request *bsg_request,
-				       unsigned int request_len,
-				       unsigned int reply_len,
-				       uint8_t *desc_buff, int *desc_len,
-				       enum query_opcode desc_op)
+static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
+				     uint8_t **desc_buff, int *desc_len,
+				     enum query_opcode desc_op)
 {
+	struct ufs_bsg_request *bsg_request = job->request;
 	struct utp_upiu_query *qr;
+	u8 *descp;
 
-	if (desc_op == UPIU_QUERY_OPCODE_READ_DESC) {
-		dev_err(hba->dev, "unsupported opcode %d\n", desc_op);
-		return -ENOTSUPP;
-	}
-
-	if (desc_op != UPIU_QUERY_OPCODE_WRITE_DESC)
+	if (desc_op != UPIU_QUERY_OPCODE_WRITE_DESC &&
+	    desc_op != UPIU_QUERY_OPCODE_READ_DESC)
 		goto out;
 
 	qr = &bsg_request->upiu_req.qr;
@@ -67,16 +58,25 @@  static int ufs_bsg_verify_query_params(struct ufs_hba *hba,
 		return -EINVAL;
 	}
 
-	if (ufs_bsg_verify_query_size(hba, request_len, reply_len, *desc_len,
-				      desc_op))
+	if (*desc_len > job->request_payload.payload_len) {
+		dev_err(hba->dev, "Illegal desc size\n");
 		return -EINVAL;
+	}
 
-	desc_buff = (uint8_t *)(bsg_request + 1);
+	descp = kzalloc(*desc_len, GFP_KERNEL);
+	if (!descp)
+		return -ENOMEM;
+
+	if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
+		sg_copy_to_buffer(job->request_payload.sg_list,
+				  job->request_payload.sg_cnt, descp,
+				  *desc_len);
+
+	*desc_buff = descp;
 
 out:
 	return 0;
 }
-
 static int ufs_bsg_request(struct bsg_job *job)
 {
 	struct ufs_bsg_request *bsg_request = job->request;
@@ -91,7 +91,7 @@  static int ufs_bsg_request(struct bsg_job *job)
 	enum query_opcode desc_op = UPIU_QUERY_OPCODE_NOP;
 	int ret;
 
-	ret = ufs_bsg_verify_query_size(hba, req_len, reply_len, 0, desc_op);
+	ret = ufs_bsg_verify_query_size(hba, req_len, reply_len);
 	if (ret)
 		goto out;
 
@@ -101,9 +101,8 @@  static int ufs_bsg_request(struct bsg_job *job)
 	switch (msgcode) {
 	case UPIU_TRANSACTION_QUERY_REQ:
 		desc_op = bsg_request->upiu_req.qr.opcode;
-		ret = ufs_bsg_verify_query_params(hba, bsg_request, req_len,
-						  reply_len, desc_buff,
-						  &desc_len, desc_op);
+		ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
+						&desc_len, desc_op);
 		if (ret)
 			goto out;
 
@@ -135,11 +134,19 @@  static int ufs_bsg_request(struct bsg_job *job)
 		break;
 	}
 
+	if (!desc_buff)
+		goto out;
+
+	if (desc_len)
+		bsg_reply->reply_payload_rcv_len =
+			sg_copy_from_buffer(job->request_payload.sg_list,
+					    job->request_payload.sg_cnt,
+					    desc_buff, desc_len);
+	kfree(desc_buff);
+
 out:
 	bsg_reply->result = ret;
-	job->reply_len = sizeof(struct ufs_bsg_reply) +
-			 bsg_reply->reply_payload_rcv_len;
-
+	job->reply_len = sizeof(struct ufs_bsg_reply);
 	bsg_job_done(job, ret, bsg_reply->reply_payload_rcv_len);
 
 	return ret;