Re: [PATCH v2] scsi: ufs: add ioctl interface for query request
From: Arnd Bergmann <arnd@arndb.de>
Date: 2015-10-07 08:12:06
Also in:
linux-arm-msm, linux-scsi, lkml
On Wednesday 07 October 2015 10:54:03 Yaniv Gardi wrote:
+/* IOCTL opcode for command - ufs set device read only */ +#define UFS_IOCTL_BLKROSET BLKROSET +
What is this for? Can't you just use the normal BLKROSET definition in user space?
+
+ ioctl_data = kzalloc(sizeof(struct ufs_ioctl_query_data), GFP_KERNEL);
+ if (!ioctl_data) {
+ err = -ENOMEM;
+ goto out;
+ }ufs_ioctl_query_data is short enough to just be on the stack.
+ }
+ length = min_t(int, QUERY_DESC_MAX_SIZE,
+ ioctl_data->buf_size);
+ desc = kzalloc(length, GFP_KERNEL);
+ if (!desc) {
+ dev_err(hba->dev, "%s: Failed allocating %d bytes\n",
+ __func__, length);
+ err = -ENOMEM;
+ goto out_release_mem;
+ }Better check for a maximum length as well, not just a minimum length. For overly long requests, just return an error without trying the kzalloc. You can also use memdup_user to avoid the extra zeroing and and simplify the calling code.
+/**
+ * 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) {
+ dev_err(hba->dev, "%s: User buffer is NULL!\n", __func__);
+ return -EINVAL;
+ }Don't print an error here, you don't want users to be able to flood syslog that easily, and the program that caused the error does not see the log anyway.
+
+ 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 UFS_IOCTL_BLKROSET:
+ err = -ENOIOCTLCMD;
+ break;
+ default:
+ err = -EINVAL;
+ dev_err(hba->dev, "%s: Illegal ufs-IOCTL cmd %d\n", __func__,
+ cmd);
+ break;same here.
quoted hunk ↗ jump to hunk
+ } + + return err; +} + +/** * ufshcd_async_scan - asynchronous execution for probing hba * @data: data pointer to pass to this function * @cookie: cookie data@@ -5106,6 +5322,7 @@ 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, .this_id = -1, .sg_tablesize = SG_ALL, .cmd_per_lun = UFSHCD_CMD_PER_LUN,
The ioctl data is compatible between 32-bit and 64-bit user space, so better add a .compat_ioctl line right away to make this work on 64-bit architectures with 32-bit user space.
+ +/* + * IOCTL opcode for ufs queries has the following opcode after + * SCSI_IOCTL_GET_PCI + */ +#define UFS_IOCTL_QUERY 0x5388
Use _IOWR() to define that number with the correct argument length
+/**
+ * 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 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;
+ /*
+ * 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;
+ /*
+ * 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];
+};Better rearrange the structure to avoid the implied padding, either by making idn a __u16, or by adding an explicit __u8 member behind it. Arnd