Hello, in this series you can find a generic platform_device driver for UIO and a few cleanups and fixes to the existing UIO code. The first patch fixes a possible Oops. Below you can find shortlog and diffstat. I appreciate any (constructive) feedback. Best regards Uwe Uwe Kleine-K
Hello,
there are three patches left in my uio queue that didn't made it into
mainline up to now.
You can find the patches in my uio branch at
git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio
and I will resend the patches as a reply to this mail.
Shortlog and diffstat can be found below.
Best regards
Uwe
Uwe Kleine-König (3):
UIO: don't let UIO_CIF and UIO_SMX depend twice on UIO
provide a dummy implementation of the clk API
UIO: generic platform driver
drivers/uio/Kconfig | 10 +++-
drivers/uio/Makefile | 1 +
drivers/uio/uio_pdrv.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++
lib/Kconfig | 6 ++
lib/Makefile | 2 +
lib/dummyclk.c | 54 +++++++++++++++++
6 files changed, 226 insertions(+), 2 deletions(-)
--
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
--Hi Uwe, thanks for your work. Greg and I are at the Hannover trade fair ATM. I'll be back home on Thursday and will have a look at your patches then. Thanks, Hans --
ae210f188614bb3d1ee3f19c64e28e3cdd44877c introduced a big "if UIO"/"endif" where all uio drivers are defined. So know there is no need for them to depend explicitly on UIO. Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com> --- Hello, these hunks were part of a patch in my first series but that patch was dropped because another patch in Greg's queue did a subset of mine. Best regards Uwe drivers/uio/Kconfig | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig index a4aaab9..78e139c 100644 --- a/drivers/uio/Kconfig +++ b/drivers/uio/Kconfig @@ -15,7 +15,7 @@ if UIO config UIO_CIF tristate "generic Hilscher CIF Card driver" - depends on UIO && PCI + depends on PCI default n help Driver for Hilscher CIF DeviceNet and Profibus cards. This @@ -28,7 +28,6 @@ config UIO_CIF config UIO_SMX tristate "SMX cryptengine UIO interface" - depends on UIO default n help Userspace IO interface to the Cryptography engine found on the -- 1.5.5 --
With this implementation clk_get and clk_enable always succeed. The
counterparts clk_put and clk_disable only do some minor error checking.
Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
lib/Kconfig | 6 ++++++
lib/Makefile | 2 ++
lib/dummyclk.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 62 insertions(+), 0 deletions(-)
create mode 100644 lib/dummyclk.c
Hello,
this patch isn't really UIO related, but as my platform driver uses the
clk API and that's not implemented on X86 here comes a dummy
implementation.
This patch was already sent but up to now I got no feedback for it.
Best regards
Uwe
diff --git a/lib/Kconfig b/lib/Kconfig
index 2d53dc0..098830d 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -144,4 +144,10 @@ config CHECK_SIGNATURE
config HAVE_LMB
boolean
+config DUMMY_CLK
+ def_bool y if X86
+ help
+ This provides a dummy implementation for the API defined in
+ linux/clk.h for platforms that don't implement it theirselves.
+
endmenu
diff --git a/lib/Makefile b/lib/Makefile
index bf8000f..e355c69 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -70,6 +70,8 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
obj-$(CONFIG_HAVE_LMB) += lmb.o
+obj-$(CONFIG_DUMMY_CLK) += dummyclk.o
+
hostprogs-y := gen_crc32table
clean-files := crc32table.h
diff --git a/lib/dummyclk.c b/lib/dummyclk.c
new file mode 100644
index 0000000..bf364df
--- /dev/null
+++ b/lib/dummyclk.c
@@ -0,0 +1,54 @@
+/*
+ * lib/dummyclk.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+
+struct clk {
+ unsigned int enablecnt;
+};
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+ struct clk ...Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
Hello,
This is the former patch 4/4 after some discussion.
Open issues:
- clock name "uio" isn't considered good by Russell King
I don't have a better suggestion
- some code could be shared with uio_smx.c
I would address that in a seperate patch after this one hits mainline.
I appreciate any feedback.
Best regards,
Uwe
drivers/uio/Kconfig | 7 ++
drivers/uio/Makefile | 1 +
drivers/uio/uio_pdrv.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 162 insertions(+), 0 deletions(-)
create mode 100644 drivers/uio/uio_pdrv.c
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 78e139c..2e9079d 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -26,6 +26,13 @@ config UIO_CIF
To compile this driver as a module, choose M here: the module
will be called uio_cif.
+config UIO_PDRV
+ tristate "Userspace I/O platform driver"
+ help
+ Generic platform driver for Userspace I/O devices.
+
+ If you don't know what to do here, say N.
+
config UIO_SMX
tristate "SMX cryptengine UIO interface"
default n
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 18c4566..e00ce0d 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_UIO) += uio.o
obj-$(CONFIG_UIO_CIF) += uio_cif.o
+obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
obj-$(CONFIG_UIO_SMX) += uio_smx.o
diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
new file mode 100644
index 0000000..f421a6e
--- /dev/null
+++ b/drivers/uio/uio_pdrv.c
@@ -0,0 +1,154 @@
+/*
+ * drivers/uio/uio_pdrv.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/clk.h>
+#include <linu...Hello, I added another branch[1] on my repo that doesn't have the dummy clk patch and variant of this one that doesn't use the clk API. This way the clk API isn't needed anymore for my patch and the issue about the clock name disappeard, too. Best regards Uwe [1] git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio2 -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
Hi Uwe, sorry for the delay, I was away for a few days and had an awful lot of work when I came back. About your generic platform driver: I think we've got two choices, both of them are acceptable as far as I'm concerned: 1.) Use the clk API and make your driver depend on it. AFAICS, only ARM and PPC implement it right now. On some platforms, it will probably never be implemented. E.g. x86 doesn't have any clocks that could be controlled that way. It's probably only useful for SoCs. Advantages: People who need it get clk support for free, without having to write much code. Disadvantages: The generic platform driver is not available for all platforms. It might not be easy to implement the dependency in Kconfig in a way acceptable to all maintainers ;-) 2.) Don't use the clk API. I don't think we would lose much. Drivers could implement clk stuff in their board support. You could add some generic function pointers in struct uio_platdata that are called in open/release/probe/remove. That way, any platform specific stuff, including clk, could be handled. Advantages: The generic platform driver is available for all platforms, no need for dependencies in Kconfig. Disadvantages: People who need clk_* must write a lot of code within their board support file. Not nice and clean... I'm ready to accept 1.) or 2.), or even both of them (why can't we have two generic platform drivers?) As you are the author (and probably user) of this driver, please decide, and send a new patch for review. Thanks, Hans --
Hello Hans,
For now I suggest 2). Using the clk API might be implemented by a
generic open/release routine. Maybe I will look into that at a later
time. For now I'm happy without clk support, too.
For now you can find two patches in my uio branch at
git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio
I rebased them to current Linus' master; otherwise they are unmodified
since the last posts. For completeness I'll resend them as a reply to
this mail.
For shortlog and diffstat see below.
Best regards
Uwe
Uwe Kleine-König (2):
UIO: don't let UIO_CIF and UIO_SMX depend twice on UIO
UIO: generic platform driver
drivers/uio/Kconfig | 10 +++-
drivers/uio/Makefile | 1 +
drivers/uio/uio_pdrv.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 127 insertions(+), 2 deletions(-)
--
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
--ae210f188614bb3d1ee3f19c64e28e3cdd44877c introduced a big "if UIO"/"endif" where all uio drivers are defined. So know there is no need for them to depend explicitly on UIO. Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com> --- drivers/uio/Kconfig | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig index a4aaab9..78e139c 100644 --- a/drivers/uio/Kconfig +++ b/drivers/uio/Kconfig @@ -15,7 +15,7 @@ if UIO config UIO_CIF tristate "generic Hilscher CIF Card driver" - depends on UIO && PCI + depends on PCI default n help Driver for Hilscher CIF DeviceNet and Profibus cards. This @@ -28,7 +28,6 @@ config UIO_CIF config UIO_SMX tristate "SMX cryptengine UIO interface" - depends on UIO default n help Userspace IO interface to the Cryptography engine found on the -- 1.5.5.1 --
--
Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
drivers/uio/Kconfig | 7 +++
drivers/uio/Makefile | 1 +
drivers/uio/uio_pdrv.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 126 insertions(+), 0 deletions(-)
create mode 100644 drivers/uio/uio_pdrv.c
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 78e139c..2e9079d 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -26,6 +26,13 @@ config UIO_CIF
To compile this driver as a module, choose M here: the module
will be called uio_cif.
+config UIO_PDRV
+ tristate "Userspace I/O platform driver"
+ help
+ Generic platform driver for Userspace I/O devices.
+
+ If you don't know what to do here, say N.
+
config UIO_SMX
tristate "SMX cryptengine UIO interface"
default n
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 18c4566..e00ce0d 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_UIO) += uio.o
obj-$(CONFIG_UIO_CIF) += uio_cif.o
+obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
obj-$(CONFIG_UIO_SMX) += uio_smx.o
diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
new file mode 100644
index 0000000..5d0d2e8
--- /dev/null
+++ b/drivers/uio/uio_pdrv.c
@@ -0,0 +1,118 @@
+/*
+ * drivers/uio/uio_pdrv.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/stringify.h>
+
+#define DRIVER_NAME "uio"
+
+struct uio_platdata {
+ struct uio_info *uioinfo;
+};
+
+static int uio_pdrv_probe(struct platform_device *pdev)
+{
+ struct uio_info *uioinfo = pdev->dev.platform_data;
+ struct uio_platdata *pdata;
+ struct uio_mem *uiomem;
+ int ret ...--
Hello, I assume now these two patches are ready go into mainline?! What's the next step? Greg, do you take them into your driver tree? Best regards Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
Yes, if everyone has finally agreed, Hans, can you resend them to me so I know what to apply, with you signed-off-by? thanks, greg k-h --
Hello Greg, If Hans did that, I didn't notice it. If you want to take the patches from me you can pull them from my uio branch at git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git uio These have the Signed-off-by: by Hans, too. Best regards Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
Hans is at LinuxTag this week and said he would send them to me when he got back. So I'll wait for his copies, I'd prefer not to pull from git, as that doesn't really work well with my patchflow process. thanks, greg k-h --
Hello Greg, I know you work with quilt, but I thought it to be no problem for you to extract the patches from my tree into your quilt queue. I'll remember that for the future. :-) Best regards Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
I sent the latest version to Greg, should get merged soon. Thanks, Hans --
I'd suggest it should be passed through from the platform code: only it knows what it's going to be (and if indeed there's going to be one at all). In fact, the lack-of-clock at the moment is fatal; it shouldn't As I said before I'd prefer to keep my driver separate and standalone, but you and HJK have expressed a wish for unification. If I get this formally I'll consider myself outvoted and submit a patch to unify it all when I submit my platform code (soon). --Ben. --
This seems to be a misunderstanding. Actually, I don't care too much. If you like to keep your driver standalone, I don't have objections, even if Uwe's generic driver is merged. I think there are still valid reasons not to reduce a driver to a platform data struct. Greg, what do you say to that? Thanks, Hans --
Otherwise the device might just disappear while /dev/uioX is being used
which results in an Oops.
Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
drivers/uio/uio.c | 40 +++++++++++++++++++++++-----------------
1 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 1175908..005fc55 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -301,25 +301,35 @@ static int uio_open(struct inode *inode, struct file *filep)
if (!idev)
return -ENODEV;
+ if (!try_module_get(idev->owner)) {
+ ret = -ENODEV;
+ goto err_module_get;
+ }
+
listener = kmalloc(sizeof(*listener), GFP_KERNEL);
- if (!listener)
- return -ENOMEM;
+ if (!listener) {
+ ret = -ENOMEM;
+ goto err_alloc_listener;
+ }
listener->dev = idev;
listener->event_count = atomic_read(&idev->event);
filep->private_data = listener;
if (idev->info->open) {
- if (!try_module_get(idev->owner))
- return -ENODEV;
ret = idev->info->open(idev->info, inode);
- module_put(idev->owner);
- }
+ if (ret) {
+ kfree(listener);
+err_alloc_listener:
- if (ret)
- kfree(listener);
+ module_put(idev->owner);
+err_module_get:
- return ret;
+ return ret;
+ }
+ }
+
+ return 0;
}
static int uio_fasync(int fd, struct file *filep, int on)
@@ -336,12 +346,11 @@ static int uio_release(struct inode *inode, struct file *filep)
struct uio_listener *listener = filep->private_data;
struct uio_device *idev = listener->dev;
- if (idev->info->release) {
- if (!try_module_get(idev->owner))
- return -ENODEV;
+ if (idev->info->release)
ret = idev->info->release(idev->info, inode);
- module_put(idev->owner);
- }
+
+ module_put(idev->owner);
+
if (filep->f_flags & FASYNC)
ret = uio_fasync(-1, filep, 0);
kfree(listener);
@@ -510,10 +519,7 @@ static int uio_mmap(struct file *filep, struct vm_area_struc...Hi Uwe,
thanks for this one, good catch! Looks fine to me. There are some minor issues, see
below.
And I'd like to hear Greg's opinion: Do you agree we can omit
try_module_get() in uio_mmap()?
Thanks,
I really don't like these labels inside the if-block. I find it hard to
read. What about this:
if (idev->info->open) {
ret = idev->info->open(idev->info, inode);
if (ret)
kfree(listener);
return ret;
}
err_alloc_listener:
module_put(idev->owner);
err_module_get:
return ret;
The label err_module_get should probably be omitted because it's used only
once and has just one line of code. You could simply write "return ret"
instead of "goto err_module_get".
--Hello, As Greg already pointed out, mmap only works for open files and so the With that you leak a reference to idev->owner if idev->info->open() fails. Things like that don't happen that easy if all error handing is in one This makes code shuffling easier. For example if someone decides that try_module_get should be done after allocating listener then you only have to exchange the two corresponding code blocks and the two groups (label + cleanup) in the error handling block. If the error handling is spread over the whole functions you can easily miss something---as happend above. :-) Best regards Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
Maybe. It's merely an example to explain what I mean. Documentation/CodingStyle says nothing about how to place labels, but I find it best to have all error path exits at the end of a function. All Well, it depends. It's all about readability. Any function should be written in a way that makes it as clear as possible what it does. Your code is certainly not critical regarding that aspect, but I think it can still be improved. And a label that is used only once and contains only one line of code is definetly unnecessary. I don't follow the maybe-one-day-in-the-future-it-might-be-useful philosophy. I like code that is as clean and readable as possible _now_. And as this patch is not just a driver but affects the UIO core, this is even more important. Could you please send an updated patch? Thanks, Hans --
Otherwise the device might just disappear while /dev/uioX is being used
which results in an Oops.
Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
That thing about code reordering is minor compared to having all error
... , it's your code, so you can find a new version below.
Best regards
Uwe
drivers/uio/uio.c | 36 +++++++++++++++++++++---------------
1 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 1175908..55cc7b8 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -301,23 +301,33 @@ static int uio_open(struct inode *inode, struct file *filep)
if (!idev)
return -ENODEV;
+ if (!try_module_get(idev->owner))
+ return -ENODEV;
+
listener = kmalloc(sizeof(*listener), GFP_KERNEL);
- if (!listener)
- return -ENOMEM;
+ if (!listener) {
+ ret = -ENOMEM;
+ goto err_alloc_listener;
+ }
listener->dev = idev;
listener->event_count = atomic_read(&idev->event);
filep->private_data = listener;
if (idev->info->open) {
- if (!try_module_get(idev->owner))
- return -ENODEV;
ret = idev->info->open(idev->info, inode);
- module_put(idev->owner);
+ if (ret)
+ goto err_infoopen;
}
- if (ret)
- kfree(listener);
+ return 0;
+
+err_infoopen:
+
+ kfree(listener);
+err_alloc_listener:
+
+ module_put(idev->owner);
return ret;
}
@@ -336,12 +346,11 @@ static int uio_release(struct inode *inode, struct file *filep)
struct uio_listener *listener = filep->private_data;
struct uio_device *idev = listener->dev;
- if (idev->info->release) {
- if (!try_module_get(idev->owner))
- return -ENODEV;
+ if (idev->info->release)
ret = idev->info->release(idev->info, inode);
- module_put(idev->owner);
- }
+
+ module_put(idev->owner);
+
if (filep->f_flags & FASYNC)
ret = uio_fasync(-1, filep, 0);
kfree(listener);
@@ -510,10 +519,7 @@ static ...Looks alright, thanks! It's not _my_ code, it's _our_ code, partly written by me. At home, I use any coding style I like. But this is in mainline, so we use the coding style the --
If it's already been grabbed in the open() call, everything should be safe, right? thanks, greg k-h --
Yes, it looks quite clean to me. module_get in open(), module_put in release() and nothing of that sort anywhere else. Maybe it just looked toooo simple to me ;-) Thanks for your confirmation. --
To help your understanding, without this patch I can trigger the Oops by starting an app that does opening and mmapping /dev/uio0 while(1); and then in another shell do rmmod uio_pdrv Best regards Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
This allows configuring CONFIG_UIO without changing into the UIO submenu Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com> --- drivers/uio/Kconfig | 8 ++------ 1 files changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig index b778ed7..a899306 100644 --- a/drivers/uio/Kconfig +++ b/drivers/uio/Kconfig @@ -1,9 +1,7 @@ -menu "Userspace I/O" - depends on !S390 - -config UIO +menuconfig UIO tristate "Userspace I/O drivers" default n + depends on !S390 help Enable this to allow the userspace driver core code to be built. This code allows userspace programs easy access to @@ -25,5 +23,3 @@ config UIO_CIF To compile this driver as a module, choose M here: the module will be called uio_cif. - -endmenu -- 1.5.4.5 --
I like it. --
You like it so much you already acked this same change from someone else, which is in my tree at: thanks, greg k-h --
Hey, I just wanted to improve my Signed-off-by statistics :-) Damn it, one less... No, seriously, it looked somehow familiar but I didn't remember. Sorry. Thanks, Hans --
currently there is only one driver, but new entries don't need to depend explicitly on UIO. Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com> --- drivers/uio/Kconfig | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig index a899306..6bc2891 100644 --- a/drivers/uio/Kconfig +++ b/drivers/uio/Kconfig @@ -11,9 +11,11 @@ menuconfig UIO If you don't know what to do here, say N. +if UIO + config UIO_CIF tristate "generic Hilscher CIF Card driver" - depends on UIO && PCI + depends on PCI default n help Driver for Hilscher CIF DeviceNet and Profibus cards. This @@ -23,3 +25,5 @@ config UIO_CIF To compile this driver as a module, choose M here: the module will be called uio_cif. + +endif -- 1.5.4.5 --
Agreed. --
Already in the -mm tree... thanks, greg k-h --
Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
drivers/uio/Kconfig | 7 ++
drivers/uio/Makefile | 1 +
drivers/uio/uio_pdrv.c | 165 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 173 insertions(+), 0 deletions(-)
create mode 100644 drivers/uio/uio_pdrv.c
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 6bc2891..5ec353f 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -26,4 +26,11 @@ config UIO_CIF
To compile this driver as a module, choose M here: the module
will be called uio_cif.
+config UIO_PDRV
+ tristate "Userspace I/O platform driver"
+ help
+ Generic platform driver for Userspace I/O devices.
+
+ If you don't know what to do here, say N.
+
endif
diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
index 7fecfb4..a6dcb99 100644
--- a/drivers/uio/Makefile
+++ b/drivers/uio/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_UIO) += uio.o
obj-$(CONFIG_UIO_CIF) += uio_cif.o
+obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o
diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c
new file mode 100644
index 0000000..31d1aaf
--- /dev/null
+++ b/drivers/uio/uio_pdrv.c
@@ -0,0 +1,165 @@
+/*
+ * drivers/uio/uio_pdrv.c
+ *
+ * Copyright (C) 2008 by Digi International Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+
+#define DRIVER_NAME "uio"
+
+/* XXX: I thought there already exists something like STRINGIFY, but I could not
+ * find it ...
+ */
+#ifndef STRINGIFY
+#define STRINGIFY(x) __STRINGIFY_HELPER(x)
+#define __STRINGIFY_HELPER(x) #x
+#endif
+
+struct uio_platdata {
+ struct uio_info *uioinfo;
+ struct clk *clk;
+};
+
+static int uio_pdrv_open(struct uio_info *info, struct inod...I looked it over again. Some comments below.
I'm beginning to like the idea...
Why do you want that macro? You only use it once. The macro definition and the
I'd prefer this:
if (i >= MAX_UIO_MAPS) {
What about this:
dev_warn(&pdev->dev, "device has more than %d"
" I/O memory resources.\n", MAX_UIO_MAPS);
How I hate labels within blocks... OK, I admit, it looks good here...
--Hello Hans,
See below.
BTW, I found it, it's in linux/stringify.h. And there are several
possible users:
ukleinek@zentaur:~/gsrc/linux-2.6$ git ls-files -z | xargs -0 perl -n -e 'print "$ARGV\n" if (/define\s+\w+\((\w+)\)\s*#\s*$1/)'
arch/arm/vfp/vfpinstr.h
arch/cris/arch-v10/boot/tools/build.c
arch/m68k/lib/checksum.c
arch/mips/kernel/unaligned.c
arch/powerpc/boot/reg.h
arch/um/drivers/mconsole_user.c
arch/um/include/sysdep-i386/kernel-offsets.h
arch/um/include/sysdep-x86_64/kernel-offsets.h
arch/x86/kernel/machine_kexec_32.c
drivers/atm/ambassador.c
drivers/media/video/cpia2/cpia2_v4l.c
drivers/mtd/maps/amd76xrom.c
drivers/mtd/maps/ichxrom.c
drivers/s390/cio/qdio.h
drivers/scsi/aacraid/aacraid.h
drivers/scsi/aacraid/linit.c
drivers/scsi/arm/acornscsi.c
drivers/scsi/g_NCR5380.h
drivers/uio/uio_pdrv.c
drivers/usb/serial/garmin_gps.c
include/asm-cris/arch-v10/irq.h
include/asm-cris/arch-v32/hwregs/supp_reg.h
include/asm-cris/arch-v32/irq.h
include/asm-m32r/assembler.h
include/asm-m68k/entry.h
include/asm-mips/mipsregs.h
include/asm-mips/sim.h
include/asm-sh/cpu-sh5/registers.h
include/asm-v850/macrology.h
include/linux/stringify.h
include/sound/core.h
sound/core/memalloc.c
I hope it cannot, that's why it is a bug if it happens. :-) And one
should expect that no BUG_ON should ever be triggered. I usually use
This is a matter of taste. In my eyes it's better to declare it here
OK.
BTW would you be open to redefine uio_info as:
struct uio_info {
struct uio_device *uio_dev;
...
size_t num_memmaps;
struct uio_mem mem[];
}
This allows to allocate exactly the number of members in the mem array
that are needed (for the cost of a size_t). (You just do:
uio_info uioinfo = kzalloc(sizeof(*uioinfo) + num_memmaps * sizeof(uioinfo->mem[0]), GFP_KERNEL);
it's still one chunk of memory and usage is similar---just with
The macro is for free, using "%d" is not:
ukleinek@zentaur:~/kbuild-ns921x$ size dri...OK, as this is a generic driver where we don't know what crap people No. It's more important to see which variables are declared in the function and which are declared elsewhere. If you have to search the whole body of a function for possible declarations, this is BAD. And if it's not clear where a variable is used, the function is too long or has other style problems. Your function is short and clean, so where's the I don't like it. It makes things more complicated without any obvious gain. sizeof(struct uio_info) would return wrong values, you need to free the extra memory, userspace applications need to be able to deal with 10000 mappings... If there's an actual usecase where 5 mappings are not enough, we can As I said, it looks OK here. You can keep it if you like it. I'd like to thank you for your work. After giving it some thought, I really like the idea of having a generic UIO driver for platform devices. I think many people (including /me) will use it. So, please send an updated patch, I think we should push it to mainline. Thanks, Hans --
Hello Hans,
I'm not conviced and still prefer it that way. I gave way for your
requests in uio.c because it's your code. I want to leave it as it is
Most use cases I imagine only use a single mapping, so the gain would be
For which definition of wrong? sizeof(struct uio_info) don't include
space for mem then, but in my eyes that's correct. Having to care about
There is no extra memory because uioinfo and it's mem member are
allocated together with a single kzalloc (and must be). (Thats the
difference to
struct uio_info {
...
size_t num_memmaps;
struct uio_mem *mem;
};
For the userspace it's exactly the same, isn't it?
Best regards
Uwe
--
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
--I looked around a bit and talked to some people. It seems that a local
variable declaration within a for{} block is OK. I still don't like it,
but I won't object any longer.
So, AFAICT you've only got that arch dependency problem left to solve.
Thanks,
Hans
--Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com> --- While reworking the code I saw that your variant is not correct because if pdev has resources with flags != IORESOURCE_MEM the constraint uiomem == &uioinfo->mem[i] doesn't hold. Below is a new version that uses linux/stringify and zeros size for unused mappings (line 102ff). Best regards Uwe drivers/uio/Kconfig | 7 ++ drivers/uio/Makefile | 1 + drivers/uio/uio_pdrv.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 0 deletions(-) create mode 100644 drivers/uio/uio_pdrv.c diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig index 6bc2891..5ec353f 100644 --- a/drivers/uio/Kconfig +++ b/drivers/uio/Kconfig @@ -26,4 +26,11 @@ config UIO_CIF To compile this driver as a module, choose M here: the module will be called uio_cif. +config UIO_PDRV + tristate "Userspace I/O platform driver" + help + Generic platform driver for Userspace I/O devices. + + If you don't know what to do here, say N. + endif diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile index 7fecfb4..a6dcb99 100644 --- a/drivers/uio/Makefile +++ b/drivers/uio/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_UIO) += uio.o obj-$(CONFIG_UIO_CIF) += uio_cif.o +obj-$(CONFIG_UIO_PDRV) += uio_pdrv.o diff --git a/drivers/uio/uio_pdrv.c b/drivers/uio/uio_pdrv.c new file mode 100644 index 0000000..47506aa --- /dev/null +++ b/drivers/uio/uio_pdrv.c @@ -0,0 +1,163 @@ +/* + * drivers/uio/uio_pdrv.c + * + * Copyright (C) 2008 by Digi International Inc. + * All rights reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ +#include <linux/clk.h> +#include <linux/platform_device.h> +#include <linux/uio_driver.h> +#include <linux/stringify.h> + +#define DRIVER_NAME "uio" + +s...
Hello, This must read __stringify(MAX_UIO_MAPS). Sorry, I didn't compile test that. Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
Care to send the latest version of this, I'm a bit lost as to what people want me to apply... thanks, greg k-h --
Hi Greg, PATCH 4/4 still has a problem. It uses some clock framework functions not available on every architecture. E.g. on x86_64 you can select this driver in menuconfig, but it won't compile. The first three patches are OK in my opinion. Uwe provided a second version of PATCH 1/4, PATCH 2/4 and PATCH 3/4 were alright in their original version. I added my Signed-off-by to 1-3, but not to 4. Thanks, Hans --
Ok, I grabbed patch 1, 2 and 3 are already in my tree and -mm :) Let me know if you all come to an agreement on patch 4. thanks, greg k-h --
Thanks, but it doesn't compile, neither with -rc8 nor with Linus' git. One problem can easily be fixed, the macro is called __stringify, not stringify. But what about this: ERROR: "clk_get" [drivers/uio/uio_pdrv.ko] undefined! ERROR: "clk_enable" [drivers/uio/uio_pdrv.ko] undefined! ERROR: "clk_disable" [drivers/uio/uio_pdrv.ko] undefined! ERROR: "clk_put" [drivers/uio/uio_pdrv.ko] undefined! Do you have any extra patches applied? Against which kernel do you test? Thanks, --
Hello, I just notice that, too. My mail address that and your's just crossed Yes I have, but nothing special. This is part of a generic API defined in include/linux/clk.h. One of it's use it to abstract away some platform dependencies. There are several architectures that define it[1]. I used it to allow enabling the device only when the device is opened. Typical things in the enable routine are enabling a clock or reserve and configure gpios etc. A minimal dummy implementation that should work here is: #define clk_get(dev, id) NULL #define clk_put(clk) ((void)0) #define clk_enable(clk) (1) #define clk_disable(clk) ((void)0) Best regards Uwe [1] Try: git ls-files -z | xargs -0 perl -n -e 'print "$ARGV\n" if /EXPORT_SYMBOL(?:_GPL)?\s*\(\s*clk_get\s*\)/;' -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
I know. Unfortunately, I tested on x86_64, and it doesn't compile. If it's depending on something, then this dependency should be added in Kconfig. If it can be selected in the configuration, I expect it to compile (and work). Thanks, --
Hello, [Added Russell King to Cc:] Maybe adding a dummy implementation that is compiled for machines that don't provide a native one. Currently there is no cpp symbol that tells if an machine supports the API. @Russell: Do you have an opinion regarding this!? Best regards Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
Only that the kernels Kconfig is turning into a real complicated mess of dependencies IMHO. We could add a HAVE_CLK and add that to the dependency of all the drivers which use linux/clk.h. The problem will be finding all those drivers and their corresponding Kconfig entries. My feeling is that we're just going to end up creating another Kconfig symbol which folk half-heartedly use. --
I don't like that either. What do you think about the patch below?
It doesn't introduce a new symbol that needs much care and attention.
This way the clk API is available on all configurations provided that
CONFIG_DUMMY_CLK is set correctly. If CONFIG_DUMMY_CLK is set wrong it
should result in a compile error. Either because there are two
implementations of clk_get or none.
The condition on when to define DUMMY_CLK isn't yet perfect, but not
defining it for a platform isn't a regression as there was no
implementation before this patch either.
This could supersede the implementation in
drivers/usb/gadget/pxa2xx_udc.c for IXP. (That driver obviously doesn't
check if clk_enable() succeeded, because it's defined as:
#define clk_enable(clk) do { } while (0)
.)
Maybe it would be fine to make these functions inline and define them
directly in linux/clk.h?
Best regards
Uwe
---->8----
From: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
Date: Mon, 14 Apr 2008 09:02:30 +0200
Subject: [PATCH] provide a dummy implementation of the clk API
With this implementation clk_get and clk_enable always succeed. The
counterparts clk_put and clk_disable only do some minor error checking.
Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
lib/Kconfig | 6 ++++++
lib/Makefile | 2 ++
lib/dummyclk.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 62 insertions(+), 0 deletions(-)
create mode 100644 lib/dummyclk.c
diff --git a/lib/Kconfig b/lib/Kconfig
index ba3d104..53fee1c 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -141,4 +141,10 @@ config HAS_DMA
config CHECK_SIGNATURE
bool
+config DUMMY_CLK
+ def_bool y if X86
+ help
+ This provides a dummy implementation for the API defined in
+ linux/clk.h for platforms that don't implement it theirselves.
+
endmenu
diff --git a/lib/Makefile b/lib/Makefile
index 23de261..2ca3e82 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -70,6 +70,8 @...Hang on. I'm lost. What are we talking about here? I thought the thread was about the one liner patch for UIO to arch/arm/Kconfi (which still hasn't hit the patch system so is still on target for being missed...) What's this drivers/uio/uio_pdrv.ko module, and why doesn't it appear in the LKML archive of this thread? --
Hi Russell, No, the topic here is a generic uio platform driver. It uses the clk API and Hans criticised that is doesn't compile on x86 (because there is no implementation of the clk API). So I suggested to implement a dummy for that. This is completly independant of the inclusion of drivers/uio/Kconfig in Don't know why lkml.org didn't link these. The start of the thread can be found at http://lkml.org/lkml/2008/4/10/110 gmane managed to link these mails: http://thread.gmane.org/gmane.linux.kernel/663884 Best regards Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
Thanks. Well, tbh, I don't know which way to go on this. Each of the
suggested ways have their downsides.
However:
+ pdata->clk = clk_get(&pdev->dev, DRIVER_NAME);
seems wrong - "uio" as a clock name?
+ /* XXX: better use dev_dbg, but which device should I use?
+ * info->uio_dev->dev isn't accessible here as struct uio_device+ * is opaque.
+ */
why not store a copy of 'dev' in struct uio_platdata ?
+ uiomem = &uioinfo->mem[0];
+ for (i = 0; i < pdev->num_resources; ++i) {
...
+ ++uiomem;
+ }
Who's to say there's pdev->num_resources entries in the 'mem' array?
Shouldn't this loop also be limited to MAX_UIO_MAPS iterations (or
maybe complain if there's more than MAX_UIO_MAPS)?
+/* XXX: I thought there already exists something like STRINGIFY, but I could not
+ * find it ...
+ */
+#ifndef STRINGIFY
+#define STRINGIFY(x) __STRINGIFY_HELPER(x)
+#define __STRINGIFY_HELPER(x) #x
+#endif
#include <linux/stringify.h> ?
--I don't like increasing the size of struct uio_platdata only for a debug
helper. In most cases pr_debug (or dev_dbg) compiles to nothing.
The code you skipped with ... from that loop includes
if (uiomem >= &uioinfo->mem[MAX_UIO_MAPS]) {
dev_warn(&pdev->dev, "...");
break;
}
that should take care of that. In v2 of this patch the remaining
If you look at the v2 patch[1] I found that in the mean time. Thanks
all the same.
Best regards and thanks for your review,
Uwe
[1] http://lkml.org/lkml/2008/4/11/81 or
http://thread.gmane.org/gmane.linux.kernel/665257
--
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
--In one of the mails, it was said: No. It's more important to see which variables are declared in the function and which are declared elsewhere. If you have to search the whole body of a function for possible declarations, this is BAD. And if it's not clear where a variable is used, the function is too long or has other style problems. Your function is short and clean, so where's the problem? Please move the declaration to the top of the function. I disagree with this statement. It's far better to limit the scope of variables so that you know they only have local use, and eg, not used inside a loop and then outside with possible unintended effects. If a variable is only used inside a loop, it should be declared _inside_ that loop. The statement goes on to talk about the function being short and clean - that's not an argument to apply any particular point of view on this subject, since you can argue that because it's short and clean you can see that the variable is only used within the loop. So, please, keep the variable declaration inside the loop, and don't pollute the outer levels with unnecessary variable declarations. --
OK, I'm finally convinced :-) I knew this style from C++ where it makes sense, because a (class-) variable declaration can implicitly call the constructor of that class which you normally want to avoid if not needed. As this doesn't happen in C, I found it unnecessary. I agree now that there's also a readability argument. The limitation of the scope is probably not that important as compilers will optimize that anyway in a lot of cases. Thanks, Hans --
BTW: I just found http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/driver-core/ui... So obviously gkh looks for that issue. In my eyes it should better go via rmk, but it doesn't really matter for me. Should I still send a patch to the patch system? Best regards Uwe -- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 --
Hi Uwe, I'm a bit slow today, I don't really understand what this is good for. It's to complicated to serve as a template, and it doesn't support interrupts, so it's not good for any real device, too. So the only usecase would be an irq-less platform_device that just needs its memory mapped. Is this what you intended? What do _you_ use it for? Thanks, --
Hello Hans, Hans J. Koch wrote: > On Thu, Apr 10, 2008 at 02:37:03PM +0200, Uwe Kleine-K
Hmm, I get the idea. You could have a UIO driver for a platform device just by setting up a struct in the board support file. Nice thought. Hmm. It's late, let me sleep over it... Tomorrow I'll look at it again. Thanks, --
I've seen this kind of thing hacked up by a few people already, mainly as a replacement for /dev/mem. Many people are being scared off using /dev/mem (and rightly so) because - They've seen discussions regarding future plans whereby /dev/mem wouldn't be allowed access to physical memory - They don't have anything like X forcing them to have /dev/mem and therefore want to disable it completely for (perceived?) security reasons. I like it, it'll sure be used. --Ben. --