[[PATCH v1]] of/pci: fix a bug in function pci_pio_to_address

[[PATCH v1]] of/pci: fix a bug in function pci_pio_to_address

[Zhichang Yuan]

In the patch whose commit id is 41f8bba7f5552d0, function pci_pio_to_address
was introduced to retieve the corresponding I/O port by CPU address. But the
convertion processing is not correct. It will return a wrong I/O port.
This patch will fix it.

Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
---
 drivers/of/address.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 78a7dcb..6906a3f 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -765,7 +765,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
 	spin_lock(&io_range_lock);
 	list_for_each_entry(res, &io_range_list, list) {
 		if (address >= res->start && address < res->start + res->size) {
-			addr = res->start - address + offset;
+			addr = address - res->start + offset;
 			break;
 		}
 		offset += res->size;
-- 
1.9.1

Re: [[PATCH v1]] of/pci: fix a bug in function pci_pio_to_address

[Liviu Dudau]

On Wed, Apr 22, 2015 at 02:35:59PM +0100, Zhichang Yuan wrote:
> In the patch whose commit id is 41f8bba7f5552d0, function pci_pio_to_address
> was introduced to retieve the corresponding I/O port by CPU address. But the
> convertion processing is not correct. It will return a wrong I/O port.
> This patch will fix it.
> 
> Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>

Acked-by: Liviu Dudau <Liviu.Dudau@arm.com>

Zhichang, you might want to Cc Greg KH for inclusion into stable.

Best regards,
Liviu

> ---
>  drivers/of/address.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 78a7dcb..6906a3f 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -765,7 +765,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>  	spin_lock(&io_range_lock);
>  	list_for_each_entry(res, &io_range_list, list) {
>  		if (address >= res->start && address < res->start + res->size) {
> -			addr = res->start - address + offset;
> +			addr = address - res->start + offset;
>  			break;
>  		}
>  		offset += res->size;
> -- 
> 1.9.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [[PATCH v1]] of/pci: fix a bug in function pci_pio_to_address

[Bjorn Helgaas]

On Wed, Apr 22, 2015 at 09:35:59PM +0800, Zhichang Yuan wrote:
> In the patch whose commit id is 41f8bba7f5552d0, function pci_pio_to_address
> was introduced to retieve the corresponding I/O port by CPU address. But the
> convertion processing is not correct. It will return a wrong I/O port.
> This patch will fix it.

Hmmm, this subject and changelog don't seem right.  41f8bba7f555 did add
pci_pio_to_address(), but that converts an I/O port to a CPU address, and
this patch doesn't touch that function.

This patch changes pci_address_to_pio(), which does return the I/O port
corresponding to a CPU physical address.  This function was modified (but
not added) by 41f8bba7f555.

Please add:

Fixes: 41f8bba7f555 ("of/pci: Add pci_register_io_range() and pci_pio_to_address()")
CC: stable@vger.kernel.org	# v3.18+

> Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
> ---
>  drivers/of/address.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 78a7dcb..6906a3f 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -765,7 +765,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>  	spin_lock(&io_range_lock);
>  	list_for_each_entry(res, &io_range_list, list) {
>  		if (address >= res->start && address < res->start + res->size) {
> -			addr = res->start - address + offset;
> +			addr = address - res->start + offset;

This looks like it's been broken since v3.18, and it's used by many
platforms.  I/O port space isn't as common as it used to be, but it's still
surprising that nobody noticed until now.

This change does look correct to me, but I want to double-check that we're
actually going to *fix* a bunch of platforms rather than breaking them.

>  			break;
>  		}
>  		offset += res->size;
> -- 
> 1.9.1
> 

Re: [[PATCH v1]] of/pci: fix a bug in function pci_pio_to_address

[Liviu Dudau]

On Wed, Apr 22, 2015 at 04:11:11PM +0100, Bjorn Helgaas wrote:
> On Wed, Apr 22, 2015 at 09:35:59PM +0800, Zhichang Yuan wrote:
> > In the patch whose commit id is 41f8bba7f5552d0, function pci_pio_to_address
> > was introduced to retieve the corresponding I/O port by CPU address. But the
> > convertion processing is not correct. It will return a wrong I/O port.
> > This patch will fix it.
> 
> Hmmm, this subject and changelog don't seem right.  41f8bba7f555 did add
> pci_pio_to_address(), but that converts an I/O port to a CPU address, and
> this patch doesn't touch that function.
> 
> This patch changes pci_address_to_pio(), which does return the I/O port
> corresponding to a CPU physical address.  This function was modified (but
> not added) by 41f8bba7f555.
> 
> Please add:
> 
> Fixes: 41f8bba7f555 ("of/pci: Add pci_register_io_range() and pci_pio_to_address()")
> CC: stable@vger.kernel.org	# v3.18+
> 
> > Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
> > ---
> >  drivers/of/address.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 78a7dcb..6906a3f 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -765,7 +765,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
> >  	spin_lock(&io_range_lock);
> >  	list_for_each_entry(res, &io_range_list, list) {
> >  		if (address >= res->start && address < res->start + res->size) {
> > -			addr = res->start - address + offset;
> > +			addr = address - res->start + offset;
> 
> This looks like it's been broken since v3.18, and it's used by many
> platforms.  I/O port space isn't as common as it used to be, but it's still
> surprising that nobody noticed until now.

Add to that the fact that the code is guarded by PCI_IOBASE and the resulting
number of affected architectures shrinks to 4: arm, arm64, microblaze and
unicore32. Microblaze gets struck off the list because it re-implements the
function, which leaves mainly ARM architectures.

Main user of this function now is of_pci_range_to_resource() which
was newly added and used by my generic parsing code. of_address_to_resource() also
uses pci_address_to_pio() but only microblaze would fall under all the right
conditions and that is protected due to re-implementation.

> 
> This change does look correct to me, but I want to double-check that we're
> actually going to *fix* a bunch of platforms rather than breaking them.

My guess is that Zhichang was the first to experience the issue unless there
are systems out there that have a host bridge with two IO ranges (or two
host bridges each one IO range) that are silently using the wrong IO port
addresses.

Fixing latest stable might be enough as there are no affected users of the earlier
code?

Best regards,
Liviu

> 
> >  			break;
> >  		}
> >  		offset += res->size;
> > -- 
> > 1.9.1
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [[PATCH v1]] of/pci: fix a bug in function pci_pio_to_address

[yuanzhichang]

On 2015/4/22 23:11, Bjorn Helgaas wrote:
> On Wed, Apr 22, 2015 at 09:35:59PM +0800, Zhichang Yuan wrote:
>> In the patch whose commit id is 41f8bba7f5552d0, function pci_pio_to_address
>> was introduced to retieve the corresponding I/O port by CPU address. But the
>> convertion processing is not correct. It will return a wrong I/O port.
>> This patch will fix it.
>
> Hmmm, this subject and changelog don't seem right.  41f8bba7f555 did add
> pci_pio_to_address(), but that converts an I/O port to a CPU address, and
> this patch doesn't touch that function.
>
> This patch changes pci_address_to_pio(), which does return the I/O port
> corresponding to a CPU physical address.  This function was modified (but
> not added) by 41f8bba7f555.
>
> Please add:
>
> Fixes: 41f8bba7f555 ("of/pci: Add pci_register_io_range() and pci_pio_to_address()")
> CC: stable@vger.kernel.org	# v3.18+
>
Ok. I will update the log and submit the v2.
>> Signed-off-by: Zhichang Yuan <yuanzhichang@hisilicon.com>
>> ---
>>   drivers/of/address.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index 78a7dcb..6906a3f 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -765,7 +765,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>>   	spin_lock(&io_range_lock);
>>   	list_for_each_entry(res, &io_range_list, list) {
>>   		if (address >= res->start && address < res->start + res->size) {
>> -			addr = res->start - address + offset;
>> +			addr = address - res->start + offset;
>
> This looks like it's been broken since v3.18, and it's used by many
> platforms.  I/O port space isn't as common as it used to be, but it's still
> surprising that nobody noticed until now.
>
> This change does look correct to me, but I want to double-check that we're
> actually going to *fix* a bunch of platforms rather than breaking them.
>
>>   			break;
>>   		}
>>   		offset += res->size;
>> --
>> 1.9.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>