diff mbox series

[v3,1/7] scsi: ufs: Add ufs-bsg module

Message ID 1535970796-25582-2-git-send-email-avri.altman@wdc.com (mailing list archive)
State Superseded
Headers show
Series scsi: ufs bsg endpoint | expand

Commit Message

Avri Altman Sept. 3, 2018, 10:33 a.m. UTC
Add a bsg endpoint that supports UPIUs.

For now, just provide an API to allocate and remove
ufs-bsg node. We will use this framework to
manage ufs devices by sending UPIU transactions.

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.

Moreover and as a result, our core structure - ufs_bsg_node has
a pretty lean structure: using upiu transactions that contains
the outmost detailed info, so we don't really need complex
constructs to support it.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/Kconfig         |  19 +++++
 drivers/scsi/ufs/Makefile        |   1 +
 drivers/scsi/ufs/ufs_bsg.c       | 173 +++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs_bsg.h       |  70 ++++++++++++++++
 include/uapi/scsi/scsi_bsg_ufs.h |  56 +++++++++++++
 5 files changed, 319 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

Comments

Christoph Hellwig Sept. 4, 2018, 7:19 p.m. UTC | #1
> +config SCSI_UFS_BSG
> +	bool "Universal Flash Storage BSG device node"

So this a bool,

>  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_BSG) += ufs_bsg.o

But built as a separate module, which is rather odd.   I think
you wan to build this into the main ufshcd-core driver as a conditionally
compiled object instead.

> +
> +struct ufs_bsg {
> +	struct ufs_hba *hba;
> +	struct ufs_bsg_node *node;
> +};
> +static struct ufs_bsg *bsg_host;

This looks odd.  Why would you want a global variable like this?
I'd expect every ufs host to have a ufs_bsg_node, and in fact
we should probably just merge the ufs_bsg_node into the ufs_hba
structure.
Bart Van Assche Sept. 4, 2018, 9:08 p.m. UTC | #2
On Mon, 2018-09-03 at 13:33 +0300, Avri Altman wrote:

> [ ... ]
> +++ b/include/uapi/scsi/scsi_bsg_ufs.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * UFS Transport SGIO v4 BSG Message Support
> + *
> + * Copyright (C) 2018 Western Digital Corporation
> + */
> +#ifndef SCSI_BSG_UFS_H
> +#define SCSI_BSG_UFS_H
> +
> +/*
> + * This file intended to be included by both kernel and user space
> + */
> +
> +
> +/**
> + * struct ufs_bsg_upiu - upiu transaction structure
> + *
> + * @header: upiu header
> + * @tsf: Transaction Specific Fields
> + * @data: payload pointer
> + *
> + * This structure supports all ufs transaction types per JEDEC
> UFSv2.1
> + * paragraph 10.7
> + */
> +struct ufs_bsg_upiu {
> + uint32_t header[3];
> + uint32_t tsf[5];
> + uint8_t data[0];
> +};


In addition to Christoph's comments: is this a redefinition of an
existing data structure (struct utp_upiu_header)? Please do not
introduce variants of existing data structures but instead proceed as
follows:
- Move the relevant existing data structures (utp_upiu_header,
utp_upiu_query, ...) from drivers/scsi/ufs/ufs.h into a header file
under include/uapi.
- Add a new patch at the beginning of this series that does nothing
else than moving these data structures.
- Use the existing data structures instead of introducing struct ufs_bsg_upiu.

Thanks,

Bart.
Avri Altman Sept. 5, 2018, 6:03 a.m. UTC | #3
> -----Original Message-----
> From: Christoph Hellwig <hch@lst.de>
> Sent: Tuesday, September 04, 2018 10:20 PM
> To: Avri Altman <Avri.Altman@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>; Johannes Thumshirn
> <jthumshirn@suse.de>; Hannes Reinecke <hare@suse.com>; Bart Van Assche
> <Bart.VanAssche@wdc.com>; James E.J. Bottomley
> <jejb@linux.vnet.ibm.com>; Martin K. Petersen
> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; Stanislav Nijnikov
> <Stanislav.Nijnikov@wdc.com>; Avi Shchislowski
> <Avi.Shchislowski@wdc.com>; Alex Lemberg <Alex.Lemberg@wdc.com>;
> Subhash Jadavani <subhashj@codeaurora.org>; Vinayak Holikatti
> <Vinayak.Holikatti@wdc.com>
> Subject: Re: [PATCH v3 1/7] scsi: ufs: Add ufs-bsg module
> 
> > +config SCSI_UFS_BSG
> > +	bool "Universal Flash Storage BSG device node"
> 
> So this a bool,
> 
> >  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_BSG) += ufs_bsg.o
> 
> But built as a separate module, which is rather odd.   I think
> you wan to build this into the main ufshcd-core driver as a conditionally
> compiled object instead.
Done.

> 
> > +
> > +struct ufs_bsg {
> > +	struct ufs_hba *hba;
> > +	struct ufs_bsg_node *node;
> > +};
> > +static struct ufs_bsg *bsg_host;
> 
> This looks odd.  Why would you want a global variable like this?
> I'd expect every ufs host to have a ufs_bsg_node, and in fact
> we should probably just merge the ufs_bsg_node into the ufs_hba
> structure.
Done.

Thanks,
Avri
Avri Altman Sept. 5, 2018, 6:25 a.m. UTC | #4
> > + */
> > +#ifndef SCSI_BSG_UFS_H
> > +#define SCSI_BSG_UFS_H
> > +
> > +/*
> > + * This file intended to be included by both kernel and user space
> > + */
> > +
> > +
> > +/**
> > + * struct ufs_bsg_upiu - upiu transaction structure
> > + *
> > + * @header: upiu header
> > + * @tsf: Transaction Specific Fields
> > + * @data: payload pointer
> > + *
> > + * This structure supports all ufs transaction types per JEDEC
> > UFSv2.1
> > + * paragraph 10.7
> > + */
> > +struct ufs_bsg_upiu {
> > + uint32_t header[3];
> > + uint32_t tsf[5];
> > + uint8_t data[0];
> > +};
> 
> 
> In addition to Christoph's comments: is this a redefinition of an
> existing data structure (struct utp_upiu_header)? Please do not
> introduce variants of existing data structures but instead proceed as
> follows:
> - Move the relevant existing data structures (utp_upiu_header,
> utp_upiu_query, ...) from drivers/scsi/ufs/ufs.h into a header file
> under include/uapi.
> - Add a new patch at the beginning of this series that does nothing
> else than moving these data structures.
> - Use the existing data structures instead of introducing struct ufs_bsg_upiu.
Actually those are leftovers of scsi transport that I forgot to omit.
Sorry about that.
ufs-bsg is doing nothing with those, so it can be just left out.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index e09fe6a..83ba979 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -109,3 +109,22 @@  config SCSI_UFS_HISI
 
 	  Select this if you have UFS controller on Hisilicon chipset.
 	  If unsure, say N.
+
+config SCSI_UFS_BSG
+	bool "Universal Flash Storage BSG device node"
+	depends on SCSI_UFSHCD
+	select BLK_DEV_BSGLIB
+	help
+	  Universal Flash Storage (UFS) is SCSI transport specification for
+	  accessing flash storage on digital cameras, mobile phones and
+	  consumer electronic devices.
+	  An UFS controller communicates with an UFS device by exchanging
+	  UFS Protocol Information Units (UPIUs).
+	  UPIUs can not only be used as a transport layer for the SCSI protocol
+	  but are also used by the UFS native command set.
+	  This transport driver supports exchanging UFS protocol information units
+	  with an UFS device. See also the ufshcd driver, which is a SCSI driver
+	  that supports UFS devices.
+
+	  Select this if you need a bsg device node for your UFS controller.
+	  If unsure, say N.
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile
index 2c50f03..8f7138a 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_BSG) += ufs_bsg.o
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
new file mode 100644
index 0000000..b099ee2
--- /dev/null
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -0,0 +1,173 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * bsg endpoint that supports UPIUs
+ *
+ * Copyright (C) 2018 Western Digital Corporation
+ */
+#include "ufs_bsg.h"
+
+struct ufs_bsg_node {
+	struct device		dev;
+	struct request_queue	*q;
+};
+
+struct ufs_bsg {
+	struct ufs_hba *hba;
+	struct ufs_bsg_node *node;
+};
+static struct ufs_bsg *bsg_host;
+
+static inline struct ufs_bsg_node *dev_to_ufs_node(struct device *d)
+{
+	return container_of(d, struct ufs_bsg_node, dev);
+}
+
+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;
+}
+
+static void ufs_bsg_node_delete(struct ufs_bsg_node *node)
+{
+	struct device *dev = &node->dev;
+
+	if (node->q)
+		bsg_unregister_queue(node->q);
+
+	device_del(dev);
+
+	put_device(dev);
+}
+
+/**
+ * ufs_bsg_remove - detach and remove the added ufs-bsg node
+ *
+ * Should be called when unloading the driver.
+ */
+void ufs_bsg_remove(void)
+{
+	if (!bsg_host)
+		return;
+
+	if (bsg_host->node)
+		ufs_bsg_node_delete(bsg_host->node);
+
+	kfree(bsg_host);
+}
+
+static void ufs_bsg_node_release(struct device *dev)
+{
+	struct ufs_bsg_node *node = dev_to_ufs_node(dev);
+
+	put_device(dev->parent);
+	kfree(node);
+}
+
+static struct ufs_bsg_node *ufs_bsg_node_alloc(struct Scsi_Host *shost)
+{
+	struct ufs_bsg_node *node;
+	struct device *parent = &shost->shost_gendev;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return NULL;
+
+	device_initialize(&node->dev);
+
+	node->dev.parent = get_device(parent);
+	node->dev.release = ufs_bsg_node_release;
+
+	dev_set_name(&node->dev, "ufs-bsg-%d:0", shost->host_no);
+
+	return node;
+}
+
+static int ufs_bsg_initialize(struct Scsi_Host *shost,
+			      struct ufs_bsg_node *node)
+{
+	struct request_queue *q;
+
+	q = bsg_setup_queue(&node->dev, dev_name(&node->dev),
+			    ufs_bsg_request, 0);
+	if (IS_ERR(q))
+		return PTR_ERR(q);
+	node->q = q;
+
+	return 0;
+}
+
+static int ufs_bsg_node_add(struct ufs_bsg_node *node)
+{
+	struct device *dev = &node->dev;
+	struct Scsi_Host *shost = dev_to_shost(dev->parent);
+	int error;
+
+	error = device_add(dev);
+	if (error)
+		return error;
+
+	if (ufs_bsg_initialize(shost, node))
+		dev_err(dev, "fail to initialize a bsg dev %d\n",
+			shost->host_no);
+
+	return 0;
+}
+
+/**
+ * ufs_bsg_probe - Add ufs bsg device node
+ * @hba: per adapter object
+ *
+ * Called during initial loading of the driver, and before scsi_scan_host.
+ */
+int ufs_bsg_probe(struct ufs_hba *hba)
+{
+	struct ufs_bsg_node *node;
+	int ret;
+
+	bsg_host = kzalloc(sizeof(*bsg_host), GFP_KERNEL);
+	if (!bsg_host) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	node = ufs_bsg_node_alloc(hba->host);
+	if (!node) {
+		ret = -ENOMEM;
+		goto out_free_bsg_host;
+	}
+
+	ret = ufs_bsg_node_add(node);
+	if (ret)
+		goto out_free_node;
+
+	bsg_host->node = node;
+	bsg_host->hba = hba;
+
+	return 0;
+
+out_free_node:
+	put_device(&node->dev);
+out_free_bsg_host:
+	kfree(bsg_host);
+out:
+	return ret;
+}
diff --git a/drivers/scsi/ufs/ufs_bsg.h b/drivers/scsi/ufs/ufs_bsg.h
new file mode 100644
index 0000000..58eda73
--- /dev/null
+++ b/drivers/scsi/ufs/ufs_bsg.h
@@ -0,0 +1,70 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Western Digital Corporation
+ */
+#ifndef UFS_BSG_H
+#define UFS_BSG_H
+
+#include <linux/bsg-lib.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_host.h>
+
+#include <uapi/scsi/scsi_bsg_ufs.h>
+#include "ufshcd.h"
+#include "ufs.h"
+
+#define UPIU_TRANSACTION_UIC_CMD 0x1F
+
+enum {
+	REQ_UPIU_SIZE_DWORDS	= 8,
+	RSP_UPIU_SIZE_DWORDS	= 8,
+};
+
+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 dwords of uic command */
+		struct utp_upiu_query		uc;
+	} tsf;
+};
+
+
+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 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;
+};
+
+struct ufs_bsg_raw_upiu {
+	struct ufs_bsg_upiu request;
+	struct ufs_bsg_upiu reply;
+};
+
+#ifdef CONFIG_SCSI_UFS_BSG
+void ufs_bsg_remove(void);
+int ufs_bsg_probe(struct ufs_hba *hba);
+#else
+static inline void ufs_bsg_remove(void) {}
+static inline int ufs_bsg_probe(struct ufs_hba *hba) {return 0; }
+#endif
+
+#endif /* UFS_BSG_H */
diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h
new file mode 100644
index 0000000..5fb37be
--- /dev/null
+++ b/include/uapi/scsi/scsi_bsg_ufs.h
@@ -0,0 +1,56 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * UFS Transport SGIO v4 BSG Message Support
+ *
+ * Copyright (C) 2018 Western Digital Corporation
+ */
+#ifndef SCSI_BSG_UFS_H
+#define SCSI_BSG_UFS_H
+
+/*
+ * This file intended to be included by both kernel and user space
+ */
+
+
+/**
+ * struct ufs_bsg_upiu - upiu transaction structure
+ *
+ * @header: upiu header
+ * @tsf: Transaction Specific Fields
+ * @data: payload pointer
+ *
+ * This structure supports all ufs transaction types per JEDEC UFSv2.1
+ * paragraph 10.7
+ */
+struct ufs_bsg_upiu {
+	uint32_t header[3];
+	uint32_t tsf[5];
+	uint8_t data[0];
+};
+
+/* request (CDB) structure of the sg_io_v4 */
+struct ufs_bsg_upiu_request {
+	uint32_t msgcode;
+	struct ufs_bsg_upiu upiu;
+} __packed;
+
+/* response (request sense data) structure of the sg_io_v4 */
+struct ufs_bsg_upiu_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 ? */
+	uint32_t reply_payload_rcv_len;
+
+	struct ufs_bsg_upiu upiu;
+
+};
+
+#endif /* SCSI_BSG_UFS_H */