[PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Abylay Ospan]

pcie->dev->of_node not always defined (NULL) and can cause crash:

[   19.053195] Unable to handle kernel NULL pointer dereference at
virtual address 00000020
[<c0b0370c>] (of_n_addr_cells) from [<c06599c4>]
(iproc_pcie_setup+0x30c/0xce0)

this patch adds sanity check to prevent crash.

Signed-off-by: Abylay Ospan <aospan@netup.ru>
---
 drivers/pci/host/pcie-iproc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 3ebc025..f2836a9 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
 	const int na = 3, ns = 2;
 	int rlen;
 
+	if (!node)
+		return -ENOENT;
+
 	parser->node = node;
 	parser->pna = of_n_addr_cells(node);
 	parser->np = parser->pna + na + ns;
-- 
2.7.4

Re: [PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Florian Fainelli]

On 01/12/2017 04:20 PM, Abylay Ospan wrote:
> pcie->dev->of_node not always defined (NULL) and can cause crash:
> 
> [   19.053195] Unable to handle kernel NULL pointer dereference at
> virtual address 00000020
> [<c0b0370c>] (of_n_addr_cells) from [<c06599c4>]
> (iproc_pcie_setup+0x30c/0xce0)
> 
> this patch adds sanity check to prevent crash.

Humm, how can it not be defined based on your earlier comment that you
are using this on NSP which is Device Tree exclusively? I would agree if
this was seen on e.g: MIPS/BCMA (47xx).

> 
> Signed-off-by: Abylay Ospan <aospan@netup.ru>
> ---
>  drivers/pci/host/pcie-iproc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 3ebc025..f2836a9 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>  	const int na = 3, ns = 2;
>  	int rlen;
>  
> +	if (!node)
> +		return -ENOENT;
> +
>  	parser->node = node;
>  	parser->pna = of_n_addr_cells(node);
>  	parser->np = parser->pna + na + ns;
> 


-- 
Florian

Re: [PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Ray Jui]

On 1/12/2017 4:20 PM, Abylay Ospan wrote:
> pcie->dev->of_node not always defined (NULL) and can cause crash:

Ah I guess this can happen with the BCMA based platforms that do not use
device tree for PCIe?

> 
> [   19.053195] Unable to handle kernel NULL pointer dereference at
> virtual address 00000020
> [<c0b0370c>] (of_n_addr_cells) from [<c06599c4>]
> (iproc_pcie_setup+0x30c/0xce0)
> 
> this patch adds sanity check to prevent crash.
> 
> Signed-off-by: Abylay Ospan <aospan@netup.ru>
> ---
>  drivers/pci/host/pcie-iproc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 3ebc025..f2836a9 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>  	const int na = 3, ns = 2;
>  	int rlen;
>  
> +	if (!node)
> +		return -ENOENT;
> +

Looks like a valid check to me.

Acked-by: Ray Jui <ray.jui@broadcom.com>

>  	parser->node = node;
>  	parser->pna = of_n_addr_cells(node);
>  	parser->np = parser->pna + na + ns;
> 

Re: [PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Ray Jui]

Hi Florian,

On 1/12/2017 4:22 PM, Florian Fainelli wrote:
> On 01/12/2017 04:20 PM, Abylay Ospan wrote:
>> pcie->dev->of_node not always defined (NULL) and can cause crash:
>>
>> [   19.053195] Unable to handle kernel NULL pointer dereference at
>> virtual address 00000020
>> [<c0b0370c>] (of_n_addr_cells) from [<c06599c4>]
>> (iproc_pcie_setup+0x30c/0xce0)
>>
>> this patch adds sanity check to prevent crash.
> 
> Humm, how can it not be defined based on your earlier comment that you
> are using this on NSP which is Device Tree exclusively? I would agree if
> this was seen on e.g: MIPS/BCMA (47xx).

I thought Abylay mentioned:

"Tested on Broadcom NorthStar machine ('Edgecore ECW7220-L') with two
PCIe wifi
adapters (b43 BCM4331 and ath10k QCA988X)."

That is a NorthStar device which is BCMA based?

> 
>>
>> Signed-off-by: Abylay Ospan <aospan@netup.ru>
>> ---
>>  drivers/pci/host/pcie-iproc.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 3ebc025..f2836a9 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>>  	const int na = 3, ns = 2;
>>  	int rlen;
>>  
>> +	if (!node)
>> +		return -ENOENT;
>> +
>>  	parser->node = node;
>>  	parser->pna = of_n_addr_cells(node);
>>  	parser->np = parser->pna + na + ns;
>>
> 
> 

Re: [PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Florian Fainelli]

On 01/12/2017 04:48 PM, Ray Jui wrote:
> Hi Florian,
> 
> On 1/12/2017 4:22 PM, Florian Fainelli wrote:
>> On 01/12/2017 04:20 PM, Abylay Ospan wrote:
>>> pcie->dev->of_node not always defined (NULL) and can cause crash:
>>>
>>> [   19.053195] Unable to handle kernel NULL pointer dereference at
>>> virtual address 00000020
>>> [<c0b0370c>] (of_n_addr_cells) from [<c06599c4>]
>>> (iproc_pcie_setup+0x30c/0xce0)
>>>
>>> this patch adds sanity check to prevent crash.
>>
>> Humm, how can it not be defined based on your earlier comment that you
>> are using this on NSP which is Device Tree exclusively? I would agree if
>> this was seen on e.g: MIPS/BCMA (47xx).
> 
> I thought Abylay mentioned:
> 
> "Tested on Broadcom NorthStar machine ('Edgecore ECW7220-L') with two
> PCIe wifi
> adapters (b43 BCM4331 and ath10k QCA988X)."
> 
> That is a NorthStar device which is BCMA based?

Still, upstream Linux support for Northstar is Device Tree, and BCMA bus
should fill in of_nodes accordingly, if not, that's a bug that must be
fixed at the BCMA layer.

> 
>>
>>>
>>> Signed-off-by: Abylay Ospan <aospan@netup.ru>
>>> ---
>>>  drivers/pci/host/pcie-iproc.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>>> index 3ebc025..f2836a9 100644
>>> --- a/drivers/pci/host/pcie-iproc.c
>>> +++ b/drivers/pci/host/pcie-iproc.c
>>> @@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>>>  	const int na = 3, ns = 2;
>>>  	int rlen;
>>>  
>>> +	if (!node)
>>> +		return -ENOENT;
>>> +
>>>  	parser->node = node;
>>>  	parser->pna = of_n_addr_cells(node);
>>>  	parser->np = parser->pna + na + ns;
>>>
>>
>>


-- 
Florian

Re: [PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Abylay Ospan]

Hi Florian,

> Still, upstream Linux support for Northstar is Device Tree, and BCMA bus
> should fill in of_nodes accordingly, if not, that's a bug that must be
> fixed at the BCMA layer.

yes, this is a source of the problem. Devices allocated in
'bcma_bus_scan' but of_node doesn't assigned.
Is some code missing in drivers/bcma/ which should assign of_node ?

I can suggest following "hacky" patch for this (works for me):

Author: Abylay Ospan <aospan@netup.ru>
Date:   Fri Jan 13 07:24:13 2017 +0300

    bcma: force assign 'of_node' for devices on the bus

    prevent other code to fail if no 'of_node' defined

    Signed-off-by: Abylay Ospan <aospan@netup.ru>

diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
index 2c1798e..4fe1c92 100644
--- a/drivers/bcma/main.c
+++ b/drivers/bcma/main.c
@@ -301,6 +301,11 @@ void bcma_init_bus(struct bcma_bus *bus)
 static void bcma_register_core(struct bcma_bus *bus, struct bcma_device *core)
 {
        int err;
+       struct device * dev;
+
+       dev = bcma_bus_get_host_dev(bus);
+       if (dev && !core->dev.of_node)
+               core->dev.of_node = dev->of_node;


if it's ok I will send this patch in separate email.

>
>>
>>>
>>>>
>>>> Signed-off-by: Abylay Ospan <aospan@netup.ru>
>>>> ---
>>>>  drivers/pci/host/pcie-iproc.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>>>> index 3ebc025..f2836a9 100644
>>>> --- a/drivers/pci/host/pcie-iproc.c
>>>> +++ b/drivers/pci/host/pcie-iproc.c
>>>> @@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>>>>     const int na = 3, ns = 2;
>>>>     int rlen;
>>>>
>>>> +   if (!node)
>>>> +           return -ENOENT;
>>>> +
>>>>     parser->node = node;
>>>>     parser->pna = of_n_addr_cells(node);
>>>>     parser->np = parser->pna + na + ns;
>>>>
>>>
>>>
>
>
> --
> Florian



-- 
Abylay Ospan,
NetUP Inc.
http://www.netup.tv

Re: [PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Abylay Ospan]

FYI,

here is my tree (based on linux-next):
https://github.com/aospan/linux-next-bcm4708-edgecore-ecw7220-l/commits/master

last patches adding defconfig and dts I'm using for this device. This
files are draft yet.

2017-01-12 19:22 GMT-05:00 Florian Fainelli <f.fainelli@gmail.com>:
> On 01/12/2017 04:20 PM, Abylay Ospan wrote:
>> pcie->dev->of_node not always defined (NULL) and can cause crash:
>>
>> [   19.053195] Unable to handle kernel NULL pointer dereference at
>> virtual address 00000020
>> [<c0b0370c>] (of_n_addr_cells) from [<c06599c4>]
>> (iproc_pcie_setup+0x30c/0xce0)
>>
>> this patch adds sanity check to prevent crash.
>
> Humm, how can it not be defined based on your earlier comment that you
> are using this on NSP which is Device Tree exclusively? I would agree if
> this was seen on e.g: MIPS/BCMA (47xx).
>
>>
>> Signed-off-by: Abylay Ospan <aospan@netup.ru>
>> ---
>>  drivers/pci/host/pcie-iproc.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>> index 3ebc025..f2836a9 100644
>> --- a/drivers/pci/host/pcie-iproc.c
>> +++ b/drivers/pci/host/pcie-iproc.c
>> @@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>>       const int na = 3, ns = 2;
>>       int rlen;
>>
>> +     if (!node)
>> +             return -ENOENT;
>> +
>>       parser->node = node;
>>       parser->pna = of_n_addr_cells(node);
>>       parser->np = parser->pna + na + ns;
>>
>
>
> --
> Florian



-- 
Abylay Ospan,
NetUP Inc.
http://www.netup.tv

Re: [PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Florian Fainelli]

+Rafal,

On 01/12/2017 08:55 PM, Abylay Ospan wrote:
> Hi Florian,
> 
>> Still, upstream Linux support for Northstar is Device Tree, and BCMA bus
>> should fill in of_nodes accordingly, if not, that's a bug that must be
>> fixed at the BCMA layer.
> 
> yes, this is a source of the problem. Devices allocated in
> 'bcma_bus_scan' but of_node doesn't assigned.
> Is some code missing in drivers/bcma/ which should assign of_node ?

bcma_of_fill_device() is responsible for assigning the core of_node
pointer based on the core discovered. It sounds like we may have to add
an entry under the "axi" bus node for the PCIe RC, very much like these
ones already exist:

              gmac0: ethernet@24000 {
                        reg = <0x24000 0x800>;
                };

                gmac1: ethernet@25000 {
                        reg = <0x25000 0x800>;
                };

                gmac2: ethernet@26000 {
                        reg = <0x26000 0x800>;
                };

                gmac3: ethernet@27000 {
                        reg = <0x27000 0x800>;
                };

Without that, you indeed do not have a proper linkage between the BCMA
discovered device and its of_node pointer.

Can you give it a spin?

> 
> I can suggest following "hacky" patch for this (works for me):
> 
> Author: Abylay Ospan <aospan@netup.ru>
> Date:   Fri Jan 13 07:24:13 2017 +0300
> 
>     bcma: force assign 'of_node' for devices on the bus
> 
>     prevent other code to fail if no 'of_node' defined
> 
>     Signed-off-by: Abylay Ospan <aospan@netup.ru>
> 
> diff --git a/drivers/bcma/main.c b/drivers/bcma/main.c
> index 2c1798e..4fe1c92 100644
> --- a/drivers/bcma/main.c
> +++ b/drivers/bcma/main.c
> @@ -301,6 +301,11 @@ void bcma_init_bus(struct bcma_bus *bus)
>  static void bcma_register_core(struct bcma_bus *bus, struct bcma_device *core)
>  {
>         int err;
> +       struct device * dev;
> +
> +       dev = bcma_bus_get_host_dev(bus);
> +       if (dev && !core->dev.of_node)
> +               core->dev.of_node = dev->of_node;
> 
> 
> if it's ok I will send this patch in separate email.
> 
>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Abylay Ospan <aospan@netup.ru>
>>>>> ---
>>>>>  drivers/pci/host/pcie-iproc.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>>>>> index 3ebc025..f2836a9 100644
>>>>> --- a/drivers/pci/host/pcie-iproc.c
>>>>> +++ b/drivers/pci/host/pcie-iproc.c
>>>>> @@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>>>>>     const int na = 3, ns = 2;
>>>>>     int rlen;
>>>>>
>>>>> +   if (!node)
>>>>> +           return -ENOENT;
>>>>> +
>>>>>     parser->node = node;
>>>>>     parser->pna = of_n_addr_cells(node);
>>>>>     parser->np = parser->pna + na + ns;
>>>>>
>>>>
>>>>
>>
>>
>> --
>> Florian
> 
> 
> 


-- 
Florian

Re: [PATCH] PCI: iproc: fix kernel crash if dev->of_node not defined

[Bjorn Helgaas]

On Fri, Jan 13, 2017 at 03:20:17AM +0300, Abylay Ospan wrote:
> pcie->dev->of_node not always defined (NULL) and can cause crash:
> 
> [   19.053195] Unable to handle kernel NULL pointer dereference at
> virtual address 00000020
> [<c0b0370c>] (of_n_addr_cells) from [<c06599c4>]
> (iproc_pcie_setup+0x30c/0xce0)
> 
> this patch adds sanity check to prevent crash.
> 
> Signed-off-by: Abylay Ospan <aospan@netup.ru>

Hi Abylay, this didn't look quite fully baked yet, so I'm dropping it for
now.  Please repost if everything's been worked out and the patch is still
necessary.

> ---
>  drivers/pci/host/pcie-iproc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 3ebc025..f2836a9 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -952,6 +952,9 @@ static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>  	const int na = 3, ns = 2;
>  	int rlen;
>  
> +	if (!node)
> +		return -ENOENT;
> +
>  	parser->node = node;
>  	parser->pna = of_n_addr_cells(node);
>  	parser->np = parser->pna + na + ns;
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel