| Message ID | 1547135584-23953-2-git-send-email-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs-bsg: Add read descriptor | expand |
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.
> > 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
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---
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---
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; }
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(-)