[PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Luck, Tony]

Callers to __acpi_ioremap_fast() pass the bit_width that they found in the
acpi_generic_address structure. Convert from bits to bytes when passing to
__acpi_find_iomap() - as it wants to see bytes, not bits.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/acpi/atomicio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
index 252888f..70ffb71 100644
--- a/drivers/acpi/atomicio.c
+++ b/drivers/acpi/atomicio.c
@@ -78,7 +78,7 @@ static void __iomem *__acpi_ioremap_fast(phys_addr_t paddr,
 {
 	struct acpi_iomap *map;
 
-	map = __acpi_find_iomap(paddr, size);
+	map = __acpi_find_iomap(paddr, size/8);
 	if (map)
 		return map->vaddr + (paddr - map->paddr);
 	else
-- 
1.7.3.1

Re: [PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Huang Ying]

Sorry for late.

On 10/22/2011 05:42 AM, Luck, Tony wrote:
> Callers to __acpi_ioremap_fast() pass the bit_width that they found in the
> acpi_generic_address structure. Convert from bits to bytes when passing to
> __acpi_find_iomap() - as it wants to see bytes, not bits.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  drivers/acpi/atomicio.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
> index 252888f..70ffb71 100644
> --- a/drivers/acpi/atomicio.c
> +++ b/drivers/acpi/atomicio.c
> @@ -78,7 +78,7 @@ static void __iomem *__acpi_ioremap_fast(phys_addr_t paddr,
>  {
>  	struct acpi_iomap *map;
>  
> -	map = __acpi_find_iomap(paddr, size);
> +	map = __acpi_find_iomap(paddr, size/8);
>  	if (map)
>  		return map->vaddr + (paddr - map->paddr);
>  	else

Good catch! Thanks.

Or change the caller acpi_atomic_read_mem/write_mem?

Best Regards,
Huang Ying

Re: [PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Bjorn Helgaas]

On Mon, Oct 24, 2011 at 8:48 PM, Huang Ying <ying.huang@intel.com> wrote:
> Sorry for late.
>
> On 10/22/2011 05:42 AM, Luck, Tony wrote:
>> Callers to __acpi_ioremap_fast() pass the bit_width that they found in the
>> acpi_generic_address structure. Convert from bits to bytes when passing to
>> __acpi_find_iomap() - as it wants to see bytes, not bits.
>>
>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>> ---
>>  drivers/acpi/atomicio.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
>> index 252888f..70ffb71 100644
>> --- a/drivers/acpi/atomicio.c
>> +++ b/drivers/acpi/atomicio.c
>> @@ -78,7 +78,7 @@ static void __iomem *__acpi_ioremap_fast(phys_addr_t paddr,
>>  {
>>       struct acpi_iomap *map;
>>
>> -     map = __acpi_find_iomap(paddr, size);
>> +     map = __acpi_find_iomap(paddr, size/8);
>>       if (map)
>>               return map->vaddr + (paddr - map->paddr);
>>       else
>
> Good catch! Thanks.
>
> Or change the caller acpi_atomic_read_mem/write_mem?

Myron posted a nice patch recently to remove atomicio.c altogether:
http://marc.info/?l=linux-acpi&m=131733358818849&w=2

I haven't seen any response to it, but I think it's a nice approach
and it gets rid of all this special-case pre-map, post-unmap, atomic
read/write stuff.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Luck, Tony]

> Myron posted a nice patch recently to remove atomicio.c altogether:
> http://marc.info/?l=linux-acpi&m=131733358818849&w=2
>
> I haven't seen any response to it, but I think it's a nice approach
> and it gets rid of all this special-case pre-map, post-unmap, atomic
> read/write stuff.

Does that meet the needs of the:

  panic in NMI/machine check context
    kmsg_dump
      pstore
        ERST
          acpi reads & writes

call chain?  I thought that atomicio was created for paths where the normal
acpi operations had problems in NMI contexts.

-Tony
	

Re: [PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Bjorn Helgaas]

[+cc Myron]

On Tue, Oct 25, 2011 at 12:39 PM, Luck, Tony <tony.luck@intel.com> wrote:
>> Myron posted a nice patch recently to remove atomicio.c altogether:
>> http://marc.info/?l=linux-acpi&m=131733358818849&w=2
>>
>> I haven't seen any response to it, but I think it's a nice approach
>> and it gets rid of all this special-case pre-map, post-unmap, atomic
>> read/write stuff.
>
> Does that meet the needs of the:
>
>  panic in NMI/machine check context
>    kmsg_dump
>      pstore
>        ERST
>          acpi reads & writes
>
> call chain?  I thought that atomicio was created for paths where the normal
> acpi operations had problems in NMI contexts.

Yes, it should meet those needs.  The reason we couldn't use
acpi_read() in NMI contexts was that it did an ioremap(), which may
need to allocate memory.  Therefore, we added acpi_pre_map_gar(),
which did the ioremap() early, in process context, and
acpi_atomic_read(), which relies on that mapping to perform the
access.

Myron's work adds acpi_os_map_generic_address(), which is basically
the same as acpi_pre_map_gar().  You call this in process context, and
it does the ioremap() and saves the resulting mapping.  Then he
changed acpi_os_read_memory() (the guts of acpi_read()) so it only
does the ioremap() if there's no existing mapping.

The net result is that acpi_read() should now be safe in NMI context,
as long as you are reading things you've previously mapped with
acpi_os_map_generic_address().

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Myron Stowe]

On Tue, Oct 25, 2011 at 1:34 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Myron]
>
> On Tue, Oct 25, 2011 at 12:39 PM, Luck, Tony <tony.luck@intel.com> wrote:
>>> Myron posted a nice patch recently to remove atomicio.c altogether:
>>> http://marc.info/?l=linux-acpi&m=131733358818849&w=2
>>>
>>> I haven't seen any response to it, but I think it's a nice approach
>>> and it gets rid of all this special-case pre-map, post-unmap, atomic
>>> read/write stuff.
>>
>> Does that meet the needs of the:
>>
>>  panic in NMI/machine check context
>>    kmsg_dump
>>      pstore
>>        ERST
>>          acpi reads & writes
>>
>> call chain?  I thought that atomicio was created for paths where the normal
>> acpi operations had problems in NMI contexts.
>
> Yes, it should meet those needs.  The reason we couldn't use
> acpi_read() in NMI contexts was that it did an ioremap(), which may
> need to allocate memory.  Therefore, we added acpi_pre_map_gar(),
> which did the ioremap() early, in process context, and
> acpi_atomic_read(), which relies on that mapping to perform the
> access.
>
> Myron's work adds acpi_os_map_generic_address(), which is basically
> the same as acpi_pre_map_gar().  You call this in process context, and
> it does the ioremap() and saves the resulting mapping.  Then he
> changed acpi_os_read_memory() (the guts of acpi_read()) so it only
> does the ioremap() if there's no existing mapping.
>
> The net result is that acpi_read() should now be safe in NMI context,
> as long as you are reading things you've previously mapped with
> acpi_os_map_generic_address().
>
> Bjorn

Thanks for seeing this and bringing up the recent patch to remove
atomicio.c Bjorn.

If for some reason the atomicio.c removal patch is not taken then the
fix here should follow what was done earlier in drivers/acpi/osl.c - see
commit b3ba1efec2a

Myron

> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Thomas Renninger]

On Friday 21 October 2011 23:42:55 Luck, Tony wrote:
> Callers to __acpi_ioremap_fast() pass the bit_width that they found in the
> acpi_generic_address structure. Convert from bits to bytes when passing to
> __acpi_find_iomap() - as it wants to see bytes, not bits.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
This one should get:
CC: stable@kernel.org
added.

   Thomas
> ---
>  drivers/acpi/atomicio.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c
> index 252888f..70ffb71 100644
> --- a/drivers/acpi/atomicio.c
> +++ b/drivers/acpi/atomicio.c
> @@ -78,7 +78,7 @@ static void __iomem *__acpi_ioremap_fast(phys_addr_t paddr,
>  {
>  	struct acpi_iomap *map;
>  
> -	map = __acpi_find_iomap(paddr, size);
> +	map = __acpi_find_iomap(paddr, size/8);
>  	if (map)
>  		return map->vaddr + (paddr - map->paddr);
>  	else
> -- 
> 1.7.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Len Brown]

> This one should get:
> CC: stable@kernel.org

Tony,
What kind of failures are seen with this bug un-patched?
(and thus, what kind of urgency do distros have to back-port this fix?)

thanks
-- 
Len Brown, Intel Open Source Technology Center

RE: [PATCH] Convert width in bits to bytes in __acpi_ioremap_fast()

[Luck, Tony]

> What kind of failures are seen with this bug un-patched?
> (and thus, what kind of urgency do distros have to back-port this fix?)

Just noticed it was wrong when reading code - It hadn't caused me a
problem - and in most cases I don't think there is an issue - since
we actually round up all mappings to pages anyway, whether we have
the size in bits or bytes will generally not make any difference.

There might be some issues with addresses near the end of a page,
where using 64 as bytes would mean addr+size overflows onto the
next page, where addr+size/8 doesn't.  But I haven't looked hard
enough to see what would happen in this case.

-Tony