Re: [PATCH 1/3] uio: Allow handling of non page-aligned memory regions

[Greg KH]

On Thu, Mar 16, 2017 at 01:45:50PM +0100, Michal Sojka wrote:
> 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

Did you change the Documentation/ABI entry for it?

> >> 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@xxxxxxxxxxx>
> >> ---
> >>  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.

No, I don't want 2 and 3 merged together, I want you to justify what you
are doing in 2 better (hint, I know what it is, but you need to do the
work...)

And I missed that this was set in 3, so you need to describe things a
bit better please.

thanks,

greg k-h