Re: [PATCH v6 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.

On Thu, Oct 29, 2015 at 04:23:45PM -0700, Rafael Antognolli wrote:

This module is heavily based on i2c-dev. Once loaded, it provides one dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.

It's possible to know which connector owns this aux channel by looking at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if the connector device pointer was correctly set in the aux helper struct.

Two main operations are provided on the registers read and write. The address of the register to be read or written is given using lseek. The seek position is updated upon read or write.

Note that we (i915 folks at least) generally like to have the changelog in the commit message itself. So maybe move it here for the final version.

Anyways, this is looking rather nice now. I spotted a few remaining style issues, and some error handling problems. Once those are dealt with I think we should be done.

Signed-off-by: Rafael Antognolli rafael.antognolli@intel.com

drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 381 +++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_dp_helper.c | 5 + include/drm/drm_dp_aux_dev.h | 50 +++++ 5 files changed, 445 insertions(+) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index c4bf9a1..daefcce 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -25,6 +25,14 @@ config DRM_MIPI_DSI bool depends on DRM

+config DRM_DP_AUX_CHARDEV

• bool "DRM DP AUX Interface"
• depends on DRM
• help

•  Choose this option to enable a /dev/drm_dp_auxN node that allows to
  

•  read and write values to arbitrary DPCD registers on the DP aux
  

•  channel.
  

config DRM_KMS_HELPER tristate depends on DRM diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 1e9ff4c..e48ec8f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \ drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o

obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o

diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c new file mode 100644 index 0000000..16dbc2e --- /dev/null +++ b/drivers/gpu/drm/drm_dp_aux_dev.c @@ -0,0 +1,381 @@ +/*

• • Copyright © 2015 Intel Corporation
• • Permission is hereby granted, free of charge, to any person obtaining a
• • copy of this software and associated documentation files (the "Software"),
• • to deal in the Software without restriction, including without limitation
• • the rights to use, copy, modify, merge, publish, distribute, sublicense,
• • and/or sell copies of the Software, and to permit persons to whom the
• • Software is furnished to do so, subject to the following conditions:
• • The above copyright notice and this permission notice (including the next
• • paragraph) shall be included in all copies or substantial portions of the
• • Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
• • THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
• • LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
• • FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
• • IN THE SOFTWARE.
• • Authors:
• • Rafael Antognolli rafael.antognolli@intel.com
• */

+#include <linux/device.h> +#include <linux/fs.h> +#include <linux/slab.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/uaccess.h> +#include <drm/drm_dp_helper.h> +#include <drm/drm_crtc.h> +#include <drm/drmP.h>

+struct drm_dp_aux_dev {

• unsigned index;
• struct drm_dp_aux *aux;
• struct device *dev;
• struct kref refcount;
• atomic_t usecount;

+};

+#define DRM_AUX_MINORS 256 +#define AUX_MAX_OFFSET (1 << 20) +static DEFINE_IDR(aux_idr); +static DEFINE_MUTEX(aux_idr_mutex); +static struct class *drm_dp_aux_dev_class; +static int drm_dev_major = -1;

+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index) +{

• struct drm_dp_aux_dev *aux_dev = NULL;
• mutex_lock(&aux_idr_mutex);
• aux_dev = idr_find(&aux_idr, index);
• if (!kref_get_unless_zero(&aux_dev->refcount))

• aux_dev = NULL;
  

• mutex_unlock(&aux_idr_mutex);
• return aux_dev;

+}

+static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux) +{

• struct drm_dp_aux_dev *aux_dev;
• int index;

spurious newline

• aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
• if (!aux_dev)

• return ERR_PTR(-ENOMEM);
  

• aux_dev->aux = aux;
• atomic_set(&aux_dev->usecount, 1);
• kref_init(&aux_dev->refcount);
• mutex_lock(&aux_idr_mutex);
• index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS,

• 		 GFP_KERNEL);
  

• mutex_unlock(&aux_idr_mutex);
• if (index < 0) {

• kfree(aux_dev);
  

• return ERR_PTR(index);
  

• }
• aux_dev->index = index;
• return aux_dev;

+}

+static void release_drm_dp_aux_dev(struct kref *ref) +{

• struct drm_dp_aux_dev *aux_dev =

• container_of(ref, struct drm_dp_aux_dev, refcount);
  

• kfree(aux_dev);

+}

+static ssize_t name_show(struct device *dev,

• 	 struct device_attribute *attr, char *buf)
  

+{

• ssize_t res;
• struct drm_dp_aux_dev *aux_dev =

• drm_dp_aux_dev_get_by_minor(MINOR(dev->devt));
  

• if (!aux_dev)

• return -ENODEV;
  

• res = sprintf(buf, "%s\n", aux_dev->aux->name);
• kref_put(&aux_dev->refcount, release_drm_dp_aux_dev);
• return res;

+} +static DEVICE_ATTR_RO(name);

+static struct attribute *drm_dp_aux_attrs[] = {

• &dev_attr_name.attr,
• NULL,

+}; +ATTRIBUTE_GROUPS(drm_dp_aux);

+static int auxdev_open(struct inode *inode, struct file *file) +{

• unsigned int minor = iminor(inode);
• struct drm_dp_aux_dev *aux_dev;
• aux_dev = drm_dp_aux_dev_get_by_minor(minor);
• if (!aux_dev)

• return -ENODEV;
  

• file->private_data = aux_dev;
• return 0;

+}

+static loff_t auxdev_llseek(struct file *file, loff_t offset, int whence) +{

• return fixed_size_llseek(file, offset, whence, AUX_MAX_OFFSET);

+}

+static ssize_t auxdev_read(struct file *file, char __user *buf, size_t count,

• 	   loff_t *offset)
  

+{

• size_t bytes_pending, num_bytes_processed = 0;
• struct drm_dp_aux_dev *aux_dev = file->private_data;
• ssize_t res = 0;
• if (count < 0)

• return -EINVAL;
  

size_t is unsigned, so this can't happen.

• if (!atomic_inc_not_zero(&aux_dev->usecount))

• return -ENODEV;
  

• bytes_pending = min((loff_t)count, AUX_MAX_OFFSET - (*offset));
• if (!access_ok(VERIFY_WRITE, buf, bytes_pending)) {

• res = -EFAULT;
  

• goto out;
  

• }
• while (bytes_pending > 0) {

• uint8_t localbuf[DP_AUX_MAX_PAYLOAD_BYTES];
  

• ssize_t todo = min_t(size_t, bytes_pending, sizeof(localbuf));
  

• res = drm_dp_dpcd_read(aux_dev->aux, *offset, localbuf, todo);
  

• if (res <= 0) {
  

• 	res = num_bytes_processed ? num_bytes_processed : res;
  

• 	goto out;
  

• }
  

• if (__copy_to_user(buf + num_bytes_processed, localbuf, res)) {
  

• 	res = num_bytes_processed ?
  

• 		num_bytes_processed : -EFAULT;
  

• 	goto out;
  

• }
  

• bytes_pending -= res;
  

• *offset += res;
  

• num_bytes_processed += res;
  

• res = num_bytes_processed;
  

• }

+out:

• atomic_dec(&aux_dev->usecount);
• wake_up_atomic_t(&aux_dev->usecount);

Interesting. First time I've seen this stuff.

• return res;

+}

+static ssize_t auxdev_write(struct file *file, const char __user *buf,

• 	    size_t count, loff_t *offset)
  

+{

• size_t bytes_pending, num_bytes_processed = 0;
• struct drm_dp_aux_dev *aux_dev = file->private_data;
• ssize_t res = 0;
• if (count < 0)

• return -EINVAL;
  

Another "size_t is unsigned" case.

• if (!atomic_inc_not_zero(&aux_dev->usecount))

• return -ENODEV;
  

• bytes_pending = min((loff_t)count, AUX_MAX_OFFSET - *offset);
• if (!access_ok(VERIFY_READ, buf, bytes_pending)) {

• res = -EFAULT;
  

• goto out;
  

• }
• while (bytes_pending > 0) {

• uint8_t localbuf[DP_AUX_MAX_PAYLOAD_BYTES];
  

• ssize_t todo = min_t(size_t, bytes_pending, sizeof(localbuf));
  

• if (__copy_from_user(localbuf,
  

• 		     buf + num_bytes_processed, todo)) {
  

• 	res = num_bytes_processed ?
  

• 		num_bytes_processed : -EFAULT;
  

• 	goto out;
  

• }
  

• res = drm_dp_dpcd_write(aux_dev->aux, *offset, localbuf, todo);
  

• if (res <= 0) {
  

• 	res = num_bytes_processed ? num_bytes_processed : res;
  

• 	goto out;
  

• }
  

• bytes_pending -= res;
  

• *offset += res;
  

• num_bytes_processed += res;
  

• res = num_bytes_processed;
  

• }

+out:

• atomic_dec(&aux_dev->usecount);
• wake_up_atomic_t(&aux_dev->usecount);
• return res;

+}

+static int auxdev_release(struct inode *inode, struct file *file) +{

• struct drm_dp_aux_dev *aux_dev = file->private_data;
• kref_put(&aux_dev->refcount, release_drm_dp_aux_dev);
• return 0;

+}

+static const struct file_operations auxdev_fops = {

• .owner = THIS_MODULE,
• .llseek = auxdev_llseek,
• .read = auxdev_read,
• .write = auxdev_write,
• .open = auxdev_open,
• .release = auxdev_release,

+};

+#define to_auxdev(d) container_of(d, struct drm_dp_aux_dev, aux)

+/**

• • drm_dp_aux_register_devnode() - register a devnode for this aux channel
• • @aux: DisplayPort AUX channel
• • Returns 0 on success or a negative error code on failure.
• */

+int drm_dp_aux_register_devnode(struct drm_dp_aux *aux) +{

• struct drm_dp_aux_dev *aux_dev;
• int res;
• aux_dev = alloc_drm_dp_aux_dev(aux);
• if (IS_ERR(aux_dev))

• return PTR_ERR(aux_dev);
  

• aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,

• 		     MKDEV(drm_dev_major, aux_dev->index), NULL,
  

• 		     "drm_dp_aux%d", aux_dev->index);
  

• if (IS_ERR(aux_dev->dev)) {

• res = PTR_ERR(aux_dev->dev);
  

• goto error;
  

• }
• pr_debug("drm_dp_aux_dev: aux [%s] registered as minor %d\n",

•  aux->name, aux_dev->index);
  

You should use DRM_DEBUG and DRM_ERROR all around to conform to standard drm subsystem style.

• return 0;

+error:

• device_destroy(drm_dp_aux_dev_class,

•        MKDEV(drm_dev_major, aux_dev->index));
  

• kfree(aux_dev);

It was already added to the idr, so this needs something a bit more. Also in theory someone might have already opened the device by now.

I guess you could just do a normal unregister here? I assume calling device_destroy() even though device_create() failed should be OK since you're already doing it, so having the unregister code do it instead should be equally fine.

Oh and I guess we don't really need to store the struct device pointer under aux_dev since it's not used by anyone, not even by device_destroy().

• return res;

+} +EXPORT_SYMBOL(drm_dp_aux_register_devnode);

+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux) +{

• struct drm_dp_aux_dev *iter, *aux_dev = NULL;
• int id;
• /* don't increase kref count here because this function should only be

• * used by drm_dp_aux_unregister_devnode. Thus, it will always have at
  

• * least one reference - the one that drm_dp_aux_register_devnode
  

• * created
  

• */
  

• mutex_lock(&aux_idr_mutex);
• idr_for_each_entry(&aux_idr, iter, id) {

• if (iter->aux == aux) {
  

• 	aux_dev = iter;
  

• 	break;
  

• }
  

• }
• mutex_unlock(&aux_idr_mutex);
• return aux_dev;

+}

+static int auxdev_wait_atomic_t(atomic_t *p) +{

• schedule();
• return 0;

+} +/**

• • drm_dp_aux_unregister_devnode() - unregister a devnode for this aux channel
• • @aux: DisplayPort AUX channel
• • Returns 0 on success or a negative error code on failure.
• */

+int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux) +{

• struct drm_dp_aux_dev *aux_dev;
• unsigned int minor;
• aux_dev = drm_dp_aux_dev_get_by_aux(aux);
• if (!aux_dev) /* attach must have failed */

• return 0;
  

Should we maybe return an error? Or maybe just make the function void since no one is likely to be interested in the return value?

• mutex_lock(&aux_idr_mutex);
• idr_remove(&aux_idr, aux_dev->index);
• mutex_unlock(&aux_idr_mutex);
• atomic_dec(&aux_dev->usecount);
• wait_on_atomic_t(&aux_dev->usecount, auxdev_wait_atomic_t,

• 	 TASK_UNINTERRUPTIBLE);
  

• minor = aux_dev->index;
• device_destroy(drm_dp_aux_dev_class, MKDEV(drm_dev_major, minor));
• kref_put(&aux_dev->refcount, release_drm_dp_aux_dev);
• pr_debug("drm_dp_aux_dev: aux [%s] unregistered\n", aux->name);

Can't dereference it safely after the kref_put().

• return 0;

+} +EXPORT_SYMBOL(drm_dp_aux_unregister_devnode);

+static int __init drm_dp_aux_dev_init(void) +{

• int res;
• drm_dp_aux_dev_class = class_create(THIS_MODULE, "drm_dp_aux_dev");
• if (IS_ERR(drm_dp_aux_dev_class)) {

• res = PTR_ERR(drm_dp_aux_dev_class);
  

• goto out;
  

• }
• drm_dp_aux_dev_class->dev_groups = drm_dp_aux_groups;
• res = register_chrdev(0, "aux", &auxdev_fops);
• if (res < 0)

• goto out;
  

Should class_destroy() in this error path.

• drm_dev_major = res;
• return 0;

+out:

• drm_err("%s: Driver Initialisation failed\n", __FILE__);
• return res;

+}

+static void __exit drm_dp_aux_dev_exit(void) +{

• class_destroy(drm_dp_aux_dev_class);
• unregister_chrdev(drm_dev_major, "aux");

Swap these around to be the opposite of init?

+}

+MODULE_AUTHOR("Rafael Antognolli rafael.antognolli@intel.com"); +MODULE_DESCRIPTION("DRM DP AUX /dev entries driver"); +MODULE_LICENSE("GPL and additional rights");

+module_init(drm_dp_aux_dev_init); +module_exit(drm_dp_aux_dev_exit); diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 9535c5b..d2565cc 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -28,6 +28,7 @@ #include <linux/sched.h> #include <linux/i2c.h> #include <drm/drm_dp_helper.h> +#include <drm/drm_dp_aux_dev.h> #include <drm/drmP.h>

/** @@ -768,6 +769,9 @@ int drm_dp_aux_register(struct drm_dp_aux *aux) strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev), sizeof(aux->ddc.name));

• if (drm_dp_aux_register_devnode(aux))

• drm_err("%s: Could not register drm_dp_aux_dev.\n", __FILE__);
  

• return i2c_add_adapter(&aux->ddc);

} EXPORT_SYMBOL(drm_dp_aux_register); @@ -778,6 +782,7 @@ EXPORT_SYMBOL(drm_dp_aux_register); */ void drm_dp_aux_unregister(struct drm_dp_aux *aux) {

• drm_dp_aux_unregister_devnode(aux); i2c_del_adapter(&aux->ddc);

} EXPORT_SYMBOL(drm_dp_aux_unregister); diff --git a/include/drm/drm_dp_aux_dev.h b/include/drm/drm_dp_aux_dev.h new file mode 100644 index 0000000..35409a2 --- /dev/null +++ b/include/drm/drm_dp_aux_dev.h @@ -0,0 +1,50 @@ +/*

• • Copyright © 2015 Intel Corporation
• • Permission is hereby granted, free of charge, to any person obtaining a
• • copy of this software and associated documentation files (the "Software"),
• • to deal in the Software without restriction, including without limitation
• • the rights to use, copy, modify, merge, publish, distribute, sublicense,
• • and/or sell copies of the Software, and to permit persons to whom the
• • Software is furnished to do so, subject to the following conditions:
• • The above copyright notice and this permission notice (including the next
• • paragraph) shall be included in all copies or substantial portions of the
• • Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
• • THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
• • LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
• • FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
• • IN THE SOFTWARE.
• • Authors:
• • Rafael Antognolli rafael.antognolli@intel.com
• */

+#ifndef DRM_DP_AUX_DEV +#define DRM_DP_AUX_DEV

+#ifdef CONFIG_DRM_DP_AUX_CHARDEV

+int drm_dp_aux_register_devnode(struct drm_dp_aux *aux); +int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);

+#else

+static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux) +{

• return 0;

+}

+static inline int drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux) +{

• return 0;

+}

+#endif

+#endif

2.4.3

---

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