netdev - Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 9 Jul 2015 20:31:16 +0200
From:	Arend van Spriel <arend@...adcom.com>
To:	Vineet Gupta <Vineet.Gupta1@...opsys.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, Arnd Bergmann <arnd@...db.de>
CC:	<linux-arch@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Brett Rudley <brudley@...adcom.com>,
	"Franky (Zhenhui) Lin" <frankyl@...adcom.com>,
	Hante Meuleman <meuleman@...adcom.com>,
	Kalle Valo <kvalo@...eaurora.org>,
	Pieter-Paul Giesberts <pieterpg@...adcom.com>,
	Daniel Kim <dekim@...adcom.com>,
	<linux-wireless@...r.kernel.org>,
	<brcm80211-dev-list@...adcom.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/3] brcmfmac: dhd_sdio.c: use existing atomic_or primitive

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@...adcom.com>
>> Cc: Arend van Spriel <arend@...adcom.com>
>> Cc: "Franky (Zhenhui) Lin" <frankyl@...adcom.com>
>> Cc: Hante Meuleman <meuleman@...adcom.com>
>> Cc: Kalle Valo <kvalo@...eaurora.org>
>> Cc: Pieter-Paul Giesberts <pieterpg@...adcom.com>
>> Cc: Daniel Kim <dekim@...adcom.com>
>> Cc: linux-wireless@...r.kernel.org
>> Cc: brcm80211-dev-list@...adcom.com
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Ingo Molnar <mingo@...nel.org>
>> Cc: netdev@...r.kernel.org
>> Cc: linux-arch@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Signed-off-by: Vineet Gupta <vgupta@...opsys.com>
>>
>> Signed-off-by: Vineet Gupta <vgupta@...opsys.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);
>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html