From: Michal Sojka <sojkam1@fel.cvut.cz>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org, lisovy@gmail.com
Subject: Re: [PATCH 1/3] uio: Allow handling of non page-aligned memory regions
In-Reply-To: <20170316082012.GA23493@kroah.com>
References: <20170307140948.30099-1-sojkam1@fel.cvut.cz> <20170316082012.GA23493@kroah.com>
Date: Thu, 16 Mar 2017 13:45:50 +0100
Message-ID: <87k27pqtj5.fsf@steelpick.2x.cz>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3335
Lines: 83

On Thu, Mar 16 2017, Greg KH wrote:
> On Tue, Mar 07, 2017 at 03:09:46PM +0100, Michal Sojka wrote:
>> Since commit b65502879556 ("uio: we cannot mmap unaligned page
>> contents") addresses and sizes of UIO memory regions must be
>> page-aligned. If the address in the BAR register is not page-aligned,
>> the mentioned commit forces the UIO driver to round the address down
>> to the page size. Then, there is no easy way for user-space to learn
>> the offset of the actual memory region within the page, because the
>> offset seen in the sysfs is calculated from the rounded address and
>> thus it is always zero.
>> 
>> Fix that problem by including the offset in struct uio_mem. UIO
>> drivers can set this field and its value is reported via sysfs.
>
> It is, where?

/sys/class/uio/uio0/maps/map0/offset

>> 
>> Drivers for hardware with page-aligned BARs need not to be modified
>> provided that they initialize struct uio_info with zeros.
>> 
>> Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
>> ---
>>  drivers/uio/uio.c          |  2 +-
>>  include/linux/uio_driver.h | 13 ++++++++-----
>>  2 files changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index fba021f5736a..27c329131350 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -66,7 +66,7 @@ static ssize_t map_size_show(struct uio_mem *mem, char *buf)
>>  
>>  static ssize_t map_offset_show(struct uio_mem *mem, char *buf)
>>  {
>> -	return sprintf(buf, "0x%llx\n", (unsigned long long)mem->addr & ~PAGE_MASK);
>> +	return sprintf(buf, "0x%llx\n", (unsigned long long)mem->offs);
>
> All you changed was this value that sysfs shows.
>
>>  }
>>  
>>  struct map_sysfs_entry {
>> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
>> index 32c0e83d6239..89b53094f127 100644
>> --- a/include/linux/uio_driver.h
>> +++ b/include/linux/uio_driver.h
>> @@ -23,11 +23,13 @@ struct uio_map;
>>  /**
>>   * struct uio_mem - description of a UIO memory region
>>   * @name:		name of the memory region for identification
>> - * @addr:		address of the device's memory (phys_addr is used since
>> - * 			addr can be logical, virtual, or physical & phys_addr_t
>> - * 			should always be large enough to handle any of the
>> - * 			address types)
>> - * @size:		size of IO
>> + * @addr:               address of the device's memory rounded to page
>> + * 			size (phys_addr is used since addr can be
>> + * 			logical, virtual, or physical & phys_addr_t
>> + * 			should always be large enough to handle any of
>> + * 			the address types)
>> + * @offs:               offset of device memory within the page
>> + * @size:		size of IO (multiple of page size)
>>   * @memtype:		type of memory addr points to
>>   * @internal_addr:	ioremap-ped version of addr, for driver internal use
>>   * @map:		for use by the UIO core only.
>> @@ -35,6 +37,7 @@ struct uio_map;
>>  struct uio_mem {
>>  	const char		*name;
>>  	phys_addr_t		addr;
>> +	unsigned long		offs;;
>
> Did you really test this patch?  Why the two ;;?  And who sets this?
>
> I think you broke things :(

This is set in patch 3/3 and it works correctly on my hardware. It seems
like you would like to have patches 2/3 and 3/3 merged in a single one.
I'll send v2 in a while and address your other comments as well.

Thanks.
-Michal