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 <[email protected]>
Cc: Arend van Spriel <[email protected]>
Cc: "Franky (Zhenhui) Lin" <[email protected]>
Cc: Hante Meuleman <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Pieter-Paul Giesberts <[email protected]>
Cc: Daniel Kim <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Vineet Gupta <[email protected]>
Signed-off-by: Vineet Gupta <[email protected]>
---
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
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.
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 <[email protected]>
>> Cc: Arend van Spriel <[email protected]>
>> Cc: "Franky (Zhenhui) Lin" <[email protected]>
>> Cc: Hante Meuleman <[email protected]>
>> Cc: Kalle Valo <[email protected]>
>> Cc: Pieter-Paul Giesberts <[email protected]>
>> Cc: Daniel Kim <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: Peter Zijlstra <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Vineet Gupta <[email protected]>
>>
>> Signed-off-by: Vineet Gupta <[email protected]>
>> ---
>> 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);
>>
>>
>
On 07/24/2015 07:22 PM, Vineet Gupta wrote:
> On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
>> Vineet Gupta <[email protected]> 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 <[email protected]>
>>>> Cc: Arend van Spriel <[email protected]>
>>>> Cc: "Franky (Zhenhui) Lin" <[email protected]>
>>>> Cc: Hante Meuleman <[email protected]>
>>>> Cc: Kalle Valo <[email protected]>
>>>> Cc: Pieter-Paul Giesberts <[email protected]>
>>>> Cc: Daniel Kim <[email protected]>
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Cc: Peter Zijlstra <[email protected]>
>>>> Cc: Ingo Molnar <[email protected]>
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Cc: [email protected]
>>>> Signed-off-by: Vineet Gupta <[email protected]>
>>>>
>>>> Signed-off-by: Vineet Gupta <[email protected]>
>> 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
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 <[email protected]>
> Cc: Arend van Spriel <[email protected]>
> Cc: "Franky (Zhenhui) Lin" <[email protected]>
> Cc: Hante Meuleman <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: Pieter-Paul Giesberts <[email protected]>
> Cc: Daniel Kim <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Vineet Gupta <[email protected]>
>
> Signed-off-by: Vineet Gupta <[email protected]>
> ---
> 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);
>
>
On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
> Vineet Gupta <[email protected]> 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 <[email protected]>
>> > Cc: Arend van Spriel <[email protected]>
>> > Cc: "Franky (Zhenhui) Lin" <[email protected]>
>> > Cc: Hante Meuleman <[email protected]>
>> > Cc: Kalle Valo <[email protected]>
>> > Cc: Pieter-Paul Giesberts <[email protected]>
>> > Cc: Daniel Kim <[email protected]>
>> > Cc: [email protected]
>> > Cc: [email protected]
>> > Cc: Peter Zijlstra <[email protected]>
>> > Cc: Ingo Molnar <[email protected]>
>> > Cc: [email protected]
>> > Cc: [email protected]
>> > Cc: [email protected]
>> > Signed-off-by: Vineet Gupta <[email protected]>
>> >
>> > Signed-off-by: Vineet Gupta <[email protected]>
> 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
Vineet Gupta <[email protected]> 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 <[email protected]>
> Cc: Arend van Spriel <[email protected]>
> Cc: "Franky (Zhenhui) Lin" <[email protected]>
> Cc: Hante Meuleman <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: Pieter-Paul Giesberts <[email protected]>
> Cc: Daniel Kim <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Vineet Gupta <[email protected]>
>
> Signed-off-by: Vineet Gupta <[email protected]>
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
Arend van Spriel <[email protected]> writes:
> On 07/24/2015 07:22 PM, Vineet Gupta wrote:
>> On Friday 24 July 2015 08:02 PM, Kalle Valo wrote:
>>> Vineet Gupta <[email protected]> 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 <[email protected]>
>>>>> Cc: Arend van Spriel <[email protected]>
>>>>> Cc: "Franky (Zhenhui) Lin" <[email protected]>
>>>>> Cc: Hante Meuleman <[email protected]>
>>>>> Cc: Kalle Valo <[email protected]>
>>>>> Cc: Pieter-Paul Giesberts <[email protected]>
>>>>> Cc: Daniel Kim <[email protected]>
>>>>> Cc: [email protected]
>>>>> Cc: [email protected]
>>>>> Cc: Peter Zijlstra <[email protected]>
>>>>> Cc: Ingo Molnar <[email protected]>
>>>>> Cc: [email protected]
>>>>> Cc: [email protected]
>>>>> Cc: [email protected]
>>>>> Signed-off-by: Vineet Gupta <[email protected]>
>>>>>
>>>>> Signed-off-by: Vineet Gupta <[email protected]>
>>> 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
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
> There's already a generic implementation so use that instead.
Thanks, applied to wireless-drivers-next.git.
Kalle Valo