Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756208AbbBHMWu (ORCPT ); Sun, 8 Feb 2015 07:22:50 -0500 Received: from out114-136.biz.mail.alibaba.com ([205.204.114.136]:59326 "EHLO out21.biz.mail.alibaba.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754627AbbBHMWt (ORCPT ); Sun, 8 Feb 2015 07:22:49 -0500 X-Alimail-AntiSpam: AC=CONTINUE;BC=0.07426073|-1;FP=0|0|0|0|0|-1|-1|-1;HT=r41g03015;MF=gang.chen@sunrus.com.cn;PH=DS;RN=10;RT=10;SR=0; Message-ID: <54D75698.7010808@sunrus.com.cn> Date: Sun, 08 Feb 2015 20:29:12 +0800 From: Chen Gang S User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Joe Perches CC: David Laight , Marcel Holtmann , Sergei Shtylyov , "Gustavo F. Padovan" , Johan Hedberg , "David S. Miller" , "linux-bluetooth@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "netdev@vger.kernel.org" Subject: Re: [PATCH v3] net: bluetooth: hci_sock: Use 'const u32 *' instead of 'void *' for 2nd parameter of hci_test_bit() References: <54D61229.9010904@sunrus.com.cn> <1423338774.2933.9.camel@perches.com> <54D6A729.1070905@sunrus.com.cn> In-Reply-To: <54D6A729.1070905@sunrus.com.cn> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3851 Lines: 85 On 2/8/15 08:00, Chen Gang S wrote: > On 2/8/15 03:52, Joe Perches wrote: >> On Sat, 2015-02-07 at 21:24 +0800, Chen Gang S wrote: >>> hci_test_bit() does not modify 2nd parameter, so it is better to let it >>> be constant, or may cause build warning. The related warning (with >>> allmodconfig under xtensa): >>> >>> net/bluetooth/hci_sock.c: In function 'hci_sock_sendmsg': >>> net/bluetooth/hci_sock.c:955:8: warning: passing argument 2 of 'hci_test_bit' discards 'const' qualifier from pointer target type [-Wdiscarded-array-qualifiers] >>> &hci_sec_filter.ocf_mask[ogf])) && >>> ^ >>> net/bluetooth/hci_sock.c:49:19: note: expected 'void *' but argument is of type 'const __u32 (*)[4] {aka const unsigned int (*)[4]}' >>> static inline int hci_test_bit(int nr, void *addr) >>> ^ >>> >>> hci_test_bit() always treats 2nd parameter is u32, and all callers also >>> know about it, so 2nd parameter of hci_test_bit() need use 'const u32 *' >>> instead of 'void *'. >>> >>> C language treats the array function parameter as a pointer, so the >>> caller need not use '&' for the 2 demotion array, or it reports warning: >>> 'const unsigned int (*)[4]' is different with 'const unsigned int *'. >> >> I still think you are possibly papering over potential bugs >> on big-endian 64 bit systems. >> >> unsigned long vs u32. >> >> How are the bits actually set? >> > >>From current usage of event_mask, "(u32 *) f->event_mask" is only for > event_mask data storage, not for calculation (always as "u32 *" for > calculation). > > [root@localhost linux-next]# grep -rn "\" include/net/bluetooth net/bluetooth > include/net/bluetooth/hci_sock.h:51: unsigned long event_mask[2]; e.g. use "unsigned char event_mask[2 * sizeof(unsigned long)]" instead of "unsigned long event_mask[2]". There is still no any issue within "hci_sock.c" (although I am not sure whether this modification may cause issues in another modules outside kernel). Thanks. > include/net/bluetooth/hci_sock.h:57: __u32 event_mask[2]; > net/bluetooth/hci_sock.c:59: __u32 event_mask[2]; > net/bluetooth/hci_sock.c:110: if (!hci_test_bit(flt_event, (u32 *)&flt->event_mask)) > net/bluetooth/hci_sock.c:1041: uf.event_mask[0] = *((u32 *) f->event_mask + 0); > net/bluetooth/hci_sock.c:1042: uf.event_mask[1] = *((u32 *) f->event_mask + 1); > net/bluetooth/hci_sock.c:1053: uf.event_mask[0] &= *((u32 *) hci_sec_filter.event_mask + 0); > net/bluetooth/hci_sock.c:1054: uf.event_mask[1] &= *((u32 *) hci_sec_filter.event_mask + 1); > net/bluetooth/hci_sock.c:1062: *((u32 *) f->event_mask + 0) = uf.event_mask[0]; > net/bluetooth/hci_sock.c:1063: *((u32 *) f->event_mask + 1) = uf.event_mask[1]; > net/bluetooth/hci_sock.c:1124: uf.event_mask[0] = *((u32 *) f->event_mask + 0); > net/bluetooth/hci_sock.c:1125: uf.event_mask[1] = *((u32 *) f->event_mask + 1); > > Calculation is machine endian dependency, but event_mask is always as > "u32 *" for calculation, so there is no any type cast for calculation, > it is OK. > > Storage is independent from machine endian, but it depends on machine > bits. In our case, 'unsigned long' array has enough space to accept u32 > array, so there is no any data overwritten, it is OK. > > > By the way, I intended to remain event_mask as 'unsigned long' type, > because I am not quite sure whether it is also used by another modules > in kernel (or any other systems). May we change it to u32? > > Thanks. > -- Chen Gang Open, share, and attitude like air, water, and life which God blessed -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/