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 at amd.com>
In general sounds like a good idea to me and the patch is "Reviewed-by: Christian König <christian.koenig at 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)