[v2,1/2] scsi: ufs: Allow reading descriptor via raw upiu
diff mbox series

Message ID 1547135584-23953-2-git-send-email-avri.altman@wdc.com
State Superseded
Headers show
Series
  • scsi: ufs-bsg: Add read descriptor
Related show

Commit Message

Avri Altman Jan. 10, 2019, 3:53 p.m. UTC
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>
---
 drivers/scsi/ufs/ufshcd.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig Jan. 21, 2019, 8:43 a.m. UTC | #1
On Thu, Jan 10, 2019 at 05:53:03PM +0200, Avri Altman wrote:
> 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.

This still looks rather odd.  Why is this using a buffer after the
bsg_request?  Nornally you'd directly pass down the pages that bsg
locked down to the low-level driver to work on.
Avri Altman Jan. 21, 2019, 11:17 a.m. UTC | #2
> 
> On Thu, Jan 10, 2019 at 05:53:03PM +0200, Avri Altman wrote:
> > 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.
> 
> This still looks rather odd.  Why is this using a buffer after the
> bsg_request?  Nornally you'd directly pass down the pages that bsg
> locked down to the low-level driver to work on.
Right. All this is done in ufs-bsg as ufshcd has no concept of bsg job - 
See the next patch.

Thanks,
Avri
Christoph Hellwig Jan. 21, 2019, 4:51 p.m. UTC | #3
On Mon, Jan 21, 2019 at 11:17:31AM +0000, Avri Altman wrote:
> > 
> > On Thu, Jan 10, 2019 at 05:53:03PM +0200, Avri Altman wrote:
> > > 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.
> > 
> > This still looks rather odd.  Why is this using a buffer after the
> > bsg_request?  Nornally you'd directly pass down the pages that bsg
> > locked down to the low-level driver to work on.
> Right. All this is done in ufs-bsg as ufshcd has no concept of bsg job - 
> See the next patch.

Well, I still don't understand what you are actually doing in this patch
then.  The second and third hunks are pretty obvious, they remove the
previous restriction on UPIU_QUERY_OPCODE_WRITE_DESC with a non-NULL
buffer, and seem to vaguely match the patch description.

The first hunk on the other hand adds new code for the
UPIU_QUERY_OPCODE_READ_DESC case which copies data to the passed
in buffer from the hardware, that goes into the ->reply field of the
bsg buffer, which basically is the scsi sense buffer, that is out
of line return information.

But why are we returning information in the out of line reply buffer,
and not the in-line data buffer?  Maybe there is a good explanation for
that, but it clear is not documented in the commit log.

> 
> Thanks,
> Avri
---end quoted text---
Avri Altman Jan. 21, 2019, 7:13 p.m. UTC | #4
Hi,
> 
> On Mon, Jan 21, 2019 at 11:17:31AM +0000, Avri Altman wrote:
> > >
> > > On Thu, Jan 10, 2019 at 05:53:03PM +0200, Avri Altman wrote:
> > > > 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.
> > >
> > > This still looks rather odd.  Why is this using a buffer after the
> > > bsg_request?  Nornally you'd directly pass down the pages that bsg
> > > locked down to the low-level driver to work on.
> > Right. All this is done in ufs-bsg as ufshcd has no concept of bsg job -
> > See the next patch.
> 
> Well, I still don't understand what you are actually doing in this patch
> then.  The second and third hunks are pretty obvious, they remove the
> previous restriction on UPIU_QUERY_OPCODE_WRITE_DESC with a non-NULL
> buffer, and seem to vaguely match the patch description.
Right

> 
> The first hunk on the other hand adds new code for the
> UPIU_QUERY_OPCODE_READ_DESC case which copies data to the passed
> in buffer from the hardware, that goes into the ->reply field of the
> bsg buffer, which basically is the scsi sense buffer, that is out
> of line return information.
Ah - no.
We are using job->request_payload on both write descriptor (sg_copy_to_buffer),
As well as for read descriptor (sg_copy_from_buffer).
See the 2nd patch.

> 
> But why are we returning information in the out of line reply buffer,
> and not the in-line data buffer?  Maybe there is a good explanation for
> that, but it clear is not documented in the commit log.
> 
> >
> > Thanks,
> > Avri
> ---end quoted text---

Patch
diff mbox series

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9ba7671..8612d4b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5769,6 +5769,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);
@@ -5804,11 +5818,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;
@@ -5849,7 +5858,6 @@  int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 		break;
 	}
 
-out:
 	return err;
 }