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

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

[Yaniv Gardi]

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 <subhashj@codeaurora.org>
Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

---
 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

[Greg KH]

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 <subhashj@codeaurora.org>
> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>

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

[ygardi]

> 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 <subhashj@codeaurora.org>
>> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> 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

[ygardi]

> 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 <subhashj@codeaurora.org>
>> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>
> 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

[Greg KH]

On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org wrote:
> > 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 <subhashj@codeaurora.org>
> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
> >
> > 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

[ygardi]

> On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org wrote:
>> > 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 <subhashj@codeaurora.org>
>> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
>> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>> >
>> > 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

>> 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

[Greg KH]

On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org wrote:
> > On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org wrote:
> >> > 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 <subhashj@codeaurora.org>
> >> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
> >> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> >> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
> >> >
> >> > 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

[ygardi]

> On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org wrote:
>> > On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org wrote:
>> >> > 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 <subhashj@codeaurora.org>
>> >> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
>> >> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>> >> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>> >> >
>> >> > 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?
>

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

>> 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

[Greg KH]

On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi@codeaurora.org wrote:
> > On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org wrote:
> >> > On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org wrote:
> >> >> > 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 <subhashj@codeaurora.org>
> >> >> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
> >> >> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> >> >> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
> >> >> >
> >> >> > 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?
> >
> 
> 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

[ygardi]

> On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi@codeaurora.org wrote:
>> > On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org wrote:
>> >> > On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org
>> wrote:
>> >> >> > 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 <subhashj@codeaurora.org>
>> >> >> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
>> >> >> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>> >> >> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>> >> >> >
>> >> >> > 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?
>> >
>>
>> 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

[Greg KH]

On Thu, Mar 10, 2016 at 04:29:55PM -0000, ygardi@codeaurora.org wrote:
> > On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi@codeaurora.org wrote:
> >> > On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org wrote:
> >> >> > On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org
> >> wrote:
> >> >> >> > 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 <subhashj@codeaurora.org>
> >> >> >> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
> >> >> >> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> >> >> >> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
> >> >> >> >
> >> >> >> > 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?
> >> >
> >>
> >> 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

[Arnd Bergmann]

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

[ygardi]

> On Thu, Mar 10, 2016 at 04:29:55PM -0000, ygardi@codeaurora.org wrote:
>> > On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi@codeaurora.org wrote:
>> >> > On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org
>> wrote:
>> >> >> > On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org
>> >> wrote:
>> >> >> >> > 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 <subhashj@codeaurora.org>
>> >> >> >> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
>> >> >> >> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
>> >> >> >> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
>> >> >> >> >
>> >> >> >> > 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?
>> >> >
>> >>
>> >> 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 :)
>
>> 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

[Greg KH]

On Thu, Mar 10, 2016 at 06:48:54PM -0000, ygardi@codeaurora.org wrote:
> > On Thu, Mar 10, 2016 at 04:29:55PM -0000, ygardi@codeaurora.org wrote:
> >> > On Thu, Mar 10, 2016 at 03:52:54PM -0000, ygardi@codeaurora.org wrote:
> >> >> > On Wed, Mar 09, 2016 at 08:52:59PM -0000, ygardi@codeaurora.org
> >> wrote:
> >> >> >> > On Wed, Mar 09, 2016 at 07:09:49PM -0000, ygardi@codeaurora.org
> >> >> wrote:
> >> >> >> >> > 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 <subhashj@codeaurora.org>
> >> >> >> >> >> Signed-off-by: Dolev Raviv <draviv@codeaurora.org>
> >> >> >> >> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org>
> >> >> >> >> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
> >> >> >> >> >
> >> >> >> >> > 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?
> >> >> >
> >> >>
> >> >> 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 :)
> >
> >> 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.

> > 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

[Arnd Bergmann]

On Thursday 10 March 2016, Arnd Bergmann wrote:
> 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.

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

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

[Martin K. Petersen]

>>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:

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

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

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

My preference is that if you want to expose any information that a user
or admin would care about, put it in sysfs where it is easily accessible
and can be scripted.

For things that need to directly send commands to the hardware (to
configure vendor specific settings or inspect parameters for development
purposes) use the sg or bsg interfaces like we do for both ATA and
SCSI. bsg is newer and supports more features like bidirectional
commands. But for this particular use case I don't think it offers any
particular advantages over SG_IO. Both interfaces take the same
descriptors so it really doesn't matter much.

See:

	http://sg.danny.cz/sg/

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Hannes Reinecke]

On 03/11/2016 02:43 AM, Martin K. Petersen wrote:
>>>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
> 
> Arnd> Looking through what other drivers do, I've found a couple of
> Arnd> patterns now. n particular, most use the SG_IO ioctl to pass down
> Arnd> commands from user space into a device specific command
> Arnd> queue. Have you looked at that interface in the past to see if it
> Arnd> would fit your use case?
> 
> Arnd> There is also a 'bsg' API that some drivers implement, which I
> Arnd> think would be another alternative.
> 
> Arnd> Could any of the SCSI experts comment on what they expect a driver
> Arnd> to use out of those three alternatives (if any):
> 
> My preference is that if you want to expose any information that a user
> or admin would care about, put it in sysfs where it is easily accessible
> and can be scripted.
> 
> For things that need to directly send commands to the hardware (to
> configure vendor specific settings or inspect parameters for development
> purposes) use the sg or bsg interfaces like we do for both ATA and
> SCSI. bsg is newer and supports more features like bidirectional
> commands. But for this particular use case I don't think it offers any
> particular advantages over SG_IO. Both interfaces take the same
> descriptors so it really doesn't matter much.
> 
> See:
> 
> 	http://sg.danny.cz/sg/
> 
And indeed I would expose this 'special' UFS RPMB as a 'generic'
SCSI device and add the required fields in sysfs, much like Greg KH
suggested.
Plus adding a 'bsg' interface for sending 'raw' UFS commands sounds
much more appealing; that's what we do for SAS and FC, too.

But then, none of the above can be done if there is no hardware
available. At the same time, you wouldn't be able to test any
proposed fixes to the original ioctl patch.
So I guess you'll have to retract this patch or get hold of the
hardware to actually _test_ something there.

Or send me some, I'll be happy to help out here :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

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

[Winkler, Tomas]

> 
> On 03/11/2016 02:43 AM, Martin K. Petersen wrote:
> >>>>>> "Arnd" == Arnd Bergmann <arnd@arndb.de> writes:
> >
> > Arnd> Looking through what other drivers do, I've found a couple of
> > Arnd> patterns now. n particular, most use the SG_IO ioctl to pass down
> > Arnd> commands from user space into a device specific command
> > Arnd> queue. Have you looked at that interface in the past to see if it
> > Arnd> would fit your use case?
> >
> > Arnd> There is also a 'bsg' API that some drivers implement, which I
> > Arnd> think would be another alternative.
> >
> > Arnd> Could any of the SCSI experts comment on what they expect a driver
> > Arnd> to use out of those three alternatives (if any):
> >
> > My preference is that if you want to expose any information that a user
> > or admin would care about, put it in sysfs where it is easily accessible
> > and can be scripted.
> >
> > For things that need to directly send commands to the hardware (to
> > configure vendor specific settings or inspect parameters for development
> > purposes) use the sg or bsg interfaces like we do for both ATA and
> > SCSI. bsg is newer and supports more features like bidirectional
> > commands. But for this particular use case I don't think it offers any
> > particular advantages over SG_IO. Both interfaces take the same
> > descriptors so it really doesn't matter much.
> >
> > See:
> >
> > 	http://sg.danny.cz/sg/
> >
> And indeed I would expose this 'special' UFS RPMB as a 'generic'
> SCSI device and add the required fields in sysfs, much like Greg KH
> suggested.
> Plus adding a 'bsg' interface for sending 'raw' UFS commands sounds
> much more appealing; that's what we do for SAS and FC, too.
> 
> But then, none of the above can be done if there is no hardware
> available. At the same time, you wouldn't be able to test any
> proposed fixes to the original ioctl patch.
> So I guess you'll have to retract this patch or get hold of the
> hardware to actually _test_ something there.
> 
> Or send me some, I'll be happy to help out here :-)
> 

I will submit proposal or new RPMB layer for both UFS and EMMC 
It also sports IOCTL interface, similar and replacing to mmc_ioc_cmd which is also used to access RPMB partition on emmc.
Blocks  of 512 bytes has to be transferred back and force so sysfs API is not good enough.  RPMB partition is not suitable to be accessed via block layer.  I've tried going via SICSI generic layers, it is a bit awkward as the protocol is very UFS specific.  
Currently I have working solution for both EMMC and UFS2.0.

Thanks
Tomas