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