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

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

[Michal Sojka]

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.

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);
 }
 
 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;;
 	resource_size_t		size;
 	int			memtype;
 	void __iomem		*internal_addr;
-- 
2.11.0

[PATCH 2/3] uio_mf624: Refactor memory info initialization

[Michal Sojka]

No functional changes.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 drivers/uio/uio_mf624.c | 44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/uio/uio_mf624.c b/drivers/uio/uio_mf624.c
index d1f95a1567bb..8f30fa1af2ab 100644
--- a/drivers/uio/uio_mf624.c
+++ b/drivers/uio/uio_mf624.c
@@ -127,6 +127,20 @@ static int mf624_irqcontrol(struct uio_info *info, s32 irq_on)
 	return 0;
 }
 
+static int mf624_setup_mem(struct pci_dev *dev, int bar, struct uio_mem *mem, const char *name)
+{
+	mem->name = name;
+	mem->addr = pci_resource_start(dev, bar);
+	if (!mem->addr)
+		return -ENODEV;
+	mem->size = pci_resource_len(dev, bar);
+	mem->memtype = UIO_MEM_PHYS;
+	mem->internal_addr = pci_ioremap_bar(dev, bar);
+	if (!mem->internal_addr)
+		return -ENODEV;
+	return 0;
+}
+
 static int mf624_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	struct uio_info *info;
@@ -147,37 +161,15 @@ static int mf624_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* Note: Datasheet says device uses BAR0, BAR1, BAR2 -- do not trust it */
 
 	/* BAR0 */
-	info->mem[0].name = "PCI chipset, interrupts, status "
-			"bits, special functions";
-	info->mem[0].addr = pci_resource_start(dev, 0);
-	if (!info->mem[0].addr)
+	if (mf624_setup_mem(dev, 0, &info->mem[0], "PCI chipset, interrupts, status "
+			    "bits, special functions"))
 		goto out_release;
-	info->mem[0].size = pci_resource_len(dev, 0);
-	info->mem[0].memtype = UIO_MEM_PHYS;
-	info->mem[0].internal_addr = pci_ioremap_bar(dev, 0);
-	if (!info->mem[0].internal_addr)
-		goto out_release;
-
 	/* BAR2 */
-	info->mem[1].name = "ADC, DAC, DIO";
-	info->mem[1].addr = pci_resource_start(dev, 2);
-	if (!info->mem[1].addr)
-		goto out_unmap0;
-	info->mem[1].size = pci_resource_len(dev, 2);
-	info->mem[1].memtype = UIO_MEM_PHYS;
-	info->mem[1].internal_addr = pci_ioremap_bar(dev, 2);
-	if (!info->mem[1].internal_addr)
+	if (mf624_setup_mem(dev, 2, &info->mem[1], "ADC, DAC, DIO"))
 		goto out_unmap0;
 
 	/* BAR4 */
-	info->mem[2].name = "Counter/timer chip";
-	info->mem[2].addr = pci_resource_start(dev, 4);
-	if (!info->mem[2].addr)
-		goto out_unmap1;
-	info->mem[2].size = pci_resource_len(dev, 4);
-	info->mem[2].memtype = UIO_MEM_PHYS;
-	info->mem[2].internal_addr = pci_ioremap_bar(dev, 4);
-	if (!info->mem[2].internal_addr)
+	if (mf624_setup_mem(dev, 4, &info->mem[2], "Counter/timer chip"))
 		goto out_unmap1;
 
 	info->irq = dev->irq;
-- 
2.11.0

[PATCH 3/3] uio_mf624: Align memory regions to page size

[Michal Sojka]

Without that, one cannot mmap() the registers to user-space, at least since
commit b65502879556 ("uio: we cannot mmap unaligned page contents").

Tested with real mf624 card.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 drivers/uio/uio_mf624.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/uio/uio_mf624.c b/drivers/uio/uio_mf624.c
index 8f30fa1af2ab..35187c052e8c 100644
--- a/drivers/uio/uio_mf624.c
+++ b/drivers/uio/uio_mf624.c
@@ -129,11 +129,15 @@ static int mf624_irqcontrol(struct uio_info *info, s32 irq_on)
 
 static int mf624_setup_mem(struct pci_dev *dev, int bar, struct uio_mem *mem, const char *name)
 {
+	resource_size_t start = pci_resource_start(dev, bar);
+	resource_size_t len = pci_resource_len(dev, bar);
+
 	mem->name = name;
-	mem->addr = pci_resource_start(dev, bar);
+	mem->addr = start & PAGE_MASK;
+	mem->offs = start & ~PAGE_MASK;
 	if (!mem->addr)
 		return -ENODEV;
-	mem->size = pci_resource_len(dev, bar);
+	mem->size = ((start & ~PAGE_MASK) + len + PAGE_SIZE - 1) & PAGE_MASK;
 	mem->memtype = UIO_MEM_PHYS;
 	mem->internal_addr = pci_ioremap_bar(dev, bar);
 	if (!mem->internal_addr)
-- 
2.11.0

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

[Greg KH]

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?

> 
> 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 :(

thanks,

greg k-h

Re: [PATCH 2/3] uio_mf624: Refactor memory info initialization

[Greg KH]

On Tue, Mar 07, 2017 at 03:09:47PM +0100, Michal Sojka wrote:
> No functional changes.

Then why did you do it?  Please be descriptive.

thanks,

greg k-h

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

[Michal Sojka]

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

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

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

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

[Michal Sojka]

On Thu, Mar 16 2017, Greg KH wrote:
> 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?

No, because it seems that UIO is not documented there.

-Michal

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

[Michal Sojka]

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 (which is the case of the mf264 card), 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
/sys/class/uio/uio?/maps/map?/offset 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 userspace can read its value from
/sys/class/uio/uio?/maps/map?/offset.

The following commits update the uio_mf264 driver to set this new offs
field.

Drivers for hardware with page-aligned BARs need not to be modified
provided that they initialize struct uio_info (which contains uio_mem)
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);
 }
 
 struct map_sysfs_entry {
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 32c0e83d6239..3c85c81b0027 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;
 	resource_size_t		size;
 	int			memtype;
 	void __iomem		*internal_addr;
-- 
2.11.0

[PATCH v2 2/3] uio_mf624: Refactor memory info initialization

[Michal Sojka]

No functional changes. Move initialization of struct uio_mem to a
function. This will allow the next commit to change the initialization
code at a single place rather that at three different places.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 drivers/uio/uio_mf624.c | 44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/uio/uio_mf624.c b/drivers/uio/uio_mf624.c
index d1f95a1567bb..8f30fa1af2ab 100644
--- a/drivers/uio/uio_mf624.c
+++ b/drivers/uio/uio_mf624.c
@@ -127,6 +127,20 @@ static int mf624_irqcontrol(struct uio_info *info, s32 irq_on)
 	return 0;
 }
 
+static int mf624_setup_mem(struct pci_dev *dev, int bar, struct uio_mem *mem, const char *name)
+{
+	mem->name = name;
+	mem->addr = pci_resource_start(dev, bar);
+	if (!mem->addr)
+		return -ENODEV;
+	mem->size = pci_resource_len(dev, bar);
+	mem->memtype = UIO_MEM_PHYS;
+	mem->internal_addr = pci_ioremap_bar(dev, bar);
+	if (!mem->internal_addr)
+		return -ENODEV;
+	return 0;
+}
+
 static int mf624_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	struct uio_info *info;
@@ -147,37 +161,15 @@ static int mf624_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* Note: Datasheet says device uses BAR0, BAR1, BAR2 -- do not trust it */
 
 	/* BAR0 */
-	info->mem[0].name = "PCI chipset, interrupts, status "
-			"bits, special functions";
-	info->mem[0].addr = pci_resource_start(dev, 0);
-	if (!info->mem[0].addr)
+	if (mf624_setup_mem(dev, 0, &info->mem[0], "PCI chipset, interrupts, status "
+			    "bits, special functions"))
 		goto out_release;
-	info->mem[0].size = pci_resource_len(dev, 0);
-	info->mem[0].memtype = UIO_MEM_PHYS;
-	info->mem[0].internal_addr = pci_ioremap_bar(dev, 0);
-	if (!info->mem[0].internal_addr)
-		goto out_release;
-
 	/* BAR2 */
-	info->mem[1].name = "ADC, DAC, DIO";
-	info->mem[1].addr = pci_resource_start(dev, 2);
-	if (!info->mem[1].addr)
-		goto out_unmap0;
-	info->mem[1].size = pci_resource_len(dev, 2);
-	info->mem[1].memtype = UIO_MEM_PHYS;
-	info->mem[1].internal_addr = pci_ioremap_bar(dev, 2);
-	if (!info->mem[1].internal_addr)
+	if (mf624_setup_mem(dev, 2, &info->mem[1], "ADC, DAC, DIO"))
 		goto out_unmap0;
 
 	/* BAR4 */
-	info->mem[2].name = "Counter/timer chip";
-	info->mem[2].addr = pci_resource_start(dev, 4);
-	if (!info->mem[2].addr)
-		goto out_unmap1;
-	info->mem[2].size = pci_resource_len(dev, 4);
-	info->mem[2].memtype = UIO_MEM_PHYS;
-	info->mem[2].internal_addr = pci_ioremap_bar(dev, 4);
-	if (!info->mem[2].internal_addr)
+	if (mf624_setup_mem(dev, 4, &info->mem[2], "Counter/timer chip"))
 		goto out_unmap1;
 
 	info->irq = dev->irq;
-- 
2.11.0

[PATCH v2 3/3] uio_mf624: Align memory regions to page size and set correct offsets

[Michal Sojka]

mf624 card has its registers not aligned to pages. Since commit
b65502879556 ("uio: we cannot mmap unaligned page contents") mmap()ing
mf624 registers fails, because now the uio drivers must set
uio_mem->addr to be page-aligned.

We align the address here and set the newly introduced offs field to
the offset of the mf264 registers within the page so that userspace
can find the address of the mmap()ed register by reading
/sys/class/uio/uio?/maps/map?/offset.

Tested with real mf624 card.

Signed-off-by: Michal Sojka <sojkam1@fel.cvut.cz>
---
 drivers/uio/uio_mf624.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/uio/uio_mf624.c b/drivers/uio/uio_mf624.c
index 8f30fa1af2ab..35187c052e8c 100644
--- a/drivers/uio/uio_mf624.c
+++ b/drivers/uio/uio_mf624.c
@@ -129,11 +129,15 @@ static int mf624_irqcontrol(struct uio_info *info, s32 irq_on)
 
 static int mf624_setup_mem(struct pci_dev *dev, int bar, struct uio_mem *mem, const char *name)
 {
+	resource_size_t start = pci_resource_start(dev, bar);
+	resource_size_t len = pci_resource_len(dev, bar);
+
 	mem->name = name;
-	mem->addr = pci_resource_start(dev, bar);
+	mem->addr = start & PAGE_MASK;
+	mem->offs = start & ~PAGE_MASK;
 	if (!mem->addr)
 		return -ENODEV;
-	mem->size = pci_resource_len(dev, bar);
+	mem->size = ((start & ~PAGE_MASK) + len + PAGE_SIZE - 1) & PAGE_MASK;
 	mem->memtype = UIO_MEM_PHYS;
 	mem->internal_addr = pci_ioremap_bar(dev, bar);
 	if (!mem->internal_addr)
-- 
2.11.0

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

[Greg KH]

On Thu, Mar 16, 2017 at 02:33:25PM +0100, Michal Sojka wrote:
> On Thu, Mar 16 2017, Greg KH wrote:
> > 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?
> 
> No, because it seems that UIO is not documented there.

Wow, it really isn't, that sucks, I messed up :(

But, the uio-howto.rst file should probably be updated, right?

thanks,

greg k-h

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

[Michal Sojka]

On Thu, Mar 16 2017, Greg KH wrote:
> On Thu, Mar 16, 2017 at 02:33:25PM +0100, Michal Sojka wrote:
>> On Thu, Mar 16 2017, Greg KH wrote:
>> > 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?
>> 
>> No, because it seems that UIO is not documented there.
>
> Wow, it really isn't, that sucks, I messed up :(
>
> But, the uio-howto.rst file should probably be updated, right?

I checked that and the document is correct. The relevant part is:

> -  ``offset``: The offset, in bytes, that has to be added to the pointer
>    returned by :c:func:`mmap()` to get to the actual device memory.
>    This is important if the device's memory is not page aligned.
>    Remember that pointers returned by :c:func:`mmap()` are always
>    page aligned, so it is good style to always add this offset.

So, as described in the commit, the only problem was that without my
patch the offset has either been zero or mmap() failed. Now, mmap() can
succeed even if offset is non-zero.

-Michal