[PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

[PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

[Oded Gabbay]

This patch moves the copy_to_user() and copy_from_user() calls from the
different ioctl functions in amdkfd to the general kfd_ioctl() function, as
this is a common code for all ioctls.

This was done according to example taken from drm_ioctl.c

Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 +++++++++++++++----------------
 1 file changed, 117 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 7d4974b..5460ad2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, struct file *filep)
 	return 0;
 }
 
-static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
-					void __user *arg)
+static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
+					void *data)
 {
-	struct kfd_ioctl_get_version_args args;
+	struct kfd_ioctl_get_version_args *args = data;
 	int err = 0;
 
-	args.major_version = KFD_IOCTL_MAJOR_VERSION;
-	args.minor_version = KFD_IOCTL_MINOR_VERSION;
-
-	if (copy_to_user(arg, &args, sizeof(args)))
-		err = -EFAULT;
+	args->major_version = KFD_IOCTL_MAJOR_VERSION;
+	args->minor_version = KFD_IOCTL_MINOR_VERSION;
 
 	return err;
 }
@@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct queue_properties *q_properties,
 	return 0;
 }
 
-static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
-					void __user *arg)
+static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
+					void *data)
 {
-	struct kfd_ioctl_create_queue_args args;
+	struct kfd_ioctl_create_queue_args *args = data;
 	struct kfd_dev *dev;
 	int err = 0;
 	unsigned int queue_id;
@@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
 
 	memset(&q_properties, 0, sizeof(struct queue_properties));
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
 	pr_debug("kfd: creating queue ioctl\n");
 
-	err = set_queue_properties_from_user(&q_properties, &args);
+	err = set_queue_properties_from_user(&q_properties, args);
 	if (err)
 		return err;
 
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
 	if (dev == NULL)
 		return -EINVAL;
 
@@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
 
 	pdd = kfd_bind_process_to_device(dev, p);
 	if (IS_ERR(pdd)) {
-		err = PTR_ERR(pdd);
+		err = -ESRCH;
 		goto err_bind_process;
 	}
 
@@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
 	if (err != 0)
 		goto err_create_queue;
 
-	args.queue_id = queue_id;
+	args->queue_id = queue_id;
 
 	/* Return gpu_id as doorbell offset for mmap usage */
-	args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
-
-	if (copy_to_user(arg, &args, sizeof(args))) {
-		err = -EFAULT;
-		goto err_copy_args_out;
-	}
+	args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
 
 	mutex_unlock(&p->mutex);
 
-	pr_debug("kfd: queue id %d was created successfully\n", args.queue_id);
+	pr_debug("kfd: queue id %d was created successfully\n", args->queue_id);
 
 	pr_debug("ring buffer address == 0x%016llX\n",
-			args.ring_base_address);
+			args->ring_base_address);
 
 	pr_debug("read ptr address    == 0x%016llX\n",
-			args.read_pointer_address);
+			args->read_pointer_address);
 
 	pr_debug("write ptr address   == 0x%016llX\n",
-			args.write_pointer_address);
+			args->write_pointer_address);
 
 	return 0;
 
-err_copy_args_out:
-	pqm_destroy_queue(&p->pqm, queue_id);
 err_create_queue:
 err_bind_process:
 	mutex_unlock(&p->mutex);
@@ -297,99 +284,90 @@ err_bind_process:
 }
 
 static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
-					void __user *arg)
+					void *data)
 {
 	int retval;
-	struct kfd_ioctl_destroy_queue_args args;
-
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
+	struct kfd_ioctl_destroy_queue_args *args = data;
 
 	pr_debug("kfd: destroying queue id %d for PASID %d\n",
-				args.queue_id,
+				args->queue_id,
 				p->pasid);
 
 	mutex_lock(&p->mutex);
 
-	retval = pqm_destroy_queue(&p->pqm, args.queue_id);
+	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
 
 	mutex_unlock(&p->mutex);
 	return retval;
 }
 
 static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
-					void __user *arg)
+					void *data)
 {
 	int retval;
-	struct kfd_ioctl_update_queue_args args;
+	struct kfd_ioctl_update_queue_args *args = data;
 	struct queue_properties properties;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
+	if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
 		pr_err("kfd: queue percentage must be between 0 to KFD_MAX_QUEUE_PERCENTAGE\n");
 		return -EINVAL;
 	}
 
-	if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
+	if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
 		pr_err("kfd: queue priority must be between 0 to KFD_MAX_QUEUE_PRIORITY\n");
 		return -EINVAL;
 	}
 
-	if ((args.ring_base_address) &&
+	if ((args->ring_base_address) &&
 		(!access_ok(VERIFY_WRITE,
-			(const void __user *) args.ring_base_address,
+			(const void __user *) args->ring_base_address,
 			sizeof(uint64_t)))) {
 		pr_err("kfd: can't access ring base address\n");
 		return -EFAULT;
 	}
 
-	if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
+	if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
 		pr_err("kfd: ring size must be a power of 2 or 0\n");
 		return -EINVAL;
 	}
 
-	properties.queue_address = args.ring_base_address;
-	properties.queue_size = args.ring_size;
-	properties.queue_percent = args.queue_percentage;
-	properties.priority = args.queue_priority;
+	properties.queue_address = args->ring_base_address;
+	properties.queue_size = args->ring_size;
+	properties.queue_percent = args->queue_percentage;
+	properties.priority = args->queue_priority;
 
 	pr_debug("kfd: updating queue id %d for PASID %d\n",
-			args.queue_id, p->pasid);
+			args->queue_id, p->pasid);
 
 	mutex_lock(&p->mutex);
 
-	retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
+	retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
 
 	mutex_unlock(&p->mutex);
 
 	return retval;
 }
 
-static long kfd_ioctl_set_memory_policy(struct file *filep,
-				struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_set_memory_policy(struct file *filep,
+					struct kfd_process *p, void *data)
 {
-	struct kfd_ioctl_set_memory_policy_args args;
+	struct kfd_ioctl_set_memory_policy_args *args = data;
 	struct kfd_dev *dev;
 	int err = 0;
 	struct kfd_process_device *pdd;
 	enum cache_policy default_policy, alternate_policy;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
-	    && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+	if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
+	    && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
 		return -EINVAL;
 	}
 
-	if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
-	    && args.alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
+	if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
+	    && args->alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
 		return -EINVAL;
 	}
 
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
 	if (dev == NULL)
 		return -EINVAL;
 
@@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct file *filep,
 
 	pdd = kfd_bind_process_to_device(dev, p);
 	if (IS_ERR(pdd)) {
-		err = PTR_ERR(pdd);
+		err = -ESRCH;
 		goto out;
 	}
 
-	default_policy = (args.default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+	default_policy = (args->default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
 			 ? cache_policy_coherent : cache_policy_noncoherent;
 
 	alternate_policy =
-		(args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
+		(args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
 		   ? cache_policy_coherent : cache_policy_noncoherent;
 
 	if (!dev->dqm->set_cache_memory_policy(dev->dqm,
 				&pdd->qpd,
 				default_policy,
 				alternate_policy,
-				(void __user *)args.alternate_aperture_base,
-				args.alternate_aperture_size))
+				(void __user *)args->alternate_aperture_base,
+				args->alternate_aperture_size))
 		err = -EINVAL;
 
 out:
@@ -422,53 +400,44 @@ out:
 	return err;
 }
 
-static long kfd_ioctl_get_clock_counters(struct file *filep,
-				struct kfd_process *p, void __user *arg)
+static int kfd_ioctl_get_clock_counters(struct file *filep,
+				struct kfd_process *p, void *data)
 {
-	struct kfd_ioctl_get_clock_counters_args args;
+	struct kfd_ioctl_get_clock_counters_args *args = data;
 	struct kfd_dev *dev;
 	struct timespec time;
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	dev = kfd_device_by_id(args.gpu_id);
+	dev = kfd_device_by_id(args->gpu_id);
 	if (dev == NULL)
 		return -EINVAL;
 
 	/* Reading GPU clock counter from KGD */
-	args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
+	args->gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
 
 	/* No access to rdtsc. Using raw monotonic time */
 	getrawmonotonic(&time);
-	args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
+	args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
 
 	get_monotonic_boottime(&time);
-	args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
+	args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
 
 	/* Since the counter is in nano-seconds we use 1GHz frequency */
-	args.system_clock_freq = 1000000000;
-
-	if (copy_to_user(arg, &args, sizeof(args)))
-		return -EFAULT;
+	args->system_clock_freq = 1000000000;
 
 	return 0;
 }
 
 
 static int kfd_ioctl_get_process_apertures(struct file *filp,
-				struct kfd_process *p, void __user *arg)
+				struct kfd_process *p, void *data)
 {
-	struct kfd_ioctl_get_process_apertures_args args;
+	struct kfd_ioctl_get_process_apertures_args *args = data;
 	struct kfd_process_device_apertures *pAperture;
 	struct kfd_process_device *pdd;
 
 	dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
 
-	if (copy_from_user(&args, arg, sizeof(args)))
-		return -EFAULT;
-
-	args.num_of_nodes = 0;
+	args->num_of_nodes = 0;
 
 	mutex_lock(&p->mutex);
 
@@ -477,7 +446,8 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
 		/* Run over all pdd of the process */
 		pdd = kfd_get_first_process_device_data(p);
 		do {
-			pAperture = &args.process_apertures[args.num_of_nodes];
+			pAperture =
+				&args->process_apertures[args->num_of_nodes];
 			pAperture->gpu_id = pdd->dev->id;
 			pAperture->lds_base = pdd->lds_base;
 			pAperture->lds_limit = pdd->lds_limit;
@@ -487,7 +457,7 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
 			pAperture->scratch_limit = pdd->scratch_limit;
 
 			dev_dbg(kfd_device,
-				"node id %u\n", args.num_of_nodes);
+				"node id %u\n", args->num_of_nodes);
 			dev_dbg(kfd_device,
 				"gpu id %u\n", pdd->dev->id);
 			dev_dbg(kfd_device,
@@ -503,23 +473,23 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
 			dev_dbg(kfd_device,
 				"scratch_limit %llX\n", pdd->scratch_limit);
 
-			args.num_of_nodes++;
+			args->num_of_nodes++;
 		} while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
-				(args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
+				(args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
 	}
 
 	mutex_unlock(&p->mutex);
 
-	if (copy_to_user(arg, &args, sizeof(args)))
-		return -EFAULT;
-
 	return 0;
 }
 
 static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
 	struct kfd_process *process;
-	long err = -EINVAL;
+	char stack_kdata[128];
+	char *kdata = NULL;
+	unsigned int usize, asize;
+	int retcode = -EINVAL;
 
 	dev_dbg(kfd_device,
 		"ioctl cmd 0x%x (#%d), arg 0x%lx\n",
@@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 	if (IS_ERR(process))
 		return PTR_ERR(process);
 
+	if (cmd & (IOC_IN | IOC_OUT)) {
+		if (asize <= sizeof(stack_kdata)) {
+			kdata = stack_kdata;
+		} else {
+			kdata = kmalloc(asize, GFP_KERNEL);
+			if (!kdata) {
+				retcode = -ENOMEM;
+				goto err_i1;
+			}
+		}
+		if (asize > usize)
+			memset(kdata + usize, 0, asize - usize);
+	}
+
+	if (cmd & IOC_IN) {
+		if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
+			retcode = -EFAULT;
+			goto err_i1;
+		}
+	} else if (cmd & IOC_OUT) {
+		memset(kdata, 0, usize);
+	}
+
+
 	switch (cmd) {
 	case KFD_IOC_GET_VERSION:
-		err = kfd_ioctl_get_version(filep, process, (void __user *)arg);
+		retcode = kfd_ioctl_get_version(filep, process, kdata);
 		break;
 	case KFD_IOC_CREATE_QUEUE:
-		err = kfd_ioctl_create_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_create_queue(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_DESTROY_QUEUE:
-		err = kfd_ioctl_destroy_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_destroy_queue(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_SET_MEMORY_POLICY:
-		err = kfd_ioctl_set_memory_policy(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_set_memory_policy(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_GET_CLOCK_COUNTERS:
-		err = kfd_ioctl_get_clock_counters(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_get_clock_counters(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_GET_PROCESS_APERTURES:
-		err = kfd_ioctl_get_process_apertures(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_get_process_apertures(filep, process,
+						kdata);
 		break;
 
 	case KFD_IOC_UPDATE_QUEUE:
-		err = kfd_ioctl_update_queue(filep, process,
-						(void __user *)arg);
+		retcode = kfd_ioctl_update_queue(filep, process,
+						kdata);
 		break;
 
 	default:
-		dev_err(kfd_device,
+		dev_dbg(kfd_device,
 			"unknown ioctl cmd 0x%x, arg 0x%lx)\n",
 			cmd, arg);
-		err = -EINVAL;
+		retcode = -EINVAL;
 		break;
 	}
 
-	if (err < 0)
-		dev_err(kfd_device,
-			"ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
-			err, cmd, _IOC_NR(cmd));
+	if (cmd & IOC_OUT)
+		if (copy_to_user((void __user *)arg, kdata, usize) != 0)
+			retcode = -EFAULT;
 
-	return err;
+err_i1:
+	if (kdata != stack_kdata)
+		kfree(kdata);
+
+	if (retcode)
+		dev_dbg(kfd_device, "ret = %d\n", retcode);
+
+	return retcode;
 }
 
 static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

[Oded Gabbay]

This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the
drm ioctls definition style.

Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
 include/uapi/linux/kfd_ioctl.h           | 37 +++++++++++++++++++-------------
 2 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 5460ad2..390385f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 
 
 	switch (cmd) {
-	case KFD_IOC_GET_VERSION:
+	case AMDKFD_IOC_GET_VERSION:
 		retcode = kfd_ioctl_get_version(filep, process, kdata);
 		break;
-	case KFD_IOC_CREATE_QUEUE:
+
+	case AMDKFD_IOC_CREATE_QUEUE:
 		retcode = kfd_ioctl_create_queue(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_DESTROY_QUEUE:
+	case AMDKFD_IOC_DESTROY_QUEUE:
 		retcode = kfd_ioctl_destroy_queue(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_SET_MEMORY_POLICY:
+	case AMDKFD_IOC_SET_MEMORY_POLICY:
 		retcode = kfd_ioctl_set_memory_policy(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_GET_CLOCK_COUNTERS:
+	case AMDKFD_IOC_GET_CLOCK_COUNTERS:
 		retcode = kfd_ioctl_get_clock_counters(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_GET_PROCESS_APERTURES:
+	case AMDKFD_IOC_GET_PROCESS_APERTURES:
 		retcode = kfd_ioctl_get_process_apertures(filep, process,
 						kdata);
 		break;
 
-	case KFD_IOC_UPDATE_QUEUE:
+	case AMDKFD_IOC_UPDATE_QUEUE:
 		retcode = kfd_ioctl_update_queue(filep, process,
 						kdata);
 		break;
diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
index 7acef41..05b53f6 100644
--- a/include/uapi/linux/kfd_ioctl.h
+++ b/include/uapi/linux/kfd_ioctl.h
@@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
 	uint32_t pad;
 };
 
-#define KFD_IOC_MAGIC 'K'
+#define AMDKFD_IOCTL_BASE 'K'
+#define AMDKFD_IO(nr)			_IO(AMDKFD_IOCTL_BASE, nr)
+#define AMDKFD_IOR(nr, type)		_IOR(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOW(nr, type)		_IOW(AMDKFD_IOCTL_BASE, nr, type)
+#define AMDKFD_IOWR(nr, type)		_IOWR(AMDKFD_IOCTL_BASE, nr, type)
 
-#define KFD_IOC_GET_VERSION \
-		_IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
+#define AMDKFD_IOC_GET_VERSION			\
+		AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
 
-#define KFD_IOC_CREATE_QUEUE \
-		_IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
+#define AMDKFD_IOC_CREATE_QUEUE			\
+		AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
 
-#define KFD_IOC_DESTROY_QUEUE \
-	_IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
+#define AMDKFD_IOC_DESTROY_QUEUE		\
+		AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
 
-#define KFD_IOC_SET_MEMORY_POLICY \
-	_IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
+#define AMDKFD_IOC_SET_MEMORY_POLICY		\
+		AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
 
-#define KFD_IOC_GET_CLOCK_COUNTERS \
-	_IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
+#define AMDKFD_IOC_GET_CLOCK_COUNTERS		\
+		AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
 
-#define KFD_IOC_GET_PROCESS_APERTURES \
-	_IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
+#define AMDKFD_IOC_GET_PROCESS_APERTURES	\
+		AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
 
-#define KFD_IOC_UPDATE_QUEUE \
-	_IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
+#define AMDKFD_IOC_UPDATE_QUEUE			\
+		AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
+
+#define KFD_COMMAND_START               0x01
+#define KFD_COMMAND_END			0x08
 
 #endif
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

[Christian König]

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:
> This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the
> drm ioctls definition style.
>
> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>

You are aware that this is a kernel API breakage?

> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
>   include/uapi/linux/kfd_ioctl.h           | 37 +++++++++++++++++++-------------
>   2 files changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 5460ad2..390385f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>   
>   
>   	switch (cmd) {
> -	case KFD_IOC_GET_VERSION:
> +	case AMDKFD_IOC_GET_VERSION:
>   		retcode = kfd_ioctl_get_version(filep, process, kdata);
>   		break;
> -	case KFD_IOC_CREATE_QUEUE:
> +
> +	case AMDKFD_IOC_CREATE_QUEUE:
>   		retcode = kfd_ioctl_create_queue(filep, process,
>   						kdata);
>   		break;
>   
> -	case KFD_IOC_DESTROY_QUEUE:
> +	case AMDKFD_IOC_DESTROY_QUEUE:
>   		retcode = kfd_ioctl_destroy_queue(filep, process,
>   						kdata);
>   		break;
>   
> -	case KFD_IOC_SET_MEMORY_POLICY:
> +	case AMDKFD_IOC_SET_MEMORY_POLICY:
>   		retcode = kfd_ioctl_set_memory_policy(filep, process,
>   						kdata);
>   		break;
>   
> -	case KFD_IOC_GET_CLOCK_COUNTERS:
> +	case AMDKFD_IOC_GET_CLOCK_COUNTERS:
>   		retcode = kfd_ioctl_get_clock_counters(filep, process,
>   						kdata);
>   		break;
>   
> -	case KFD_IOC_GET_PROCESS_APERTURES:
> +	case AMDKFD_IOC_GET_PROCESS_APERTURES:
>   		retcode = kfd_ioctl_get_process_apertures(filep, process,
>   						kdata);
>   		break;
>   
> -	case KFD_IOC_UPDATE_QUEUE:
> +	case AMDKFD_IOC_UPDATE_QUEUE:
>   		retcode = kfd_ioctl_update_queue(filep, process,
>   						kdata);
>   		break;
> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
> index 7acef41..05b53f6 100644
> --- a/include/uapi/linux/kfd_ioctl.h
> +++ b/include/uapi/linux/kfd_ioctl.h
> @@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
>   	uint32_t pad;
>   };
>   
> -#define KFD_IOC_MAGIC 'K'
> +#define AMDKFD_IOCTL_BASE 'K'
> +#define AMDKFD_IO(nr)			_IO(AMDKFD_IOCTL_BASE, nr)
> +#define AMDKFD_IOR(nr, type)		_IOR(AMDKFD_IOCTL_BASE, nr, type)
> +#define AMDKFD_IOW(nr, type)		_IOW(AMDKFD_IOCTL_BASE, nr, type)
> +#define AMDKFD_IOWR(nr, type)		_IOWR(AMDKFD_IOCTL_BASE, nr, type)
>   
> -#define KFD_IOC_GET_VERSION \
> -		_IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
> +#define AMDKFD_IOC_GET_VERSION			\
> +		AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
>   
> -#define KFD_IOC_CREATE_QUEUE \
> -		_IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
> +#define AMDKFD_IOC_CREATE_QUEUE			\
> +		AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
>   
> -#define KFD_IOC_DESTROY_QUEUE \
> -	_IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
> +#define AMDKFD_IOC_DESTROY_QUEUE		\
> +		AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
>   
> -#define KFD_IOC_SET_MEMORY_POLICY \
> -	_IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
> +#define AMDKFD_IOC_SET_MEMORY_POLICY		\
> +		AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
>   
> -#define KFD_IOC_GET_CLOCK_COUNTERS \
> -	_IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
> +#define AMDKFD_IOC_GET_CLOCK_COUNTERS		\
> +		AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
>   
> -#define KFD_IOC_GET_PROCESS_APERTURES \
> -	_IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
> +#define AMDKFD_IOC_GET_PROCESS_APERTURES	\
> +		AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
>   
> -#define KFD_IOC_UPDATE_QUEUE \
> -	_IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
> +#define AMDKFD_IOC_UPDATE_QUEUE			\
> +		AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
> +
> +#define KFD_COMMAND_START               0x01
> +#define KFD_COMMAND_END			0x08

If you rename everything to AMDKFD_* you probably want to do so as well 
for KFD_COMMAND_(START|END).

Regards,
Christian.

>   
>   #endif

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

[Oded Gabbay]

On 12/29/2014 03:06 PM, Christian König wrote:
> Am 29.12.2014 um 13:42 schrieb Oded Gabbay:
>> This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the
>> drm ioctls definition style.
>>
>> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
>
> You are aware that this is a kernel API breakage?
Yes, but as long as 3.19 isn't released yet, I'm allowed to do this, no ?
and I want this patch-set to be inserted to 3.19 so there won't be API breakage 
afterwards.

>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
>>   include/uapi/linux/kfd_ioctl.h           | 37 +++++++++++++++++++-------------
>>   2 files changed, 30 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index 5460ad2..390385f 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, unsigned int
>> cmd, unsigned long arg)
>>       switch (cmd) {
>> -    case KFD_IOC_GET_VERSION:
>> +    case AMDKFD_IOC_GET_VERSION:
>>           retcode = kfd_ioctl_get_version(filep, process, kdata);
>>           break;
>> -    case KFD_IOC_CREATE_QUEUE:
>> +
>> +    case AMDKFD_IOC_CREATE_QUEUE:
>>           retcode = kfd_ioctl_create_queue(filep, process,
>>                           kdata);
>>           break;
>> -    case KFD_IOC_DESTROY_QUEUE:
>> +    case AMDKFD_IOC_DESTROY_QUEUE:
>>           retcode = kfd_ioctl_destroy_queue(filep, process,
>>                           kdata);
>>           break;
>> -    case KFD_IOC_SET_MEMORY_POLICY:
>> +    case AMDKFD_IOC_SET_MEMORY_POLICY:
>>           retcode = kfd_ioctl_set_memory_policy(filep, process,
>>                           kdata);
>>           break;
>> -    case KFD_IOC_GET_CLOCK_COUNTERS:
>> +    case AMDKFD_IOC_GET_CLOCK_COUNTERS:
>>           retcode = kfd_ioctl_get_clock_counters(filep, process,
>>                           kdata);
>>           break;
>> -    case KFD_IOC_GET_PROCESS_APERTURES:
>> +    case AMDKFD_IOC_GET_PROCESS_APERTURES:
>>           retcode = kfd_ioctl_get_process_apertures(filep, process,
>>                           kdata);
>>           break;
>> -    case KFD_IOC_UPDATE_QUEUE:
>> +    case AMDKFD_IOC_UPDATE_QUEUE:
>>           retcode = kfd_ioctl_update_queue(filep, process,
>>                           kdata);
>>           break;
>> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
>> index 7acef41..05b53f6 100644
>> --- a/include/uapi/linux/kfd_ioctl.h
>> +++ b/include/uapi/linux/kfd_ioctl.h
>> @@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
>>       uint32_t pad;
>>   };
>> -#define KFD_IOC_MAGIC 'K'
>> +#define AMDKFD_IOCTL_BASE 'K'
>> +#define AMDKFD_IO(nr)            _IO(AMDKFD_IOCTL_BASE, nr)
>> +#define AMDKFD_IOR(nr, type)        _IOR(AMDKFD_IOCTL_BASE, nr, type)
>> +#define AMDKFD_IOW(nr, type)        _IOW(AMDKFD_IOCTL_BASE, nr, type)
>> +#define AMDKFD_IOWR(nr, type)        _IOWR(AMDKFD_IOCTL_BASE, nr, type)
>> -#define KFD_IOC_GET_VERSION \
>> -        _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
>> +#define AMDKFD_IOC_GET_VERSION            \
>> +        AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
>> -#define KFD_IOC_CREATE_QUEUE \
>> -        _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
>> +#define AMDKFD_IOC_CREATE_QUEUE            \
>> +        AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
>> -#define KFD_IOC_DESTROY_QUEUE \
>> -    _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
>> +#define AMDKFD_IOC_DESTROY_QUEUE        \
>> +        AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
>> -#define KFD_IOC_SET_MEMORY_POLICY \
>> -    _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
>> +#define AMDKFD_IOC_SET_MEMORY_POLICY        \
>> +        AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
>> -#define KFD_IOC_GET_CLOCK_COUNTERS \
>> -    _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
>> +#define AMDKFD_IOC_GET_CLOCK_COUNTERS        \
>> +        AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
>> -#define KFD_IOC_GET_PROCESS_APERTURES \
>> -    _IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
>> +#define AMDKFD_IOC_GET_PROCESS_APERTURES    \
>> +        AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
>> -#define KFD_IOC_UPDATE_QUEUE \
>> -    _IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
>> +#define AMDKFD_IOC_UPDATE_QUEUE            \
>> +        AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
>> +
>> +#define KFD_COMMAND_START               0x01
>> +#define KFD_COMMAND_END            0x08
>
> If you rename everything to AMDKFD_* you probably want to do so as well for
> KFD_COMMAND_(START|END).

Thanks, I'll do that.

	Oded
>
> Regards,
> Christian.
>
>>   #endif
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

[Christian König]

Am 29.12.2014 um 14:17 schrieb Oded Gabbay:
>
>
> On 12/29/2014 03:06 PM, Christian König wrote:
>> Am 29.12.2014 um 13:42 schrieb Oded Gabbay:
>>> This patch reformats the ioctl definitions in kfd_ioctl.h to be 
>>> similar to the
>>> drm ioctls definition style.
>>>
>>> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
>>
>> You are aware that this is a kernel API breakage?
> Yes, but as long as 3.19 isn't released yet, I'm allowed to do this, no ?

Strictly speaking only if you fix a bug with it, but since we are still 
pretty early in the cycle it's ok with we (especially since it looks 
like a valid cleanup). I'm just not sure what Dave/Linus think of this.

Christian.

> and I want this patch-set to be inserted to 3.19 so there won't be API 
> breakage afterwards.
>
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
>>>   include/uapi/linux/kfd_ioctl.h           | 37 
>>> +++++++++++++++++++-------------
>>>   2 files changed, 30 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index 5460ad2..390385f 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, 
>>> unsigned int
>>> cmd, unsigned long arg)
>>>       switch (cmd) {
>>> -    case KFD_IOC_GET_VERSION:
>>> +    case AMDKFD_IOC_GET_VERSION:
>>>           retcode = kfd_ioctl_get_version(filep, process, kdata);
>>>           break;
>>> -    case KFD_IOC_CREATE_QUEUE:
>>> +
>>> +    case AMDKFD_IOC_CREATE_QUEUE:
>>>           retcode = kfd_ioctl_create_queue(filep, process,
>>>                           kdata);
>>>           break;
>>> -    case KFD_IOC_DESTROY_QUEUE:
>>> +    case AMDKFD_IOC_DESTROY_QUEUE:
>>>           retcode = kfd_ioctl_destroy_queue(filep, process,
>>>                           kdata);
>>>           break;
>>> -    case KFD_IOC_SET_MEMORY_POLICY:
>>> +    case AMDKFD_IOC_SET_MEMORY_POLICY:
>>>           retcode = kfd_ioctl_set_memory_policy(filep, process,
>>>                           kdata);
>>>           break;
>>> -    case KFD_IOC_GET_CLOCK_COUNTERS:
>>> +    case AMDKFD_IOC_GET_CLOCK_COUNTERS:
>>>           retcode = kfd_ioctl_get_clock_counters(filep, process,
>>>                           kdata);
>>>           break;
>>> -    case KFD_IOC_GET_PROCESS_APERTURES:
>>> +    case AMDKFD_IOC_GET_PROCESS_APERTURES:
>>>           retcode = kfd_ioctl_get_process_apertures(filep, process,
>>>                           kdata);
>>>           break;
>>> -    case KFD_IOC_UPDATE_QUEUE:
>>> +    case AMDKFD_IOC_UPDATE_QUEUE:
>>>           retcode = kfd_ioctl_update_queue(filep, process,
>>>                           kdata);
>>>           break;
>>> diff --git a/include/uapi/linux/kfd_ioctl.h 
>>> b/include/uapi/linux/kfd_ioctl.h
>>> index 7acef41..05b53f6 100644
>>> --- a/include/uapi/linux/kfd_ioctl.h
>>> +++ b/include/uapi/linux/kfd_ioctl.h
>>> @@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
>>>       uint32_t pad;
>>>   };
>>> -#define KFD_IOC_MAGIC 'K'
>>> +#define AMDKFD_IOCTL_BASE 'K'
>>> +#define AMDKFD_IO(nr)            _IO(AMDKFD_IOCTL_BASE, nr)
>>> +#define AMDKFD_IOR(nr, type)        _IOR(AMDKFD_IOCTL_BASE, nr, type)
>>> +#define AMDKFD_IOW(nr, type)        _IOW(AMDKFD_IOCTL_BASE, nr, type)
>>> +#define AMDKFD_IOWR(nr, type)        _IOWR(AMDKFD_IOCTL_BASE, nr, 
>>> type)
>>> -#define KFD_IOC_GET_VERSION \
>>> -        _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
>>> +#define AMDKFD_IOC_GET_VERSION            \
>>> +        AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
>>> -#define KFD_IOC_CREATE_QUEUE \
>>> -        _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
>>> +#define AMDKFD_IOC_CREATE_QUEUE            \
>>> +        AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
>>> -#define KFD_IOC_DESTROY_QUEUE \
>>> -    _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
>>> +#define AMDKFD_IOC_DESTROY_QUEUE        \
>>> +        AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
>>> -#define KFD_IOC_SET_MEMORY_POLICY \
>>> -    _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
>>> +#define AMDKFD_IOC_SET_MEMORY_POLICY        \
>>> +        AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
>>> -#define KFD_IOC_GET_CLOCK_COUNTERS \
>>> -    _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
>>> +#define AMDKFD_IOC_GET_CLOCK_COUNTERS        \
>>> +        AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
>>> -#define KFD_IOC_GET_PROCESS_APERTURES \
>>> -    _IOR(KFD_IOC_MAGIC, 6, struct 
>>> kfd_ioctl_get_process_apertures_args)
>>> +#define AMDKFD_IOC_GET_PROCESS_APERTURES    \
>>> +        AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
>>> -#define KFD_IOC_UPDATE_QUEUE \
>>> -    _IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
>>> +#define AMDKFD_IOC_UPDATE_QUEUE            \
>>> +        AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
>>> +
>>> +#define KFD_COMMAND_START               0x01
>>> +#define KFD_COMMAND_END            0x08
>>
>> If you rename everything to AMDKFD_* you probably want to do so as 
>> well for
>> KFD_COMMAND_(START|END).
>
> Thanks, I'll do that.
>
>     Oded
>>
>> Regards,
>> Christian.
>>
>>>   #endif
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/amdkfd: reformat IOCTL definitions to drm-style

[Oded Gabbay]

On 12/29/2014 03:32 PM, Christian König wrote:
> Am 29.12.2014 um 14:17 schrieb Oded Gabbay:
>>
>>
>> On 12/29/2014 03:06 PM, Christian König wrote:
>>> Am 29.12.2014 um 13:42 schrieb Oded Gabbay:
>>>> This patch reformats the ioctl definitions in kfd_ioctl.h to be similar to the
>>>> drm ioctls definition style.
>>>>
>>>> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
>>>
>>> You are aware that this is a kernel API breakage?
>> Yes, but as long as 3.19 isn't released yet, I'm allowed to do this, no ?
>
> Strictly speaking only if you fix a bug with it, but since we are still pretty
> early in the cycle it's ok with we (especially since it looks like a valid
> cleanup). I'm just not sure what Dave/Linus think of this.
>
> Christian.
Well, Dave said I should remove the error prints from the ioctl function and 
generally make it similar to drm ioctl handling. And as you said, this looks 
like a valid cleanup so I assume it would be ok from his POV to merge it for -rc2/3.

	Oded

>
>> and I want this patch-set to be inserted to 3.19 so there won't be API
>> breakage afterwards.
>>
>>>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 +++++++------
>>>>   include/uapi/linux/kfd_ioctl.h           | 37
>>>> +++++++++++++++++++-------------
>>>>   2 files changed, 30 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>>> index 5460ad2..390385f 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>>> @@ -524,35 +524,36 @@ static long kfd_ioctl(struct file *filep, unsigned int
>>>> cmd, unsigned long arg)
>>>>       switch (cmd) {
>>>> -    case KFD_IOC_GET_VERSION:
>>>> +    case AMDKFD_IOC_GET_VERSION:
>>>>           retcode = kfd_ioctl_get_version(filep, process, kdata);
>>>>           break;
>>>> -    case KFD_IOC_CREATE_QUEUE:
>>>> +
>>>> +    case AMDKFD_IOC_CREATE_QUEUE:
>>>>           retcode = kfd_ioctl_create_queue(filep, process,
>>>>                           kdata);
>>>>           break;
>>>> -    case KFD_IOC_DESTROY_QUEUE:
>>>> +    case AMDKFD_IOC_DESTROY_QUEUE:
>>>>           retcode = kfd_ioctl_destroy_queue(filep, process,
>>>>                           kdata);
>>>>           break;
>>>> -    case KFD_IOC_SET_MEMORY_POLICY:
>>>> +    case AMDKFD_IOC_SET_MEMORY_POLICY:
>>>>           retcode = kfd_ioctl_set_memory_policy(filep, process,
>>>>                           kdata);
>>>>           break;
>>>> -    case KFD_IOC_GET_CLOCK_COUNTERS:
>>>> +    case AMDKFD_IOC_GET_CLOCK_COUNTERS:
>>>>           retcode = kfd_ioctl_get_clock_counters(filep, process,
>>>>                           kdata);
>>>>           break;
>>>> -    case KFD_IOC_GET_PROCESS_APERTURES:
>>>> +    case AMDKFD_IOC_GET_PROCESS_APERTURES:
>>>>           retcode = kfd_ioctl_get_process_apertures(filep, process,
>>>>                           kdata);
>>>>           break;
>>>> -    case KFD_IOC_UPDATE_QUEUE:
>>>> +    case AMDKFD_IOC_UPDATE_QUEUE:
>>>>           retcode = kfd_ioctl_update_queue(filep, process,
>>>>                           kdata);
>>>>           break;
>>>> diff --git a/include/uapi/linux/kfd_ioctl.h b/include/uapi/linux/kfd_ioctl.h
>>>> index 7acef41..05b53f6 100644
>>>> --- a/include/uapi/linux/kfd_ioctl.h
>>>> +++ b/include/uapi/linux/kfd_ioctl.h
>>>> @@ -128,27 +128,34 @@ struct kfd_ioctl_get_process_apertures_args {
>>>>       uint32_t pad;
>>>>   };
>>>> -#define KFD_IOC_MAGIC 'K'
>>>> +#define AMDKFD_IOCTL_BASE 'K'
>>>> +#define AMDKFD_IO(nr)            _IO(AMDKFD_IOCTL_BASE, nr)
>>>> +#define AMDKFD_IOR(nr, type)        _IOR(AMDKFD_IOCTL_BASE, nr, type)
>>>> +#define AMDKFD_IOW(nr, type)        _IOW(AMDKFD_IOCTL_BASE, nr, type)
>>>> +#define AMDKFD_IOWR(nr, type)        _IOWR(AMDKFD_IOCTL_BASE, nr, type)
>>>> -#define KFD_IOC_GET_VERSION \
>>>> -        _IOR(KFD_IOC_MAGIC, 1, struct kfd_ioctl_get_version_args)
>>>> +#define AMDKFD_IOC_GET_VERSION            \
>>>> +        AMDKFD_IOR(0x01, struct kfd_ioctl_get_version_args)
>>>> -#define KFD_IOC_CREATE_QUEUE \
>>>> -        _IOWR(KFD_IOC_MAGIC, 2, struct kfd_ioctl_create_queue_args)
>>>> +#define AMDKFD_IOC_CREATE_QUEUE            \
>>>> +        AMDKFD_IOWR(0x02, struct kfd_ioctl_create_queue_args)
>>>> -#define KFD_IOC_DESTROY_QUEUE \
>>>> -    _IOWR(KFD_IOC_MAGIC, 3, struct kfd_ioctl_destroy_queue_args)
>>>> +#define AMDKFD_IOC_DESTROY_QUEUE        \
>>>> +        AMDKFD_IOWR(0x03, struct kfd_ioctl_destroy_queue_args)
>>>> -#define KFD_IOC_SET_MEMORY_POLICY \
>>>> -    _IOW(KFD_IOC_MAGIC, 4, struct kfd_ioctl_set_memory_policy_args)
>>>> +#define AMDKFD_IOC_SET_MEMORY_POLICY        \
>>>> +        AMDKFD_IOW(0x04, struct kfd_ioctl_set_memory_policy_args)
>>>> -#define KFD_IOC_GET_CLOCK_COUNTERS \
>>>> -    _IOWR(KFD_IOC_MAGIC, 5, struct kfd_ioctl_get_clock_counters_args)
>>>> +#define AMDKFD_IOC_GET_CLOCK_COUNTERS        \
>>>> +        AMDKFD_IOWR(0x05, struct kfd_ioctl_get_clock_counters_args)
>>>> -#define KFD_IOC_GET_PROCESS_APERTURES \
>>>> -    _IOR(KFD_IOC_MAGIC, 6, struct kfd_ioctl_get_process_apertures_args)
>>>> +#define AMDKFD_IOC_GET_PROCESS_APERTURES    \
>>>> +        AMDKFD_IOR(0x06, struct kfd_ioctl_get_process_apertures_args)
>>>> -#define KFD_IOC_UPDATE_QUEUE \
>>>> -    _IOW(KFD_IOC_MAGIC, 7, struct kfd_ioctl_update_queue_args)
>>>> +#define AMDKFD_IOC_UPDATE_QUEUE            \
>>>> +        AMDKFD_IOW(0x07, struct kfd_ioctl_update_queue_args)
>>>> +
>>>> +#define KFD_COMMAND_START               0x01
>>>> +#define KFD_COMMAND_END            0x08
>>>
>>> If you rename everything to AMDKFD_* you probably want to do so as well for
>>> KFD_COMMAND_(START|END).
>>
>> Thanks, I'll do that.
>>
>>     Oded
>>>
>>> Regards,
>>> Christian.
>>>
>>>>   #endif
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/amdkfd: rewrite kfd_ioctl() according to drm_ioctl()

[Oded Gabbay]

This patch changes kfd_ioctl() to be very similar to drm_ioctl().

The patch defines an array of amdkfd_ioctls, which maps IOCTL definition to the
ioctl function.

The kfd_ioctl() uses that mapping to call the appropriate ioctl function,
through a function pointer.

This patch also declares a new typedef for the ioctl function pointer.

Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
---
 drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 116 ++++++++++++++++++-------------
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h    |  18 +++++
 2 files changed, 86 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 390385f..06693fa 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -483,21 +483,79 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
 	return 0;
 }
 
+#define AMDKFD_IOCTL_DEF(ioctl, _func, _flags) \
+	[_IOC_NR(ioctl)] = {.cmd = ioctl, .func = _func, .flags = _flags, .cmd_drv = 0, .name = #ioctl}
+
+/** Ioctl table */
+static const struct amdkfd_ioctl_desc amdkfd_ioctls[] = {
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_GET_VERSION,
+			kfd_ioctl_get_version, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_CREATE_QUEUE,
+			kfd_ioctl_create_queue, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_DESTROY_QUEUE,
+			kfd_ioctl_destroy_queue, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_SET_MEMORY_POLICY,
+			kfd_ioctl_set_memory_policy, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_GET_CLOCK_COUNTERS,
+			kfd_ioctl_get_clock_counters, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_GET_PROCESS_APERTURES,
+			kfd_ioctl_get_process_apertures, 0),
+
+	AMDKFD_IOCTL_DEF(AMDKFD_IOC_UPDATE_QUEUE,
+			kfd_ioctl_update_queue, 0),
+};
+
+#define AMDKFD_CORE_IOCTL_COUNT	ARRAY_SIZE(amdkfd_ioctls)
+
 static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 {
 	struct kfd_process *process;
+	amdkfd_ioctl_t *func;
+	const struct amdkfd_ioctl_desc *ioctl = NULL;
+	unsigned int nr = _IOC_NR(cmd);
 	char stack_kdata[128];
 	char *kdata = NULL;
 	unsigned int usize, asize;
 	int retcode = -EINVAL;
 
-	dev_dbg(kfd_device,
-		"ioctl cmd 0x%x (#%d), arg 0x%lx\n",
-		cmd, _IOC_NR(cmd), arg);
+	if (nr >= AMDKFD_CORE_IOCTL_COUNT)
+		goto err_i1;
+
+	if ((nr >= KFD_COMMAND_START) && (nr < KFD_COMMAND_END)) {
+		u32 amdkfd_size;
+
+		ioctl = &amdkfd_ioctls[nr];
+
+		amdkfd_size = _IOC_SIZE(ioctl->cmd);
+		usize = asize = _IOC_SIZE(cmd);
+		if (amdkfd_size > asize)
+			asize = amdkfd_size;
+
+		cmd = ioctl->cmd;
+	} else
+		goto err_i1;
+
+	dev_dbg(kfd_device, "ioctl cmd 0x%x (#%d), arg 0x%lx\n", cmd, nr, arg);
 
 	process = kfd_get_process(current);
-	if (IS_ERR(process))
-		return PTR_ERR(process);
+	if (IS_ERR(process)) {
+		dev_dbg(kfd_device, "no process\n");
+		goto err_i1;
+	}
+
+	/* Do not trust userspace, use our own definition */
+	func = ioctl->func;
+
+	if (unlikely(!func)) {
+		dev_dbg(kfd_device, "no function\n");
+		retcode = -EINVAL;
+		goto err_i1;
+	}
 
 	if (cmd & (IOC_IN | IOC_OUT)) {
 		if (asize <= sizeof(stack_kdata)) {
@@ -522,55 +580,17 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
 		memset(kdata, 0, usize);
 	}
 
-
-	switch (cmd) {
-	case AMDKFD_IOC_GET_VERSION:
-		retcode = kfd_ioctl_get_version(filep, process, kdata);
-		break;
-
-	case AMDKFD_IOC_CREATE_QUEUE:
-		retcode = kfd_ioctl_create_queue(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_DESTROY_QUEUE:
-		retcode = kfd_ioctl_destroy_queue(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_SET_MEMORY_POLICY:
-		retcode = kfd_ioctl_set_memory_policy(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_GET_CLOCK_COUNTERS:
-		retcode = kfd_ioctl_get_clock_counters(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_GET_PROCESS_APERTURES:
-		retcode = kfd_ioctl_get_process_apertures(filep, process,
-						kdata);
-		break;
-
-	case AMDKFD_IOC_UPDATE_QUEUE:
-		retcode = kfd_ioctl_update_queue(filep, process,
-						kdata);
-		break;
-
-	default:
-		dev_dbg(kfd_device,
-			"unknown ioctl cmd 0x%x, arg 0x%lx)\n",
-			cmd, arg);
-		retcode = -EINVAL;
-		break;
-	}
+	retcode = func(filep, process, kdata);
 
 	if (cmd & IOC_OUT)
 		if (copy_to_user((void __user *)arg, kdata, usize) != 0)
 			retcode = -EFAULT;
 
 err_i1:
+	if (!ioctl)
+		dev_dbg(kfd_device, "invalid ioctl: pid=%d, cmd=0x%02x, nr=0x%02x\n",
+			  task_pid_nr(current), cmd, nr);
+
 	if (kdata != stack_kdata)
 		kfree(kdata);
 
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index f9fb81e..a5edb29 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -463,6 +463,24 @@ struct kfd_process {
 	bool is_32bit_user_mode;
 };
 
+/**
+ * Ioctl function type.
+ *
+ * \param filep pointer to file structure.
+ * \param p amdkfd process pointer.
+ * \param data pointer to arg that was copied from user.
+ */
+typedef int amdkfd_ioctl_t(struct file *filep, struct kfd_process *p,
+				void *data);
+
+struct amdkfd_ioctl_desc {
+	unsigned int cmd;
+	int flags;
+	amdkfd_ioctl_t *func;
+	unsigned int cmd_drv;
+	const char *name;
+};
+
 void kfd_process_create_wq(void);
 void kfd_process_destroy_wq(void);
 struct kfd_process *kfd_create_process(const struct task_struct *);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

[Christian König]

Am 29.12.2014 um 13:42 schrieb Oded Gabbay:
> This patch moves the copy_to_user() and copy_from_user() calls from the
> different ioctl functions in amdkfd to the general kfd_ioctl() function, as
> this is a common code for all ioctls.
>
> This was done according to example taken from drm_ioctl.c
>
> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>

In general sounds like a good idea to me and the patch is "Reviewed-by: 
Christian König <christian.koenig@amd.com>" for now.

What really questions me is why we need all this code duplication and 
can't reuse the DRM infrastructure for this. But that's more a problem 
in the long term.

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 +++++++++++++++----------------
>   1 file changed, 117 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 7d4974b..5460ad2 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, struct file *filep)
>   	return 0;
>   }
>   
> -static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
> -					void __user *arg)
> +static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
> +					void *data)
>   {
> -	struct kfd_ioctl_get_version_args args;
> +	struct kfd_ioctl_get_version_args *args = data;
>   	int err = 0;
>   
> -	args.major_version = KFD_IOCTL_MAJOR_VERSION;
> -	args.minor_version = KFD_IOCTL_MINOR_VERSION;
> -
> -	if (copy_to_user(arg, &args, sizeof(args)))
> -		err = -EFAULT;
> +	args->major_version = KFD_IOCTL_MAJOR_VERSION;
> +	args->minor_version = KFD_IOCTL_MINOR_VERSION;
>   
>   	return err;
>   }
> @@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct queue_properties *q_properties,
>   	return 0;
>   }
>   
> -static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
> -					void __user *arg)
> +static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
> +					void *data)
>   {
> -	struct kfd_ioctl_create_queue_args args;
> +	struct kfd_ioctl_create_queue_args *args = data;
>   	struct kfd_dev *dev;
>   	int err = 0;
>   	unsigned int queue_id;
> @@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
>   
>   	memset(&q_properties, 0, sizeof(struct queue_properties));
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
>   	pr_debug("kfd: creating queue ioctl\n");
>   
> -	err = set_queue_properties_from_user(&q_properties, &args);
> +	err = set_queue_properties_from_user(&q_properties, args);
>   	if (err)
>   		return err;
>   
> -	dev = kfd_device_by_id(args.gpu_id);
> +	dev = kfd_device_by_id(args->gpu_id);
>   	if (dev == NULL)
>   		return -EINVAL;
>   
> @@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
>   
>   	pdd = kfd_bind_process_to_device(dev, p);
>   	if (IS_ERR(pdd)) {
> -		err = PTR_ERR(pdd);
> +		err = -ESRCH;
>   		goto err_bind_process;
>   	}
>   
> @@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
>   	if (err != 0)
>   		goto err_create_queue;
>   
> -	args.queue_id = queue_id;
> +	args->queue_id = queue_id;
>   
>   	/* Return gpu_id as doorbell offset for mmap usage */
> -	args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
> -
> -	if (copy_to_user(arg, &args, sizeof(args))) {
> -		err = -EFAULT;
> -		goto err_copy_args_out;
> -	}
> +	args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
>   
>   	mutex_unlock(&p->mutex);
>   
> -	pr_debug("kfd: queue id %d was created successfully\n", args.queue_id);
> +	pr_debug("kfd: queue id %d was created successfully\n", args->queue_id);
>   
>   	pr_debug("ring buffer address == 0x%016llX\n",
> -			args.ring_base_address);
> +			args->ring_base_address);
>   
>   	pr_debug("read ptr address    == 0x%016llX\n",
> -			args.read_pointer_address);
> +			args->read_pointer_address);
>   
>   	pr_debug("write ptr address   == 0x%016llX\n",
> -			args.write_pointer_address);
> +			args->write_pointer_address);
>   
>   	return 0;
>   
> -err_copy_args_out:
> -	pqm_destroy_queue(&p->pqm, queue_id);
>   err_create_queue:
>   err_bind_process:
>   	mutex_unlock(&p->mutex);
> @@ -297,99 +284,90 @@ err_bind_process:
>   }
>   
>   static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
> -					void __user *arg)
> +					void *data)
>   {
>   	int retval;
> -	struct kfd_ioctl_destroy_queue_args args;
> -
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> +	struct kfd_ioctl_destroy_queue_args *args = data;
>   
>   	pr_debug("kfd: destroying queue id %d for PASID %d\n",
> -				args.queue_id,
> +				args->queue_id,
>   				p->pasid);
>   
>   	mutex_lock(&p->mutex);
>   
> -	retval = pqm_destroy_queue(&p->pqm, args.queue_id);
> +	retval = pqm_destroy_queue(&p->pqm, args->queue_id);
>   
>   	mutex_unlock(&p->mutex);
>   	return retval;
>   }
>   
>   static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
> -					void __user *arg)
> +					void *data)
>   {
>   	int retval;
> -	struct kfd_ioctl_update_queue_args args;
> +	struct kfd_ioctl_update_queue_args *args = data;
>   	struct queue_properties properties;
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
> -	if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
> +	if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
>   		pr_err("kfd: queue percentage must be between 0 to KFD_MAX_QUEUE_PERCENTAGE\n");
>   		return -EINVAL;
>   	}
>   
> -	if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
> +	if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
>   		pr_err("kfd: queue priority must be between 0 to KFD_MAX_QUEUE_PRIORITY\n");
>   		return -EINVAL;
>   	}
>   
> -	if ((args.ring_base_address) &&
> +	if ((args->ring_base_address) &&
>   		(!access_ok(VERIFY_WRITE,
> -			(const void __user *) args.ring_base_address,
> +			(const void __user *) args->ring_base_address,
>   			sizeof(uint64_t)))) {
>   		pr_err("kfd: can't access ring base address\n");
>   		return -EFAULT;
>   	}
>   
> -	if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
> +	if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
>   		pr_err("kfd: ring size must be a power of 2 or 0\n");
>   		return -EINVAL;
>   	}
>   
> -	properties.queue_address = args.ring_base_address;
> -	properties.queue_size = args.ring_size;
> -	properties.queue_percent = args.queue_percentage;
> -	properties.priority = args.queue_priority;
> +	properties.queue_address = args->ring_base_address;
> +	properties.queue_size = args->ring_size;
> +	properties.queue_percent = args->queue_percentage;
> +	properties.priority = args->queue_priority;
>   
>   	pr_debug("kfd: updating queue id %d for PASID %d\n",
> -			args.queue_id, p->pasid);
> +			args->queue_id, p->pasid);
>   
>   	mutex_lock(&p->mutex);
>   
> -	retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
> +	retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
>   
>   	mutex_unlock(&p->mutex);
>   
>   	return retval;
>   }
>   
> -static long kfd_ioctl_set_memory_policy(struct file *filep,
> -				struct kfd_process *p, void __user *arg)
> +static int kfd_ioctl_set_memory_policy(struct file *filep,
> +					struct kfd_process *p, void *data)
>   {
> -	struct kfd_ioctl_set_memory_policy_args args;
> +	struct kfd_ioctl_set_memory_policy_args *args = data;
>   	struct kfd_dev *dev;
>   	int err = 0;
>   	struct kfd_process_device *pdd;
>   	enum cache_policy default_policy, alternate_policy;
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
> -	if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
> -	    && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
> +	if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
> +	    && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>   		return -EINVAL;
>   	}
>   
> -	if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
> -	    && args.alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
> +	if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
> +	    && args->alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>   		return -EINVAL;
>   	}
>   
> -	dev = kfd_device_by_id(args.gpu_id);
> +	dev = kfd_device_by_id(args->gpu_id);
>   	if (dev == NULL)
>   		return -EINVAL;
>   
> @@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct file *filep,
>   
>   	pdd = kfd_bind_process_to_device(dev, p);
>   	if (IS_ERR(pdd)) {
> -		err = PTR_ERR(pdd);
> +		err = -ESRCH;
>   		goto out;
>   	}
>   
> -	default_policy = (args.default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
> +	default_policy = (args->default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>   			 ? cache_policy_coherent : cache_policy_noncoherent;
>   
>   	alternate_policy =
> -		(args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
> +		(args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>   		   ? cache_policy_coherent : cache_policy_noncoherent;
>   
>   	if (!dev->dqm->set_cache_memory_policy(dev->dqm,
>   				&pdd->qpd,
>   				default_policy,
>   				alternate_policy,
> -				(void __user *)args.alternate_aperture_base,
> -				args.alternate_aperture_size))
> +				(void __user *)args->alternate_aperture_base,
> +				args->alternate_aperture_size))
>   		err = -EINVAL;
>   
>   out:
> @@ -422,53 +400,44 @@ out:
>   	return err;
>   }
>   
> -static long kfd_ioctl_get_clock_counters(struct file *filep,
> -				struct kfd_process *p, void __user *arg)
> +static int kfd_ioctl_get_clock_counters(struct file *filep,
> +				struct kfd_process *p, void *data)
>   {
> -	struct kfd_ioctl_get_clock_counters_args args;
> +	struct kfd_ioctl_get_clock_counters_args *args = data;
>   	struct kfd_dev *dev;
>   	struct timespec time;
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
> -	dev = kfd_device_by_id(args.gpu_id);
> +	dev = kfd_device_by_id(args->gpu_id);
>   	if (dev == NULL)
>   		return -EINVAL;
>   
>   	/* Reading GPU clock counter from KGD */
> -	args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
> +	args->gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
>   
>   	/* No access to rdtsc. Using raw monotonic time */
>   	getrawmonotonic(&time);
> -	args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
> +	args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
>   
>   	get_monotonic_boottime(&time);
> -	args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
> +	args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
>   
>   	/* Since the counter is in nano-seconds we use 1GHz frequency */
> -	args.system_clock_freq = 1000000000;
> -
> -	if (copy_to_user(arg, &args, sizeof(args)))
> -		return -EFAULT;
> +	args->system_clock_freq = 1000000000;
>   
>   	return 0;
>   }
>   
>   
>   static int kfd_ioctl_get_process_apertures(struct file *filp,
> -				struct kfd_process *p, void __user *arg)
> +				struct kfd_process *p, void *data)
>   {
> -	struct kfd_ioctl_get_process_apertures_args args;
> +	struct kfd_ioctl_get_process_apertures_args *args = data;
>   	struct kfd_process_device_apertures *pAperture;
>   	struct kfd_process_device *pdd;
>   
>   	dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
>   
> -	if (copy_from_user(&args, arg, sizeof(args)))
> -		return -EFAULT;
> -
> -	args.num_of_nodes = 0;
> +	args->num_of_nodes = 0;
>   
>   	mutex_lock(&p->mutex);
>   
> @@ -477,7 +446,8 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
>   		/* Run over all pdd of the process */
>   		pdd = kfd_get_first_process_device_data(p);
>   		do {
> -			pAperture = &args.process_apertures[args.num_of_nodes];
> +			pAperture =
> +				&args->process_apertures[args->num_of_nodes];
>   			pAperture->gpu_id = pdd->dev->id;
>   			pAperture->lds_base = pdd->lds_base;
>   			pAperture->lds_limit = pdd->lds_limit;
> @@ -487,7 +457,7 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
>   			pAperture->scratch_limit = pdd->scratch_limit;
>   
>   			dev_dbg(kfd_device,
> -				"node id %u\n", args.num_of_nodes);
> +				"node id %u\n", args->num_of_nodes);
>   			dev_dbg(kfd_device,
>   				"gpu id %u\n", pdd->dev->id);
>   			dev_dbg(kfd_device,
> @@ -503,23 +473,23 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
>   			dev_dbg(kfd_device,
>   				"scratch_limit %llX\n", pdd->scratch_limit);
>   
> -			args.num_of_nodes++;
> +			args->num_of_nodes++;
>   		} while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
> -				(args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
> +				(args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
>   	}
>   
>   	mutex_unlock(&p->mutex);
>   
> -	if (copy_to_user(arg, &args, sizeof(args)))
> -		return -EFAULT;
> -
>   	return 0;
>   }
>   
>   static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>   {
>   	struct kfd_process *process;
> -	long err = -EINVAL;
> +	char stack_kdata[128];
> +	char *kdata = NULL;
> +	unsigned int usize, asize;
> +	int retcode = -EINVAL;
>   
>   	dev_dbg(kfd_device,
>   		"ioctl cmd 0x%x (#%d), arg 0x%lx\n",
> @@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>   	if (IS_ERR(process))
>   		return PTR_ERR(process);
>   
> +	if (cmd & (IOC_IN | IOC_OUT)) {
> +		if (asize <= sizeof(stack_kdata)) {
> +			kdata = stack_kdata;
> +		} else {
> +			kdata = kmalloc(asize, GFP_KERNEL);
> +			if (!kdata) {
> +				retcode = -ENOMEM;
> +				goto err_i1;
> +			}
> +		}
> +		if (asize > usize)
> +			memset(kdata + usize, 0, asize - usize);
> +	}
> +
> +	if (cmd & IOC_IN) {
> +		if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
> +			retcode = -EFAULT;
> +			goto err_i1;
> +		}
> +	} else if (cmd & IOC_OUT) {
> +		memset(kdata, 0, usize);
> +	}
> +
> +
>   	switch (cmd) {
>   	case KFD_IOC_GET_VERSION:
> -		err = kfd_ioctl_get_version(filep, process, (void __user *)arg);
> +		retcode = kfd_ioctl_get_version(filep, process, kdata);
>   		break;
>   	case KFD_IOC_CREATE_QUEUE:
> -		err = kfd_ioctl_create_queue(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_create_queue(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_DESTROY_QUEUE:
> -		err = kfd_ioctl_destroy_queue(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_destroy_queue(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_SET_MEMORY_POLICY:
> -		err = kfd_ioctl_set_memory_policy(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_set_memory_policy(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_GET_CLOCK_COUNTERS:
> -		err = kfd_ioctl_get_clock_counters(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_get_clock_counters(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_GET_PROCESS_APERTURES:
> -		err = kfd_ioctl_get_process_apertures(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_get_process_apertures(filep, process,
> +						kdata);
>   		break;
>   
>   	case KFD_IOC_UPDATE_QUEUE:
> -		err = kfd_ioctl_update_queue(filep, process,
> -						(void __user *)arg);
> +		retcode = kfd_ioctl_update_queue(filep, process,
> +						kdata);
>   		break;
>   
>   	default:
> -		dev_err(kfd_device,
> +		dev_dbg(kfd_device,
>   			"unknown ioctl cmd 0x%x, arg 0x%lx)\n",
>   			cmd, arg);
> -		err = -EINVAL;
> +		retcode = -EINVAL;
>   		break;
>   	}
>   
> -	if (err < 0)
> -		dev_err(kfd_device,
> -			"ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
> -			err, cmd, _IOC_NR(cmd));
> +	if (cmd & IOC_OUT)
> +		if (copy_to_user((void __user *)arg, kdata, usize) != 0)
> +			retcode = -EFAULT;
>   
> -	return err;
> +err_i1:
> +	if (kdata != stack_kdata)
> +		kfree(kdata);
> +
> +	if (retcode)
> +		dev_dbg(kfd_device, "ret = %d\n", retcode);
> +
> +	return retcode;
>   }
>   
>   static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

[Oded Gabbay]

On 12/29/2014 03:05 PM, Christian König wrote:
> Am 29.12.2014 um 13:42 schrieb Oded Gabbay:
>> This patch moves the copy_to_user() and copy_from_user() calls from the
>> different ioctl functions in amdkfd to the general kfd_ioctl() function, as
>> this is a common code for all ioctls.
>>
>> This was done according to example taken from drm_ioctl.c
>>
>> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
>
> In general sounds like a good idea to me and the patch is "Reviewed-by:
> Christian König <christian.koenig@amd.com>" for now.
>
> What really questions me is why we need all this code duplication and can't
> reuse the DRM infrastructure for this. But that's more a problem in the long term.
>
Do you mean registering as DRM IOCTL ?
Or do you mean something else ?

If it is the former, than I think the main problem is that we use different 
devices (/dev/kfd vs. /dev/dri/)

If it is the latter, could you give me more specifics ?

	Oded

> Regards,
> Christian.
>
>> ---
>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 +++++++++++++++----------------
>>   1 file changed, 117 insertions(+), 117 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> index 7d4974b..5460ad2 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>> @@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, struct file
>> *filep)
>>       return 0;
>>   }
>> -static long kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
>> -                    void __user *arg)
>> +static int kfd_ioctl_get_version(struct file *filep, struct kfd_process *p,
>> +                    void *data)
>>   {
>> -    struct kfd_ioctl_get_version_args args;
>> +    struct kfd_ioctl_get_version_args *args = data;
>>       int err = 0;
>> -    args.major_version = KFD_IOCTL_MAJOR_VERSION;
>> -    args.minor_version = KFD_IOCTL_MINOR_VERSION;
>> -
>> -    if (copy_to_user(arg, &args, sizeof(args)))
>> -        err = -EFAULT;
>> +    args->major_version = KFD_IOCTL_MAJOR_VERSION;
>> +    args->minor_version = KFD_IOCTL_MINOR_VERSION;
>>       return err;
>>   }
>> @@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct
>> queue_properties *q_properties,
>>       return 0;
>>   }
>> -static long kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
>> -                    void __user *arg)
>> +static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,
>> +                    void *data)
>>   {
>> -    struct kfd_ioctl_create_queue_args args;
>> +    struct kfd_ioctl_create_queue_args *args = data;
>>       struct kfd_dev *dev;
>>       int err = 0;
>>       unsigned int queue_id;
>> @@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file *filep,
>> struct kfd_process *p,
>>       memset(&q_properties, 0, sizeof(struct queue_properties));
>> -    if (copy_from_user(&args, arg, sizeof(args)))
>> -        return -EFAULT;
>> -
>>       pr_debug("kfd: creating queue ioctl\n");
>> -    err = set_queue_properties_from_user(&q_properties, &args);
>> +    err = set_queue_properties_from_user(&q_properties, args);
>>       if (err)
>>           return err;
>> -    dev = kfd_device_by_id(args.gpu_id);
>> +    dev = kfd_device_by_id(args->gpu_id);
>>       if (dev == NULL)
>>           return -EINVAL;
>> @@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file *filep,
>> struct kfd_process *p,
>>       pdd = kfd_bind_process_to_device(dev, p);
>>       if (IS_ERR(pdd)) {
>> -        err = PTR_ERR(pdd);
>> +        err = -ESRCH;
>>           goto err_bind_process;
>>       }
>> @@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file *filep,
>> struct kfd_process *p,
>>       if (err != 0)
>>           goto err_create_queue;
>> -    args.queue_id = queue_id;
>> +    args->queue_id = queue_id;
>>       /* Return gpu_id as doorbell offset for mmap usage */
>> -    args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
>> -
>> -    if (copy_to_user(arg, &args, sizeof(args))) {
>> -        err = -EFAULT;
>> -        goto err_copy_args_out;
>> -    }
>> +    args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
>>       mutex_unlock(&p->mutex);
>> -    pr_debug("kfd: queue id %d was created successfully\n", args.queue_id);
>> +    pr_debug("kfd: queue id %d was created successfully\n", args->queue_id);
>>       pr_debug("ring buffer address == 0x%016llX\n",
>> -            args.ring_base_address);
>> +            args->ring_base_address);
>>       pr_debug("read ptr address    == 0x%016llX\n",
>> -            args.read_pointer_address);
>> +            args->read_pointer_address);
>>       pr_debug("write ptr address   == 0x%016llX\n",
>> -            args.write_pointer_address);
>> +            args->write_pointer_address);
>>       return 0;
>> -err_copy_args_out:
>> -    pqm_destroy_queue(&p->pqm, queue_id);
>>   err_create_queue:
>>   err_bind_process:
>>       mutex_unlock(&p->mutex);
>> @@ -297,99 +284,90 @@ err_bind_process:
>>   }
>>   static int kfd_ioctl_destroy_queue(struct file *filp, struct kfd_process *p,
>> -                    void __user *arg)
>> +                    void *data)
>>   {
>>       int retval;
>> -    struct kfd_ioctl_destroy_queue_args args;
>> -
>> -    if (copy_from_user(&args, arg, sizeof(args)))
>> -        return -EFAULT;
>> +    struct kfd_ioctl_destroy_queue_args *args = data;
>>       pr_debug("kfd: destroying queue id %d for PASID %d\n",
>> -                args.queue_id,
>> +                args->queue_id,
>>                   p->pasid);
>>       mutex_lock(&p->mutex);
>> -    retval = pqm_destroy_queue(&p->pqm, args.queue_id);
>> +    retval = pqm_destroy_queue(&p->pqm, args->queue_id);
>>       mutex_unlock(&p->mutex);
>>       return retval;
>>   }
>>   static int kfd_ioctl_update_queue(struct file *filp, struct kfd_process *p,
>> -                    void __user *arg)
>> +                    void *data)
>>   {
>>       int retval;
>> -    struct kfd_ioctl_update_queue_args args;
>> +    struct kfd_ioctl_update_queue_args *args = data;
>>       struct queue_properties properties;
>> -    if (copy_from_user(&args, arg, sizeof(args)))
>> -        return -EFAULT;
>> -
>> -    if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
>> +    if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
>>           pr_err("kfd: queue percentage must be between 0 to
>> KFD_MAX_QUEUE_PERCENTAGE\n");
>>           return -EINVAL;
>>       }
>> -    if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
>> +    if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
>>           pr_err("kfd: queue priority must be between 0 to
>> KFD_MAX_QUEUE_PRIORITY\n");
>>           return -EINVAL;
>>       }
>> -    if ((args.ring_base_address) &&
>> +    if ((args->ring_base_address) &&
>>           (!access_ok(VERIFY_WRITE,
>> -            (const void __user *) args.ring_base_address,
>> +            (const void __user *) args->ring_base_address,
>>               sizeof(uint64_t)))) {
>>           pr_err("kfd: can't access ring base address\n");
>>           return -EFAULT;
>>       }
>> -    if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
>> +    if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
>>           pr_err("kfd: ring size must be a power of 2 or 0\n");
>>           return -EINVAL;
>>       }
>> -    properties.queue_address = args.ring_base_address;
>> -    properties.queue_size = args.ring_size;
>> -    properties.queue_percent = args.queue_percentage;
>> -    properties.priority = args.queue_priority;
>> +    properties.queue_address = args->ring_base_address;
>> +    properties.queue_size = args->ring_size;
>> +    properties.queue_percent = args->queue_percentage;
>> +    properties.priority = args->queue_priority;
>>       pr_debug("kfd: updating queue id %d for PASID %d\n",
>> -            args.queue_id, p->pasid);
>> +            args->queue_id, p->pasid);
>>       mutex_lock(&p->mutex);
>> -    retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
>> +    retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
>>       mutex_unlock(&p->mutex);
>>       return retval;
>>   }
>> -static long kfd_ioctl_set_memory_policy(struct file *filep,
>> -                struct kfd_process *p, void __user *arg)
>> +static int kfd_ioctl_set_memory_policy(struct file *filep,
>> +                    struct kfd_process *p, void *data)
>>   {
>> -    struct kfd_ioctl_set_memory_policy_args args;
>> +    struct kfd_ioctl_set_memory_policy_args *args = data;
>>       struct kfd_dev *dev;
>>       int err = 0;
>>       struct kfd_process_device *pdd;
>>       enum cache_policy default_policy, alternate_policy;
>> -    if (copy_from_user(&args, arg, sizeof(args)))
>> -        return -EFAULT;
>> -
>> -    if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
>> -        && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>> +    if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
>> +        && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>>           return -EINVAL;
>>       }
>> -    if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
>> -        && args.alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>> +    if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
>> +        && args->alternate_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>>           return -EINVAL;
>>       }
>> -    dev = kfd_device_by_id(args.gpu_id);
>> +    dev = kfd_device_by_id(args->gpu_id);
>>       if (dev == NULL)
>>           return -EINVAL;
>> @@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct file *filep,
>>       pdd = kfd_bind_process_to_device(dev, p);
>>       if (IS_ERR(pdd)) {
>> -        err = PTR_ERR(pdd);
>> +        err = -ESRCH;
>>           goto out;
>>       }
>> -    default_policy = (args.default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>> +    default_policy = (args->default_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>>                ? cache_policy_coherent : cache_policy_noncoherent;
>>       alternate_policy =
>> -        (args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>> +        (args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>>              ? cache_policy_coherent : cache_policy_noncoherent;
>>       if (!dev->dqm->set_cache_memory_policy(dev->dqm,
>>                   &pdd->qpd,
>>                   default_policy,
>>                   alternate_policy,
>> -                (void __user *)args.alternate_aperture_base,
>> -                args.alternate_aperture_size))
>> +                (void __user *)args->alternate_aperture_base,
>> +                args->alternate_aperture_size))
>>           err = -EINVAL;
>>   out:
>> @@ -422,53 +400,44 @@ out:
>>       return err;
>>   }
>> -static long kfd_ioctl_get_clock_counters(struct file *filep,
>> -                struct kfd_process *p, void __user *arg)
>> +static int kfd_ioctl_get_clock_counters(struct file *filep,
>> +                struct kfd_process *p, void *data)
>>   {
>> -    struct kfd_ioctl_get_clock_counters_args args;
>> +    struct kfd_ioctl_get_clock_counters_args *args = data;
>>       struct kfd_dev *dev;
>>       struct timespec time;
>> -    if (copy_from_user(&args, arg, sizeof(args)))
>> -        return -EFAULT;
>> -
>> -    dev = kfd_device_by_id(args.gpu_id);
>> +    dev = kfd_device_by_id(args->gpu_id);
>>       if (dev == NULL)
>>           return -EINVAL;
>>       /* Reading GPU clock counter from KGD */
>> -    args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
>> +    args->gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
>>       /* No access to rdtsc. Using raw monotonic time */
>>       getrawmonotonic(&time);
>> -    args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
>> +    args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
>>       get_monotonic_boottime(&time);
>> -    args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
>> +    args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
>>       /* Since the counter is in nano-seconds we use 1GHz frequency */
>> -    args.system_clock_freq = 1000000000;
>> -
>> -    if (copy_to_user(arg, &args, sizeof(args)))
>> -        return -EFAULT;
>> +    args->system_clock_freq = 1000000000;
>>       return 0;
>>   }
>>   static int kfd_ioctl_get_process_apertures(struct file *filp,
>> -                struct kfd_process *p, void __user *arg)
>> +                struct kfd_process *p, void *data)
>>   {
>> -    struct kfd_ioctl_get_process_apertures_args args;
>> +    struct kfd_ioctl_get_process_apertures_args *args = data;
>>       struct kfd_process_device_apertures *pAperture;
>>       struct kfd_process_device *pdd;
>>       dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
>> -    if (copy_from_user(&args, arg, sizeof(args)))
>> -        return -EFAULT;
>> -
>> -    args.num_of_nodes = 0;
>> +    args->num_of_nodes = 0;
>>       mutex_lock(&p->mutex);
>> @@ -477,7 +446,8 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
>>           /* Run over all pdd of the process */
>>           pdd = kfd_get_first_process_device_data(p);
>>           do {
>> -            pAperture = &args.process_apertures[args.num_of_nodes];
>> +            pAperture =
>> +                &args->process_apertures[args->num_of_nodes];
>>               pAperture->gpu_id = pdd->dev->id;
>>               pAperture->lds_base = pdd->lds_base;
>>               pAperture->lds_limit = pdd->lds_limit;
>> @@ -487,7 +457,7 @@ static int kfd_ioctl_get_process_apertures(struct file *filp,
>>               pAperture->scratch_limit = pdd->scratch_limit;
>>               dev_dbg(kfd_device,
>> -                "node id %u\n", args.num_of_nodes);
>> +                "node id %u\n", args->num_of_nodes);
>>               dev_dbg(kfd_device,
>>                   "gpu id %u\n", pdd->dev->id);
>>               dev_dbg(kfd_device,
>> @@ -503,23 +473,23 @@ static int kfd_ioctl_get_process_apertures(struct file
>> *filp,
>>               dev_dbg(kfd_device,
>>                   "scratch_limit %llX\n", pdd->scratch_limit);
>> -            args.num_of_nodes++;
>> +            args->num_of_nodes++;
>>           } while ((pdd = kfd_get_next_process_device_data(p, pdd)) != NULL &&
>> -                (args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
>> +                (args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
>>       }
>>       mutex_unlock(&p->mutex);
>> -    if (copy_to_user(arg, &args, sizeof(args)))
>> -        return -EFAULT;
>> -
>>       return 0;
>>   }
>>   static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>>   {
>>       struct kfd_process *process;
>> -    long err = -EINVAL;
>> +    char stack_kdata[128];
>> +    char *kdata = NULL;
>> +    unsigned int usize, asize;
>> +    int retcode = -EINVAL;
>>       dev_dbg(kfd_device,
>>           "ioctl cmd 0x%x (#%d), arg 0x%lx\n",
>> @@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, unsigned int
>> cmd, unsigned long arg)
>>       if (IS_ERR(process))
>>           return PTR_ERR(process);
>> +    if (cmd & (IOC_IN | IOC_OUT)) {
>> +        if (asize <= sizeof(stack_kdata)) {
>> +            kdata = stack_kdata;
>> +        } else {
>> +            kdata = kmalloc(asize, GFP_KERNEL);
>> +            if (!kdata) {
>> +                retcode = -ENOMEM;
>> +                goto err_i1;
>> +            }
>> +        }
>> +        if (asize > usize)
>> +            memset(kdata + usize, 0, asize - usize);
>> +    }
>> +
>> +    if (cmd & IOC_IN) {
>> +        if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
>> +            retcode = -EFAULT;
>> +            goto err_i1;
>> +        }
>> +    } else if (cmd & IOC_OUT) {
>> +        memset(kdata, 0, usize);
>> +    }
>> +
>> +
>>       switch (cmd) {
>>       case KFD_IOC_GET_VERSION:
>> -        err = kfd_ioctl_get_version(filep, process, (void __user *)arg);
>> +        retcode = kfd_ioctl_get_version(filep, process, kdata);
>>           break;
>>       case KFD_IOC_CREATE_QUEUE:
>> -        err = kfd_ioctl_create_queue(filep, process,
>> -                        (void __user *)arg);
>> +        retcode = kfd_ioctl_create_queue(filep, process,
>> +                        kdata);
>>           break;
>>       case KFD_IOC_DESTROY_QUEUE:
>> -        err = kfd_ioctl_destroy_queue(filep, process,
>> -                        (void __user *)arg);
>> +        retcode = kfd_ioctl_destroy_queue(filep, process,
>> +                        kdata);
>>           break;
>>       case KFD_IOC_SET_MEMORY_POLICY:
>> -        err = kfd_ioctl_set_memory_policy(filep, process,
>> -                        (void __user *)arg);
>> +        retcode = kfd_ioctl_set_memory_policy(filep, process,
>> +                        kdata);
>>           break;
>>       case KFD_IOC_GET_CLOCK_COUNTERS:
>> -        err = kfd_ioctl_get_clock_counters(filep, process,
>> -                        (void __user *)arg);
>> +        retcode = kfd_ioctl_get_clock_counters(filep, process,
>> +                        kdata);
>>           break;
>>       case KFD_IOC_GET_PROCESS_APERTURES:
>> -        err = kfd_ioctl_get_process_apertures(filep, process,
>> -                        (void __user *)arg);
>> +        retcode = kfd_ioctl_get_process_apertures(filep, process,
>> +                        kdata);
>>           break;
>>       case KFD_IOC_UPDATE_QUEUE:
>> -        err = kfd_ioctl_update_queue(filep, process,
>> -                        (void __user *)arg);
>> +        retcode = kfd_ioctl_update_queue(filep, process,
>> +                        kdata);
>>           break;
>>       default:
>> -        dev_err(kfd_device,
>> +        dev_dbg(kfd_device,
>>               "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
>>               cmd, arg);
>> -        err = -EINVAL;
>> +        retcode = -EINVAL;
>>           break;
>>       }
>> -    if (err < 0)
>> -        dev_err(kfd_device,
>> -            "ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
>> -            err, cmd, _IOC_NR(cmd));
>> +    if (cmd & IOC_OUT)
>> +        if (copy_to_user((void __user *)arg, kdata, usize) != 0)
>> +            retcode = -EFAULT;
>> -    return err;
>> +err_i1:
>> +    if (kdata != stack_kdata)
>> +        kfree(kdata);
>> +
>> +    if (retcode)
>> +        dev_dbg(kfd_device, "ret = %d\n", retcode);
>> +
>> +    return retcode;
>>   }
>>   static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/amdkfd: Do copy_to/from_user in general kfd_ioctl()

[Christian König]

Am 29.12.2014 um 14:22 schrieb Oded Gabbay:
>
>
> On 12/29/2014 03:05 PM, Christian König wrote:
>> Am 29.12.2014 um 13:42 schrieb Oded Gabbay:
>>> This patch moves the copy_to_user() and copy_from_user() calls from the
>>> different ioctl functions in amdkfd to the general kfd_ioctl() 
>>> function, as
>>> this is a common code for all ioctls.
>>>
>>> This was done according to example taken from drm_ioctl.c
>>>
>>> Signed-off-by: Oded Gabbay <oded.gabbay@amd.com>
>>
>> In general sounds like a good idea to me and the patch is "Reviewed-by:
>> Christian König <christian.koenig@amd.com>" for now.
>>
>> What really questions me is why we need all this code duplication and 
>> can't
>> reuse the DRM infrastructure for this. But that's more a problem in 
>> the long term.
>>
> Do you mean registering as DRM IOCTL ?
> Or do you mean something else ?
>
> If it is the former, than I think the main problem is that we use 
> different devices (/dev/kfd vs. /dev/dri/)

Ah, yes of course. I simply keep forgetting that we have two device 
nodes for the same physical hardware.

Christian.

>
> If it is the latter, could you give me more specifics ?
>
>     Oded
>
>> Regards,
>> Christian.
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 234 
>>> +++++++++++++++----------------
>>>   1 file changed, 117 insertions(+), 117 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> index 7d4974b..5460ad2 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
>>> @@ -127,17 +127,14 @@ static int kfd_open(struct inode *inode, 
>>> struct file
>>> *filep)
>>>       return 0;
>>>   }
>>> -static long kfd_ioctl_get_version(struct file *filep, struct 
>>> kfd_process *p,
>>> -                    void __user *arg)
>>> +static int kfd_ioctl_get_version(struct file *filep, struct 
>>> kfd_process *p,
>>> +                    void *data)
>>>   {
>>> -    struct kfd_ioctl_get_version_args args;
>>> +    struct kfd_ioctl_get_version_args *args = data;
>>>       int err = 0;
>>> -    args.major_version = KFD_IOCTL_MAJOR_VERSION;
>>> -    args.minor_version = KFD_IOCTL_MINOR_VERSION;
>>> -
>>> -    if (copy_to_user(arg, &args, sizeof(args)))
>>> -        err = -EFAULT;
>>> +    args->major_version = KFD_IOCTL_MAJOR_VERSION;
>>> +    args->minor_version = KFD_IOCTL_MINOR_VERSION;
>>>       return err;
>>>   }
>>> @@ -221,10 +218,10 @@ static int set_queue_properties_from_user(struct
>>> queue_properties *q_properties,
>>>       return 0;
>>>   }
>>> -static long kfd_ioctl_create_queue(struct file *filep, struct 
>>> kfd_process *p,
>>> -                    void __user *arg)
>>> +static int kfd_ioctl_create_queue(struct file *filep, struct 
>>> kfd_process *p,
>>> +                    void *data)
>>>   {
>>> -    struct kfd_ioctl_create_queue_args args;
>>> +    struct kfd_ioctl_create_queue_args *args = data;
>>>       struct kfd_dev *dev;
>>>       int err = 0;
>>>       unsigned int queue_id;
>>> @@ -233,16 +230,13 @@ static long kfd_ioctl_create_queue(struct file 
>>> *filep,
>>> struct kfd_process *p,
>>>       memset(&q_properties, 0, sizeof(struct queue_properties));
>>> -    if (copy_from_user(&args, arg, sizeof(args)))
>>> -        return -EFAULT;
>>> -
>>>       pr_debug("kfd: creating queue ioctl\n");
>>> -    err = set_queue_properties_from_user(&q_properties, &args);
>>> +    err = set_queue_properties_from_user(&q_properties, args);
>>>       if (err)
>>>           return err;
>>> -    dev = kfd_device_by_id(args.gpu_id);
>>> +    dev = kfd_device_by_id(args->gpu_id);
>>>       if (dev == NULL)
>>>           return -EINVAL;
>>> @@ -250,7 +244,7 @@ static long kfd_ioctl_create_queue(struct file 
>>> *filep,
>>> struct kfd_process *p,
>>>       pdd = kfd_bind_process_to_device(dev, p);
>>>       if (IS_ERR(pdd)) {
>>> -        err = PTR_ERR(pdd);
>>> +        err = -ESRCH;
>>>           goto err_bind_process;
>>>       }
>>> @@ -263,33 +257,26 @@ static long kfd_ioctl_create_queue(struct file 
>>> *filep,
>>> struct kfd_process *p,
>>>       if (err != 0)
>>>           goto err_create_queue;
>>> -    args.queue_id = queue_id;
>>> +    args->queue_id = queue_id;
>>>       /* Return gpu_id as doorbell offset for mmap usage */
>>> -    args.doorbell_offset = args.gpu_id << PAGE_SHIFT;
>>> -
>>> -    if (copy_to_user(arg, &args, sizeof(args))) {
>>> -        err = -EFAULT;
>>> -        goto err_copy_args_out;
>>> -    }
>>> +    args->doorbell_offset = args->gpu_id << PAGE_SHIFT;
>>>       mutex_unlock(&p->mutex);
>>> -    pr_debug("kfd: queue id %d was created successfully\n", 
>>> args.queue_id);
>>> +    pr_debug("kfd: queue id %d was created successfully\n", 
>>> args->queue_id);
>>>       pr_debug("ring buffer address == 0x%016llX\n",
>>> -            args.ring_base_address);
>>> +            args->ring_base_address);
>>>       pr_debug("read ptr address    == 0x%016llX\n",
>>> -            args.read_pointer_address);
>>> +            args->read_pointer_address);
>>>       pr_debug("write ptr address   == 0x%016llX\n",
>>> -            args.write_pointer_address);
>>> +            args->write_pointer_address);
>>>       return 0;
>>> -err_copy_args_out:
>>> -    pqm_destroy_queue(&p->pqm, queue_id);
>>>   err_create_queue:
>>>   err_bind_process:
>>>       mutex_unlock(&p->mutex);
>>> @@ -297,99 +284,90 @@ err_bind_process:
>>>   }
>>>   static int kfd_ioctl_destroy_queue(struct file *filp, struct 
>>> kfd_process *p,
>>> -                    void __user *arg)
>>> +                    void *data)
>>>   {
>>>       int retval;
>>> -    struct kfd_ioctl_destroy_queue_args args;
>>> -
>>> -    if (copy_from_user(&args, arg, sizeof(args)))
>>> -        return -EFAULT;
>>> +    struct kfd_ioctl_destroy_queue_args *args = data;
>>>       pr_debug("kfd: destroying queue id %d for PASID %d\n",
>>> -                args.queue_id,
>>> +                args->queue_id,
>>>                   p->pasid);
>>>       mutex_lock(&p->mutex);
>>> -    retval = pqm_destroy_queue(&p->pqm, args.queue_id);
>>> +    retval = pqm_destroy_queue(&p->pqm, args->queue_id);
>>>       mutex_unlock(&p->mutex);
>>>       return retval;
>>>   }
>>>   static int kfd_ioctl_update_queue(struct file *filp, struct 
>>> kfd_process *p,
>>> -                    void __user *arg)
>>> +                    void *data)
>>>   {
>>>       int retval;
>>> -    struct kfd_ioctl_update_queue_args args;
>>> +    struct kfd_ioctl_update_queue_args *args = data;
>>>       struct queue_properties properties;
>>> -    if (copy_from_user(&args, arg, sizeof(args)))
>>> -        return -EFAULT;
>>> -
>>> -    if (args.queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
>>> +    if (args->queue_percentage > KFD_MAX_QUEUE_PERCENTAGE) {
>>>           pr_err("kfd: queue percentage must be between 0 to
>>> KFD_MAX_QUEUE_PERCENTAGE\n");
>>>           return -EINVAL;
>>>       }
>>> -    if (args.queue_priority > KFD_MAX_QUEUE_PRIORITY) {
>>> +    if (args->queue_priority > KFD_MAX_QUEUE_PRIORITY) {
>>>           pr_err("kfd: queue priority must be between 0 to
>>> KFD_MAX_QUEUE_PRIORITY\n");
>>>           return -EINVAL;
>>>       }
>>> -    if ((args.ring_base_address) &&
>>> +    if ((args->ring_base_address) &&
>>>           (!access_ok(VERIFY_WRITE,
>>> -            (const void __user *) args.ring_base_address,
>>> +            (const void __user *) args->ring_base_address,
>>>               sizeof(uint64_t)))) {
>>>           pr_err("kfd: can't access ring base address\n");
>>>           return -EFAULT;
>>>       }
>>> -    if (!is_power_of_2(args.ring_size) && (args.ring_size != 0)) {
>>> +    if (!is_power_of_2(args->ring_size) && (args->ring_size != 0)) {
>>>           pr_err("kfd: ring size must be a power of 2 or 0\n");
>>>           return -EINVAL;
>>>       }
>>> -    properties.queue_address = args.ring_base_address;
>>> -    properties.queue_size = args.ring_size;
>>> -    properties.queue_percent = args.queue_percentage;
>>> -    properties.priority = args.queue_priority;
>>> +    properties.queue_address = args->ring_base_address;
>>> +    properties.queue_size = args->ring_size;
>>> +    properties.queue_percent = args->queue_percentage;
>>> +    properties.priority = args->queue_priority;
>>>       pr_debug("kfd: updating queue id %d for PASID %d\n",
>>> -            args.queue_id, p->pasid);
>>> +            args->queue_id, p->pasid);
>>>       mutex_lock(&p->mutex);
>>> -    retval = pqm_update_queue(&p->pqm, args.queue_id, &properties);
>>> +    retval = pqm_update_queue(&p->pqm, args->queue_id, &properties);
>>>       mutex_unlock(&p->mutex);
>>>       return retval;
>>>   }
>>> -static long kfd_ioctl_set_memory_policy(struct file *filep,
>>> -                struct kfd_process *p, void __user *arg)
>>> +static int kfd_ioctl_set_memory_policy(struct file *filep,
>>> +                    struct kfd_process *p, void *data)
>>>   {
>>> -    struct kfd_ioctl_set_memory_policy_args args;
>>> +    struct kfd_ioctl_set_memory_policy_args *args = data;
>>>       struct kfd_dev *dev;
>>>       int err = 0;
>>>       struct kfd_process_device *pdd;
>>>       enum cache_policy default_policy, alternate_policy;
>>> -    if (copy_from_user(&args, arg, sizeof(args)))
>>> -        return -EFAULT;
>>> -
>>> -    if (args.default_policy != KFD_IOC_CACHE_POLICY_COHERENT
>>> -        && args.default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>>> +    if (args->default_policy != KFD_IOC_CACHE_POLICY_COHERENT
>>> +        && args->default_policy != KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>>>           return -EINVAL;
>>>       }
>>> -    if (args.alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
>>> -        && args.alternate_policy != 
>>> KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>>> +    if (args->alternate_policy != KFD_IOC_CACHE_POLICY_COHERENT
>>> +        && args->alternate_policy != 
>>> KFD_IOC_CACHE_POLICY_NONCOHERENT) {
>>>           return -EINVAL;
>>>       }
>>> -    dev = kfd_device_by_id(args.gpu_id);
>>> +    dev = kfd_device_by_id(args->gpu_id);
>>>       if (dev == NULL)
>>>           return -EINVAL;
>>> @@ -397,23 +375,23 @@ static long kfd_ioctl_set_memory_policy(struct 
>>> file *filep,
>>>       pdd = kfd_bind_process_to_device(dev, p);
>>>       if (IS_ERR(pdd)) {
>>> -        err = PTR_ERR(pdd);
>>> +        err = -ESRCH;
>>>           goto out;
>>>       }
>>> -    default_policy = (args.default_policy == 
>>> KFD_IOC_CACHE_POLICY_COHERENT)
>>> +    default_policy = (args->default_policy == 
>>> KFD_IOC_CACHE_POLICY_COHERENT)
>>>                ? cache_policy_coherent : cache_policy_noncoherent;
>>>       alternate_policy =
>>> -        (args.alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>>> +        (args->alternate_policy == KFD_IOC_CACHE_POLICY_COHERENT)
>>>              ? cache_policy_coherent : cache_policy_noncoherent;
>>>       if (!dev->dqm->set_cache_memory_policy(dev->dqm,
>>>                   &pdd->qpd,
>>>                   default_policy,
>>>                   alternate_policy,
>>> -                (void __user *)args.alternate_aperture_base,
>>> -                args.alternate_aperture_size))
>>> +                (void __user *)args->alternate_aperture_base,
>>> +                args->alternate_aperture_size))
>>>           err = -EINVAL;
>>>   out:
>>> @@ -422,53 +400,44 @@ out:
>>>       return err;
>>>   }
>>> -static long kfd_ioctl_get_clock_counters(struct file *filep,
>>> -                struct kfd_process *p, void __user *arg)
>>> +static int kfd_ioctl_get_clock_counters(struct file *filep,
>>> +                struct kfd_process *p, void *data)
>>>   {
>>> -    struct kfd_ioctl_get_clock_counters_args args;
>>> +    struct kfd_ioctl_get_clock_counters_args *args = data;
>>>       struct kfd_dev *dev;
>>>       struct timespec time;
>>> -    if (copy_from_user(&args, arg, sizeof(args)))
>>> -        return -EFAULT;
>>> -
>>> -    dev = kfd_device_by_id(args.gpu_id);
>>> +    dev = kfd_device_by_id(args->gpu_id);
>>>       if (dev == NULL)
>>>           return -EINVAL;
>>>       /* Reading GPU clock counter from KGD */
>>> -    args.gpu_clock_counter = kfd2kgd->get_gpu_clock_counter(dev->kgd);
>>> +    args->gpu_clock_counter = 
>>> kfd2kgd->get_gpu_clock_counter(dev->kgd);
>>>       /* No access to rdtsc. Using raw monotonic time */
>>>       getrawmonotonic(&time);
>>> -    args.cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
>>> +    args->cpu_clock_counter = (uint64_t)timespec_to_ns(&time);
>>>       get_monotonic_boottime(&time);
>>> -    args.system_clock_counter = (uint64_t)timespec_to_ns(&time);
>>> +    args->system_clock_counter = (uint64_t)timespec_to_ns(&time);
>>>       /* Since the counter is in nano-seconds we use 1GHz frequency */
>>> -    args.system_clock_freq = 1000000000;
>>> -
>>> -    if (copy_to_user(arg, &args, sizeof(args)))
>>> -        return -EFAULT;
>>> +    args->system_clock_freq = 1000000000;
>>>       return 0;
>>>   }
>>>   static int kfd_ioctl_get_process_apertures(struct file *filp,
>>> -                struct kfd_process *p, void __user *arg)
>>> +                struct kfd_process *p, void *data)
>>>   {
>>> -    struct kfd_ioctl_get_process_apertures_args args;
>>> +    struct kfd_ioctl_get_process_apertures_args *args = data;
>>>       struct kfd_process_device_apertures *pAperture;
>>>       struct kfd_process_device *pdd;
>>>       dev_dbg(kfd_device, "get apertures for PASID %d", p->pasid);
>>> -    if (copy_from_user(&args, arg, sizeof(args)))
>>> -        return -EFAULT;
>>> -
>>> -    args.num_of_nodes = 0;
>>> +    args->num_of_nodes = 0;
>>>       mutex_lock(&p->mutex);
>>> @@ -477,7 +446,8 @@ static int 
>>> kfd_ioctl_get_process_apertures(struct file *filp,
>>>           /* Run over all pdd of the process */
>>>           pdd = kfd_get_first_process_device_data(p);
>>>           do {
>>> -            pAperture = &args.process_apertures[args.num_of_nodes];
>>> +            pAperture =
>>> + &args->process_apertures[args->num_of_nodes];
>>>               pAperture->gpu_id = pdd->dev->id;
>>>               pAperture->lds_base = pdd->lds_base;
>>>               pAperture->lds_limit = pdd->lds_limit;
>>> @@ -487,7 +457,7 @@ static int 
>>> kfd_ioctl_get_process_apertures(struct file *filp,
>>>               pAperture->scratch_limit = pdd->scratch_limit;
>>>               dev_dbg(kfd_device,
>>> -                "node id %u\n", args.num_of_nodes);
>>> +                "node id %u\n", args->num_of_nodes);
>>>               dev_dbg(kfd_device,
>>>                   "gpu id %u\n", pdd->dev->id);
>>>               dev_dbg(kfd_device,
>>> @@ -503,23 +473,23 @@ static int 
>>> kfd_ioctl_get_process_apertures(struct file
>>> *filp,
>>>               dev_dbg(kfd_device,
>>>                   "scratch_limit %llX\n", pdd->scratch_limit);
>>> -            args.num_of_nodes++;
>>> +            args->num_of_nodes++;
>>>           } while ((pdd = kfd_get_next_process_device_data(p, pdd)) 
>>> != NULL &&
>>> -                (args.num_of_nodes < NUM_OF_SUPPORTED_GPUS));
>>> +                (args->num_of_nodes < NUM_OF_SUPPORTED_GPUS));
>>>       }
>>>       mutex_unlock(&p->mutex);
>>> -    if (copy_to_user(arg, &args, sizeof(args)))
>>> -        return -EFAULT;
>>> -
>>>       return 0;
>>>   }
>>>   static long kfd_ioctl(struct file *filep, unsigned int cmd, 
>>> unsigned long arg)
>>>   {
>>>       struct kfd_process *process;
>>> -    long err = -EINVAL;
>>> +    char stack_kdata[128];
>>> +    char *kdata = NULL;
>>> +    unsigned int usize, asize;
>>> +    int retcode = -EINVAL;
>>>       dev_dbg(kfd_device,
>>>           "ioctl cmd 0x%x (#%d), arg 0x%lx\n",
>>> @@ -529,54 +499,84 @@ static long kfd_ioctl(struct file *filep, 
>>> unsigned int
>>> cmd, unsigned long arg)
>>>       if (IS_ERR(process))
>>>           return PTR_ERR(process);
>>> +    if (cmd & (IOC_IN | IOC_OUT)) {
>>> +        if (asize <= sizeof(stack_kdata)) {
>>> +            kdata = stack_kdata;
>>> +        } else {
>>> +            kdata = kmalloc(asize, GFP_KERNEL);
>>> +            if (!kdata) {
>>> +                retcode = -ENOMEM;
>>> +                goto err_i1;
>>> +            }
>>> +        }
>>> +        if (asize > usize)
>>> +            memset(kdata + usize, 0, asize - usize);
>>> +    }
>>> +
>>> +    if (cmd & IOC_IN) {
>>> +        if (copy_from_user(kdata, (void __user *)arg, usize) != 0) {
>>> +            retcode = -EFAULT;
>>> +            goto err_i1;
>>> +        }
>>> +    } else if (cmd & IOC_OUT) {
>>> +        memset(kdata, 0, usize);
>>> +    }
>>> +
>>> +
>>>       switch (cmd) {
>>>       case KFD_IOC_GET_VERSION:
>>> -        err = kfd_ioctl_get_version(filep, process, (void __user 
>>> *)arg);
>>> +        retcode = kfd_ioctl_get_version(filep, process, kdata);
>>>           break;
>>>       case KFD_IOC_CREATE_QUEUE:
>>> -        err = kfd_ioctl_create_queue(filep, process,
>>> -                        (void __user *)arg);
>>> +        retcode = kfd_ioctl_create_queue(filep, process,
>>> +                        kdata);
>>>           break;
>>>       case KFD_IOC_DESTROY_QUEUE:
>>> -        err = kfd_ioctl_destroy_queue(filep, process,
>>> -                        (void __user *)arg);
>>> +        retcode = kfd_ioctl_destroy_queue(filep, process,
>>> +                        kdata);
>>>           break;
>>>       case KFD_IOC_SET_MEMORY_POLICY:
>>> -        err = kfd_ioctl_set_memory_policy(filep, process,
>>> -                        (void __user *)arg);
>>> +        retcode = kfd_ioctl_set_memory_policy(filep, process,
>>> +                        kdata);
>>>           break;
>>>       case KFD_IOC_GET_CLOCK_COUNTERS:
>>> -        err = kfd_ioctl_get_clock_counters(filep, process,
>>> -                        (void __user *)arg);
>>> +        retcode = kfd_ioctl_get_clock_counters(filep, process,
>>> +                        kdata);
>>>           break;
>>>       case KFD_IOC_GET_PROCESS_APERTURES:
>>> -        err = kfd_ioctl_get_process_apertures(filep, process,
>>> -                        (void __user *)arg);
>>> +        retcode = kfd_ioctl_get_process_apertures(filep, process,
>>> +                        kdata);
>>>           break;
>>>       case KFD_IOC_UPDATE_QUEUE:
>>> -        err = kfd_ioctl_update_queue(filep, process,
>>> -                        (void __user *)arg);
>>> +        retcode = kfd_ioctl_update_queue(filep, process,
>>> +                        kdata);
>>>           break;
>>>       default:
>>> -        dev_err(kfd_device,
>>> +        dev_dbg(kfd_device,
>>>               "unknown ioctl cmd 0x%x, arg 0x%lx)\n",
>>>               cmd, arg);
>>> -        err = -EINVAL;
>>> +        retcode = -EINVAL;
>>>           break;
>>>       }
>>> -    if (err < 0)
>>> -        dev_err(kfd_device,
>>> -            "ioctl error %ld for ioctl cmd 0x%x (#%d)\n",
>>> -            err, cmd, _IOC_NR(cmd));
>>> +    if (cmd & IOC_OUT)
>>> +        if (copy_to_user((void __user *)arg, kdata, usize) != 0)
>>> +            retcode = -EFAULT;
>>> -    return err;
>>> +err_i1:
>>> +    if (kdata != stack_kdata)
>>> +        kfree(kdata);
>>> +
>>> +    if (retcode)
>>> +        dev_dbg(kfd_device, "ret = %d\n", retcode);
>>> +
>>> +    return retcode;
>>>   }
>>>   static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel