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"?
> 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"?
> + 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.
> +} __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).
> +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"?
> + 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?
Thanks,
Bart.
