RE: [PATCH 2/6] scsi: ufs: Add ufs-bsg module
Bart,
Thanks a lot,
Avri
> -Original Message-
> From: Bart Van Assche
> Sent: Wednesday, August 01, 2018 6:13 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 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_reqtr;
> > + /* 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_rsptr;
> > + 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.
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"?
> 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_reqtr;
> + /* 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_rsptr;
> + 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.
[PATCH 2/6] scsi: ufs: Add ufs-bsg module
A LLD companion for the ufs scsi transport.
For now, just provide an API to instantiate the ufs transport,
allocating and removing ufs-ports.
As we are attaching the transport template to the scsi host
that was already alocated for the ufs hba,
we'll need to pay extra attention on where to call the
provided API from ufshcd.
For the time being, implements an empty bsg_request() -
will add some more functionality in coming patches.
Nonetheless, we reveal here the protocol we are planning to use:
UFS Transport Protocol Transactions. UFS transactions consist
of packets called UFS Protocol Information Units (UPIU).
There are UPIU’s defined for UFS SCSI commands, responses,
data in and data out, task management, utility functions,
vendor functions, transaction synchronization and control, and more.
By using UPIUs, we get access to the most fine-grained internals
of this protocol, and able to communicate with the device in ways,
that are sometimes beyond the capacity of the ufs driver.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/Makefile| 1 +
drivers/scsi/ufs/ufs_bsg.c | 127 +++
drivers/scsi/ufs/ufs_bsg.h | 74 +++
include/uapi/scsi/scsi_bsg_ufs.h | 56 +
4 files changed, 258 insertions(+)
create mode 100644 drivers/scsi/ufs/ufs_bsg.c
create mode 100644 drivers/scsi/ufs/ufs_bsg.h
create mode 100644 include/uapi/scsi/scsi_bsg_ufs.h
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 2c50f03..5227bfb 100644
--- a/drivers/scsi/ufs/Makefile
+++ b/drivers/scsi/ufs/Makefile
@@ -8,3 +8,4 @@ ufshcd-core-objs := ufshcd.o ufs-sysfs.o
obj-$(CONFIG_SCSI_UFSHCD_PCI) += ufshcd-pci.o
obj-$(CONFIG_SCSI_UFSHCD_PLATFORM) += ufshcd-pltfrm.o
obj-$(CONFIG_SCSI_UFS_HISI) += ufs-hisi.o
+obj-$(CONFIG_SCSI_UFS_ATTRS) += ufs_bsg.o
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
new file mode 100644
index 000..71826ba
--- /dev/null
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -0,0 +1,127 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SSCSI transport companion for UFS HBA
+ *
+ * Copyright (C) 2018 Western Digital Corporation
+ */
+#include "ufs_bsg.h"
+
+struct ufs_bsg {
+ struct ufs_hba *hba;
+ struct ufs_port *port;
+};
+static struct ufs_bsg *bsg_host;
+
+static int ufs_bsg_request(struct bsg_job *job);
+
+static struct scsi_transport_template *ufs_transport_template;
+static struct ufs_function_template ufs_transport_functions = {
+ .bsg_request = ufs_bsg_request,
+};
+
+static int ufs_bsg_request(struct bsg_job *job)
+{
+ struct ufs_bsg_request *bsg_request = job->request;
+ struct ufs_bsg_reply *bsg_reply = job->reply;
+ int ret = -ENOTSUPP;
+
+ bsg_reply->reply_payload_rcv_len = 0;
+
+ /* Do Nothing for now */
+ dev_err(job->dev, "unsupported message_code 0x%x\n",
+ bsg_request->msgcode);
+
+ bsg_reply->result = ret;
+ if (ret)
+ job->reply_len = sizeof(uint32_t);
+ else
+ job->reply_len = sizeof(struct ufs_bsg_reply) +
+bsg_reply->reply_payload_rcv_len;
+
+ bsg_job_done(job, bsg_reply->result, bsg_reply->reply_payload_rcv_len);
+
+ return ret;
+}
+
+/**
+ * ufs_bsg_attach_transport - instantiate UFS transport template
+ * @hba: hba to attach the transport template to
+ *
+ * Call this before scsi_add_host() publishes it to the scsi midlayer
+ */
+void ufs_bsg_attach_transport(struct ufs_hba *hba)
+{
+ ufs_transport_template =
+ ufs_attach_transport(_transport_functions);
+ if (!ufs_transport_template)
+ return;
+
+ hba->host->transportt = ufs_transport_template;
+}
+
+/**
+ * ufs_bsg_remove - detach and remove the added ufs-ports
+ * @shost: scsi host that is parenting the ufs-ports
+ *
+ * Should be called when unloading the driver.
+ */
+void ufs_bsg_remove(struct Scsi_Host *shost)
+{
+ if (!bsg_host)
+ return;
+
+ ufs_remove_host(shost);
+
+ if (ufs_transport_template) {
+ ufs_release_transport(ufs_transport_template);
+ ufs_transport_template = NULL;
+ }
+
+ kfree(bsg_host);
+}
+
+/**
+ * ufs_bsg_probe - Add and report ufs device to ufs transport
+ * @hba: per adapter object
+ *
+ * Called during initial loading of the driver, and before scsi_scan_host.
+ * Should be called only if the attach phase was successful
+ */
+
+int ufs_bsg_probe(struct ufs_hba *hba)
+{
+ struct ufs_port *port;
+ int ret;
+
+ if (WARN_ON(!ufs_transport_template))
+ return -ENODEV;
+
+ bsg_host = kzalloc(sizeof(*bsg_host), GFP_KERNEL);
+ if (!bsg_host) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ port = ufs_port_alloc(hba->host);
+ if (!port) {
+ ret = -ENOMEM;
+ goto out_free_bsg_host;
+ }
+
+ ret =
