Thanks,
Avri
> -----Original Message-----
> From: Bart Van Assche
> Sent: Wednesday, August 01, 2018 6:36 PM
> To: h...@lst.de; Avri Altman ; linux-scsi@vger.kernel.org;
> jthumsh...@suse.de; h...@suse.com; martin.peter...@oracle.com;
> j...@linux.vnet.ibm.com
> Cc: Vinayak Holikatti ; Avi Shchislowski ; Alex Lemberg ; Stanislav Nijnikov ;
> subha...@codeaurora.org
> Subject: Re: [PATCH 5/6] scsi: ufs-bsg: Add support for raw upiu in
> ufs_bsg_request()
>
> On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote:
> > + if (qr->opcode != UPIU_QUERY_OPCODE_WRITE_DESC ||
> > + desc_size <= 0)
> > + return -EINVAL;
>
> Please use the full line length and don't split lines if that is not
> necessary.
Done.
>
> > + ret = ufshcd_map_desc_id_to_length(bsg_host->hba, desc_id,
> buff_len);
> > +
> > + if (ret || !buff_len)
> > + return -EINVAL;
>
> Why is buff_len only tested after it has been passed as an argument to
> ufshcd_map_desc_id_to_length()? That seems weird to me.
buff_len here is an int * and I'm using it to verify that enough space was
allocated
in the case it is a "write descriptor" upiu.
So I need to fix 2 things:
1) should be if (ret || !(*buff_len)) and not if (ret || !buff_len)
2) don't utilize the buff_len variable for that, which is using to obtain
The reply_payload_rcv_len eventually. Use a separate desc_len variable.
>
> > +static int ufs_bsg_verify_query_size(unsigned int request_len,
> > + unsigned int reply_len,
> > + int rw, int buff_len)
> > +{
> > + int min_req_len = sizeof(struct ufs_bsg_request);
> > + int min_rsp_len = sizeof(struct ufs_bsg_reply);
> > +
> > + if (rw == UFS_BSG_NOP)
> > + goto null_buff;
> > +
> > + if (rw == WRITE)
> > + min_req_len += buff_len;
>
> Can the "goto" statement be avoided by using a switch/case on 'rw'?
Yes. Actually if (rw == UFS_BSG_NOP) is not needed at all.
>
> Thanks,
>
> Bart.
