Skip to contentSkip to conversation

UIO: missing resource mapping

PATCH Andreas Schallenberg 4 participants 17 messages Jul 12, 2012 – Aug 8, 2012

Subsystems (3)
KVM Userspace I/O (UIO) Generic UIO driver for PCI devices
lore
Share & tools
lore

Andreas Schallenberg

author
#0 lore raw

Hello,

I'm doing experiments with the Userspace IO driver (UIO_PCI_GENERIC) and a set of PCIe cards. The kernel version is 3.4.4, CPU is a Marvell MV78200 (ARMv5te). Example with an Intel ethernet card:

This makes /dev/uio0 appear
echo -n "8086 10d3" >/sys/bus/pci/drivers/uio_pci_generic/new_id

# lspci -v -k -s 0000:00:01.0
00:01.0 Ethernet controller: Intel Corporation 82574L Gigabit Network Connection
        Subsystem: Intel Corporation Gigabit CT Desktop Adapter
        Flags: bus master, fast devsel, latency 0, IRQ 36
        Memory at c00c0000 (32-bit, non-prefetchable) [size=128K]
        Memory at c0000000 (32-bit, non-prefetchable) [size=512K]
        I/O ports at f0800000 [size=32]
        Memory at c00e0000 (32-bit, non-prefetchable) [size=16K]
        [virtual] Expansion ROM at c0080000 [disabled] [size=256K]
        Capabilities: [c8] Power Management version 2
        Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+
        Capabilities: [e0] Express Endpoint, MSI 00
        Capabilities: [a0] MSI-X: Enable- Count=5 Masked-
        Capabilities: [100] Advanced Error Reporting
        Capabilities: [140] Device Serial Number 00-1b-21-ff-ff-c4-f7-2f
        Kernel driver in use: uio_pci_generic

Suprisingly, there is no "maps" directory in /sys/class/uio/uio0 now. I made a small change to the UIO code:

drivers/uio/uio.c +4 -2
index a783d53..b639654 100644
@@ -274,8 +274,10 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 
        for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
                mem = &idev->info->mem[mi];
-               if (mem->size == 0)
-                       break;
+               if (mem->size == 0) {
+                       dev_err(idev->dev, "map %d has zero size\n", mi);
+                       continue; // was: break
+               }
                if (!map_found) {
                        map_found = 1;
                        idev->map_dir = kobject_create_and_add("maps",

If I have this in the kernel and give the "echo" command as shown above I get:

[   43.761723] uio uio0: map 0 has zero size
[   43.765760] uio uio0: map 1 has zero size
[   43.769787] uio uio0: map 2 has zero size
[   43.774298] uio uio0: map 3 has zero size
[   43.778333] uio uio0: map 4 has zero size

On the other hand, I can see the resources:

# cat /sys/class/uio/uio0/device/resource
0x00000000c00c0000 0x00000000c00dffff 0x0000000000040200
0x00000000c0000000 0x00000000c007ffff 0x0000000000040200
0x00000000f0800000 0x00000000f080001f 0x0000000000040101
0x00000000c00e0000 0x00000000c00e3fff 0x0000000000040200
...more...

Looking further at the code, I cannot see where the mem fields are being filled at all. Which code is supposed to write the struct uio_mem?

Best,
Andreas

#1 lore raw

On Thu, Jul 12, 2012 at 09:26:23AM +0200, Andreas Schallenberg wrote:

[Added more people to Cc:]

Quoted 65 lines

Hello,

I'm doing experiments with the Userspace IO driver (UIO_PCI_GENERIC) and a set of PCIe cards. The kernel version is 3.4.4, CPU is a Marvell MV78200 (ARMv5te). Example with an Intel ethernet card:

This makes /dev/uio0 appear
echo -n "8086 10d3" >/sys/bus/pci/drivers/uio_pci_generic/new_id

# lspci -v -k -s 0000:00:01.0
00:01.0 Ethernet controller: Intel Corporation 82574L Gigabit
Network Connection
        Subsystem: Intel Corporation Gigabit CT Desktop Adapter
        Flags: bus master, fast devsel, latency 0, IRQ 36
        Memory at c00c0000 (32-bit, non-prefetchable) [size=128K]
        Memory at c0000000 (32-bit, non-prefetchable) [size=512K]
        I/O ports at f0800000 [size=32]
        Memory at c00e0000 (32-bit, non-prefetchable) [size=16K]
        [virtual] Expansion ROM at c0080000 [disabled] [size=256K]
        Capabilities: [c8] Power Management version 2
        Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+
        Capabilities: [e0] Express Endpoint, MSI 00
        Capabilities: [a0] MSI-X: Enable- Count=5 Masked-
        Capabilities: [100] Advanced Error Reporting
        Capabilities: [140] Device Serial Number 00-1b-21-ff-ff-c4-f7-2f
        Kernel driver in use: uio_pci_generic

Suprisingly, there is no "maps" directory in /sys/class/uio/uio0 now. I made a small change to the UIO code:

drivers/uio/uio.c +4 -2
index a783d53..b639654 100644
@@ -274,8 +274,10 @@ static int uio_dev_add_attributes(struct
 uio_device *idev)
 
        for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
                mem = &idev->info->mem[mi];
-               if (mem->size == 0)
-                       break;
+               if (mem->size == 0) {
+                       dev_err(idev->dev, "map %d has zero size\n", mi);
+                       continue; // was: break
+               }
                if (!map_found) {
                        map_found = 1;

                        idev->map_dir = kobject_create_and_add("maps",

If I have this in the kernel and give the "echo" command as shown above I get:

[   43.761723] uio uio0: map 0 has zero size
[   43.765760] uio uio0: map 1 has zero size
[   43.769787] uio uio0: map 2 has zero size
[   43.774298] uio uio0: map 3 has zero size
[   43.778333] uio uio0: map 4 has zero size

On the other hand, I can see the resources:

# cat /sys/class/uio/uio0/device/resource
0x00000000c00c0000 0x00000000c00dffff 0x0000000000040200
0x00000000c0000000 0x00000000c007ffff 0x0000000000040200
0x00000000f0800000 0x00000000f080001f 0x0000000000040101
0x00000000c00e0000 0x00000000c00e3fff 0x0000000000040200
...more...
Quoted 3 lines

Looking further at the code, I cannot see where the mem fields are being filled at all.
Which code is supposed to write the struct uio_mem?

In my opinion, the driver should. However, Michael's idea is to use /sys/bus/pci/devices/XXXXXresourceX for mapping purposes.

That is of course also possible, but obviously it leads to confusion. We already had a long thread about this:

http://www.spinics.net/lists/kvm/msg73837.html

Michael, can we change the driver to offer all available PCI BARs in the normal UIO way? I'm afraid otherwise we'll have the same discussion over and over again.

Thanks,
Hans

maintainer
#2 lore raw
Quoted 79 lines · Hans J. Koch

On Thu, Jul 12, 2012 at 09:26:23AM +0200, Andreas Schallenberg wrote:

[Added more people to Cc:]

Quoted 65 lines

Hello,

I'm doing experiments with the Userspace IO driver (UIO_PCI_GENERIC) and a set of PCIe cards. The kernel version is 3.4.4, CPU is a Marvell MV78200 (ARMv5te). Example with an Intel ethernet card:

This makes /dev/uio0 appear
echo -n "8086 10d3" >/sys/bus/pci/drivers/uio_pci_generic/new_id

# lspci -v -k -s 0000:00:01.0
00:01.0 Ethernet controller: Intel Corporation 82574L Gigabit
Network Connection
        Subsystem: Intel Corporation Gigabit CT Desktop Adapter
        Flags: bus master, fast devsel, latency 0, IRQ 36
        Memory at c00c0000 (32-bit, non-prefetchable) [size=128K]
        Memory at c0000000 (32-bit, non-prefetchable) [size=512K]
        I/O ports at f0800000 [size=32]
        Memory at c00e0000 (32-bit, non-prefetchable) [size=16K]
        [virtual] Expansion ROM at c0080000 [disabled] [size=256K]
        Capabilities: [c8] Power Management version 2
        Capabilities: [d0] MSI: Enable- Count=1/1 Maskable- 64bit+
        Capabilities: [e0] Express Endpoint, MSI 00
        Capabilities: [a0] MSI-X: Enable- Count=5 Masked-
        Capabilities: [100] Advanced Error Reporting
        Capabilities: [140] Device Serial Number 00-1b-21-ff-ff-c4-f7-2f
        Kernel driver in use: uio_pci_generic

Suprisingly, there is no "maps" directory in /sys/class/uio/uio0 now. I made a small change to the UIO code:

drivers/uio/uio.c +4 -2
index a783d53..b639654 100644
@@ -274,8 +274,10 @@ static int uio_dev_add_attributes(struct
 uio_device *idev)
 
        for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
                mem = &idev->info->mem[mi];
-               if (mem->size == 0)
-                       break;
+               if (mem->size == 0) {
+                       dev_err(idev->dev, "map %d has zero size\n", mi);
+                       continue; // was: break
+               }
                if (!map_found) {
                        map_found = 1;

                        idev->map_dir = kobject_create_and_add("maps",

If I have this in the kernel and give the "echo" command as shown above I get:

[   43.761723] uio uio0: map 0 has zero size
[   43.765760] uio uio0: map 1 has zero size
[   43.769787] uio uio0: map 2 has zero size
[   43.774298] uio uio0: map 3 has zero size
[   43.778333] uio uio0: map 4 has zero size

On the other hand, I can see the resources:

# cat /sys/class/uio/uio0/device/resource
0x00000000c00c0000 0x00000000c00dffff 0x0000000000040200
0x00000000c0000000 0x00000000c007ffff 0x0000000000040200
0x00000000f0800000 0x00000000f080001f 0x0000000000040101
0x00000000c00e0000 0x00000000c00e3fff 0x0000000000040200
...more...
Quoted 3 lines

Looking further at the code, I cannot see where the mem fields are being filled at all.
Which code is supposed to write the struct uio_mem?

In my opinion, the driver should. However, Michael's idea is to use /sys/bus/pci/devices/XXXXXresourceX for mapping purposes.

That is of course also possible, but obviously it leads to confusion. We already had a long thread about this:

Quoted 8 lines

http://www.spinics.net/lists/kvm/msg73837.html

Michael, can we change the driver to offer all available PCI BARs in the normal UIO way? I'm afraid otherwise we'll have the same discussion over and over again.

Thanks,
Hans

My concern was people will ask for more and more stuff that pci sysfs already has.
If we do add these is there a way to not duplicate code from pci? Export pci_mmap_resource and use it? Or make the uio attributes softlinks to pci sysfs somehow?
Alternatively we could just clarify this in the documentation which is of course easier and gives us less code to maintain ...

Signature · 1 line
MST
#3 lore raw
Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Hans J. Koch
Quoted 3 lines

Looking further at the code, I cannot see where the mem fields are being filled at all.
Which code is supposed to write the struct uio_mem?

In my opinion, the driver should. However, Michael's idea is to use /sys/bus/pci/devices/XXXXXresourceX for mapping purposes.

That is of course also possible, but obviously it leads to confusion. We already had a long thread about this:

http://www.spinics.net/lists/kvm/msg73837.html

Michael, can we change the driver to offer all available PCI BARs in the normal UIO way? I'm afraid otherwise we'll have the same discussion over and over again.

Quoted 8 lines
Quoted 2 lines

Thanks,
Hans

My concern was people will ask for more and more stuff that pci sysfs already has.
If we do add these is there a way to not duplicate code from pci? Export pci_mmap_resource and use it? Or make the uio attributes softlinks to pci sysfs somehow?

I understand your concern. Of course I'm also not in favor of duplicating code, but I don't think there's much overhead. PCI already exports all information needed by pci_resource_start(dev, bar) and pci_resource_len(dev, bar). Have a look at other UIO PCI drivers like uio_cif.c or uio_netx.c to see how it works. It's just a few more lines of code to loop through all BARs and add the to info->mem[i].

By the way, the current size of the info->mem[] array was exactly made with PCI in mind...

Thanks,
Hans

maintainer
#4 lore raw
Quoted 34 lines · Hans J. Koch
Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Hans J. Koch
Quoted 3 lines

Looking further at the code, I cannot see where the mem fields are being filled at all.
Which code is supposed to write the struct uio_mem?

In my opinion, the driver should. However, Michael's idea is to use /sys/bus/pci/devices/XXXXXresourceX for mapping purposes.

That is of course also possible, but obviously it leads to confusion. We already had a long thread about this:

http://www.spinics.net/lists/kvm/msg73837.html

Michael, can we change the driver to offer all available PCI BARs in the normal UIO way? I'm afraid otherwise we'll have the same discussion over and over again.

Quoted 8 lines
Quoted 2 lines

Thanks,
Hans

My concern was people will ask for more and more stuff that pci sysfs already has.
If we do add these is there a way to not duplicate code from pci? Export pci_mmap_resource and use it? Or make the uio attributes softlinks to pci sysfs somehow?

I understand your concern. Of course I'm also not in favor of duplicating code, but I don't think there's much overhead. PCI already exports all information needed by pci_resource_start(dev, bar) and pci_resource_len(dev, bar). Have a look at other UIO PCI drivers like uio_cif.c or uio_netx.c to see how it works. It's just a few more lines of code to loop through all BARs and add the to info->mem[i].

Quoted 5 lines

By the way, the current size of the info->mem[] array was exactly made with PCI in mind...

Thanks,
Hans

Well we also need to check resource type and probably iomem_is_exclusive.

Signature · 1 line
MST
#5 lore raw
In reply to Michael S. Tsirkin

Am Freitag, den 13.07.2012, 02:16 +0300 schrieb Michael S. Tsirkin:

Quoted 3 lines

My concern was people will ask for more and more stuff that pci sysfs already has.
If we do add these is there a way to not duplicate code from pci?

I have some concerns about the placing for the BAR mapping code inside the kernel. The point is, that sysfs currently makes it possible to map BARs of all card which are handled by any driver. This is fine in case of UIO, because it is intended that a user-space program maps BARs, but it is also possible to map BARs that are already handle by a kernel driver. It i therefore possible to jam the system by confusing sysfs entries.

I don't know which implications this has, but I would move the BAR mapping capabilities completely to UIO. This should ensure that only BARs can be mapped, which are handled by UIO and no other kernel-space driver.

Signature · 9 lines
Gruß
  Dominic

Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany

Phone:  +49 69 79844114
maintainer
#6 lore raw
Quoted 13 lines · Dominic Eschweiler

Am Freitag, den 13.07.2012, 02:16 +0300 schrieb Michael S. Tsirkin:

Quoted 3 lines

My concern was people will ask for more and more stuff that pci sysfs already has.
If we do add these is there a way to not duplicate code from pci?

I have some concerns about the placing for the BAR mapping code inside the kernel. The point is, that sysfs currently makes it possible to map BARs of all card which are handled by any driver. This is fine in case of UIO, because it is intended that a user-space program maps BARs, but it is also possible to map BARs that are already handle by a kernel driver. It i therefore possible to jam the system by confusing sysfs entries.

Quoted 4 lines

I don't know which implications this has, but I would move the BAR mapping capabilities completely to UIO. This should ensure that only BARs can be mapped, which are handled by UIO and no other kernel-space driver.

Could you give an example of the problem? How do you bind both UIO and another driver to the same device?

Signature · 1 line
MST
#7 lore raw
Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Dominic Eschweiler

Am Freitag, den 13.07.2012, 02:16 +0300 schrieb Michael S. Tsirkin:

Quoted 3 lines

My concern was people will ask for more and more stuff that pci sysfs already has.
If we do add these is there a way to not duplicate code from pci?

I have some concerns about the placing for the BAR mapping code inside the kernel. The point is, that sysfs currently makes it possible to map BARs of all card which are handled by any driver. This is fine in case of UIO, because it is intended that a user-space program maps BARs, but it is also possible to map BARs that are already handle by a kernel driver. It i therefore possible to jam the system by confusing sysfs entries.

Quoted 7 lines
Quoted 4 lines

I don't know which implications this has, but I would move the BAR mapping capabilities completely to UIO. This should ensure that only BARs can be mapped, which are handled by UIO and no other kernel-space driver.

Could you give an example of the problem? How do you bind both UIO and another driver to the same device?

You don't. If you already have a driver for your PCI device, why would you want to write a UIO driver? The sysfs files we were talking about are generated by the PCI core, which is not a driver. They simply reflect what the kernel can find out about the device.

If somebody maps the card's memory through the UIO driver and somebody else also maps it using sysfs, that is possible. What happens if both write to the same hardware registers is a different topic. Writing a driver implies some responsibility, even if it's done in userspace.

Thanks,
Hans

#8 lore raw

Am Freitag, den 13.07.2012, 16:18 +0200 schrieb Hans J. Koch:

Quoted 4 lines

If somebody maps the card's memory through the UIO driver and somebody else also maps it using sysfs, that is possible. What happens if both write to the same hardware registers is a different topic. Writing a driver implies some responsibility, even if it's done in userspace.

That's true and I would not complain. I'm just saying that there is the possibility to disturb a kernel driver by mapping memory etc. I think it is worth to considerate moving the BAR mapping code to UIO. Especially, when I look at the discussions about what effects mappable DMA memory can have on the system security.

Signature · 2 lines
Gruß
  Dominic
#9 lore raw
In reply to Michael S. Tsirkin

Am Freitag, den 13.07.2012, 16:22 +0300 schrieb Michael S. Tsirkin:

Quoted 2 lines

Could you give an example of the problem? How do you bind both UIO and another driver to the same device?

Sorry, I'm looking on it from the user-space perspective. Maybe I'm wrong, but I can give you an example :

lspci -v
...
03:00.0 Network controller: Broadcom Corporation BCM4331 802.11a/b/g/n (rev 02)
Subsystem: Apple Inc. AirPort Extreme
Flags: bus master, fast devsel, latency 0, IRQ 17 Memory at b0600000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [40] Power Management version 3
Capabilities: [58] Vendor Specific Information: Len=78 <?>
Capabilities: [48] MSI: Enable- Count=1/1 Maskable- 64bit+
Capabilities: [d0] Express Endpoint, MSI 00
Capabilities: [100] Advanced Error Reporting
Capabilities: [13c] Virtual Channel
Capabilities: [160] Device Serial Number 85-f2-6d-ff-ff-42-68-a8
Capabilities: [16c] Power Budgeting <?>
Kernel driver in use: bcma-pci-bridge
...

This Device has one 64 Bit Bar. When I look at the related sysfs entry ...

ls /sys/bus/pci/devices/0000:03:00.0
...
--w------- 1 root root 4.0K Jul 13 16:35 reset -r--r--r-- 1 root root 4.0K Jul 12 21:43 resource -rw------- 1 root root 16K Jul 13 16:35 resource0
lrwxrwxrwx 1 root root 0 Jul 12 23:41 subsystem
-> ../../../../bus/pci
...

... I can see that it should be possible to map resource0 and directly write into a BAR which is already managed by a kernel drivers.

Moving this functionality to UIO would only generate those resource files, if the device is handled by UIO and therefore intended to be managed from the user-space.

Signature · 2 lines
Gruß
  Dominic
maintainer
#10 lore raw
Quoted 38 lines · Dominic Eschweiler

Am Freitag, den 13.07.2012, 16:22 +0300 schrieb Michael S. Tsirkin:

Quoted 2 lines

Could you give an example of the problem? How do you bind both UIO and another driver to the same device?

Sorry, I'm looking on it from the user-space perspective. Maybe I'm wrong, but I can give you an example :

lspci -v
...
03:00.0 Network controller: Broadcom Corporation BCM4331 802.11a/b/g/n (rev 02)
Subsystem: Apple Inc. AirPort Extreme
Flags: bus master, fast devsel, latency 0, IRQ 17 Memory at b0600000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [40] Power Management version 3
Capabilities: [58] Vendor Specific Information: Len=78 <?>
Capabilities: [48] MSI: Enable- Count=1/1 Maskable- 64bit+
Capabilities: [d0] Express Endpoint, MSI 00
Capabilities: [100] Advanced Error Reporting
Capabilities: [13c] Virtual Channel
Capabilities: [160] Device Serial Number 85-f2-6d-ff-ff-42-68-a8
Capabilities: [16c] Power Budgeting <?>
Kernel driver in use: bcma-pci-bridge
...

This Device has one 64 Bit Bar. When I look at the related sysfs entry ...

ls /sys/bus/pci/devices/0000:03:00.0
...
--w------- 1 root root 4.0K Jul 13 16:35 reset

(quoted diff) +0 -2
-r--r--r-- 1 root root 4.0K Jul 12 21:43 resource
-rw------- 1 root root  16K Jul 13 16:35 resource0

lrwxrwxrwx 1 root root 0 Jul 12 23:41 subsystem
-> ../../../../bus/pci
...

Quoted 6 lines

... I can see that it should be possible to map resource0 and directly write into a BAR which is already managed by a kernel drivers.

Moving this functionality to UIO would only generate those resource files, if the device is handled by UIO and therefore intended to be managed from the user-space.

UIO has the same property, doesn't it? Multiple users can access device memory through sysfs.

Quoted 3 lines
Signature · 2 lines
Gruß
  Dominic
#11 lore raw

Am Freitag, den 13.07.2012, 21:19 +0300 schrieb Michael S. Tsirkin:

Quoted 3 lines

UIO has the same property, doesn't it? Multiple users can access device memory through sysfs.

Indeed, that's a similar problem. I haven't tried it (yet), but this particular problem can maybe circumvented by using mmap with the MAP_PRIVATE flag. Doing so is the responsibility of the driver programmer (like Hans already said). Even if that mmap trick does not work, it is pretty much sure that a BAR is already used by another program, if a related kernel driver is loaded. In that case the kernel has a chance to avoid such BAR race conditions by not giving the possibility to map them to the userspace.

Nevertheless, I'm pretty sure that the possibility via sysfs to access BARs, which are already managed by a kernel driver, opens the door for denial of service attacks.

On the other hand, I'm quite a newbie on this topic and maybe I don't see the big picture here. Therefore it is up to you guys to make the right decision (if needed).

Signature · 2 lines
Gruß
  Dominic
#12 lore raw
Quoted 6 lines · Dominic Eschweiler

Am Freitag, den 13.07.2012, 21:19 +0300 schrieb Michael S. Tsirkin:

Quoted 3 lines

UIO has the same property, doesn't it? Multiple users can access device memory through sysfs.

Quoted 8 lines

Indeed, that's a similar problem. I haven't tried it (yet), but this particular problem can maybe circumvented by using mmap with the MAP_PRIVATE flag. Doing so is the responsibility of the driver programmer (like Hans already said). Even if that mmap trick does not work, it is pretty much sure that a BAR is already used by another program, if a related kernel driver is loaded. In that case the kernel has a chance to avoid such BAR race conditions by not giving the possibility to map them to the userspace.

Don't make it more complicated than it is. I see no general problem in mapping BARs in uio_pci_generic like in any other UIO PCI driver.

Quoted 4 lines

Nevertheless, I'm pretty sure that the possibility via sysfs to access BARs, which are already managed by a kernel driver, opens the door for denial of service attacks.

There are also a few other possible attack scenarios, depending on the hardware.
That's a general problem of all userspace drivers (e.g. X graphics drivers). You have to make sure access rights are correct. Making a UIO device node available to all normal users is foolishly dangerous.

Quoted 4 lines

On the other hand, I'm quite a newbie on this topic and maybe I don't see the big picture here. Therefore it is up to you guys to make the right decision (if needed).

Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c and post it for review.

Thanks,
Hans

#13 lore raw

Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:

Quoted 3 lines

Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c and post it for review.

Here we go ...

Quoted 1 line
Signed-off-by Dominic Eschweiler <[email protected]>
(quoted diff) +0 -0

b/drivers/uio/uio_pci_generic.c
index 0bd08ef..e25991e 100644

drivers/uio/uio_pci_generic.c +5 -3
@@ -25,10 +25,12 @@
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
 
-#define DRIVER_VERSION	"0.01.0"
+#define DRIVER_VERSION	"0.02.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <[email protected]>"
 #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
 
+#define DRV_NAME "uio_pci_generic"
+
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;

@@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
 {
 	struct uio_pci_generic_dev *gdev;
 	int err;
+	int i;
 
 	err = pci_enable_device(pdev);
 	if (err) {

@@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
 	}
 
 	if (!pdev->irq) {
-		dev_warn(&pdev->dev, "No IRQ assigned to device: "
-			 "no support for interrupts?\n");
+		dev_warn(&pdev->dev, "No IRQ assigned to device: no support for

interrupts?\n");
pci_disable_device(pdev);
return -ENODEV;
}
@@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
gdev->info.handler = irqhandler;
gdev->pdev = pdev;

+ /* request regions */
+ err = pci_request_regions(pdev, DRV_NAME);
+ if (err) {
+ dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
+ return err;
+ }
+
+ /* create attributes for BAR mappings */
+ for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+ if (pdev->resource[i].flags &&
+ (pdev->resource[i].flags & IORESOURCE_MEM)) {
+ gdev->info.mem[i].addr = pci_resource_start(pdev, i);
+ gdev->info.mem[i].size = pci_resource_len(pdev, i);
+ gdev->info.mem[i].internal_addr = NULL;
+ gdev->info.mem[i].memtype = UIO_MEM_PHYS;
+ }
+ }
+
if (uio_register_device(&pdev->dev, &gdev->info))
goto err_register;
pci_set_drvdata(pdev, gdev);

+ pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",
+ pdev->vendor, pdev->device);
+
return 0;
err_register:
kfree(gdev);
@@ -107,17 +130,21 @@ err_verify:
static void remove(struct pci_dev *pdev)
{
struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
-
uio_unregister_device(&gdev->info);
+
+ pci_release_regions(pdev);
pci_disable_device(pdev);
kfree(gdev);
+
+ pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",
+ pdev->vendor, pdev->device);
}

static struct pci_driver driver = {
- .name = "uio_pci_generic",
+ .name = DRV_NAME,
.id_table = NULL, /* only dynamic id's */
- .probe = probe,
- .remove = remove,
+ .probe = probe,
+ .remove = remove,
};

static int __init init(void)

Signature · 9 lines
Gruß
  Dominic

Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany

Phone:  +49 69 79844114
#14 lore raw
Quoted 6 lines · Dominic Eschweiler

Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:

Quoted 3 lines

Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c and post it for review.

Here we go ...

Great! I'm a bit under time pressure with my current work ATM. A call to open("/dev/hansjkoch", O_NONBLOCK) only returns -EBUSY. So it'll take me one or two days to review it thoroughly. But at first sight, it is what I had in mind.

You'll hear from me soon, thanks for your work! Comments and reviews from others are welcome...

Hans

Quoted 111 lines
Quoted 1 line
Signed-off-by Dominic Eschweiler <[email protected]>
drivers/uio/uio_pci_generic.c +5 -3
index 0bd08ef..e25991e 100644
@@ -25,10 +25,12 @@
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
 
-#define DRIVER_VERSION	"0.01.0"
+#define DRIVER_VERSION	"0.02.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <[email protected]>"
 #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
 
+#define DRV_NAME "uio_pci_generic"
+
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;

@@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
 {
 	struct uio_pci_generic_dev *gdev;
 	int err;
+	int i;
 
 	err = pci_enable_device(pdev);
 	if (err) {

@@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
 	}
 
 	if (!pdev->irq) {
-		dev_warn(&pdev->dev, "No IRQ assigned to device: "
-			 "no support for interrupts?\n");
+		dev_warn(&pdev->dev, "No IRQ assigned to device: no support for
 interrupts?\n");
 		pci_disable_device(pdev);
 		return -ENODEV;
drivers/uio/uio_pci_generic.c +29 -4
 	}

@@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
 	gdev->info.handler = irqhandler;
 	gdev->pdev = pdev;
 
+	/* request regions */
+	err = pci_request_regions(pdev, DRV_NAME);
+	if (err) {
+		dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
+		return err;
+	}
+
+	/* create attributes for BAR mappings */
+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		if (pdev->resource[i].flags &&
+		(pdev->resource[i].flags & IORESOURCE_MEM)) {
+			gdev->info.mem[i].addr = pci_resource_start(pdev, i);
+			gdev->info.mem[i].size = pci_resource_len(pdev, i);
+			gdev->info.mem[i].internal_addr = NULL;
+			gdev->info.mem[i].memtype = UIO_MEM_PHYS;
+		}
+	}
+
 	if (uio_register_device(&pdev->dev, &gdev->info))
 		goto err_register;
 	pci_set_drvdata(pdev, gdev);
 
+	pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",
+	pdev->vendor, pdev->device);
+
 	return 0;
 err_register:
 	kfree(gdev);

@@ -107,17 +130,21 @@ err_verify:
 static void remove(struct pci_dev *pdev)
 {
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
-
 	uio_unregister_device(&gdev->info);
+
+	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
+
+	pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",
+	pdev->vendor, pdev->device);
 }
 
 static struct pci_driver driver = {
-	.name = "uio_pci_generic",
+	.name     = DRV_NAME,
 	.id_table = NULL, /* only dynamic id's */
-	.probe = probe,
-	.remove = remove,
+	.probe    = probe,
+	.remove   = remove,
 };
 
 static int __init init(void)
Signature · 10 lines
-- 
Gruß
  Dominic

Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany

Phone:  +49 69 79844114
#15 lore raw

Hello Hans

Am Donnerstag, den 19.07.2012, 01:47 +0200 schrieb Hans J. Koch:

Quoted 2 lines

You'll hear from me soon, thanks for your work! Comments and reviews from others are welcome...

Is there any progress on this topic?

Signature · 9 lines
Gruß
  Dominic

Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany

Phone:  +49 69 79844114
#16 lore raw
In reply to Dominic Eschweiler
Quoted 6 lines · Dominic Eschweiler

Am Montag, den 16.07.2012, 23:58 +0200 schrieb Hans J. Koch:

Quoted 3 lines

Try to hack up a patch to add generic BAR mapping to uio_pci_generic.c and post it for review.

Here we go ...

Thank you very much for your work. I'm really sorry for the long delay, but I was busy finishing a project because I go to vacation tomorrow. Sorry, that might cause further delay since I don't know yet how often I can read my mail...
Greg, can you review the next one?

Here's a first review.

Thanks,
Hans

Quoted 30 lines
Quoted 1 line
Signed-off-by Dominic Eschweiler <[email protected]>
drivers/uio/uio_pci_generic.c +4 -1
index 0bd08ef..e25991e 100644
@@ -25,10 +25,12 @@
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
 
-#define DRIVER_VERSION	"0.01.0"
+#define DRIVER_VERSION	"0.02.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <[email protected]>"
 #define DRIVER_DESC	"Generic UIO driver for PCI 2.3 devices"
 
+#define DRV_NAME "uio_pci_generic"
+
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;

@@ -58,6 +60,7 @@ static int __devinit probe(struct pci_dev *pdev,
 {
 	struct uio_pci_generic_dev *gdev;
 	int err;
+	int i;
 
 	err = pci_enable_device(pdev);
 	if (err) {

drivers/uio/uio_pci_generic.c static int __devinit probe(struct pci_dev *pdev, · line 74

Quoted 7 lines
drivers/uio/uio_pci_generic.c +1 -2
@@ -67,8 +70,7 @@ static int __devinit probe(struct pci_dev *pdev,
 	}
 
 	if (!pdev->irq) {
-		dev_warn(&pdev->dev, "No IRQ assigned to device: "
-			 "no support for interrupts?\n");
+		dev_warn(&pdev->dev, "No IRQ assigned to device: no support for
 interrupts?\n");

Please configure your mail client not to break lines when sending a patch. It can't be applied like this.

Why did you make that change anyway? If it's just coding style, please send another patch, don't mix functional changes with coding style fixes.

Quoted 20 lines
drivers/uio/uio_pci_generic.c +0 -0
 		pci_disable_device(pdev);
 		return -ENODEV;
drivers/uio/uio_pci_generic.c +13 -0
 	}

@@ -91,10 +93,31 @@ static int __devinit probe(struct pci_dev *pdev,
 	gdev->info.handler = irqhandler;
 	gdev->pdev = pdev;
 
+	/* request regions */
+	err = pci_request_regions(pdev, DRV_NAME);
+	if (err) {
+		dev_err(&pdev->dev, "Couldn't get PCI resources, aborting\n");
+		return err;
+	}
+
+	/* create attributes for BAR mappings */
+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		if (pdev->resource[i].flags &&
+		(pdev->resource[i].flags & IORESOURCE_MEM)) {
+			gdev->info.mem[i].addr = pci_resource_start(pdev, i);
+			gdev->info.mem[i].size = pci_resource_len(pdev, i);

drivers/uio/uio_pci_generic.c static int __devinit probe(struct pci_dev *pdev, · line 118

Quoted 10 lines
drivers/uio/uio_pci_generic.c +6 -0
+			gdev->info.mem[i].internal_addr = NULL;
+			gdev->info.mem[i].memtype = UIO_MEM_PHYS;
+		}
+	}
+
 	if (uio_register_device(&pdev->dev, &gdev->info))
 		goto err_register;
 	pci_set_drvdata(pdev, gdev);
 
+	pr_info("UIO_PCI_GENERIC : initialized new device (%x %x)\n",

Please use dev_info()

Quoted 7 lines
drivers/uio/uio_pci_generic.c +2 -0
+	pdev->vendor, pdev->device);
+
 	return 0;
 err_register:
 	kfree(gdev);

@@ -107,17 +130,21 @@ err_verify:
 static void remove(struct pci_dev *pdev)

drivers/uio/uio_pci_generic.c err_verify: · line 139

Quoted 10 lines
drivers/uio/uio_pci_generic.c +4 -1
 {
 	struct uio_pci_generic_dev *gdev = pci_get_drvdata(pdev);
-
 	uio_unregister_device(&gdev->info);
+
+	pci_release_regions(pdev);
 	pci_disable_device(pdev);
 	kfree(gdev);
+
+	pr_info("UIO_PCI_GENERIC : removed device (%x %x)\n",

ditto

drivers/uio/uio_pci_generic.c err_verify: · line 147

Quoted 11 lines
drivers/uio/uio_pci_generic.c +4 -3
+	pdev->vendor, pdev->device);
 }
 
 static struct pci_driver driver = {
-	.name = "uio_pci_generic",
+	.name     = DRV_NAME,
 	.id_table = NULL, /* only dynamic id's */
-	.probe = probe,
-	.remove = remove,
+	.probe    = probe,
+	.remove   = remove,

As above: Please put coding style fixes in an extra patch (if you really insist on tabs instead of spaces...)

Quoted 16 lines
drivers/uio/uio_pci_generic.c +0 -0
 };
 
 static int __init init(void)
Signature · 10 lines
-- 
Gruß
  Dominic

Frankfurt Institute for Advanced Studies (FIAS)
Ruth-Moufang-Straße 1
D-60438 Frankfurt am Main
Germany

Phone:  +49 69 79844114