Bart,
Thanks a lot,
Avri
> -----Original Message-----
> From: Bart Van Assche
> Sent: Wednesday, August 01, 2018 6:13 PM
> To: [email protected]; Avri Altman ; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Cc: Vinayak Holikatti ; Avi Shchislowski ; Alex Lemberg ; Stanislav Nijnikov ;
> [email protected]
> Subject: Re: [PATCH 2/6] scsi: ufs: Add ufs-bsg module
>
> On Wed, 2018-08-01 at 11:04 +0300, Avri Altman wrote:
> > +++ b/drivers/scsi/ufs/ufs_bsg.c
> > @@ -0,0 +1,127 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SSCSI transport companion for UFS HBA
>
> What is "SSCSI"?
Done
>
> > diff --git a/drivers/scsi/ufs/ufs_bsg.h b/drivers/scsi/ufs/ufs_bsg.h
> > new file mode 100644
>
> [ ... ]
>
> > +struct ufs_bsg_request {
> > + uint32_t msgcode;
> > + struct utp_upiu_header header;
> > + union {
> > + struct utp_upiu_query qr;
> > + struct utp_upiu_task_req tr;
> > + /* use utp_upiu_query to host the 4 dwards of uic command
> */
>
> What are "dwards"?
Done.
>
> > + struct utp_upiu_query uc;
> > + } tsf;
> > + uint8_t data[0];
>
> The offset of the data member will change if a member will be added to the
> union with a larger size than the existing members. That seems like an API
> design bug to me.
Actually I am not using it, so will just remove it.
>
> > +} __packed;
>
> Would the data member offsets be the same without "__packed"? If so,
> __packed
> should be left out because it results in generation of suboptimal code on
> architectures that do not support unaligned multi-byte reads (e.g. IA-64).
Done
>
> > +struct ufs_bsg_reply {
> > + /*
> > + * The completion result. Result exists in two forms:
> > + * if negative, it is an -Exxx system errno value. There will
> > + * be no further reply information supplied.
> > + * else, it's the 4-byte scsi error result, with driver, host,
> > + * msg and status fields. The per-msgcode reply structure
> > + * will contain valid data.
> > + */
> > + uint32_t result;
> > +
> > + /* If there was reply_payload, how much was recevied ? */
> ^^^^^^^^
> Did you perhaps mean "received"?
Done.
>
> > + uint32_t reply_payload_rcv_len;
> > +
> > + struct utp_upiu_header header;
> > + union {
> > + struct utp_upiu_query qr;
> > + struct utp_upiu_task_rsp tr;
> > + struct utp_upiu_query uc;
> > + } tsf;
> > + uint8_t data[0];
> > +};
> > +
> > +struct ufs_bsg_raw_upiu {
> > + struct ufs_bsg_upiu request;
> > + struct ufs_bsg_upiu reply;
> > +};
>
> Are any of the above data structures needed by user space software? Should
> these
> data structure definitions perhaps be moved to a header file under
> include/uapi?
I wanted to benefit from ufs.h and ufshcd.h so used it here.
In include/uapi/scsi/scsi_bsg_ufs.h I defined an 8 dw*o*rds opaque structs for
the request and reply.
>
> Thanks,
>
> Bart.
