[RFC][PATCH 1/2] uio: allow drivers to override the pgprot for mmap

Subsystems: the rest, userspace i/o (uio)

8 messages, 3 authors, 2011-10-31 · open the first message on its own page

[RFC][PATCH 1/2] uio: allow drivers to override the pgprot for mmap

From: Kumar Gala <hidden>
Date: 2011-10-28 15:50:45

For some devices, the default behavior of pgprot_noncached() is not
appropriate for all of its mappable regions. This provides a means for
the kernel side of the UIO driver to override the flags without having
to implement its own full mmap callback.

Signed-off-by: Kumar Gala <redacted>
Signed-off-by: Geoff Thorpe <redacted>
---
 drivers/uio/uio.c          |    6 +++++-
 include/linux/uio_driver.h |    3 +++
 2 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index dc27d89..0aebe27 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -655,7 +655,11 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 
 	vma->vm_flags |= VM_IO | VM_RESERVED;
 
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	if (idev->info->set_pgprot)
+		vma->vm_page_prot = idev->info->set_pgprot(idev->info, mi,
+							   vma->vm_page_prot);
+	else
+		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
 	return remap_pfn_range(vma,
 			       vma->vm_start,
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index fd99ff9..edfe7c8 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -80,6 +80,7 @@ struct uio_device;
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
  * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
+ * @set_pgprot:		allow driver to override default(noncached) pgprot
  */
 struct uio_info {
 	struct uio_device	*uio_dev;
@@ -95,6 +96,8 @@ struct uio_info {
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
 	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
+	pgprot_t (*set_pgprot)(struct uio_info *uio, unsigned int mem_idx,
+			       pgprot_t prot);
 };
 
 extern int __must_check
-- 
1.7.3.4

[RFC][PATCH 2/2] Example to show use of uio pgprot

From: Kumar Gala <hidden>
Date: 2011-10-28 15:50:55

---
 drivers/uio/uio_dpa.c |  200 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 200 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_dpa.c
diff --git a/drivers/uio/uio_dpa.c b/drivers/uio/uio_dpa.c
new file mode 100644
index 0000000..19360f2
--- /dev/null
+++ b/drivers/uio/uio_dpa.c
@@ -0,0 +1,200 @@
+/* Copyright 2011 Freescale Semiconductor, Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in the
+ *       documentation and/or other materials provided with the distribution.
+ *     * Neither the name of Freescale Semiconductor nor the
+ *       names of its contributors may be used to endorse or promote products
+ *       derived from this software without specific prior written permission.
+ *
+ *
+ * ALTERNATIVELY, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") as published by the Free Software
+ * Foundation, either version 2 of that License or (at your option) any
+ * later version.
+ *
+ * THIS SOFTWARE IS PROVIDED BY Freescale Semiconductor ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL Freescale Semiconductor BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "bman_private.h"
+#include "qman_private.h"
+
+static const char dpa_uio_version[] = "USDPAA UIO portal driver v0.2";
+
+static LIST_HEAD(uio_portal_list);
+
+struct dpa_uio_info {
+	atomic_t ref; /* exclusive, only one open() at a time */
+	struct uio_info uio;
+	void *addr_ci;
+	char name[16]; /* big enough for "qman-uio-xx" */
+	struct platform_device *pdev;
+	struct list_head node;
+};
+
+static int dpa_uio_open(struct uio_info *info, struct inode *inode)
+{
+	struct dpa_uio_info *i = container_of(info, struct dpa_uio_info, uio);
+	if (!atomic_dec_and_test(&i->ref)) {
+		atomic_inc(&i->ref);
+		return -EBUSY;
+	}
+	return 0;
+}
+
+static int dpa_uio_release(struct uio_info *info, struct inode *inode)
+{
+	struct dpa_uio_info *i = container_of(info, struct dpa_uio_info, uio);
+	atomic_inc(&i->ref);
+	return 0;
+}
+
+static pgprot_t dpa_uio_pgprot(struct uio_info *info, unsigned int mem_idx,
+				   pgprot_t prot)
+{
+	if (mem_idx == DPA_PORTAL_CE)
+		/* It's the cache-enabled portal region. NB, we shouldn't use
+		 * pgprot_cached() here because it includes _PAGE_COHERENT. The
+		 * region is cachable but *not* coherent - stashing (if enabled)
+		 * leads to "coherent-like" behaviour, otherwise the driver
+		 * explicitly invalidates/prefetches. */
+		return pgprot_cached_noncoherent(prot);
+	/* Otherwise it's the cache-inhibited portal region */
+	return pgprot_noncached(prot);
+}
+
+static irqreturn_t dpa_uio_irq_handler(int irq, struct uio_info *info)
+{
+	struct dpa_uio_info *i = container_of(info, struct dpa_uio_info, uio);
+	/* This is the only code outside the regular portal driver that
+	 * manipulates any portal register, so rather than breaking that
+	 * encapsulation I am simply hard-coding the offset to the inhibit
+	 * register here. */
+	out_be32(i->addr_ci + 0xe0c, ~(u32)0);
+	return IRQ_HANDLED;
+}
+
+static void __init dpa_uio_portal_init(struct dpa_uio_portal *p,
+				const struct dpa_uio_class *c)
+{
+	struct dpa_uio_info *info;
+	const struct resource *res;
+	u32 index;
+	int irq, ret;
+
+	/* allocate 'info' */
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return;
+	atomic_set(&info->ref, 1);
+	if (p->type == dpa_uio_portal_bman) {
+		res = &p->bm_cfg->addr_phys[0];
+		index = p->bm_cfg->public_cfg.index;
+		irq = p->bm_cfg->public_cfg.irq;
+	} else {
+		res = &p->qm_cfg->addr_phys[0];
+		index = p->qm_cfg->public_cfg.index;
+		irq = p->qm_cfg->public_cfg.irq;
+	}
+	/* We need to map the cache-inhibited region in the kernel for
+	 * interrupt-handling purposes. */
+	info->addr_ci = ioremap_prot(res[DPA_PORTAL_CI].start,
+				resource_size(&res[DPA_PORTAL_CI]),
+				_PAGE_GUARDED | _PAGE_NO_CACHE);
+	/* Name the UIO device according to the cell-index. It's supposed to be
+	 * unique for each device class (Qman/Bman), and is also a convenient
+	 * way for user-space to find the UIO device that corresponds to a given
+	 * portal device-tree node. */
+	sprintf(info->name, "%s%x", c->dev_prefix, index);
+	info->pdev = platform_device_alloc(info->name, -1);
+	if (!info->pdev) {
+		iounmap(info->addr_ci);
+		kfree(info);
+		pr_err("dpa_uio_portal: platform_device_alloc() failed\n");
+		return;
+	}
+	ret = platform_device_add(info->pdev);
+	if (ret) {
+		platform_device_put(info->pdev);
+		iounmap(info->addr_ci);
+		kfree(info);
+		pr_err("dpa_uio_portal: platform_device_add() failed\n");
+		return;
+	}
+	info->uio.name = info->name;
+	info->uio.version = dpa_uio_version;
+	info->uio.mem[DPA_PORTAL_CE].name = "cena";
+	info->uio.mem[DPA_PORTAL_CE].addr = res[DPA_PORTAL_CE].start;
+	info->uio.mem[DPA_PORTAL_CE].size = resource_size(&res[DPA_PORTAL_CE]);
+	info->uio.mem[DPA_PORTAL_CE].memtype = UIO_MEM_PHYS;
+	info->uio.mem[DPA_PORTAL_CI].name = "cinh";
+	info->uio.mem[DPA_PORTAL_CI].addr = res[DPA_PORTAL_CI].start;
+	info->uio.mem[DPA_PORTAL_CI].size = resource_size(&res[DPA_PORTAL_CI]);
+	info->uio.mem[DPA_PORTAL_CI].memtype = UIO_MEM_PHYS;
+	info->uio.irq = irq;
+	info->uio.handler = dpa_uio_irq_handler;
+	info->uio.set_pgprot = dpa_uio_pgprot;
+	info->uio.open = dpa_uio_open;
+	info->uio.release = dpa_uio_release;
+	ret = uio_register_device(&info->pdev->dev, &info->uio);
+	if (ret) {
+		platform_device_del(info->pdev);
+		platform_device_put(info->pdev);
+		iounmap(info->addr_ci);
+		kfree(info);
+		pr_err("dpa_uio_portal: UIO registration failed\n");
+		return;
+	}
+	list_add_tail(&info->node, &uio_portal_list);
+	pr_info("USDPAA portal initialised, %s\n", info->name);
+}
+
+static int __init dpa_uio_init(void)
+{
+	const struct dpa_uio_class *classes[3], **c = classes;
+	classes[0] = dpa_uio_bman();
+	classes[1] = dpa_uio_qman();
+	classes[2] = NULL;
+	while (*c) {
+		struct dpa_uio_portal *p;
+		list_for_each_entry(p, &(*c)->list, node)
+			dpa_uio_portal_init(p, *c);
+		c++;
+	}
+	pr_info("USDPAA portal layer loaded\n");
+	return 0;
+}
+
+static void __exit dpa_uio_exit(void)
+{
+	struct dpa_uio_info *info, *tmp;
+	list_for_each_entry_safe(info, tmp, &uio_portal_list, node) {
+		list_del(&info->node);
+		uio_unregister_device(&info->uio);
+		platform_device_del(info->pdev);
+		platform_device_put(info->pdev);
+		iounmap(info->addr_ci);
+		pr_info("USDPAA portal removed, %s\n", info->name);
+		kfree(info);
+	}
+	pr_info("USDPAA portal layer unloaded\n");
+}
+
+
+module_init(dpa_uio_init)
+module_exit(dpa_uio_exit)
+MODULE_LICENSE("GPL");
+
-- 
1.7.3.4

Re: [RFC][PATCH 2/2] Example to show use of uio pgprot

From: Greg KH <hidden>
Date: 2011-10-28 16:40:49

On Fri, Oct 28, 2011 at 10:50:30AM -0500, Kumar Gala wrote:
---
 drivers/uio/uio_dpa.c |  200 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 200 insertions(+), 0 deletions(-)
 create mode 100644 drivers/uio/uio_dpa.c
You do realize this does not build, right?

thanks,

greg k-h

Re: [RFC][PATCH 2/2] Example to show use of uio pgprot

From: Kumar Gala <hidden>
Date: 2011-10-28 17:52:29

On Oct 28, 2011, at 11:37 AM, Greg KH wrote:
On Fri, Oct 28, 2011 at 10:50:30AM -0500, Kumar Gala wrote:
quoted
---
drivers/uio/uio_dpa.c |  200 =
+++++++++++++++++++++++++++++++++++++++++++++++++
quoted
1 files changed, 200 insertions(+), 0 deletions(-)
create mode 100644 drivers/uio/uio_dpa.c
=20
You do realize this does not build, right?
=20
thanks,
=20
greg k-h
Yes, it was meant to show Hans and example of why we need to allow a UIO =
driver to set its own pgprot.  We plan on upstream all that code, but =
wanted at least to get feedback on the general UIO change.

- k=

Re: [RFC][PATCH 2/2] Example to show use of uio pgprot

From: Hans J. Koch <hidden>
Date: 2011-10-28 21:41:00

On Fri, Oct 28, 2011 at 10:50:30AM -0500, Kumar Gala wrote:

A few remarks below.
+static void __init dpa_uio_portal_init(struct dpa_uio_portal *p,
+				const struct dpa_uio_class *c)
This can't be "void". You have to return apropiate errors.
+{
+	struct dpa_uio_info *info;
+	const struct resource *res;
+	u32 index;
+	int irq, ret;
+
+	/* allocate 'info' */
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return;
return -ENOMEM (more similar cases below)
+	atomic_set(&info->ref, 1);
+	if (p->type == dpa_uio_portal_bman) {
+		res = &p->bm_cfg->addr_phys[0];
+		index = p->bm_cfg->public_cfg.index;
+		irq = p->bm_cfg->public_cfg.irq;
+	} else {
+		res = &p->qm_cfg->addr_phys[0];
+		index = p->qm_cfg->public_cfg.index;
+		irq = p->qm_cfg->public_cfg.irq;
+	}
+	/* We need to map the cache-inhibited region in the kernel for
+	 * interrupt-handling purposes. */
+	info->addr_ci = ioremap_prot(res[DPA_PORTAL_CI].start,
+				resource_size(&res[DPA_PORTAL_CI]),
+				_PAGE_GUARDED | _PAGE_NO_CACHE);
+	/* Name the UIO device according to the cell-index. It's supposed to be
+	 * unique for each device class (Qman/Bman), and is also a convenient
+	 * way for user-space to find the UIO device that corresponds to a given
+	 * portal device-tree node. */
+	sprintf(info->name, "%s%x", c->dev_prefix, index);
+	info->pdev = platform_device_alloc(info->name, -1);
+	if (!info->pdev) {
+		iounmap(info->addr_ci);
+		kfree(info);
+		pr_err("dpa_uio_portal: platform_device_alloc() failed\n");
+		return;
+	}
+	ret = platform_device_add(info->pdev);
+	if (ret) {
+		platform_device_put(info->pdev);
+		iounmap(info->addr_ci);
+		kfree(info);
+		pr_err("dpa_uio_portal: platform_device_add() failed\n");
+		return;
+	}
+	info->uio.name = info->name;
+	info->uio.version = dpa_uio_version;
+	info->uio.mem[DPA_PORTAL_CE].name = "cena";
+	info->uio.mem[DPA_PORTAL_CE].addr = res[DPA_PORTAL_CE].start;
+	info->uio.mem[DPA_PORTAL_CE].size = resource_size(&res[DPA_PORTAL_CE]);
+	info->uio.mem[DPA_PORTAL_CE].memtype = UIO_MEM_PHYS;
+	info->uio.mem[DPA_PORTAL_CI].name = "cinh";
+	info->uio.mem[DPA_PORTAL_CI].addr = res[DPA_PORTAL_CI].start;
+	info->uio.mem[DPA_PORTAL_CI].size = resource_size(&res[DPA_PORTAL_CI]);
+	info->uio.mem[DPA_PORTAL_CI].memtype = UIO_MEM_PHYS;
+	info->uio.irq = irq;
+	info->uio.handler = dpa_uio_irq_handler;
+	info->uio.set_pgprot = dpa_uio_pgprot;
+	info->uio.open = dpa_uio_open;
+	info->uio.release = dpa_uio_release;
+	ret = uio_register_device(&info->pdev->dev, &info->uio);
This should be the last thing you do before return. You can have
interrupts even before uio_register_device returns.
+	if (ret) {
+		platform_device_del(info->pdev);
+		platform_device_put(info->pdev);
+		iounmap(info->addr_ci);
+		kfree(info);
+		pr_err("dpa_uio_portal: UIO registration failed\n");
+		return;
+	}
+	list_add_tail(&info->node, &uio_portal_list);
That should probably be done prior to uio_register_device.
+	pr_info("USDPAA portal initialised, %s\n", info->name);
+}
+
+static int __init dpa_uio_init(void)
+{
+	const struct dpa_uio_class *classes[3], **c = classes;
+	classes[0] = dpa_uio_bman();
+	classes[1] = dpa_uio_qman();
+	classes[2] = NULL;
+	while (*c) {
+		struct dpa_uio_portal *p;
+		list_for_each_entry(p, &(*c)->list, node)
+			dpa_uio_portal_init(p, *c);
+		c++;
+	}
+	pr_info("USDPAA portal layer loaded\n");
+	return 0;
You can't just return OK here if dpa_uio_portal_init() fails.
+}
+
+static void __exit dpa_uio_exit(void)
+{
+	struct dpa_uio_info *info, *tmp;
+	list_for_each_entry_safe(info, tmp, &uio_portal_list, node) {
+		list_del(&info->node);
+		uio_unregister_device(&info->uio);
+		platform_device_del(info->pdev);
+		platform_device_put(info->pdev);
+		iounmap(info->addr_ci);
+		pr_info("USDPAA portal removed, %s\n", info->name);
+		kfree(info);
+	}
+	pr_info("USDPAA portal layer unloaded\n");
+}
+
+
+module_init(dpa_uio_init)
+module_exit(dpa_uio_exit)
+MODULE_LICENSE("GPL");
+
-- 
1.7.3.4

Re: [RFC][PATCH 1/2] uio: allow drivers to override the pgprot for mmap

From: Hans J. Koch <hidden>
Date: 2011-10-28 21:51:23

On Fri, Oct 28, 2011 at 10:50:29AM -0500, Kumar Gala wrote:
For some devices, the default behavior of pgprot_noncached() is not
appropriate for all of its mappable regions. This provides a means for
the kernel side of the UIO driver to override the flags without having
to implement its own full mmap callback.
Thanks for also providing an example driver showing the use of this.
You should also post this driver in a mainline-ready version, I'm a bit
uncomfortable with adding a new function pointer without having any users.

And since you change uio_driver.h you should also update documentation
accordingly (Documentation/DocBook/uio-howto.tmpl).

Otherwise, I have no general objections.

Thanks,
Hans
quoted hunk
Signed-off-by: Kumar Gala <redacted>
Signed-off-by: Geoff Thorpe <redacted>
---
 drivers/uio/uio.c          |    6 +++++-
 include/linux/uio_driver.h |    3 +++
 2 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index dc27d89..0aebe27 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -655,7 +655,11 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 
 	vma->vm_flags |= VM_IO | VM_RESERVED;
 
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	if (idev->info->set_pgprot)
+		vma->vm_page_prot = idev->info->set_pgprot(idev->info, mi,
+							   vma->vm_page_prot);
+	else
+		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
 	return remap_pfn_range(vma,
 			       vma->vm_start,
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index fd99ff9..edfe7c8 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -80,6 +80,7 @@ struct uio_device;
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
  * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
+ * @set_pgprot:		allow driver to override default(noncached) pgprot
  */
 struct uio_info {
 	struct uio_device	*uio_dev;
@@ -95,6 +96,8 @@ struct uio_info {
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
 	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
+	pgprot_t (*set_pgprot)(struct uio_info *uio, unsigned int mem_idx,
+			       pgprot_t prot);
 };
 
 extern int __must_check
-- 
1.7.3.4

Re: [RFC][PATCH 1/2] uio: allow drivers to override the pgprot for mmap

From: Greg KH <hidden>
Date: 2011-10-29 06:38:30

On Fri, Oct 28, 2011 at 11:48:12PM +0200, Hans J. Koch wrote:
On Fri, Oct 28, 2011 at 10:50:29AM -0500, Kumar Gala wrote:
quoted
For some devices, the default behavior of pgprot_noncached() is not
appropriate for all of its mappable regions. This provides a means for
the kernel side of the UIO driver to override the flags without having
to implement its own full mmap callback.
Thanks for also providing an example driver showing the use of this.
You should also post this driver in a mainline-ready version, I'm a bit
uncomfortable with adding a new function pointer without having any users.
I'm more than "uncomfortable", I'll refuse to take any such patch unless
there is a in-kernel user, otherwise it makes no sense to add the
pointer at all.

thanks,

greg k-h

Re: [RFC][PATCH 1/2] uio: allow drivers to override the pgprot for mmap

From: Kumar Gala <hidden>
Date: 2011-10-31 13:44:55

On Oct 29, 2011, at 1:38 AM, Greg KH wrote:
On Fri, Oct 28, 2011 at 11:48:12PM +0200, Hans J. Koch wrote:
quoted
On Fri, Oct 28, 2011 at 10:50:29AM -0500, Kumar Gala wrote:
quoted
For some devices, the default behavior of pgprot_noncached() is not
appropriate for all of its mappable regions. This provides a means =
for
quoted
quoted
the kernel side of the UIO driver to override the flags without =
having
quoted
quoted
to implement its own full mmap callback.
=20
Thanks for also providing an example driver showing the use of this.
You should also post this driver in a mainline-ready version, I'm a =
bit
quoted
uncomfortable with adding a new function pointer without having any =
users.
=20
I'm more than "uncomfortable", I'll refuse to take any such patch =
unless
there is a in-kernel user, otherwise it makes no sense to add the
pointer at all.
=20
thanks,
=20
greg k-h
I'm in agreement with this view.  I wanted to post this to make sure the =
direction we took was ok so when the upstream driver is posted this =
patch / change isn't a concern.

- k=
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help