Thanks Arnd.
comments inline and will upload V3 shortly
Yaniv
> 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?
>
correct. will be removed
>> +
>> + 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.
ok
>
>> + }
>> + 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.
>
ok
>> +
>> + 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.
>
ok
>> + }
>> +
>> + 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.
>
ok
>> +
>> +/*
>> + * 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
i'd rather leave it as is, to be compatible with the SCSI_IOCTL
opcodes that are defined in scsi_ioctl.h
>
>> +/**
>> + * 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.
>
correct
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
