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