[PATCH v1] ARM:sa1100: Remove a redundant spin lock

[PATCH v1] ARM:sa1100: Remove a redundant spin lock

[Wei.Yang]

From: Yang Wei <Wei.Yang@windriver.com>

The pair read/write of accessing pci confiuration space function
has already protected by pci_lock. so remove nano_lock.

Signed-off-by: Yang Wei <Wei.Yang@windriver.com>
---
 arch/arm/mach-sa1100/pci-nanoengine.c |    9 ---------
 1 file changed, 9 deletions(-)

diff --git a/arch/arm/mach-sa1100/pci-nanoengine.c b/arch/arm/mach-sa1100/pci-nanoengine.c
index ff02e2d..b944c99 100644
--- a/arch/arm/mach-sa1100/pci-nanoengine.c
+++ b/arch/arm/mach-sa1100/pci-nanoengine.c
@@ -30,7 +30,6 @@
 #include <mach/nanoengine.h>
 #include <mach/hardware.h>
 
-static DEFINE_SPINLOCK(nano_lock);
 
 static int nanoengine_get_pci_address(struct pci_bus *bus,
 	unsigned int devfn, int where, unsigned long *address)
@@ -52,7 +51,6 @@ static int nanoengine_read_config(struct pci_bus *bus, unsigned int devfn, int w
 {
 	int ret;
 	unsigned long address;
-	unsigned long flags;
 	u32 v;
 
 	/* nanoEngine PCI bridge does not return -1 for a non-existing
@@ -64,15 +62,12 @@ static int nanoengine_read_config(struct pci_bus *bus, unsigned int devfn, int w
 		goto exit_function;
 	}
 
-	spin_lock_irqsave(&nano_lock, flags);
 
 	ret = nanoengine_get_pci_address(bus, devfn, where, &address);
 	if (ret != PCIBIOS_SUCCESSFUL)
 		return ret;
 	v = __raw_readl(address);
 
-	spin_unlock_irqrestore(&nano_lock, flags);
-
 	v >>= ((where & 3) * 8);
 	v &= (unsigned long)(-1) >> ((4 - size) * 8);
 
@@ -86,13 +81,11 @@ static int nanoengine_write_config(struct pci_bus *bus, unsigned int devfn, int
 {
 	int ret;
 	unsigned long address;
-	unsigned long flags;
 	unsigned shift;
 	u32 v;
 
 	shift = (where & 3) * 8;
 
-	spin_lock_irqsave(&nano_lock, flags);
 
 	ret = nanoengine_get_pci_address(bus, devfn, where, &address);
 	if (ret != PCIBIOS_SUCCESSFUL)
@@ -113,8 +106,6 @@ static int nanoengine_write_config(struct pci_bus *bus, unsigned int devfn, int
 	}
 	__raw_writel(v, address);
 
-	spin_unlock_irqrestore(&nano_lock, flags);
-
 	return PCIBIOS_SUCCESSFUL;
 }
 
-- 
1.7.9.5

Re: [PATCH v1] ARM:sa1100: Remove a redundant spin lock

[Yang,Wei]

Hi Guys,

What about this patch?

Thanks
Wei
On 07/01/2014 03:41 PM, Wei.Yang@windriver.com wrote:
> From: Yang Wei <Wei.Yang@windriver.com>
>
> The pair read/write of accessing pci confiuration space function
> has already protected by pci_lock. so remove nano_lock.
>
> Signed-off-by: Yang Wei <Wei.Yang@windriver.com>
> ---
>   arch/arm/mach-sa1100/pci-nanoengine.c |    9 ---------
>   1 file changed, 9 deletions(-)
>
> diff --git a/arch/arm/mach-sa1100/pci-nanoengine.c b/arch/arm/mach-sa1100/pci-nanoengine.c
> index ff02e2d..b944c99 100644
> --- a/arch/arm/mach-sa1100/pci-nanoengine.c
> +++ b/arch/arm/mach-sa1100/pci-nanoengine.c
> @@ -30,7 +30,6 @@
>   #include <mach/nanoengine.h>
>   #include <mach/hardware.h>
>   
> -static DEFINE_SPINLOCK(nano_lock);
>   
>   static int nanoengine_get_pci_address(struct pci_bus *bus,
>   	unsigned int devfn, int where, unsigned long *address)
> @@ -52,7 +51,6 @@ static int nanoengine_read_config(struct pci_bus *bus, unsigned int devfn, int w
>   {
>   	int ret;
>   	unsigned long address;
> -	unsigned long flags;
>   	u32 v;
>   
>   	/* nanoEngine PCI bridge does not return -1 for a non-existing
> @@ -64,15 +62,12 @@ static int nanoengine_read_config(struct pci_bus *bus, unsigned int devfn, int w
>   		goto exit_function;
>   	}
>   
> -	spin_lock_irqsave(&nano_lock, flags);
>   
>   	ret = nanoengine_get_pci_address(bus, devfn, where, &address);
>   	if (ret != PCIBIOS_SUCCESSFUL)
>   		return ret;
>   	v = __raw_readl(address);
>   
> -	spin_unlock_irqrestore(&nano_lock, flags);
> -
>   	v >>= ((where & 3) * 8);
>   	v &= (unsigned long)(-1) >> ((4 - size) * 8);
>   
> @@ -86,13 +81,11 @@ static int nanoengine_write_config(struct pci_bus *bus, unsigned int devfn, int
>   {
>   	int ret;
>   	unsigned long address;
> -	unsigned long flags;
>   	unsigned shift;
>   	u32 v;
>   
>   	shift = (where & 3) * 8;
>   
> -	spin_lock_irqsave(&nano_lock, flags);
>   
>   	ret = nanoengine_get_pci_address(bus, devfn, where, &address);
>   	if (ret != PCIBIOS_SUCCESSFUL)
> @@ -113,8 +106,6 @@ static int nanoengine_write_config(struct pci_bus *bus, unsigned int devfn, int
>   	}
>   	__raw_writel(v, address);
>   
> -	spin_unlock_irqrestore(&nano_lock, flags);
> -
>   	return PCIBIOS_SUCCESSFUL;
>   }
>   

Re: [PATCH v1] ARM:sa1100: Remove a redundant spin lock

[Russell King - ARM Linux]

On Wed, Jul 02, 2014 at 05:19:40PM +0800, Yang,Wei wrote:
> Hi Guys,
>
> What about this patch?

The only concern on the face of it is that it removes mutual exclusion
from the pci config write path, where a read-modifiy-write operation is
performed.

However, the PCI code already gives that guarantee (drivers/pci/access.c)
so this should be safe.

As this is fairly old code, it would be useful to know what the motivation
is behind this change.  Is it purely clean up, or is it something that
you've tested, or is this a change that you require for something else?

Thanks.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH v1] ARM:sa1100: Remove a redundant spin lock

[Yang,Wei]

On 07/02/2014 06:19 PM, Russell King - ARM Linux wrote:
> On Wed, Jul 02, 2014 at 05:19:40PM +0800, Yang,Wei wrote:
>> Hi Guys,
>>
>> What about this patch?
> The only concern on the face of it is that it removes mutual exclusion
> from the pci config write path, where a read-modifiy-write operation is
> performed.
>
> However, the PCI code already gives that guarantee (drivers/pci/access.c)
> so this should be safe.
>
> As this is fairly old code, it would be useful to know what the motivation
> is behind this change.  Is it purely clean up, or is it something that
> you've tested, or is this a change that you require for something else?
>
> Thanks.
>
Hmm, I fixed a bug of pci config read/write of an ARM board, but 
unfortunately, so far ML does not support it. So
I cannot send the fix, but I grepped the pci source code of ARM 
architecure, I found that the pci code of a few boards always
define a static spin lock to excluse the pci config read/write path. I 
just intended to clean up them. If you agree the change,
I want to send v2 to also remove this mutual exclusion for something 
else board which also used the mutual exclusion.

Thanks
Wei