Messages in this thread | | | Date | Fri, 19 Sep 2008 10:49:14 +0200 | From | Wolfram Sang <> | Subject | Re: [PATCH] [UIO] Add alignment warnings for uio-mem |
| |
Hello Hans,
On Thu, Sep 18, 2008 at 11:53:18PM +0200, Hans J. Koch wrote:
> 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.
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 mmap.
> 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...
ACK.
> 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 returned by mmap(). > This way, the userspace part of the driver will always work, no matter > if the memory is page aligned or not. The "addr" and "size" attributes > still report the true physical base address and size. _Every_ userspace > driver, existing or yet to be written, can (and should) simply do > something like > > base_addr = address_returned_by_mmap + offset_from_sysfs; > > What do you think about this? If you think this might work for you, > could you please test the patch below? I haven't got such a hardware > handy at the moment, so my patch is just compile-tested (but it looks > obvious).
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 this value is about.
> Thanks again for pointing this out,
You're welcome. Thanks for dealing with the issue.
All the best,
Wolfram
-- Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de Pengutronix - Linux Solutions for Science and Industry [unhandled content-type:application/pgp-signature] | |