[PATCH 0/3] atomic_or() related changes

[PATCH 0/3] atomic_or() related changes

[Vineet Gupta]

Hi,

This started off as an effort to convert a cmpxchg based loop in arc/kernel/smp.c
to an API which is more LLOCK/SCOND friendly.

e.g.
        do {
                new = old = ACCESS_ONCE(*ipi_data_ptr);
                new |= 1U << msg;
        } while (cmpxchg(ipi_data_ptr, old, new) != old);

The generated code is horrible. There are 2 useless branches here and a
LD/LLOCK to same address all inside a loop.

8015cefc:	ld_s       r2,[r3,0]
8015cefe:	or         r5,r2,r1
8015cf02:	llock      r4,[r3]
8015cf06:	brne       r4,r2,8015cf12
8015cf0a:	scond      r5,[r3]
8015cf0e:	bnz        8015cf02
8015cf12:	brne       r2,r4,8015cefc

An atomic_or() kind of API is better suited to generate something like below

8015cf02:	llock      r4,[r3]
8015cf06:	or         r5,r2,r1
8015cf0a:	scond      r5,[r3]
8015cf0e:	bnz        8015cf02

Although this doesn't work for the specific instance I wanted to fix as
ipi_data_ptr is not atomic_t, I did run into a few things which could be
improved, hence this series.

Compile tested on ARC, ARM, x86.

I do have some concern about mixing long and int on 64 bit arch, which I've
captured inline in patch 2/3. It is most likely a lack of understand on my
part, but worth asking..

Thx,
-Vineet

Vineet Gupta (3):
  asm-generic/atomic.h: ARCH_HAS_ATOMIC_OR -> CONFIG_ARCH_HAS_ATOMIC_OR
  brcmfmac: dhd_sdio.c: use existing atomic_or primitive
  ARC: provide atomic_or() and define ARCH_HAS_ATOMIC_OR

 arch/arc/include/asm/atomic.h                  |  9 +++++++++
 drivers/net/wireless/brcm80211/brcmfmac/sdio.c | 13 ++-----------
 include/asm-generic/atomic.h                   |  2 +-
 include/linux/atomic.h                         |  4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

-- 
1.9.1

[PATCH 1/3] asm-generic/atomic.h: ARCH_HAS_ATOMIC_OR -> CONFIG_ARCH_HAS_ATOMIC_OR

[Vineet Gupta]

Since this is not backed by a Kconfig option, remove CONFIG_ prefix

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: linux-arch@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 include/asm-generic/atomic.h | 2 +-
 include/linux/atomic.h       | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/asm-generic/atomic.h b/include/asm-generic/atomic.h
index 1973ad2b13f4..1a1cdab6e702 100644
--- a/include/asm-generic/atomic.h
+++ b/include/asm-generic/atomic.h
@@ -104,7 +104,7 @@ ATOMIC_OP(and, &)
 #endif
 
 #ifndef atomic_set_mask
-#define CONFIG_ARCH_HAS_ATOMIC_OR
+#define ARCH_HAS_ATOMIC_OR
 ATOMIC_OP(or, |)
 #define atomic_set_mask(i, v)	atomic_or((i), (v))
 #endif
diff --git a/include/linux/atomic.h b/include/linux/atomic.h
index 5b08a8540ecf..195881eec33e 100644
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -111,7 +111,7 @@ static inline int atomic_dec_if_positive(atomic_t *v)
 }
 #endif
 
-#ifndef CONFIG_ARCH_HAS_ATOMIC_OR
+#ifndef ARCH_HAS_ATOMIC_OR
 static inline void atomic_or(int i, atomic_t *v)
 {
 	int old;
@@ -122,7 +122,7 @@ static inline void atomic_or(int i, atomic_t *v)
 		new = old | i;
 	} while (atomic_cmpxchg(v, old, new) != old);
 }
-#endif /* #ifndef CONFIG_ARCH_HAS_ATOMIC_OR */
+#endif /* #ifndef ARCH_HAS_ATOMIC_OR */
 
 #include <asm-generic/atomic-long.h>
 #ifdef CONFIG_GENERIC_ATOMIC64
-- 
1.9.1

[PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Vineet Gupta]

There's already a generic implementation so use that instead.
---
I'm not sure if the driver usage of atomic_or?() is correct in terms of
storage size of @val for 64 bit arches.

Assuming LP64 programming model for linux on say x86_64: atomic_or()
callers in this driver use long (sana 64 bit) storage and pass it to
atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
---
Cc: Brett Rudley <brudley@broadcom.com>
Cc: Arend van Spriel <arend@broadcom.com>
Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
Cc: Hante Meuleman <meuleman@broadcom.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Cc: Daniel Kim <dekim@broadcom.com>
Cc: linux-wireless@vger.kernel.org
Cc: brcm80211-dev-list@broadcom.com
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: netdev@vger.kernel.org
Cc: linux-arch@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 drivers/net/wireless/brcm80211/brcmfmac/sdio.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
index d36f5f3d931b..f990e3d0e696 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
@@ -2564,15 +2564,6 @@ static inline void brcmf_sdio_clrintr(struct brcmf_sdio *bus)
 	}
 }
 
-static void atomic_orr(int val, atomic_t *v)
-{
-	int old_val;
-
-	old_val = atomic_read(v);
-	while (atomic_cmpxchg(v, old_val, val | old_val) != old_val)
-		old_val = atomic_read(v);
-}
-
 static int brcmf_sdio_intr_rstatus(struct brcmf_sdio *bus)
 {
 	struct brcmf_core *buscore;
@@ -2595,7 +2586,7 @@ static int brcmf_sdio_intr_rstatus(struct brcmf_sdio *bus)
 	if (val) {
 		brcmf_sdiod_regwl(bus->sdiodev, addr, val, &ret);
 		bus->sdcnt.f1regdata++;
-		atomic_orr(val, &bus->intstatus);
+		atomic_or(val, &bus->intstatus);
 	}
 
 	return ret;
@@ -2712,7 +2703,7 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus)
 
 	/* Keep still-pending events for next scheduling */
 	if (intstatus)
-		atomic_orr(intstatus, &bus->intstatus);
+		atomic_or(intstatus, &bus->intstatus);
 
 	brcmf_sdio_clrintr(bus);
 
-- 
1.9.1

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Arend van Spriel]

On 07/09/2015 10:13 AM, Vineet Gupta wrote:
> There's already a generic implementation so use that instead.

There is or there was? If there is now I am fine with this patch, but if 
it already was there the author might have had a reason for adding a 
local function and I would like to hear that reason.

> ---
> I'm not sure if the driver usage of atomic_or?() is correct in terms of
> storage size of @val for 64 bit arches.
>
> Assuming LP64 programming model for linux on say x86_64: atomic_or()
> callers in this driver use long (sana 64 bit) storage and pass it to
> atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?

The function is used with 32bit register value from the device so I 
think it is ok.

Regards,
Arend

> ---
> Cc: Brett Rudley <brudley@broadcom.com>
> Cc: Arend van Spriel <arend@broadcom.com>
> Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
> Cc: Hante Meuleman <meuleman@broadcom.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
> Cc: Daniel Kim <dekim@broadcom.com>
> Cc: linux-wireless@vger.kernel.org
> Cc: brcm80211-dev-list@broadcom.com
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: netdev@vger.kernel.org
> Cc: linux-arch@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>   drivers/net/wireless/brcm80211/brcmfmac/sdio.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
> index d36f5f3d931b..f990e3d0e696 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
> @@ -2564,15 +2564,6 @@ static inline void brcmf_sdio_clrintr(struct brcmf_sdio *bus)
>   	}
>   }
>
> -static void atomic_orr(int val, atomic_t *v)
> -{
> -	int old_val;
> -
> -	old_val = atomic_read(v);
> -	while (atomic_cmpxchg(v, old_val, val | old_val) != old_val)
> -		old_val = atomic_read(v);
> -}
> -
>   static int brcmf_sdio_intr_rstatus(struct brcmf_sdio *bus)
>   {
>   	struct brcmf_core *buscore;
> @@ -2595,7 +2586,7 @@ static int brcmf_sdio_intr_rstatus(struct brcmf_sdio *bus)
>   	if (val) {
>   		brcmf_sdiod_regwl(bus->sdiodev, addr, val, &ret);
>   		bus->sdcnt.f1regdata++;
> -		atomic_orr(val, &bus->intstatus);
> +		atomic_or(val, &bus->intstatus);
>   	}
>
>   	return ret;
> @@ -2712,7 +2703,7 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus)
>
>   	/* Keep still-pending events for next scheduling */
>   	if (intstatus)
> -		atomic_orr(intstatus, &bus->intstatus);
> +		atomic_or(intstatus, &bus->intstatus);
>
>   	brcmf_sdio_clrintr(bus);
>
>

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Arend van Spriel]

On 07/09/2015 08:25 PM, Arend van Spriel wrote:
> On 07/09/2015 10:13 AM, Vineet Gupta wrote:
>> There's already a generic implementation so use that instead.
>
> There is or there was? If there is now I am fine with this patch, but if
> it already was there the author might have had a reason for adding a
> local function and I would like to hear that reason.

Nevermind. Just noticed you are proposing the generic implementation in 
this series. Currently on vacation and want to discuss with Hante about 
this change.

Regards,
Arend

>> ---
>> I'm not sure if the driver usage of atomic_or?() is correct in terms of
>> storage size of @val for 64 bit arches.
>>
>> Assuming LP64 programming model for linux on say x86_64: atomic_or()
>> callers in this driver use long (sana 64 bit) storage and pass it to
>> atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
>
> The function is used with 32bit register value from the device so I
> think it is ok.
>
> Regards,
> Arend
>
>> ---
>> Cc: Brett Rudley <brudley@broadcom.com>
>> Cc: Arend van Spriel <arend@broadcom.com>
>> Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
>> Cc: Hante Meuleman <meuleman@broadcom.com>
>> Cc: Kalle Valo <kvalo@codeaurora.org>
>> Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>> Cc: Daniel Kim <dekim@broadcom.com>
>> Cc: linux-wireless@vger.kernel.org
>> Cc: brcm80211-dev-list@broadcom.com
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-arch@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
>>   drivers/net/wireless/brcm80211/brcmfmac/sdio.c | 13 ++-----------
>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
>> b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
>> index d36f5f3d931b..f990e3d0e696 100644
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/sdio.c
>> @@ -2564,15 +2564,6 @@ static inline void brcmf_sdio_clrintr(struct
>> brcmf_sdio *bus)
>>       }
>>   }
>>
>> -static void atomic_orr(int val, atomic_t *v)
>> -{
>> -    int old_val;
>> -
>> -    old_val = atomic_read(v);
>> -    while (atomic_cmpxchg(v, old_val, val | old_val) != old_val)
>> -        old_val = atomic_read(v);
>> -}
>> -
>>   static int brcmf_sdio_intr_rstatus(struct brcmf_sdio *bus)
>>   {
>>       struct brcmf_core *buscore;
>> @@ -2595,7 +2586,7 @@ static int brcmf_sdio_intr_rstatus(struct
>> brcmf_sdio *bus)
>>       if (val) {
>>           brcmf_sdiod_regwl(bus->sdiodev, addr, val, &ret);
>>           bus->sdcnt.f1regdata++;
>> -        atomic_orr(val, &bus->intstatus);
>> +        atomic_or(val, &bus->intstatus);
>>       }
>>
>>       return ret;
>> @@ -2712,7 +2703,7 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus)
>>
>>       /* Keep still-pending events for next scheduling */
>>       if (intstatus)
>> -        atomic_orr(intstatus, &bus->intstatus);
>> +        atomic_or(intstatus, &bus->intstatus);
>>
>>       brcmf_sdio_clrintr(bus);
>>
>>
>

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Peter Zijlstra]

On Thu, Jul 09, 2015 at 08:31:16PM +0200, Arend van Spriel wrote:
> >There is or there was? If there is now I am fine with this patch, but if
> >it already was there the author might have had a reason for adding a
> >local function and I would like to hear that reason.
> 
> Nevermind. Just noticed you are proposing the generic implementation in this
> series. Currently on vacation and want to discuss with Hante about this
> change.

No there is one in linux/atomic.h, he just renamed the #ifdef guard and
provided a 'sane' implementation for his arch.

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Vineet Gupta]

On Thursday 09 July 2015 11:55 PM, Arend van Spriel wrote:
> On 07/09/2015 10:13 AM, Vineet Gupta wrote:
>> > There's already a generic implementation so use that instead.
> There is or there was? If there is now I am fine with this patch, but if 
> it already was there the author might have had a reason for adding a 
> local function and I would like to hear that reason.
>

atomic_orr() was introduced to this driver with

2014-03-06 5cbb9c285bdc brcmfmac: Use atomic functions for intstatus update. 

as it seems atomic_set_mask() was not available cross arch. And atomic_or() in
generic code was indeed introduced after that

2014-04-23 560cb12a4080 locking,arch: Rewrite generic atomic support 

Hence likely the reason author went with home grown atomic_orr()

-Vineet

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Arend van Spriel]

On 07/10/2015 06:49 AM, Vineet Gupta wrote:
> On Thursday 09 July 2015 11:55 PM, Arend van Spriel wrote:
>> On 07/09/2015 10:13 AM, Vineet Gupta wrote:
>>>> There's already a generic implementation so use that instead.
>> There is or there was? If there is now I am fine with this patch, but if
>> it already was there the author might have had a reason for adding a
>> local function and I would like to hear that reason.
>>
>
> atomic_orr() was introduced to this driver with
>
> 2014-03-06 5cbb9c285bdc brcmfmac: Use atomic functions for intstatus update.
>
> as it seems atomic_set_mask() was not available cross arch. And atomic_or() in
> generic code was indeed introduced after that
>
> 2014-04-23 560cb12a4080 locking,arch: Rewrite generic atomic support
>
> Hence likely the reason author went with home grown atomic_orr()

Hi Vineet

Thanks for looking into the timeline. Will look into it and let you know.

Regards,
Arend

> -Vineet
>

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Arend van Spriel]

On 07/10/2015 06:49 AM, Vineet Gupta wrote:
> On Thursday 09 July 2015 11:55 PM, Arend van Spriel wrote:
>> On 07/09/2015 10:13 AM, Vineet Gupta wrote:
>>>> There's already a generic implementation so use that instead.
>> There is or there was? If there is now I am fine with this patch, but if
>> it already was there the author might have had a reason for adding a
>> local function and I would like to hear that reason.
>>
>
> atomic_orr() was introduced to this driver with
>
> 2014-03-06 5cbb9c285bdc brcmfmac: Use atomic functions for intstatus update.
>
> as it seems atomic_set_mask() was not available cross arch. And atomic_or() in
> generic code was indeed introduced after that
>
> 2014-04-23 560cb12a4080 locking,arch: Rewrite generic atomic support
>
> Hence likely the reason author went with home grown atomic_orr()

Hi Vineet

Thanks for looking into the timeline. Will look into it and let you know.

Regards,
Arend

> -Vineet
>

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Kalle Valo]

Vineet Gupta <Vineet.Gupta1@synopsys.com> writes:

> There's already a generic implementation so use that instead.
> ---
> I'm not sure if the driver usage of atomic_or?() is correct in terms of
> storage size of @val for 64 bit arches.
>
> Assuming LP64 programming model for linux on say x86_64: atomic_or()
> callers in this driver use long (sana 64 bit) storage and pass it to
> atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
> ---
> Cc: Brett Rudley <brudley@broadcom.com>
> Cc: Arend van Spriel <arend@broadcom.com>
> Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
> Cc: Hante Meuleman <meuleman@broadcom.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
> Cc: Daniel Kim <dekim@broadcom.com>
> Cc: linux-wireless@vger.kernel.org
> Cc: brcm80211-dev-list@broadcom.com
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: netdev@vger.kernel.org
> Cc: linux-arch@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>

What's the plan with this patch? Should I take it to my
wireless-drivers-next tree or will someone else take it?

-- 
Kalle Valo

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Vineet Gupta]

On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
> Vineet Gupta <Vineet.Gupta1@synopsys.com> writes:
> 
>> > There's already a generic implementation so use that instead.
>> > ---
>> > I'm not sure if the driver usage of atomic_or?() is correct in terms of
>> > storage size of @val for 64 bit arches.
>> >
>> > Assuming LP64 programming model for linux on say x86_64: atomic_or()
>> > callers in this driver use long (sana 64 bit) storage and pass it to
>> > atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
>> > ---
>> > Cc: Brett Rudley <brudley@broadcom.com>
>> > Cc: Arend van Spriel <arend@broadcom.com>
>> > Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
>> > Cc: Hante Meuleman <meuleman@broadcom.com>
>> > Cc: Kalle Valo <kvalo@codeaurora.org>
>> > Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>> > Cc: Daniel Kim <dekim@broadcom.com>
>> > Cc: linux-wireless@vger.kernel.org
>> > Cc: brcm80211-dev-list@broadcom.com
>> > Cc: Peter Zijlstra <peterz@infradead.org>
>> > Cc: Ingo Molnar <mingo@kernel.org>
>> > Cc: netdev@vger.kernel.org
>> > Cc: linux-arch@vger.kernel.org
>> > Cc: linux-kernel@vger.kernel.org
>> > Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> >
>> > Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> What's the plan with this patch? Should I take it to my
> wireless-drivers-next tree or will someone else take it?


Per last discussion on this topic, Arend wanted to discuss abt this with Hante.
I'm not taking it anyways so feel free to pick it up if you want !

-Vineet

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Vineet Gupta]

On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
> Vineet Gupta <Vineet.Gupta1@synopsys.com> writes:
> 
>> > There's already a generic implementation so use that instead.
>> > ---
>> > I'm not sure if the driver usage of atomic_or?() is correct in terms of
>> > storage size of @val for 64 bit arches.
>> >
>> > Assuming LP64 programming model for linux on say x86_64: atomic_or()
>> > callers in this driver use long (sana 64 bit) storage and pass it to
>> > atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
>> > ---
>> > Cc: Brett Rudley <brudley@broadcom.com>
>> > Cc: Arend van Spriel <arend@broadcom.com>
>> > Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
>> > Cc: Hante Meuleman <meuleman@broadcom.com>
>> > Cc: Kalle Valo <kvalo@codeaurora.org>
>> > Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>> > Cc: Daniel Kim <dekim@broadcom.com>
>> > Cc: linux-wireless@vger.kernel.org
>> > Cc: brcm80211-dev-list@broadcom.com
>> > Cc: Peter Zijlstra <peterz@infradead.org>
>> > Cc: Ingo Molnar <mingo@kernel.org>
>> > Cc: netdev@vger.kernel.org
>> > Cc: linux-arch@vger.kernel.org
>> > Cc: linux-kernel@vger.kernel.org
>> > Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> >
>> > Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> What's the plan with this patch? Should I take it to my
> wireless-drivers-next tree or will someone else take it?


Per last discussion on this topic, Arend wanted to discuss abt this with Hante.
I'm not taking it anyways so feel free to pick it up if you want !

-Vineet

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Arend van Spriel]

On 07/24/2015 07:22 PM, Vineet Gupta wrote:
> On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
>> Vineet Gupta <Vineet.Gupta1@synopsys.com> writes:
>>
>>>> There's already a generic implementation so use that instead.
>>>> ---
>>>> I'm not sure if the driver usage of atomic_or?() is correct in terms of
>>>> storage size of @val for 64 bit arches.
>>>>
>>>> Assuming LP64 programming model for linux on say x86_64: atomic_or()
>>>> callers in this driver use long (sana 64 bit) storage and pass it to
>>>> atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
>>>> ---
>>>> Cc: Brett Rudley <brudley@broadcom.com>
>>>> Cc: Arend van Spriel <arend@broadcom.com>
>>>> Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
>>>> Cc: Hante Meuleman <meuleman@broadcom.com>
>>>> Cc: Kalle Valo <kvalo@codeaurora.org>
>>>> Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>>>> Cc: Daniel Kim <dekim@broadcom.com>
>>>> Cc: linux-wireless@vger.kernel.org
>>>> Cc: brcm80211-dev-list@broadcom.com
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>> Cc: netdev@vger.kernel.org
>>>> Cc: linux-arch@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>>>
>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> What's the plan with this patch? Should I take it to my
>> wireless-drivers-next tree or will someone else take it?
>
>
> Per last discussion on this topic, Arend wanted to discuss abt this with Hante.
> I'm not taking it anyways so feel free to pick it up if you want !

Well, that was before your "timeline" clarification about the generic 
function. One what tree is this patch based?

Regards,
Arend

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Arend van Spriel]

On 07/24/2015 07:22 PM, Vineet Gupta wrote:
> On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
>> Vineet Gupta <Vineet.Gupta1@synopsys.com> writes:
>>
>>>> There's already a generic implementation so use that instead.
>>>> ---
>>>> I'm not sure if the driver usage of atomic_or?() is correct in terms of
>>>> storage size of @val for 64 bit arches.
>>>>
>>>> Assuming LP64 programming model for linux on say x86_64: atomic_or()
>>>> callers in this driver use long (sana 64 bit) storage and pass it to
>>>> atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
>>>> ---
>>>> Cc: Brett Rudley <brudley@broadcom.com>
>>>> Cc: Arend van Spriel <arend@broadcom.com>
>>>> Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
>>>> Cc: Hante Meuleman <meuleman@broadcom.com>
>>>> Cc: Kalle Valo <kvalo@codeaurora.org>
>>>> Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>>>> Cc: Daniel Kim <dekim@broadcom.com>
>>>> Cc: linux-wireless@vger.kernel.org
>>>> Cc: brcm80211-dev-list@broadcom.com
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>> Cc: netdev@vger.kernel.org
>>>> Cc: linux-arch@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>>>
>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> What's the plan with this patch? Should I take it to my
>> wireless-drivers-next tree or will someone else take it?
>
>
> Per last discussion on this topic, Arend wanted to discuss abt this with Hante.
> I'm not taking it anyways so feel free to pick it up if you want !

Well, that was before your "timeline" clarification about the generic 
function. One what tree is this patch based?

Regards,
Arend

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Kalle Valo]

Arend van Spriel <arend@broadcom.com> writes:

> On 07/24/2015 07:22 PM, Vineet Gupta wrote:
>> On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
>>> Vineet Gupta <Vineet.Gupta1@synopsys.com> writes:
>>>
>>>>> There's already a generic implementation so use that instead.
>>>>> ---
>>>>> I'm not sure if the driver usage of atomic_or?() is correct in terms of
>>>>> storage size of @val for 64 bit arches.
>>>>>
>>>>> Assuming LP64 programming model for linux on say x86_64: atomic_or()
>>>>> callers in this driver use long (sana 64 bit) storage and pass it to
>>>>> atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
>>>>> ---
>>>>> Cc: Brett Rudley <brudley@broadcom.com>
>>>>> Cc: Arend van Spriel <arend@broadcom.com>
>>>>> Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
>>>>> Cc: Hante Meuleman <meuleman@broadcom.com>
>>>>> Cc: Kalle Valo <kvalo@codeaurora.org>
>>>>> Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>>>>> Cc: Daniel Kim <dekim@broadcom.com>
>>>>> Cc: linux-wireless@vger.kernel.org
>>>>> Cc: brcm80211-dev-list@broadcom.com
>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>> Cc: netdev@vger.kernel.org
>>>>> Cc: linux-arch@vger.kernel.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>>>>
>>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>> What's the plan with this patch? Should I take it to my
>>> wireless-drivers-next tree or will someone else take it?
>>
>>
>> Per last discussion on this topic, Arend wanted to discuss abt this with Hante.
>> I'm not taking it anyways so feel free to pick it up if you want !
>
> Well, that was before your "timeline" clarification about the generic
> function. One what tree is this patch based?

Yeah, if this patch depends on another patch I need to know about it.
Otherwise I might break something when I apply this patch.

-- 
Kalle Valo

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Kalle Valo]

Arend van Spriel <arend@broadcom.com> writes:

> On 07/24/2015 07:22 PM, Vineet Gupta wrote:
>> On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
>>> Vineet Gupta <Vineet.Gupta1@synopsys.com> writes:
>>>
>>>>> There's already a generic implementation so use that instead.
>>>>> ---
>>>>> I'm not sure if the driver usage of atomic_or?() is correct in terms of
>>>>> storage size of @val for 64 bit arches.
>>>>>
>>>>> Assuming LP64 programming model for linux on say x86_64: atomic_or()
>>>>> callers in this driver use long (sana 64 bit) storage and pass it to
>>>>> atomic_orr/atomic_or which downcasts it to 32 bits. Is that OK ?
>>>>> ---
>>>>> Cc: Brett Rudley <brudley@broadcom.com>
>>>>> Cc: Arend van Spriel <arend@broadcom.com>
>>>>> Cc: "Franky (Zhenhui) Lin" <frankyl@broadcom.com>
>>>>> Cc: Hante Meuleman <meuleman@broadcom.com>
>>>>> Cc: Kalle Valo <kvalo@codeaurora.org>
>>>>> Cc: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>>>>> Cc: Daniel Kim <dekim@broadcom.com>
>>>>> Cc: linux-wireless@vger.kernel.org
>>>>> Cc: brcm80211-dev-list@broadcom.com
>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>>> Cc: Ingo Molnar <mingo@kernel.org>
>>>>> Cc: netdev@vger.kernel.org
>>>>> Cc: linux-arch@vger.kernel.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>>>>
>>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>> What's the plan with this patch? Should I take it to my
>>> wireless-drivers-next tree or will someone else take it?
>>
>>
>> Per last discussion on this topic, Arend wanted to discuss abt this with Hante.
>> I'm not taking it anyways so feel free to pick it up if you want !
>
> Well, that was before your "timeline" clarification about the generic
> function. One what tree is this patch based?

Yeah, if this patch depends on another patch I need to know about it.
Otherwise I might break something when I apply this patch.

-- 
Kalle Valo

Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Vineet Gupta]

On Monday 27 July 2015 01:08 PM, Kalle Valo wrote:
>>> >> Per last discussion on this topic, Arend wanted to discuss abt this with Hante.
>>> >> I'm not taking it anyways so feel free to pick it up if you want !
>> >
>> > Well, that was before your "timeline" clarification about the generic
>> > function. One what tree is this patch based?
> Yeah, if this patch depends on another patch I need to know about it.
> Otherwise I might break something when I apply this patch.

It was latest linux-next at the time, 4.1-rcx perhaps, don't remember exactly. But
it certainly doesn't depend on any new code  - the patch simply makes use of an
existing API vs. using a local hard coded version of same.

Give it a spin off your existing tree - shdn't be too difficult to test  I presume.

-Vineet

Re: [2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Kalle Valo]

> There's already a generic implementation so use that instead.

Thanks, applied to wireless-drivers-next.git.

Kalle Valo

Re: [2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

[Kalle Valo]

> There's already a generic implementation so use that instead.

Thanks, applied to wireless-drivers-next.git.

Kalle Valo

[PATCH 3/3] ARC: provide atomic_or() and define ARCH_HAS_ATOMIC_OR

[Vineet Gupta]

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: linux-arch@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/atomic.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arc/include/asm/atomic.h b/arch/arc/include/asm/atomic.h
index 03484cb4d16d..fab27dbc562b 100644
--- a/arch/arc/include/asm/atomic.h
+++ b/arch/arc/include/asm/atomic.h
@@ -141,10 +141,19 @@ static inline int atomic_##op##_return(int i, atomic_t *v)		\
 	ATOMIC_OP(op, c_op, asm_op)					\
 	ATOMIC_OP_RETURN(op, c_op, asm_op)
 
+/* atomic_add(), atomic_add_return() */
 ATOMIC_OPS(add, +=, add)
+
+/* atomic_sub(), atomic_sub_return() */
 ATOMIC_OPS(sub, -=, sub)
+
+/* atomic_and() */
 ATOMIC_OP(and, &=, and)
 
+#define ARCH_HAS_ATOMIC_OR
+/* atomic_or() */
+ATOMIC_OP(or, |=, or)
+
 #define atomic_clear_mask(mask, v) atomic_and(~(mask), (v))
 
 #undef ATOMIC_OPS
-- 
1.9.1

Re: [PATCH 0/3] atomic_or() related changes

[Peter Zijlstra]

I see what you did there.. gimme a few more hours, I'll finish what I
have and stuff it through the build bot.

I've just not managed to finish tile, but did do frv this time.

Re: [PATCH 0/3] atomic_or() related changes

[Vineet Gupta]

On Thursday 09 July 2015 06:01 PM, Peter Zijlstra wrote:
> I've just not managed to finish tile, but did do frv this time.

Not sure what you mean - are you also doing a similar series which extends
atomic_or to other arches too which will probably we simpler now given ur earlier
macro-fication of this code !

-vineet

Re: [PATCH 0/3] atomic_or() related changes

[Peter Zijlstra]

On Thu, Jul 09, 2015 at 01:05:25PM +0000, Vineet Gupta wrote:
> On Thursday 09 July 2015 06:01 PM, Peter Zijlstra wrote:
> > I've just not managed to finish tile, but did do frv this time.
> 
> Not sure what you mean - are you also doing a similar series which extends
> atomic_or to other arches too which will probably we simpler now given ur earlier
> macro-fication of this code !

Yes, one I started a long time ago, I got two rounds of arch/* atomic
cleanups in already, but never got around to actually finishing it.

The next round is almost done, only tile is missing.

My current series is here:

  https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/log/?h=locking/arch

And I just found there's a bisection fail in there, so let me go fix
that before posting.

Once I got this sorted, there's maybe one more round left.