[PATCH v7] scsi: ufs: add ioctl interface for query request

Subsystems: scsi subsystem, the rest

STALE3811d REVIEWED: 1 (0M)

1 review trailer.

15 messages, 3 authors, 2016-03-10 · open the first message on its own page

[PATCH v7] scsi: ufs: add ioctl interface for query request

From: Yaniv Gardi <hidden>
Date: 2016-03-09 14:12:00

This patch exposes the ioctl interface for UFS driver via SCSI device
ioctl interface. As of now UFS driver would provide the ioctl for query
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>

---
 drivers/scsi/ufs/ufs.h        |  53 +++--------
 drivers/scsi/ufs/ufshcd.c     | 208 +++++++++++++++++++++++++++++++++++++++++-
 include/uapi/scsi/Kbuild      |   1 +
 include/uapi/scsi/ufs/Kbuild  |   3 +
 include/uapi/scsi/ufs/ioctl.h |  58 ++++++++++++
 include/uapi/scsi/ufs/ufs.h   |  67 ++++++++++++++
 6 files changed, 347 insertions(+), 43 deletions(-)
 create mode 100644 include/uapi/scsi/ufs/Kbuild
 create mode 100644 include/uapi/scsi/ufs/ioctl.h
 create mode 100644 include/uapi/scsi/ufs/ufs.h
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index b291fa6..d39410f 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -38,6 +38,7 @@
 
 #include <linux/mutex.h>
 #include <linux/types.h>
+#include <scsi/ufs/ufs.h>
 
 #define MAX_CDB_SIZE	16
 #define GENERAL_UPIU_REQUEST_SIZE 32
@@ -72,6 +73,16 @@ enum {
 	UFS_UPIU_RPMB_WLUN		= 0xC4,
 };
 
+/**
+ * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit descriptor
+ * @lun: LU number to check
+ * @return: true if the lun has a matching unit descriptor, false otherwise
+ */
+static inline bool ufs_is_valid_unit_desc_lun(u8 lun)
+{
+	return lun == UFS_UPIU_RPMB_WLUN || (lun < UFS_UPIU_MAX_GENERAL_LUN);
+}
+
 /*
  * UFS Protocol Information Unit related definitions
  */
@@ -127,35 +138,6 @@ enum {
 	UPIU_QUERY_FUNC_STANDARD_WRITE_REQUEST          = 0x81,
 };
 
-/* Flag idn for Query Requests*/
-enum flag_idn {
-	QUERY_FLAG_IDN_FDEVICEINIT      = 0x01,
-	QUERY_FLAG_IDN_PWR_ON_WPE	= 0x03,
-	QUERY_FLAG_IDN_BKOPS_EN         = 0x04,
-};
-
-/* Attribute idn for Query requests */
-enum attr_idn {
-	QUERY_ATTR_IDN_ACTIVE_ICC_LVL	= 0x03,
-	QUERY_ATTR_IDN_BKOPS_STATUS	= 0x05,
-	QUERY_ATTR_IDN_EE_CONTROL	= 0x0D,
-	QUERY_ATTR_IDN_EE_STATUS	= 0x0E,
-};
-
-/* Descriptor idn for Query requests */
-enum desc_idn {
-	QUERY_DESC_IDN_DEVICE		= 0x0,
-	QUERY_DESC_IDN_CONFIGURAION	= 0x1,
-	QUERY_DESC_IDN_UNIT		= 0x2,
-	QUERY_DESC_IDN_RFU_0		= 0x3,
-	QUERY_DESC_IDN_INTERCONNECT	= 0x4,
-	QUERY_DESC_IDN_STRING		= 0x5,
-	QUERY_DESC_IDN_RFU_1		= 0x6,
-	QUERY_DESC_IDN_GEOMETRY		= 0x7,
-	QUERY_DESC_IDN_POWER		= 0x8,
-	QUERY_DESC_IDN_MAX,
-};
-
 enum desc_header_offset {
 	QUERY_DESC_LENGTH_OFFSET	= 0x00,
 	QUERY_DESC_DESC_TYPE_OFFSET	= 0x01,
@@ -279,19 +261,6 @@ enum bkops_status {
 	BKOPS_STATUS_MAX		 = BKOPS_STATUS_CRITICAL,
 };
 
-/* UTP QUERY Transaction Specific Fields OpCode */
-enum query_opcode {
-	UPIU_QUERY_OPCODE_NOP		= 0x0,
-	UPIU_QUERY_OPCODE_READ_DESC	= 0x1,
-	UPIU_QUERY_OPCODE_WRITE_DESC	= 0x2,
-	UPIU_QUERY_OPCODE_READ_ATTR	= 0x3,
-	UPIU_QUERY_OPCODE_WRITE_ATTR	= 0x4,
-	UPIU_QUERY_OPCODE_READ_FLAG	= 0x5,
-	UPIU_QUERY_OPCODE_SET_FLAG	= 0x6,
-	UPIU_QUERY_OPCODE_CLEAR_FLAG	= 0x7,
-	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
-};
-
 /* Query response result code */
 enum {
 	QUERY_RESULT_SUCCESS                    = 0x00,
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index da882cf..65a9d39 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -38,6 +38,7 @@
  */
 
 #include <linux/async.h>
+#include <scsi/ufs/ioctl.h>
 #include <linux/devfreq.h>
 #include <linux/nls.h>
 #include <linux/of.h>
@@ -2178,7 +2179,7 @@ static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba,
 	 * Unit descriptors are only available for general purpose LUs (LUN id
 	 * from 0 to 7) and RPMB Well known LU.
 	 */
-	if (lun != UFS_UPIU_RPMB_WLUN && (lun >= UFS_UPIU_MAX_GENERAL_LUN))
+	if (!ufs_is_valid_unit_desc_lun(lun))
 		return -EOPNOTSUPP;
 
 	return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun,
@@ -5001,6 +5002,207 @@ out:
 }
 
 /**
+ * ufshcd_query_ioctl - perform user read queries
+ * @hba: per-adapter instance
+ * @lun: used for lun specific queries
+ * @buffer: user space buffer for reading and submitting query data and params
+ * @return: 0 for success negative error code otherwise
+ *
+ * Expected/Submitted buffer structure is struct ufs_ioctl_query_data.
+ * It will read the opcode, idn and buf_length parameters, and, put the
+ * response in the buffer field while updating the used size in buf_length.
+ */
+static int ufshcd_query_ioctl(struct ufs_hba *hba, u8 lun, void __user *buffer)
+{
+	struct ufs_ioctl_query_data ioctl_data;
+	int err = 0;
+	int length = 0;
+	void *data_ptr;
+	bool flag;
+	u32 att;
+	u8 index;
+	u8 *desc = NULL;
+
+	/* extract params from user buffer */
+	if (copy_from_user(&ioctl_data, buffer,
+			sizeof(struct ufs_ioctl_query_data))) {
+		err = -EFAULT;
+		goto out;
+	}
+
+	/* verify legal parameters & send query */
+	switch (ioctl_data.opcode) {
+	case UPIU_QUERY_OPCODE_READ_DESC:
+		switch (ioctl_data.idn) {
+		case QUERY_DESC_IDN_DEVICE:
+		case QUERY_DESC_IDN_CONFIGURAION:
+		case QUERY_DESC_IDN_INTERCONNECT:
+		case QUERY_DESC_IDN_GEOMETRY:
+		case QUERY_DESC_IDN_POWER:
+			index = 0;
+			break;
+		case QUERY_DESC_IDN_UNIT:
+			if (!ufs_is_valid_unit_desc_lun(lun)) {
+				dev_err(hba->dev,
+					"%s: No unit descriptor for lun 0x%x\n",
+					__func__, lun);
+				err = -EINVAL;
+				goto out;
+			}
+			index = lun;
+			break;
+		default:
+			goto out;
+		}
+
+		if (ioctl_data.buf_size > QUERY_DESC_MAX_SIZE) {
+			err = -EINVAL;
+			goto out;
+		}
+
+		desc = kzalloc(ioctl_data.buf_size, GFP_KERNEL);
+		if (!desc) {
+			err = -ENOMEM;
+			goto out;
+		}
+
+		length = ioctl_data.buf_size;
+		err = ufshcd_query_descriptor_retry(hba, ioctl_data.opcode,
+				ioctl_data.idn, index, 0, desc, &length);
+		break;
+	case UPIU_QUERY_OPCODE_READ_ATTR:
+		switch (ioctl_data.idn) {
+		case QUERY_ATTR_IDN_BOOT_LU_EN:
+		case QUERY_ATTR_IDN_POWER_MODE:
+		case QUERY_ATTR_IDN_ACTIVE_ICC_LVL:
+		case QUERY_ATTR_IDN_OOO_DATA_EN:
+		case QUERY_ATTR_IDN_BKOPS_STATUS:
+		case QUERY_ATTR_IDN_PURGE_STATUS:
+		case QUERY_ATTR_IDN_MAX_DATA_IN:
+		case QUERY_ATTR_IDN_MAX_DATA_OUT:
+		case QUERY_ATTR_IDN_REF_CLK_FREQ:
+		case QUERY_ATTR_IDN_CONF_DESC_LOCK:
+		case QUERY_ATTR_IDN_MAX_NUM_OF_RTT:
+		case QUERY_ATTR_IDN_EE_CONTROL:
+		case QUERY_ATTR_IDN_EE_STATUS:
+		case QUERY_ATTR_IDN_SECONDS_PASSED:
+			index = 0;
+			break;
+		case QUERY_ATTR_IDN_DYN_CAP_NEEDED:
+		case QUERY_ATTR_IDN_CORR_PRG_BLK_NUM:
+			index = lun;
+			break;
+		default:
+			goto out_einval;
+		}
+		err = ufshcd_query_attr(hba, ioctl_data.opcode,
+					ioctl_data.idn, index, 0, &att);
+		break;
+	case UPIU_QUERY_OPCODE_READ_FLAG:
+		switch (ioctl_data.idn) {
+		case QUERY_FLAG_IDN_FDEVICEINIT:
+		case QUERY_FLAG_IDN_PERMANENT_WPE:
+		case QUERY_FLAG_IDN_PWR_ON_WPE:
+		case QUERY_FLAG_IDN_BKOPS_EN:
+		case QUERY_FLAG_IDN_PURGE_ENABLE:
+		case QUERY_FLAG_IDN_FPHYRESOURCEREMOVAL:
+		case QUERY_FLAG_IDN_BUSY_RTC:
+			break;
+		default:
+			goto out_einval;
+		}
+		err = ufshcd_query_flag_retry(hba, ioctl_data.opcode,
+				ioctl_data.idn, &flag);
+		break;
+	default:
+		goto out_einval;
+	}
+
+	if (err) {
+		dev_err(hba->dev, "%s: Query for idn %d failed\n", __func__,
+				ioctl_data.idn);
+		goto out_release_mem;
+	}
+
+	/* copy response data */
+	switch (ioctl_data.opcode) {
+	case UPIU_QUERY_OPCODE_READ_DESC:
+		ioctl_data.buf_size = length;
+		data_ptr = desc;
+		break;
+	case UPIU_QUERY_OPCODE_READ_ATTR:
+		ioctl_data.buf_size = sizeof(u32);
+		data_ptr = &att;
+		break;
+	case UPIU_QUERY_OPCODE_READ_FLAG:
+		ioctl_data.buf_size = 1;
+		data_ptr = &flag;
+		break;
+	default:
+		BUG_ON(true);
+	}
+
+	/* copy to user */
+	err = copy_to_user(buffer, &ioctl_data,
+			sizeof(struct ufs_ioctl_query_data));
+	if (err)
+		dev_err(hba->dev, "%s: Failed copying back to user.\n",
+			__func__);
+	err = copy_to_user(buffer + sizeof(struct ufs_ioctl_query_data),
+			data_ptr, ioctl_data.buf_size);
+	if (err)
+		dev_err(hba->dev, "%s: err %d copying back to user.\n",
+				__func__, err);
+	goto out_release_mem;
+
+out_einval:
+	dev_err(hba->dev,
+		"%s: illegal ufs query ioctl data, opcode 0x%x, idn 0x%x\n",
+		__func__, ioctl_data.opcode, (unsigned int)ioctl_data.idn);
+	err = -EINVAL;
+out_release_mem:
+	kfree(desc);
+out:
+	return err;
+}
+
+/**
+ * ufshcd_ioctl - ufs ioctl callback registered in scsi_host
+ * @dev: scsi device required for per LUN queries
+ * @cmd: command opcode
+ * @buffer: user space buffer for transferring data
+ *
+ * Supported commands:
+ * UFS_IOCTL_QUERY
+ */
+static int ufshcd_ioctl(struct scsi_device *dev, int cmd, void __user *buffer)
+{
+	struct ufs_hba *hba = shost_priv(dev->host);
+	int err = 0;
+
+	BUG_ON(!hba);
+	if (!buffer)
+		return -EINVAL;
+
+	switch (cmd) {
+	case UFS_IOCTL_QUERY:
+		pm_runtime_get_sync(hba->dev);
+		err = ufshcd_query_ioctl(hba, ufshcd_scsi_to_upiu_lun(dev->lun),
+				buffer);
+		pm_runtime_put_sync(hba->dev);
+		break;
+	case BLKROSET:
+		err = -ENOIOCTLCMD;
+		break;
+	default:
+		err = -EINVAL;
+		break;
+	}
+
+	return err;
+}
+
+/**
  * ufshcd_async_scan - asynchronous execution for probing hba
  * @data: data pointer to pass to this function
  * @cookie: cookie data
@@ -5060,6 +5262,10 @@ static struct scsi_host_template ufshcd_driver_template = {
 	.eh_device_reset_handler = ufshcd_eh_device_reset_handler,
 	.eh_host_reset_handler   = ufshcd_eh_host_reset_handler,
 	.eh_timed_out		= ufshcd_eh_timed_out,
+	.ioctl			= ufshcd_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl		= ufshcd_ioctl,
+#endif
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
 	.cmd_per_lun		= UFSHCD_CMD_PER_LUN,
diff --git a/include/uapi/scsi/Kbuild b/include/uapi/scsi/Kbuild
index d791e0a..fad00e0 100644
--- a/include/uapi/scsi/Kbuild
+++ b/include/uapi/scsi/Kbuild
@@ -1,5 +1,6 @@
 # UAPI Header export list
 header-y += fc/
+header-y += ufs/
 header-y += scsi_bsg_fc.h
 header-y += scsi_netlink.h
 header-y += scsi_netlink_fc.h
diff --git a/include/uapi/scsi/ufs/Kbuild b/include/uapi/scsi/ufs/Kbuild
new file mode 100644
index 0000000..cc3ef20
--- /dev/null
+++ b/include/uapi/scsi/ufs/Kbuild
@@ -0,0 +1,3 @@
+# UAPI Header export list
+header-y += ioctl.h
+header-y += ufs.h
diff --git a/include/uapi/scsi/ufs/ioctl.h b/include/uapi/scsi/ufs/ioctl.h
new file mode 100644
index 0000000..3a8fa77
--- /dev/null
+++ b/include/uapi/scsi/ufs/ioctl.h
@@ -0,0 +1,58 @@
+#ifndef UAPI_UFS_IOCTL_H_
+#define UAPI_UFS_IOCTL_H_
+
+#include <linux/types.h>
+
+/*
+ *  IOCTL opcode for ufs queries has the following opcode after
+ *  SCSI_IOCTL_GET_PCI
+ */
+#define UFS_IOCTL_QUERY		_IOWR('L', 0x88, struct ufs_ioctl_query_data)
+
+/**
+ * struct ufs_ioctl_query_data - used to transfer data to and from
+ * user via ioctl
+ * @opcode: type of data to query (descriptor/attribute/flag)
+ * @idn: id of the data structure
+ * @buf_size: number of allocated bytes/data size on return
+ * @buffer: data location
+ *
+ * Received: buffer and buf_size (available space for transferred data)
+ * Submitted: opcode, idn, length, buf_size
+ */
+struct ufs_ioctl_query_data {
+	/*
+	 * User should select one of the opcode defined in "enum query_opcode".
+	 * Please check include/uapi/scsi/ufs/ufs.h for the definition of it.
+	 * Note that only UPIU_QUERY_OPCODE_READ_DESC,
+	 * UPIU_QUERY_OPCODE_READ_ATTR & UPIU_QUERY_OPCODE_READ_FLAG are
+	 * supported as of now. All other query_opcode would be considered
+	 * invalid.
+	 * As of now only read query operations are supported.
+	 */
+	__u32 opcode;
+	/*
+	 * User should specify the size of the buffer (buffer[0] below) where
+	 * it wants to read the query data (attribute/flag/descriptor).
+	 * As we might end up reading less data then what is specified in
+	 * buf_size. So we are updating buf_size to what exactly we have read.
+	 */
+	__u16 buf_size;
+	/*
+	 * User should select one of the idn from "enum flag_idn" or "enum
+	 * attr_idn" or "enum desc_idn" based on whether opcode above is
+	 * attribute, flag or descriptor.
+	 * Please check include/uapi/scsi/ufs/ufs.h for the definition of it.
+	 */
+	__u8 idn;
+	/*
+	 * placeholder for the start of the data buffer where kernel will copy
+	 * the query data (attribute/flag/descriptor) read from the UFS device
+	 * Note:
+	 * For Read Attribute you will have to allocate 4 bytes
+	 * For Read Flag you will have to allocate 1 byte
+	 */
+	__u8 buffer[0];
+};
+
+#endif /* UAPI_UFS_IOCTL_H_ */
diff --git a/include/uapi/scsi/ufs/ufs.h b/include/uapi/scsi/ufs/ufs.h
new file mode 100644
index 0000000..8e4baff
--- /dev/null
+++ b/include/uapi/scsi/ufs/ufs.h
@@ -0,0 +1,67 @@
+#ifndef UAPI_UFS_H_
+#define UAPI_UFS_H_
+
+/* Flag idn for Query Requests*/
+enum flag_idn {
+	QUERY_FLAG_IDN_FDEVICEINIT		= 0x01,
+	QUERY_FLAG_IDN_PERMANENT_WPE		= 0x02,
+	QUERY_FLAG_IDN_PWR_ON_WPE		= 0x03,
+	QUERY_FLAG_IDN_BKOPS_EN			= 0x04,
+	QUERY_FLAG_IDN_RESERVED1		= 0x05,
+	QUERY_FLAG_IDN_PURGE_ENABLE		= 0x06,
+	QUERY_FLAG_IDN_RESERVED2		= 0x07,
+	QUERY_FLAG_IDN_FPHYRESOURCEREMOVAL      = 0x08,
+	QUERY_FLAG_IDN_BUSY_RTC			= 0x09,
+};
+
+/* Attribute idn for Query requests */
+enum attr_idn {
+	QUERY_ATTR_IDN_BOOT_LU_EN		= 0x00,
+	QUERY_ATTR_IDN_RESERVED			= 0x01,
+	QUERY_ATTR_IDN_POWER_MODE		= 0x02,
+	QUERY_ATTR_IDN_ACTIVE_ICC_LVL		= 0x03,
+	QUERY_ATTR_IDN_OOO_DATA_EN		= 0x04,
+	QUERY_ATTR_IDN_BKOPS_STATUS		= 0x05,
+	QUERY_ATTR_IDN_PURGE_STATUS		= 0x06,
+	QUERY_ATTR_IDN_MAX_DATA_IN		= 0x07,
+	QUERY_ATTR_IDN_MAX_DATA_OUT		= 0x08,
+	QUERY_ATTR_IDN_DYN_CAP_NEEDED		= 0x09,
+	QUERY_ATTR_IDN_REF_CLK_FREQ		= 0x0A,
+	QUERY_ATTR_IDN_CONF_DESC_LOCK		= 0x0B,
+	QUERY_ATTR_IDN_MAX_NUM_OF_RTT		= 0x0C,
+	QUERY_ATTR_IDN_EE_CONTROL		= 0x0D,
+	QUERY_ATTR_IDN_EE_STATUS		= 0x0E,
+	QUERY_ATTR_IDN_SECONDS_PASSED		= 0x0F,
+	QUERY_ATTR_IDN_CNTX_CONF		= 0x10,
+	QUERY_ATTR_IDN_CORR_PRG_BLK_NUM		= 0x11,
+};
+
+/* Descriptor idn for Query requests */
+enum desc_idn {
+	QUERY_DESC_IDN_DEVICE		= 0x0,
+	QUERY_DESC_IDN_CONFIGURAION	= 0x1,
+	QUERY_DESC_IDN_UNIT		= 0x2,
+	QUERY_DESC_IDN_RFU_0		= 0x3,
+	QUERY_DESC_IDN_INTERCONNECT	= 0x4,
+	QUERY_DESC_IDN_STRING		= 0x5,
+	QUERY_DESC_IDN_RFU_1		= 0x6,
+	QUERY_DESC_IDN_GEOMETRY		= 0x7,
+	QUERY_DESC_IDN_POWER		= 0x8,
+	QUERY_DESC_IDN_RFU_2		= 0x9,
+	QUERY_DESC_IDN_MAX,
+};
+
+/* UTP QUERY Transaction Specific Fields OpCode */
+enum query_opcode {
+	UPIU_QUERY_OPCODE_NOP		= 0x0,
+	UPIU_QUERY_OPCODE_READ_DESC	= 0x1,
+	UPIU_QUERY_OPCODE_WRITE_DESC	= 0x2,
+	UPIU_QUERY_OPCODE_READ_ATTR	= 0x3,
+	UPIU_QUERY_OPCODE_WRITE_ATTR	= 0x4,
+	UPIU_QUERY_OPCODE_READ_FLAG	= 0x5,
+	UPIU_QUERY_OPCODE_SET_FLAG	= 0x6,
+	UPIU_QUERY_OPCODE_CLEAR_FLAG	= 0x7,
+	UPIU_QUERY_OPCODE_TOGGLE_FLAG	= 0x8,
+	UPIU_QUERY_OPCODE_MAX,
+};
+#endif /* UAPI_UFS_H_ */
-- 
1.8.5.2

-- 
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2016-03-09 16:29:55

On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
This patch exposes the ioctl interface for UFS driver via SCSI device
ioctl interface. As of now UFS driver would provide the ioctl for query
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want to do
something "special" with UFS devices?  Shouldn't they just be treated
like any other normal block device?

thanks,

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: <hidden>
Date: 2016-03-09 19:09:53

On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI device
ioctl interface. As of now UFS driver would provide the ioctl for query
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want to do
something "special" with UFS devices?  Shouldn't they just be treated
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and descriptors.
Not only ioctl interface is a useful way to interact with the device,
we used it, and found it very helpful in varies cases.
hence, this patch.
This patch has been already addressed all comments of Arnd Bergman from 5
months ago, and now, re-uploaded again.

thanks,
Yaniv
thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: <hidden>
Date: 2016-03-09 19:10:14

On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI device
ioctl interface. As of now UFS driver would provide the ioctl for query
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want to do
something "special" with UFS devices?  Shouldn't they just be treated
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and descriptors.
Not only ioctl interface is a useful way to interact with the device,
we used it, and found it very helpful in varies cases.
hence, this patch.
This patch has been already addressed all comments of Arnd Bergman from 5
months ago, and now, re-uploaded again.

thanks,
Yaniv
thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2016-03-09 20:18:24

On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org wrote:
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI device
ioctl interface. As of now UFS driver would provide the ioctl for query
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want to do
something "special" with UFS devices?  Shouldn't they just be treated
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and descriptors.
But do you want to do with that information?  Why does userspace care?
Not only ioctl interface is a useful way to interact with the device,
we used it, and found it very helpful in varies cases.
In what case was it helpful?  Why does userspace care about ufs
specifics, it should just treat it like any other block device and not
care at all.

thanks,

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: <hidden>
Date: 2016-03-09 20:53:10

On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI device
ioctl interface. As of now UFS driver would provide the ioctl for
query
quoted
quoted
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want to do
something "special" with UFS devices?  Shouldn't they just be treated
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and descriptors.
But do you want to do with that information?  Why does userspace care?
i don't really understand the subtext of your question -
as ANY ioctl cb, we decided to implement the ioctl callback of this scsi
device in order to get information like UNIT DESC, DEVICE DESC, FLAGs,
ATTRIBUTES.
When dealing with UFS devices, one should be able to read the
characteristics of the device. why ? well, why not ?
during development of this driver, it was useful in many cases to be able
to communicate with the device, by simple IOCTL command, rather than
implementing ad-hock.

regards,
Yaniv
quoted
Not only ioctl interface is a useful way to interact with the device,
we used it, and found it very helpful in varies cases.
In what case was it helpful?  Why does userspace care about ufs
specifics, it should just treat it like any other block device and not
care at all.

thanks,

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2016-03-09 22:47:23

On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI device
ioctl interface. As of now UFS driver would provide the ioctl for
query
quoted
quoted
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want to do
something "special" with UFS devices?  Shouldn't they just be treated
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and descriptors.
But do you want to do with that information?  Why does userspace care?
i don't really understand the subtext of your question -
as ANY ioctl cb, we decided to implement the ioctl callback of this scsi
device in order to get information like UNIT DESC, DEVICE DESC, FLAGs,
ATTRIBUTES.
When dealing with UFS devices, one should be able to read the
characteristics of the device. why ? well, why not ?
Why aren't those characteristics just exported as sysfs attributes under
control by the UFS controller driver?  Why do you need/want an ioctl for
this?
during development of this driver, it was useful in many cases to be able
to communicate with the device, by simple IOCTL command, rather than
implementing ad-hock.
Do other storage busses have these types of "custom" ioctls for their
bus-type alone?  For simple attributes like this, shouldn't you be using
sysfs instead so that it is much easier for userspace tools to get
access to them?

thanks,

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: <hidden>
Date: 2016-03-10 15:53:06

On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI
device
quoted
quoted
quoted
quoted
ioctl interface. As of now UFS driver would provide the ioctl for
query
quoted
quoted
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want to
do
quoted
quoted
quoted
something "special" with UFS devices?  Shouldn't they just be
treated
quoted
quoted
quoted
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and
descriptors.
quoted
But do you want to do with that information?  Why does userspace care?
i don't really understand the subtext of your question -
as ANY ioctl cb, we decided to implement the ioctl callback of this scsi
device in order to get information like UNIT DESC, DEVICE DESC, FLAGs,
ATTRIBUTES.
When dealing with UFS devices, one should be able to read the
characteristics of the device. why ? well, why not ?
Why aren't those characteristics just exported as sysfs attributes under
control by the UFS controller driver?  Why do you need/want an ioctl for
this?
Hi greg k-h,

in our code, we used the IOCTL during runtime, in order to determine some
information about the RPMB well known lun.
with the rpmb lun ID we could then go to /dev/sgX and issue
UFS_IOCTL_QUERY to this lun and get the data -
reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the
QUERY_DESC_IDN_UNIT descriptor.

this was crucial to the work we do in RPMB.

thanks,
Yaniv
quoted
during development of this driver, it was useful in many cases to be
able
to communicate with the device, by simple IOCTL command, rather than
implementing ad-hock.
Do other storage busses have these types of "custom" ioctls for their
bus-type alone?  For simple attributes like this, shouldn't you be using
sysfs instead so that it is much easier for userspace tools to get
access to them?

thanks,

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2016-03-10 16:24:50

On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI
device
quoted
quoted
quoted
quoted
ioctl interface. As of now UFS driver would provide the ioctl for
query
quoted
quoted
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want to
do
quoted
quoted
quoted
something "special" with UFS devices?  Shouldn't they just be
treated
quoted
quoted
quoted
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and
descriptors.
quoted
But do you want to do with that information?  Why does userspace care?
i don't really understand the subtext of your question -
as ANY ioctl cb, we decided to implement the ioctl callback of this scsi
device in order to get information like UNIT DESC, DEVICE DESC, FLAGs,
ATTRIBUTES.
When dealing with UFS devices, one should be able to read the
characteristics of the device. why ? well, why not ?
Why aren't those characteristics just exported as sysfs attributes under
control by the UFS controller driver?  Why do you need/want an ioctl for
this?
Hi greg k-h,

in our code, we used the IOCTL during runtime, in order to determine some
information about the RPMB well known lun.
with the rpmb lun ID we could then go to /dev/sgX and issue
UFS_IOCTL_QUERY to this lun and get the data -
reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the
QUERY_DESC_IDN_UNIT descriptor.

this was crucial to the work we do in RPMB.
What is RPMB?

And again, why not just use sysfs attributes on your host controller
device?  Why does this have to be a custom ioctl?

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: <hidden>
Date: 2016-03-10 16:30:05

On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi@codeaurora.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org
wrote:
quoted
quoted
quoted
quoted
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI
device
quoted
quoted
quoted
quoted
ioctl interface. As of now UFS driver would provide the ioctl
for
quoted
quoted
quoted
quoted
query
quoted
quoted
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want
to
quoted
quoted
do
quoted
quoted
quoted
something "special" with UFS devices?  Shouldn't they just be
treated
quoted
quoted
quoted
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that
sent
quoted
quoted
quoted
quoted
queries to the UFS devices in order to get information and
descriptors.
quoted
But do you want to do with that information?  Why does userspace
care?
quoted
quoted
quoted
i don't really understand the subtext of your question -
as ANY ioctl cb, we decided to implement the ioctl callback of this
scsi
quoted
quoted
device in order to get information like UNIT DESC, DEVICE DESC,
FLAGs,
quoted
quoted
ATTRIBUTES.
When dealing with UFS devices, one should be able to read the
characteristics of the device. why ? well, why not ?
Why aren't those characteristics just exported as sysfs attributes
under
quoted
control by the UFS controller driver?  Why do you need/want an ioctl
for
quoted
this?
Hi greg k-h,

in our code, we used the IOCTL during runtime, in order to determine
some
information about the RPMB well known lun.
with the rpmb lun ID we could then go to /dev/sgX and issue
UFS_IOCTL_QUERY to this lun and get the data -
reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the
QUERY_DESC_IDN_UNIT descriptor.

this was crucial to the work we do in RPMB.
What is RPMB?

And again, why not just use sysfs attributes on your host controller
device?  Why does this have to be a custom ioctl?

greg k-h
RPMB is spcial Logical Unit in the UFS device. you can read about it in
the UFS spec. Greg, you are insisting on sysfs, but i can't implement it
now, as i don't have the Hardware anymore, or the time.

This is a tested and verified code that was accepted and reviewed already,
so i'm not sure what is wrong with this solution, not to say, it's already
implemented, tested and verified.
hope you are help us push it.

thanks,
Yaniv

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2016-03-10 16:39:44

On Thu, Mar 10, 2016 at 04:29:55PM -0000, ygardi@codeaurora.org wrote:
quoted
On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi@codeaurora.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org
wrote:
quoted
quoted
quoted
quoted
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via SCSI
device
quoted
quoted
quoted
quoted
ioctl interface. As of now UFS driver would provide the ioctl
for
quoted
quoted
quoted
quoted
query
quoted
quoted
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae want
to
quoted
quoted
do
quoted
quoted
quoted
something "special" with UFS devices?  Shouldn't they just be
treated
quoted
quoted
quoted
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that
sent
quoted
quoted
quoted
quoted
queries to the UFS devices in order to get information and
descriptors.
quoted
But do you want to do with that information?  Why does userspace
care?
quoted
quoted
quoted
i don't really understand the subtext of your question -
as ANY ioctl cb, we decided to implement the ioctl callback of this
scsi
quoted
quoted
device in order to get information like UNIT DESC, DEVICE DESC,
FLAGs,
quoted
quoted
ATTRIBUTES.
When dealing with UFS devices, one should be able to read the
characteristics of the device. why ? well, why not ?
Why aren't those characteristics just exported as sysfs attributes
under
quoted
control by the UFS controller driver?  Why do you need/want an ioctl
for
quoted
this?
Hi greg k-h,

in our code, we used the IOCTL during runtime, in order to determine
some
information about the RPMB well known lun.
with the rpmb lun ID we could then go to /dev/sgX and issue
UFS_IOCTL_QUERY to this lun and get the data -
reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the
QUERY_DESC_IDN_UNIT descriptor.

this was crucial to the work we do in RPMB.
What is RPMB?

And again, why not just use sysfs attributes on your host controller
device?  Why does this have to be a custom ioctl?

greg k-h
RPMB is spcial Logical Unit in the UFS device. you can read about it in
the UFS spec. Greg, you are insisting on sysfs, but i can't implement it
now, as i don't have the Hardware anymore, or the time.
If you don't have the hardware, how are you testing this patch?

And if you don't have the hardware I guess you don't need this change :)
This is a tested and verified code that was accepted and reviewed already,
so i'm not sure what is wrong with this solution, not to say, it's already
implemented, tested and verified.
hope you are help us push it.
That's a horrible reason to merge a patch that someone else is going to
have to support for 20+ years with an api that doesn't make much sense.

If you don't have the hardware, then this isn't needed.  But if you do,
then please look into using sysfs for this, as I think that should be
the correct interface here, again, not some random ioctl.

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: Arnd Bergmann <arnd@arndb.de>
Date: 2016-03-10 17:19:21

On Wednesday 09 March 2016, ygardi@codeaurora.org wrote:
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and descriptors.
Not only ioctl interface is a useful way to interact with the device,
we used it, and found it very helpful in varies cases.
hence, this patch.
This patch has been already addressed all comments of Arnd Bergman from 5
months ago, and now, re-uploaded again.
Do you have a pointer to that review? It's been a long while, so I
have completely forgotten what issues I raised and how it got resolved.

	Arnd

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: <hidden>
Date: 2016-03-10 18:49:10

On Thu, Mar 10, 2016 at 04:29:55PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
quoted
On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org
wrote:
quoted
quoted
quoted
quoted
quoted
On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org
wrote:
quoted
quoted
quoted
quoted
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via
SCSI
quoted
quoted
quoted
quoted
device
quoted
quoted
quoted
quoted
ioctl interface. As of now UFS driver would provide the
ioctl
quoted
quoted
for
quoted
quoted
quoted
quoted
query
quoted
quoted
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae
want
quoted
quoted
to
quoted
quoted
do
quoted
quoted
quoted
something "special" with UFS devices?  Shouldn't they just be
treated
quoted
quoted
quoted
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that
sent
quoted
quoted
quoted
quoted
queries to the UFS devices in order to get information and
descriptors.
quoted
But do you want to do with that information?  Why does userspace
care?
quoted
quoted
quoted
i don't really understand the subtext of your question -
as ANY ioctl cb, we decided to implement the ioctl callback of
this
quoted
quoted
scsi
quoted
quoted
device in order to get information like UNIT DESC, DEVICE DESC,
FLAGs,
quoted
quoted
ATTRIBUTES.
When dealing with UFS devices, one should be able to read the
characteristics of the device. why ? well, why not ?
Why aren't those characteristics just exported as sysfs attributes
under
quoted
control by the UFS controller driver?  Why do you need/want an
ioctl
quoted
quoted
for
quoted
this?
Hi greg k-h,

in our code, we used the IOCTL during runtime, in order to determine
some
information about the RPMB well known lun.
with the rpmb lun ID we could then go to /dev/sgX and issue
UFS_IOCTL_QUERY to this lun and get the data -
reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the
QUERY_DESC_IDN_UNIT descriptor.

this was crucial to the work we do in RPMB.
What is RPMB?

And again, why not just use sysfs attributes on your host controller
device?  Why does this have to be a custom ioctl?

greg k-h
RPMB is spcial Logical Unit in the UFS device. you can read about it in
the UFS spec. Greg, you are insisting on sysfs, but i can't implement it
now, as i don't have the Hardware anymore, or the time.
If you don't have the hardware, how are you testing this patch?
This patch is already tested and verified, and also was much helpful
during development.
Also, this patch is a few months old and was tested when previous version
were uploaded.
Currently HW is not available.
And if you don't have the hardware I guess you don't need this change :)
quoted
This is a tested and verified code that was accepted and reviewed
already,
so i'm not sure what is wrong with this solution, not to say, it's
already
implemented, tested and verified.
hope you are help us push it.
That's a horrible reason to merge a patch that someone else is going to
have to support for 20+ years with an api that doesn't make much sense.

If you don't have the hardware, then this isn't needed.  But if you do,
Even if currently HW is not available it doesn't mean this is not needed
in the future again.
then please look into using sysfs for this, as I think that should be
the correct interface here, again, not some random ioctl.
Why sysfs makes more sense than this one ?

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2016-03-10 18:58:59

On Thu, Mar 10, 2016 at 06:48:54PM -0000, ygardi@codeaurora.org wrote:
quoted
On Thu, Mar 10, 2016 at 04:29:55PM -0000, ygardi@codeaurora.org wrote:
quoted
quoted
On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi@codeaurora.org wrote:
quoted
quoted
On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org
wrote:
quoted
quoted
quoted
quoted
quoted
On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org
wrote:
quoted
quoted
quoted
quoted
quoted
On Wed, Mar 09, 2016 at 04:11:33PM +0200, Yaniv Gardi wrote:
quoted
This patch exposes the ioctl interface for UFS driver via
SCSI
quoted
quoted
quoted
quoted
device
quoted
quoted
quoted
quoted
ioctl interface. As of now UFS driver would provide the
ioctl
quoted
quoted
for
quoted
quoted
quoted
quoted
query
quoted
quoted
interface to connected UFS device.

Reviewed-by: Subhash Jadavani <redacted>
Signed-off-by: Dolev Raviv <redacted>
Signed-off-by: Gilad Broner <redacted>
Signed-off-by: Yaniv Gardi <redacted>
What tool is going to use this ioctl?  Why does userspcae
want
quoted
quoted
to
quoted
quoted
do
quoted
quoted
quoted
something "special" with UFS devices?  Shouldn't they just be
treated
quoted
quoted
quoted
like any other normal block device?
Any userspace application can be a tool.
We already implemented and used a user space application, that
sent
quoted
quoted
quoted
quoted
queries to the UFS devices in order to get information and
descriptors.
quoted
But do you want to do with that information?  Why does userspace
care?
quoted
quoted
quoted
i don't really understand the subtext of your question -
as ANY ioctl cb, we decided to implement the ioctl callback of
this
quoted
quoted
scsi
quoted
quoted
device in order to get information like UNIT DESC, DEVICE DESC,
FLAGs,
quoted
quoted
ATTRIBUTES.
When dealing with UFS devices, one should be able to read the
characteristics of the device. why ? well, why not ?
Why aren't those characteristics just exported as sysfs attributes
under
quoted
control by the UFS controller driver?  Why do you need/want an
ioctl
quoted
quoted
for
quoted
this?
Hi greg k-h,

in our code, we used the IOCTL during runtime, in order to determine
some
information about the RPMB well known lun.
with the rpmb lun ID we could then go to /dev/sgX and issue
UFS_IOCTL_QUERY to this lun and get the data -
reading the QUERY_DESC_IDN_GEOMETRY descriptor and reading the
QUERY_DESC_IDN_UNIT descriptor.

this was crucial to the work we do in RPMB.
What is RPMB?

And again, why not just use sysfs attributes on your host controller
device?  Why does this have to be a custom ioctl?

greg k-h
RPMB is spcial Logical Unit in the UFS device. you can read about it in
the UFS spec. Greg, you are insisting on sysfs, but i can't implement it
now, as i don't have the Hardware anymore, or the time.
If you don't have the hardware, how are you testing this patch?
This patch is already tested and verified, and also was much helpful
during development.
Also, this patch is a few months old and was tested when previous version
were uploaded.
Currently HW is not available.
quoted
And if you don't have the hardware I guess you don't need this change :)
quoted
This is a tested and verified code that was accepted and reviewed
already,
so i'm not sure what is wrong with this solution, not to say, it's
already
implemented, tested and verified.
hope you are help us push it.
That's a horrible reason to merge a patch that someone else is going to
have to support for 20+ years with an api that doesn't make much sense.

If you don't have the hardware, then this isn't needed.  But if you do,
Even if currently HW is not available it doesn't mean this is not needed
in the future again.
Great, if in the future, you need this again, please resubmit it.
Adding code to the kernel for no real user is a maintaince burden on
others.  Please don't do that.  Especially for an API that is now
required to be supported for forever.
quoted
then please look into using sysfs for this, as I think that should be
the correct interface here, again, not some random ioctl.
Why sysfs makes more sense than this one ?
Why doesn't it?  Why would an ioctl make sense to get simple attributes
from a host controller?  Why not use the interface that makes it trivial
to use from userspace that other drivers already use in this type of
situation.

In other words, why do you have to have this ioctl?  What requires this
to be the way the API works?

But again, as you don't need this code, let's just drop it.  Feel free
to revisit it sometime in the future if you ever get hardware and need
this again.

thanks,

greg k-h

Re: [PATCH v7] scsi: ufs: add ioctl interface for query request

From: Arnd Bergmann <arnd@arndb.de>
Date: 2016-03-10 19:20:21

On Thursday 10 March 2016, Arnd Bergmann wrote:
On Wednesday 09 March 2016, ygardi-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org wrote:
quoted
Any userspace application can be a tool.
We already implemented and used a user space application, that sent
queries to the UFS devices in order to get information and descriptors.
Not only ioctl interface is a useful way to interact with the device,
we used it, and found it very helpful in varies cases.
hence, this patch.
This patch has been already addressed all comments of Arnd Bergman from 5
months ago, and now, re-uploaded again.
Do you have a pointer to that review? It's been a long while, so I
have completely forgotten what issues I raised and how it got resolved.
I got your link in private message and read up on it again now.

To clarify: I commented on the formal API definition, and you indeed addressed
all my concerns, so this is now an ioctl command that follows our usual
calling conventions.

However, this is orthogonal to the question of whether it is a good idea
to have this interface implemented as an ioctl as asked by Greg, and who
is actually using it. 

I'm lacking the detailed subsystem knowledge to answer this, but
I note that other block drivers have similar passthrough interfaces.

Looking through what other drivers do, I've found a couple of patterns
now. n particular, most use the SG_IO ioctl to pass down commands
from user space into a device specific command queue. Have you looked
at that interface in the past to see if it would fit your use case?

There is also a 'bsg' API that some drivers implement, which I think
would be another alternative.

Could any of the SCSI experts comment on what they expect a driver
to use out of those three alternatives (if any): 

* private ioctl
* bsg
* sg_io

	Arnd
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help