[PATCH v2 0/3] fix the pci device malfunction if a wrong bus address is assigned by firmware

[PATCH v2 0/3] fix the pci device malfunction if a wrong bus address is assigned by firmware

[Kevin Hao]

v2:
 * Use the method suggested by Bjorn to check if we have a wrong bus address
   set in the PCI BAR register.
 * Split the "move the pcibios_bus_to_resource() call out of the if/else wrap"
   change into a new patch.
 * A minor new patch is introduced to add 0x prefix for the PCI BAR register
   position.

v1:
On a fsl p2020rdb-pc board, the onboard pci device is assigned the bus address
0xa0000000 by firmware. But the kernel allocate the cpu address 0xa0000000 ~ 0xbfffffff
to this PCIe controller and use 0xe0000000 ~ 0xdfffffff as its bus address.
In this case the kernel would think that the bus address in this pci device
is correct and leave it unchanged. Then causes this device not work.

Even though this looks like a mismatch of the address space between firmware
and kernel. I think we should detect this kind of error and fix it automatically
if we don't want just to probe only (PCI_PROBE_ONLY is not set).

---
Kevin Hao (3):
  PCI: add 0x prefix when printing the BAR registers position in    
    __pci_read_base
  PCI: converge the unnecessary sprinkling of pcibios_bus_to_resource in
        __pci_read_base
  PCI: unset the resource if a wrong bus address is assigned by firmware

 drivers/pci/probe.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

-- 
1.8.1.4

[PATCH 1/3] PCI: add 0x prefix when printing the BAR registers position in __pci_read_base

[Kevin Hao]

We print the BAR registers position in hexadecimal format, so it
is more readable if 0x prefix is added. Also fix the following
checkpatch warning:
  WARNING: Prefer dev_dbg(... to dev_printk(KERN_DEBUG, ...

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v2: A new patch introduced in v2.

 drivers/pci/probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 70f10fa..2505d5e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -278,9 +278,9 @@ out:
 		pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
 
 	if (bar_too_big)
-		dev_err(&dev->dev, "reg %x: can't handle 64-bit BAR\n", pos);
+		dev_err(&dev->dev, "reg 0x%x: can't handle 64-bit BAR\n", pos);
 	if (res->flags && !bar_disabled)
-		dev_printk(KERN_DEBUG, &dev->dev, "reg %x: %pR\n", pos, res);
+		dev_dbg(&dev->dev, "reg 0x%x: %pR\n", pos, res);
 
 	return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
 }
-- 
1.8.1.4

[PATCH 2/3] PCI: converge the unnecessary sprinkling of pcibios_bus_to_resource in __pci_read_base

[Kevin Hao]

Since we will invoke pcibios_bus_to_resource unconditionally if we
don't goto fail, move it out of if/else wrap. No function change.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v2: Split these changed into a new patch compare with the v1 patch.

 drivers/pci/probe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2505d5e..c57eb27 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -250,12 +250,10 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			pci_write_config_dword(dev, pos + 4, 0);
 			region.start = 0;
 			region.end = sz64;
-			pcibios_bus_to_resource(dev, res, &region);
 			bar_disabled = true;
 		} else {
 			region.start = l64;
 			region.end = l64 + sz64;
-			pcibios_bus_to_resource(dev, res, &region);
 		}
 	} else {
 		sz = pci_size(l, sz, mask);
@@ -265,9 +263,10 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 		region.start = l;
 		region.end = l + sz;
-		pcibios_bus_to_resource(dev, res, &region);
 	}
 
+	pcibios_bus_to_resource(dev, res, &region);
+
 	goto out;
 
 
-- 
1.8.1.4

[PATCH 3/3] PCI: unset the resource if a wrong bus address is assigned by firmware

[Kevin Hao]

In some situations, the bus and memory address used by kernel are
not equal and the firmware may assign a bus address which happen
to be a legitimate memory address to a PCI device, then the kernel
would not find a matching bus region in host bridge and assume an
offset of zero and translate it to a memory address the same as the
bus address. This will leave the bus address in the PCI BAR register
unchanged and this address is definitely not a legal bus address,
then cause the device malfunction. We try to detect this by doing
a invert translation. We can make sure that we run into this case
if the value we get from the invert translation is not equal to the
original bus address. In this case we would unset this resource and
wish the kernel would trigger a reassign later.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v2:
Instead of tweak the pcibios_bus_to_resource and assume we know
about all the host bridge window, we do an invert translation to
check if we have a wrong bus address set in the PCI BAR register.

 drivers/pci/probe.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c57eb27..33c2e72 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -170,7 +170,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 {
 	u32 l, sz, mask;
 	u16 orig_cmd;
-	struct pci_bus_region region;
+	struct pci_bus_region region, inverted_region;
 	bool bar_too_big = false, bar_disabled = false;
 
 	mask = type ? PCI_ROM_ADDRESS_MASK : ~0;
@@ -267,6 +267,29 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 	pcibios_bus_to_resource(dev, res, &region);
 
+	/*
+	 * In some situations, the bus and memory address used by kernel are
+	 * not equal and the firmware may assign a bus address which happen
+	 * to be a legitimate memory address to a PCI device, then the kernel
+	 * would not find a matching bus region in host bridge and assume an
+	 * offset of zero and translate it to a memory address the same as the
+	 * bus address. This will leave the bus address in the PCI BAR register
+	 * unchanged and this address is definitely not a legal bus address,
+	 * then cause the device malfunction. We try to detect this by doing
+	 * a invert translation. We can make sure that we run into this case
+	 * if the value we get from the invert translation is not equal to the
+	 * original bus address.
+	 */
+	pcibios_resource_to_bus(dev, &inverted_region, res);
+	if (inverted_region.start != region.start ||
+		 inverted_region.end != region.end) {
+		dev_info(&dev->dev, "reg 0x%x: bus address [%pa - %pa] allocated by firmware is not in any of the address regions of PCI host bridge, we would force to reassign it later\n",
+			pos, &region.start, &region.end);
+		res->flags |= IORESOURCE_UNSET;
+		res->end -= res->start;
+		res->start = 0;
+	}
+
 	goto out;
 
 
-- 
1.8.1.4

Re: [PATCH 1/3] PCI: add 0x prefix when printing the BAR registers position in __pci_read_base

[Liu Jiang]

On 05/25/2013 07:36 PM, Kevin Hao wrote:
> We print the BAR registers position in hexadecimal format, so it
> is more readable if 0x prefix is added. Also fix the following
> checkpatch warning:
>    WARNING: Prefer dev_dbg(... to dev_printk(KERN_DEBUG, ...
>
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
> v2: A new patch introduced in v2.
>
>   drivers/pci/probe.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 70f10fa..2505d5e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -278,9 +278,9 @@ out:
>   		pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
>   
>   	if (bar_too_big)
> -		dev_err(&dev->dev, "reg %x: can't handle 64-bit BAR\n", pos);
> +		dev_err(&dev->dev, "reg 0x%x: can't handle 64-bit BAR\n", pos);
>   	if (res->flags && !bar_disabled)
> -		dev_printk(KERN_DEBUG, &dev->dev, "reg %x: %pR\n", pos, res);
> +		dev_dbg(&dev->dev, "reg 0x%x: %pR\n", pos, res);
Hi Kevin,
     dev_printk(KERN_DEBUG) -> dev_dbg() is not an equivalent change, it 
depends
on CONFIG_DEBUG and CONFIG_DYNAMIC_DEBUG.
Regards!
Gerry

>   
>   	return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
>   }

Re: [PATCH v2 0/3] fix the pci device malfunction if a wrong bus address is assigned by firmware

[Bjorn Helgaas]

On Sat, May 25, 2013 at 5:36 AM, Kevin Hao <haokexin@gmail.com> wrote:
> v2:
>  * Use the method suggested by Bjorn to check if we have a wrong bus address
>    set in the PCI BAR register.
>  * Split the "move the pcibios_bus_to_resource() call out of the if/else wrap"
>    change into a new patch.
>  * A minor new patch is introduced to add 0x prefix for the PCI BAR register
>    position.
>
> v1:
> On a fsl p2020rdb-pc board, the onboard pci device is assigned the bus address
> 0xa0000000 by firmware. But the kernel allocate the cpu address 0xa0000000 ~ 0xbfffffff
> to this PCIe controller and use 0xe0000000 ~ 0xdfffffff as its bus address.
> In this case the kernel would think that the bus address in this pci device
> is correct and leave it unchanged. Then causes this device not work.
>
> Even though this looks like a mismatch of the address space between firmware
> and kernel. I think we should detect this kind of error and fix it automatically
> if we don't want just to probe only (PCI_PROBE_ONLY is not set).
>
> ---
> Kevin Hao (3):
>   PCI: add 0x prefix when printing the BAR registers position in
>     __pci_read_base
>   PCI: converge the unnecessary sprinkling of pcibios_bus_to_resource in
>         __pci_read_base
>   PCI: unset the resource if a wrong bus address is assigned by firmware
>
>  drivers/pci/probe.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)

Applied to my "next" branch for v3.11, thanks.

Bjorn

Re: [PATCH 1/3] PCI: add 0x prefix when printing the BAR registers position in __pci_read_base

[Bjorn Helgaas]

On Sat, May 25, 2013 at 10:30 AM, Liu Jiang <liuj97@gmail.com> wrote:
> On 05/25/2013 07:36 PM, Kevin Hao wrote:
>>
>> We print the BAR registers position in hexadecimal format, so it
>> is more readable if 0x prefix is added. Also fix the following
>> checkpatch warning:
>>    WARNING: Prefer dev_dbg(... to dev_printk(KERN_DEBUG, ...
>>
>> Signed-off-by: Kevin Hao <haokexin@gmail.com>
>> ---
>> v2: A new patch introduced in v2.
>>
>>   drivers/pci/probe.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 70f10fa..2505d5e 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -278,9 +278,9 @@ out:
>>                 pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
>>         if (bar_too_big)
>> -               dev_err(&dev->dev, "reg %x: can't handle 64-bit BAR\n",
>> pos);
>> +               dev_err(&dev->dev, "reg 0x%x: can't handle 64-bit BAR\n",
>> pos);
>>         if (res->flags && !bar_disabled)
>> -               dev_printk(KERN_DEBUG, &dev->dev, "reg %x: %pR\n", pos,
>> res);
>> +               dev_dbg(&dev->dev, "reg 0x%x: %pR\n", pos, res);
>
> Hi Kevin,
>     dev_printk(KERN_DEBUG) -> dev_dbg() is not an equivalent change, it
> depends
> on CONFIG_DEBUG and CONFIG_DYNAMIC_DEBUG.

I did keep the "dev_printk" here for that reason.  I don't want that
information to be missing from dmesg.

>
>
>>         return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
>>   }
>
>

Re: [PATCH 1/3] PCI: add 0x prefix when printing the BAR registers position in __pci_read_base

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On Sat, May 25, 2013 at 05:47:28PM -0600, Bjorn Helgaas wrote:
> On Sat, May 25, 2013 at 10:30 AM, Liu Jiang <liuj97@gmail.com> wrote:
> > On 05/25/2013 07:36 PM, Kevin Hao wrote:
> >>
> >> We print the BAR registers position in hexadecimal format, so it
> >> is more readable if 0x prefix is added. Also fix the following
> >> checkpatch warning:
> >>    WARNING: Prefer dev_dbg(... to dev_printk(KERN_DEBUG, ...
> >>
> >> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> >> ---
> >> v2: A new patch introduced in v2.
> >>
> >>   drivers/pci/probe.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> >> index 70f10fa..2505d5e 100644
> >> --- a/drivers/pci/probe.c
> >> +++ b/drivers/pci/probe.c
> >> @@ -278,9 +278,9 @@ out:
> >>                 pci_write_config_word(dev, PCI_COMMAND, orig_cmd);
> >>         if (bar_too_big)
> >> -               dev_err(&dev->dev, "reg %x: can't handle 64-bit BAR\n",
> >> pos);
> >> +               dev_err(&dev->dev, "reg 0x%x: can't handle 64-bit BAR\n",
> >> pos);
> >>         if (res->flags && !bar_disabled)
> >> -               dev_printk(KERN_DEBUG, &dev->dev, "reg %x: %pR\n", pos,
> >> res);
> >> +               dev_dbg(&dev->dev, "reg 0x%x: %pR\n", pos, res);
> >
> > Hi Kevin,
> >     dev_printk(KERN_DEBUG) -> dev_dbg() is not an equivalent change, it
> > depends
> > on CONFIG_DEBUG and CONFIG_DYNAMIC_DEBUG.
> 
> I did keep the "dev_printk" here for that reason.  I don't want that
> information to be missing from dmesg.

Obviously I neglected this difference. :-(
Thanks for the reminder and fix Jiang and Bjorn.

Thanks,
Kevin

> 
> >
> >
> >>         return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
> >>   }
> >
> >

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]