[PATCH v4 0/3] scsi: ufs-bsg: Add read descriptor

[PATCH v4 0/3] scsi: ufs-bsg: Add read descriptor

[Avri Altman]

UFS Protocol Information Units (UPIU) are UFS packets that travel
between the host and the device on the UniPro bus. Our previous series
added the capability to send UPIUs to the ufs driver. It does not cover
all the possible UPIU types - we are mainly focused on device management,
provisioning, testing and validation, so it covers UPIUs that falls in
that box.

Our intension is to publish ufs-utils soon - an open source user space
utility that relies on that infrastructure to perform those tasks.
This short series is adding one last functionality needed by ufs-utils
that was somehow left behind - allowing reading descriptors as well.

V3->v4:
Improve code readability in ufs-bsg: Allow reading descriptors
Update Reviewed-by tag.

V2->v3:
Add a prep patch with write descriptor calling convention changes.
Elaborate the commit log of ufs-bsg: Allow reading descriptors
Add Reviewed-by tag.

v1->v2:
Withdraw from the attempt to change the reply buffer, instead place the
descriptor being read in the actual data buffer in the bio.

Avri Altman (3):
  scsi: ufs-bsg: Change the calling convention for write descriptor
  scsi: ufs: Allow reading descriptor via raw upiu
  scsi: ufs-bsg: Allow reading descriptors

 Documentation/scsi/ufs.txt | 11 ++++++++
 drivers/scsi/ufs/ufs_bsg.c | 63 ++++++++++++++++++++++++++--------------------
 drivers/scsi/ufs/ufshcd.c  | 20 ++++++++++-----
 3 files changed, 61 insertions(+), 33 deletions(-)

-- 
1.9.1

[PATCH v4 1/3] scsi: ufs-bsg: Change the calling convention for write descriptor

[Avri Altman]

When we had a write descriptor query upiu, we appended the descriptor
right after the bsg request.  This was fine as the bsg driver allows to
allocate whatever buffer we needed in its job request.

Still, the proper way to deliver payload, however small (we only write
config descriptors of 144 bytes), is by using the job request payload
data buffer.

So change this ABI now, while ufs-bsg is still new, and nobody is
actually using it.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Evan Green <evgreen@chromium.org>
---
 Documentation/scsi/ufs.txt |  6 ++++++
 drivers/scsi/ufs/ufs_bsg.c | 47 +++++++++++++++++++++++++---------------------
 2 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/Documentation/scsi/ufs.txt b/Documentation/scsi/ufs.txt
index 520b5b0..78fe7cb 100644
--- a/Documentation/scsi/ufs.txt
+++ b/Documentation/scsi/ufs.txt
@@ -147,6 +147,12 @@ 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;
+	}
+
+If you wish to write a descriptor, use the dout_xferp 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..2fd0769 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,14 +40,13 @@ 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);
@@ -67,11 +62,19 @@ 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;
+	}
+
+	descp = kzalloc(*desc_len, GFP_KERNEL);
+	if (!descp)
+		return -ENOMEM;
 
-	desc_buff = (uint8_t *)(bsg_request + 1);
+	sg_copy_to_buffer(job->request_payload.sg_list,
+			  job->request_payload.sg_cnt, descp, *desc_len);
+
+	*desc_buff = descp;
 
 out:
 	return 0;
@@ -91,7 +94,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 +104,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 +137,14 @@ static int ufs_bsg_request(struct bsg_job *job)
 		break;
 	}
 
+	if (!desc_buff)
+		goto out;
+
+	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;
-- 
1.9.1

[PATCH v4 2/3] scsi: ufs: Allow reading descriptor via raw upiu

[Avri Altman]

Allow to read descriptors via raw upiu. This in fact was forbidden just
as a precaution, as ufs-bsg actually enforces which functionality is
supported.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Evan Green <evgreen@chromium.org>
---
 drivers/scsi/ufs/ufshcd.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2ddf244..6b9ed21 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5775,6 +5775,20 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 
 	/* just copy the upiu response as it is */
 	memcpy(rsp_upiu, lrbp->ucd_rsp_ptr, sizeof(*rsp_upiu));
+	if (desc_buff && desc_op == UPIU_QUERY_OPCODE_READ_DESC) {
+		u8 *descp = (u8 *)lrbp->ucd_rsp_ptr + sizeof(*rsp_upiu);
+		u16 resp_len = be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_2) &
+			       MASK_QUERY_DATA_SEG_LEN;
+
+		if (*buff_len >= resp_len) {
+			memcpy(desc_buff, descp, resp_len);
+			*buff_len = resp_len;
+		} else {
+			dev_warn(hba->dev, "rsp size is bigger than buffer");
+			*buff_len = 0;
+			err = -EINVAL;
+		}
+	}
 
 	ufshcd_put_dev_cmd_tag(hba, tag);
 	wake_up(&hba->dev_cmd.tag_wq);
@@ -5810,11 +5824,6 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 	int ocs_value;
 	u8 tm_f = be32_to_cpu(req_upiu->header.dword_1) >> 16 & MASK_TM_FUNC;
 
-	if (desc_buff && desc_op != UPIU_QUERY_OPCODE_WRITE_DESC) {
-		err = -ENOTSUPP;
-		goto out;
-	}
-
 	switch (msgcode) {
 	case UPIU_TRANSACTION_NOP_OUT:
 		cmd_type = DEV_CMD_TYPE_NOP;
@@ -5855,7 +5864,6 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 		break;
 	}
 
-out:
 	return err;
 }
 
-- 
1.9.1

[PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Avri Altman]

Add this functionality, placing the descriptor being read in the actual
data buffer in the bio.

That is, for both read and write descriptors query upiu, we are using
the job's request_payload.  This in turn, is mapped back in user land to
the applicable sg_io_v4 xferp: dout_xferp for write descriptor,
and din_xferp for read descriptor.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 Documentation/scsi/ufs.txt |  7 ++++++-
 drivers/scsi/ufs/ufs_bsg.c | 20 ++++++++++++--------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/Documentation/scsi/ufs.txt b/Documentation/scsi/ufs.txt
index 78fe7cb..1769f71 100644
--- a/Documentation/scsi/ufs.txt
+++ b/Documentation/scsi/ufs.txt
@@ -150,9 +150,14 @@ send SG_IO with the applicable sg_io_v4:
 	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 write a descriptor, use the dout_xferp sg_io_v4.
+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 2fd0769..869e71f 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -48,12 +48,8 @@ static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
 	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;
@@ -71,8 +67,10 @@ static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
 	if (!descp)
 		return -ENOMEM;
 
-	sg_copy_to_buffer(job->request_payload.sg_list,
-			  job->request_payload.sg_cnt, descp, *desc_len);
+	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;
 
@@ -140,6 +138,12 @@ static int ufs_bsg_request(struct bsg_job *job)
 	if (!desc_buff)
 		goto out;
 
+	if (desc_op == UPIU_QUERY_OPCODE_READ_DESC && 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:
-- 
1.9.1

Re: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Evan Green]

On Sat, Jan 26, 2019 at 11:08 PM Avri Altman <avri.altman@wdc.com> wrote:
>
> Add this functionality, placing the descriptor being read in the actual
> data buffer in the bio.
>
> That is, for both read and write descriptors query upiu, we are using
> the job's request_payload.  This in turn, is mapped back in user land to
> the applicable sg_io_v4 xferp: dout_xferp for write descriptor,
> and din_xferp for read descriptor.
>
> Signed-off-by: Avri Altman <avri.altman@wdc.com>

Reviewed-by: Evan Green <evgreen@chromium.org>

RE: [EXT] [PATCH v4 2/3] scsi: ufs: Allow reading descriptor via raw upiu

[Bean Huo (beanhuo)]

>
>Signed-off-by: Avri Altman <avri.altman@wdc.com>
>Reviewed-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>

RE: [EXT] [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Bean Huo (beanhuo)]

>-----Original Message-----
>From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>owner@vger.kernel.org] On Behalf Of Avri Altman
>Sent: Sunday, January 27, 2019 8:08 AM
>To: James E.J. Bottomley <jejb@linux.vnet.ibm.com>; Martin K. Petersen
><martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux-
>kernel@vger.kernel.org; Evan Green <evgreen@chromium.org>
>Cc: Avi Shchislowski <avi.shchislowski@wdc.com>; Alex Lemberg
><alex.lemberg@wdc.com>; Avri Altman <avri.altman@wdc.com>
>Subject: [EXT] [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors
>
>Add this functionality, placing the descriptor being read in the actual data buffer
>in the bio.
>
>That is, for both read and write descriptors query upiu, we are using the job's
>request_payload.  This in turn, is mapped back in user land to the applicable
>sg_io_v4 xferp: dout_xferp for write descriptor, and din_xferp for read descriptor.
>
>Signed-off-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>

RE: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Avri Altman]

Martin,
Any further comments?

Thanks,
Avri


> -----Original Message-----
> From: Evan Green <evgreen@chromium.org>
> Sent: Monday, January 28, 2019 8:42 PM
> To: Avri Altman <Avri.Altman@wdc.com>
> Cc: James E.J. Bottomley <jejb@linux.vnet.ibm.com>; Martin K. Petersen
> <martin.petersen@oracle.com>; SCSI <linux-scsi@vger.kernel.org>; LKML
> <linux-kernel@vger.kernel.org>; Avi Shchislowski <Avi.Shchislowski@wdc.com>;
> Alex Lemberg <Alex.Lemberg@wdc.com>
> Subject: Re: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors
> 
> On Sat, Jan 26, 2019 at 11:08 PM Avri Altman <avri.altman@wdc.com> wrote:
> >
> > Add this functionality, placing the descriptor being read in the actual
> > data buffer in the bio.
> >
> > That is, for both read and write descriptors query upiu, we are using
> > the job's request_payload.  This in turn, is mapped back in user land to
> > the applicable sg_io_v4 xferp: dout_xferp for write descriptor,
> > and din_xferp for read descriptor.
> >
> > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> 
> Reviewed-by: Evan Green <evgreen@chromium.org>

RE: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Avri Altman]

Martin,
Is there any reason why this sires is not applied for 5.1?

Thanks,
Avri

> -----Original Message-----
> From: Avri Altman
> Sent: Tuesday, February 05, 2019 2:07 PM
> To: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: James E.J. Bottomley <jejb@linux.vnet.ibm.com>; SCSI <linux-
> scsi@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Avi
> Shchislowski <Avi.Shchislowski@wdc.com>; Alex Lemberg
> <Alex.Lemberg@wdc.com>; 'Evan Green' <evgreen@chromium.org>
> Subject: RE: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors
> 
> Martin,
> Any further comments?
> 
> Thanks,
> Avri
> 
> 
> > -----Original Message-----
> > From: Evan Green <evgreen@chromium.org>
> > Sent: Monday, January 28, 2019 8:42 PM
> > To: Avri Altman <Avri.Altman@wdc.com>
> > Cc: James E.J. Bottomley <jejb@linux.vnet.ibm.com>; Martin K. Petersen
> > <martin.petersen@oracle.com>; SCSI <linux-scsi@vger.kernel.org>; LKML
> > <linux-kernel@vger.kernel.org>; Avi Shchislowski
> <Avi.Shchislowski@wdc.com>;
> > Alex Lemberg <Alex.Lemberg@wdc.com>
> > Subject: Re: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors
> >
> > On Sat, Jan 26, 2019 at 11:08 PM Avri Altman <avri.altman@wdc.com>
> wrote:
> > >
> > > Add this functionality, placing the descriptor being read in the actual
> > > data buffer in the bio.
> > >
> > > That is, for both read and write descriptors query upiu, we are using
> > > the job's request_payload.  This in turn, is mapped back in user land to
> > > the applicable sg_io_v4 xferp: dout_xferp for write descriptor,
> > > and din_xferp for read descriptor.
> > >
> > > Signed-off-by: Avri Altman <avri.altman@wdc.com>
> >
> > Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Martin K. Petersen]

Avri,

> Is there any reason why this sires is not applied for 5.1?

I haven't had time to review it yet. I'll get there...

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Avri Altman]

> Avri,
> 
> > Is there any reason why this sires is not applied for 5.1?
> 
> I haven't had time to review it yet. I'll get there...
A kind reminder.

Thanks,
Avri

> 
> --
> Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors

[Martin K. Petersen]

Hi Avri,

> A kind reminder.

It's expired from patchwork so now is an excellent time to repost.

Generally speaking, resubmitting patches is superior to pinging. While I
personally dig through the archives to revisit older patches, I can
almost guarantee that no other potential reviewers will do the same. The
best way to get a patch back on people's radar is to repost it.

-- 
Martin K. Petersen	Oracle Linux Engineering