ARM/PCI: set MPS before pci_bus_add_devices()
diff mbox

Message ID 1437514519-18545-1-git-send-email-m-karicheri2@ti.com
State Accepted
Headers show

Commit Message

Murali Karicheri July 21, 2015, 9:35 p.m. UTC
The MPS configuration should be done *before* pci_bus_add_devices().
After pci_bus_add_devices(), drivers may be bound to devices, and
the PCI core shouldn't touch device configuration while a driver
owns the device.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Reported-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/arm/kernel/bios32.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

Comments

Murali Karicheri July 23, 2015, 2:24 p.m. UTC | #1
On 07/21/2015 05:35 PM, Murali Karicheri wrote:
> The MPS configuration should be done *before* pci_bus_add_devices().
> After pci_bus_add_devices(), drivers may be bound to devices, and
> the PCI core shouldn't touch device configuration while a driver
> owns the device.
>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   arch/arm/kernel/bios32.c | 19 +++++--------------
>   1 file changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1..17efde7 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -520,7 +520,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>   	list_for_each_entry(sys, &head, node) {
>   		struct pci_bus *bus = sys->bus;
>
> -		if (!pci_has_flag(PCI_PROBE_ONLY)) {
> +		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
> +			struct pci_bus *child;
>   			/*
>   			 * Size the bridge windows.
>   			 */
> @@ -530,25 +531,15 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>   			 * Assign resources.
>   			 */
>   			pci_bus_assign_resources(bus);
> -		}
>
> +			list_for_each_entry(child, &bus->children, node)
> +				pcie_bus_configure_settings(child);
> +		}
>   		/*
>   		 * Tell drivers about devices found.
>   		 */
>   		pci_bus_add_devices(bus);
>   	}
> -
> -	list_for_each_entry(sys, &head, node) {
> -		struct pci_bus *bus = sys->bus;
> -
> -		/* Configure PCI Express settings */
> -		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
> -			struct pci_bus *child;
> -
> -			list_for_each_entry(child, &bus->children, node)
> -				pcie_bus_configure_settings(child);
> -		}
> -	}
>   }
>
>   #ifndef CONFIG_PCI_HOST_ITE8152
>
Bjorn,

When you get a chance, please review this. We discussed this change in a 
separate thread and agreed for a patch from me.

Thanks and regards
Bjorn Helgaas July 23, 2015, 2:59 p.m. UTC | #2
On Tue, Jul 21, 2015 at 05:35:19PM -0400, Murali Karicheri wrote:
> The MPS configuration should be done *before* pci_bus_add_devices().
> After pci_bus_add_devices(), drivers may be bound to devices, and
> the PCI core shouldn't touch device configuration while a driver
> owns the device.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>

Applied to pci/enumeration for v4.3, thanks!  I removed the "bus" test; let
me know if you think that's incorrect.

> ---
>  arch/arm/kernel/bios32.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1..17efde7 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -520,7 +520,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  	list_for_each_entry(sys, &head, node) {
>  		struct pci_bus *bus = sys->bus;
>  
> -		if (!pci_has_flag(PCI_PROBE_ONLY)) {
> +		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {

I think the test for "bus" being non-NULL is superfluous here.  If "bus"
were NULL, we would already oops in pci_bus_add_devices().

> +			struct pci_bus *child;
>  			/*
>  			 * Size the bridge windows.
>  			 */
> @@ -530,25 +531,15 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  			 * Assign resources.
>  			 */
>  			pci_bus_assign_resources(bus);
> -		}
>  
> +			list_for_each_entry(child, &bus->children, node)
> +				pcie_bus_configure_settings(child);
> +		}
>  		/*
>  		 * Tell drivers about devices found.
>  		 */
>  		pci_bus_add_devices(bus);
>  	}
> -
> -	list_for_each_entry(sys, &head, node) {
> -		struct pci_bus *bus = sys->bus;
> -
> -		/* Configure PCI Express settings */
> -		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
> -			struct pci_bus *child;
> -
> -			list_for_each_entry(child, &bus->children, node)
> -				pcie_bus_configure_settings(child);
> -		}
> -	}
>  }
>  
>  #ifndef CONFIG_PCI_HOST_ITE8152
> -- 
> 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
Murali Karicheri Aug. 3, 2015, 6:18 p.m. UTC | #3
On 07/23/2015 10:59 AM, Bjorn Helgaas wrote:
> On Tue, Jul 21, 2015 at 05:35:19PM -0400, Murali Karicheri wrote:
>> The MPS configuration should be done *before* pci_bus_add_devices().
>> After pci_bus_add_devices(), drivers may be bound to devices, and
>> the PCI core shouldn't touch device configuration while a driver
>> owns the device.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Applied to pci/enumeration for v4.3, thanks!  I removed the "bus" test; let
> me know if you think that's incorrect.
Ok. Will do. Thanks.

Murali

>
>> ---
>>   arch/arm/kernel/bios32.c | 19 +++++--------------
>>   1 file changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>> index fcbbbb1..17efde7 100644
>> --- a/arch/arm/kernel/bios32.c
>> +++ b/arch/arm/kernel/bios32.c
>> @@ -520,7 +520,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>>   	list_for_each_entry(sys, &head, node) {
>>   		struct pci_bus *bus = sys->bus;
>>
>> -		if (!pci_has_flag(PCI_PROBE_ONLY)) {
>> +		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>
> I think the test for "bus" being non-NULL is superfluous here.  If "bus"
> were NULL, we would already oops in pci_bus_add_devices().
>
>> +			struct pci_bus *child;
>>   			/*
>>   			 * Size the bridge windows.
>>   			 */
>> @@ -530,25 +531,15 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>>   			 * Assign resources.
>>   			 */
>>   			pci_bus_assign_resources(bus);
>> -		}
>>
>> +			list_for_each_entry(child, &bus->children, node)
>> +				pcie_bus_configure_settings(child);
>> +		}
>>   		/*
>>   		 * Tell drivers about devices found.
>>   		 */
>>   		pci_bus_add_devices(bus);
>>   	}
>> -
>> -	list_for_each_entry(sys, &head, node) {
>> -		struct pci_bus *bus = sys->bus;
>> -
>> -		/* Configure PCI Express settings */
>> -		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>> -			struct pci_bus *child;
>> -
>> -			list_for_each_entry(child, &bus->children, node)
>> -				pcie_bus_configure_settings(child);
>> -		}
>> -	}
>>   }
>>
>>   #ifndef CONFIG_PCI_HOST_ITE8152
>> --
>> 1.9.1
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Russell King - ARM Linux Aug. 3, 2015, 7:13 p.m. UTC | #4
On Tue, Jul 21, 2015 at 05:35:19PM -0400, Murali Karicheri wrote:
> The MPS configuration should be done *before* pci_bus_add_devices().
> After pci_bus_add_devices(), drivers may be bound to devices, and
> the PCI core shouldn't touch device configuration while a driver
> owns the device.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  arch/arm/kernel/bios32.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1..17efde7 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -520,7 +520,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  	list_for_each_entry(sys, &head, node) {
>  		struct pci_bus *bus = sys->bus;
>  
> -		if (!pci_has_flag(PCI_PROBE_ONLY)) {
> +		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {

Let's get rid of that useless check.  bus can't be NULL here.

In the original code (below) if bus was NULL, then we would've already
oopsed before we got here.  As we don't oops here, no one is ever
seeing it being NULL, so the test is redundant.

> -	list_for_each_entry(sys, &head, node) {
> -		struct pci_bus *bus = sys->bus;
> -
> -		/* Configure PCI Express settings */
> -		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {

Thanks.
Bjorn Helgaas Aug. 3, 2015, 10:34 p.m. UTC | #5
On Mon, Aug 3, 2015 at 2:13 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jul 21, 2015 at 05:35:19PM -0400, Murali Karicheri wrote:
>> The MPS configuration should be done *before* pci_bus_add_devices().
>> After pci_bus_add_devices(), drivers may be bound to devices, and
>> the PCI core shouldn't touch device configuration while a driver
>> owns the device.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> Reported-by: Bjorn Helgaas <bhelgaas@google.com>
>> ---
>>  arch/arm/kernel/bios32.c | 19 +++++--------------
>>  1 file changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>> index fcbbbb1..17efde7 100644
>> --- a/arch/arm/kernel/bios32.c
>> +++ b/arch/arm/kernel/bios32.c
>> @@ -520,7 +520,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>>       list_for_each_entry(sys, &head, node) {
>>               struct pci_bus *bus = sys->bus;
>>
>> -             if (!pci_has_flag(PCI_PROBE_ONLY)) {
>> +             if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>
> Let's get rid of that useless check.  bus can't be NULL here.
>
> In the original code (below) if bus was NULL, then we would've already
> oopsed before we got here.  As we don't oops here, no one is ever
> seeing it being NULL, so the test is redundant.
>
>> -     list_for_each_entry(sys, &head, node) {
>> -             struct pci_bus *bus = sys->bus;
>> -
>> -             /* Configure PCI Express settings */
>> -             if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>

Sorry, I had forgotten to push this branch, but I did already get rid
of the check for bus being NULL:

http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=808b27a5ae05
--
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

Patch
diff mbox

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index fcbbbb1..17efde7 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -520,7 +520,8 @@  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 	list_for_each_entry(sys, &head, node) {
 		struct pci_bus *bus = sys->bus;
 
-		if (!pci_has_flag(PCI_PROBE_ONLY)) {
+		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
+			struct pci_bus *child;
 			/*
 			 * Size the bridge windows.
 			 */
@@ -530,25 +531,15 @@  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 			 * Assign resources.
 			 */
 			pci_bus_assign_resources(bus);
-		}
 
+			list_for_each_entry(child, &bus->children, node)
+				pcie_bus_configure_settings(child);
+		}
 		/*
 		 * Tell drivers about devices found.
 		 */
 		pci_bus_add_devices(bus);
 	}
-
-	list_for_each_entry(sys, &head, node) {
-		struct pci_bus *bus = sys->bus;
-
-		/* Configure PCI Express settings */
-		if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
-			struct pci_bus *child;
-
-			list_for_each_entry(child, &bus->children, node)
-				pcie_bus_configure_settings(child);
-		}
-	}
 }
 
 #ifndef CONFIG_PCI_HOST_ITE8152