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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>

In general sounds like a good idea to me and the patch is "Reviewed-by:
Christian König <christian.koenig@xxxxxxx>" 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux