Re: [PATCH] [UIO] Add alignment warnings for uio-mem

Previous thread: Re: New IOCTLs by Bodo Eggert on Thursday, September 18, 2008 - 7:54 am. (1 message)

Next thread: [PATCH 0/3] block changes for request-based dm-multipath by Kiyoshi Ueda on Thursday, September 18, 2008 - 7:43 am. (5 messages)
From: Wolfram Sang
Date: Thursday, September 18, 2008 - 7:46 am

mmap works page aligned. If uio-mem areas were set up unaligned, mmap
would silently align it and the corresponding attributes in sysfs would
not reflect it. This patch fixes such values during init to what the
kernel will do anyway and adds a warning.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/uio/uio.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Index: drivers/uio/uio.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- drivers/uio/uio.c.orig
+++ drivers/uio/uio.c
@@ -656,6 +656,8 @@
 			  struct uio_info *info)
 {
 	struct uio_device *idev;
+	struct uio_mem *mem;
+	unsigned int offset;
 	int ret =3D 0;
=20
 	if (!parent || !info || !info->name || !info->version)
@@ -691,6 +693,16 @@
 		goto err_device_create;
 	}
=20
+	for (mem =3D info->mem; mem->size; mem++) {
+		offset =3D mem->addr & ~PAGE_MASK;
+		if (offset) {
+			mem->addr -=3D offset;
+			mem->size +=3D offset;
+			dev_warn(idev->dev, "mem[%d] not page aligned!"
+				"Fixing values.\n", mem - info->mem);
+		}
+	}
+
 	ret =3D uio_dev_add_attributes(idev);
 	if (ret)
 		goto err_uio_dev_add_attributes;
--=20
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
From: Andreas Schwab
Date: Thursday, September 18, 2008 - 8:03 am

Minor nit: please add a space to the message.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--

From: Hans J. Koch
Date: Thursday, September 18, 2008 - 2:53 pm

Hi Wolfram,
thanks for mentioning this. I already know of one case where such a
situation lead to ugly workarounds, so the problem needs to be
addressed. However, I'd like to choose a different approach. Let's take
the following situation as an example:

- A chip with 16 bytes of registers
- Its base address is 0x12345080 (4k page size, not aligned)

With your approach, sysfs would report a base address of 0x12345000 and
a size of 0x90. Both is a lie. We don't want to encourage the user to
access addresses outside the 16 bytes range the driver author originally
announced.

UIO drivers are often used in embedded devices, where developers usually
know the physical addresses of their devices and have them hardcoded in
a #define. It's confusing if sysfs suddenly reports something different.

The userspace part of the driver expects a 16 bytes range but is told
there are 0x90 bytes at his disposal. It has to guess where the devices
registers are (or if this is a completely different device...). It might
also check the physical base address and find that this is wrong, too.

The situation becomes even worse if you have two such chips on your
board, and each reports a different size. If their addresses were in the
same page, both would have the same base address, but different sizes...

Another issue: You print a warning if a mem->addr is not page aligned.
Why? Either the driver (both kernel and userspace) can handle this, or
it can't. In the first case, nothing needs to be printed, in the second
case it's not a warning but an error. IMO a warning only makes sense if
there's something the driver author can do to fix it. But if you've got
hardware with certain addresses, there's usually nothing you can do.

I'd like to suggest the following solution (patch at the end of this
mail):

Let's leave it to userspace. All userspace needs is the "offset"
information you calculate in your patch. If we add a new sysfs attribute
for that, userspace can simply add it to the address ...
From: Wolfram Sang
Date: Friday, September 19, 2008 - 1:49 am

Hello Hans,


True. It's actually quite dangerous to be able to write outside that
region at all, but I guess this can't be helped due to the nature of


I'm perfectly fine with this approach. I tested it and it worked as
expected. The only thing I'd like to add is that 'offset' could be
renamed to 'map_offset' or 'mmap_offset' to be a bit more precise what

You're welcome. Thanks for dealing with the issue.

All the best,

   Wolfram

--=20
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
Previous thread: Re: New IOCTLs by Bodo Eggert on Thursday, September 18, 2008 - 7:54 am. (1 message)

Next thread: