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)
^
Signed-off-by: Chen Gang <[email protected]>
---
net/bluetooth/hci_sock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 80c5a79..858b53a 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -46,7 +46,7 @@ struct hci_pinfo {
unsigned short channel;
};
-static inline int hci_test_bit(int nr, void *addr)
+static inline int hci_test_bit(int nr, const void *addr)
{
return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
}
--
1.9.3
On Tue, 2015-02-03 at 05:14 +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):
[]
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
[]
> @@ -46,7 +46,7 @@ struct hci_pinfo {
> unsigned short channel;
> };
>
> -static inline int hci_test_bit(int nr, void *addr)
> +static inline int hci_test_bit(int nr, const void *addr)
> {
> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
> }
It's probably better to use const __u32 * here too, but the
real thing I wonder is whether or not there's an issue with
one of the 2 uses of this function.
One of them passes a unsigned long *, the other a u32 *.
$ git grep -w hci_test_bit
net/bluetooth/hci_sock.c:static inline int hci_test_bit(int nr, void *addr)
net/bluetooth/hci_sock.c: if (!hci_test_bit(flt_event, &flt->event_mask))
net/bluetooth/hci_sock.c: !hci_test_bit(ocf & HCI_FLT_OCF_BITS,
net/bluetooth/hci_sock.c- &hci_sec_filter.ocf_mask[ogf])) &&
hci_sec_filter.ocf_mask is __u32
but flt->event_mask is unsigned long.
Any possible issue here on 64-bit systems?
---
$ git grep -A4 "struct hci_filter {"
include/net/bluetooth/hci_sock.h:struct hci_filter {
include/net/bluetooth/hci_sock.h- unsigned long type_mask;
include/net/bluetooth/hci_sock.h- unsigned long event_mask[2];
include/net/bluetooth/hci_sock.h- __le16 opcode;
include/net/bluetooth/hci_sock.h-};
---
static bool is_filtered_packet(struct sock *sk, struct sk_buff *skb)
{
struct hci_filter *flt;
[...]
if (!hci_test_bit(flt_event, &flt->event_mask))
return true;
On 2/3/15 05:20, Joe Perches wrote:
> On Tue, 2015-02-03 at 05:14 +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):
> []
>> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> []
>> @@ -46,7 +46,7 @@ struct hci_pinfo {
>> unsigned short channel;
>> };
>>
>> -static inline int hci_test_bit(int nr, void *addr)
>> +static inline int hci_test_bit(int nr, const void *addr)
>> {
>> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
>> }
>
> It's probably better to use const __u32 * here too, but the
> real thing I wonder is whether or not there's an issue with
> one of the 2 uses of this function.
>
> One of them passes a unsigned long *, the other a u32 *.
>
> $ git grep -w hci_test_bit
> net/bluetooth/hci_sock.c:static inline int hci_test_bit(int nr, void *addr)
> net/bluetooth/hci_sock.c: if (!hci_test_bit(flt_event, &flt->event_mask))
> net/bluetooth/hci_sock.c: !hci_test_bit(ocf & HCI_FLT_OCF_BITS,
> net/bluetooth/hci_sock.c- &hci_sec_filter.ocf_mask[ogf])) &&
>
> hci_sec_filter.ocf_mask is __u32
> but flt->event_mask is unsigned long.
>
> Any possible issue here on 64-bit systems?
>
For me, it can not cause issue on 64-bit systems. hci_test_bit() treats
'addr' as "__u32 *", and has to use the pointer to do something.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
On 2/3/15 10:32, Chen Gang S wrote:
> On 2/3/15 05:20, Joe Perches wrote:
>> On Tue, 2015-02-03 at 05:14 +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):
>> []
>>> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
>> []
>>> @@ -46,7 +46,7 @@ struct hci_pinfo {
>>> unsigned short channel;
>>> };
>>>
>>> -static inline int hci_test_bit(int nr, void *addr)
>>> +static inline int hci_test_bit(int nr, const void *addr)
>>> {
>>> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
>>> }
>>
>> It's probably better to use const __u32 * here too, but the
>> real thing I wonder is whether or not there's an issue with
>> one of the 2 uses of this function.
>>
>> One of them passes a unsigned long *, the other a u32 *.
>>
>> $ git grep -w hci_test_bit
>> net/bluetooth/hci_sock.c:static inline int hci_test_bit(int nr, void *addr)
>> net/bluetooth/hci_sock.c: if (!hci_test_bit(flt_event, &flt->event_mask))
>> net/bluetooth/hci_sock.c: !hci_test_bit(ocf & HCI_FLT_OCF_BITS,
>> net/bluetooth/hci_sock.c- &hci_sec_filter.ocf_mask[ogf])) &&
>>
>> hci_sec_filter.ocf_mask is __u32
>> but flt->event_mask is unsigned long.
>>
>> Any possible issue here on 64-bit systems?
>>
>
> For me, it can not cause issue on 64-bit systems. hci_test_bit() treats
> 'addr' as "__u32 *", and has to use the pointer to do something.
>
'event_mask' is intended to type cast to "__u32 *" within 'hci_sock.c'.
So for me, "const __u32 *" is better than "const void *" for 2nd
parameter of hci_test_bit().
If what I said above is correct, and also if necessary, I shall patch v3
for it.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
From: Chen Gang S
> -static inline int hci_test_bit(int nr, void *addr)
> +static inline int hci_test_bit(int nr, const void *addr)
> {
> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
> }
Is there a 'standard' function lurking that will do the above.
On x86 the cpus 'bit test' instruction will handle bit numbers
greater than the word size - so it can be a single instruction.
David
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
From: Gang S
> On 2/3/15 10:32, Chen Gang S wrote:
> > On 2/3/15 05:20, Joe Perches wrote:
> >> On Tue, 2015-02-03 at 05:14 +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):
> >> []
> >>> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> >> []
> >>> @@ -46,7 +46,7 @@ struct hci_pinfo {
> >>> unsigned short channel;
> >>> };
> >>>
> >>> -static inline int hci_test_bit(int nr, void *addr)
> >>> +static inline int hci_test_bit(int nr, const void *addr)
> >>> {
> >>> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
> >>> }
> >>
> >> It's probably better to use const __u32 * here too, but the
> >> real thing I wonder is whether or not there's an issue with
> >> one of the 2 uses of this function.
> >>
> >> One of them passes a unsigned long *, the other a u32 *.
> >>
> >> $ git grep -w hci_test_bit
> >> net/bluetooth/hci_sock.c:static inline int hci_test_bit(int nr, void *addr)
> >> net/bluetooth/hci_sock.c: if (!hci_test_bit(flt_event, &flt->event_mask))
> >> net/bluetooth/hci_sock.c: !hci_test_bit(ocf & HCI_FLT_OCF_BITS,
> >> net/bluetooth/hci_sock.c- &hci_sec_filter.ocf_mask[ogf])) &&
> >>
> >> hci_sec_filter.ocf_mask is __u32
> >> but flt->event_mask is unsigned long.
> >>
> >> Any possible issue here on 64-bit systems?
> >>
> >
> > For me, it can not cause issue on 64-bit systems. hci_test_bit() treats
> > 'addr' as "__u32 *", and has to use the pointer to do something.
> >
>
> 'event_mask' is intended to type cast to "__u32 *" within 'hci_sock.c'.
> So for me, "const __u32 *" is better than "const void *" for 2nd
> parameter of hci_test_bit().
>
> If what I said above is correct, and also if necessary, I shall patch v3
> for it.
How are the bits set in the first place?
If the array is ever indexed as long [] then the code above is unlikely
to be testing the correct bits.
David
Hello.
On 02/04/2015 02:59 PM, David Laight wrote:
>> -static inline int hci_test_bit(int nr, void *addr)
>> +static inline int hci_test_bit(int nr, const void *addr)
>> {
>> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
>> }
> Is there a 'standard' function lurking that will do the above.
> On x86 the cpus 'bit test' instruction will handle bit numbers
> greater than the word size - so it can be a single instruction.
Of course, there's test_bit().
> David
WBR, Sergei
Hi Sergei,
>>> -static inline int hci_test_bit(int nr, void *addr)
>>> +static inline int hci_test_bit(int nr, const void *addr)
>>> {
>>> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
>>> }
>
>> Is there a 'standard' function lurking that will do the above.
>> On x86 the cpus 'bit test' instruction will handle bit numbers
>> greater than the word size - so it can be a single instruction.
>
> Of course, there's test_bit().
we did leave hci_test_bit in the code since there are some userspace facing API that we can not change. Remember that the origin of this code is from 2.4.6 kernel.
So we can only change this if you can ensure not to break the userspace API. So might want to write unit tests to ensure working HCI filter before even considering touching this.
Regards
Marcel
On 2/5/15 05:09, Marcel Holtmann wrote:
> Hi Sergei,
>
>>>> -static inline int hci_test_bit(int nr, void *addr)
>>>> +static inline int hci_test_bit(int nr, const void *addr)
>>>> {
>>>> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
>>>> }
>>
>>> Is there a 'standard' function lurking that will do the above.
>>> On x86 the cpus 'bit test' instruction will handle bit numbers
>>> greater than the word size - so it can be a single instruction.
>>
>> Of course, there's test_bit().
>
> we did leave hci_test_bit in the code since there are some userspace facing API that we can not change. Remember that the origin of this code is from 2.4.6 kernel.
>
> So we can only change this if you can ensure not to break the userspace API. So might want to write unit tests to ensure working HCI filter before even considering touching this.
>
For me, we have to remain hci_test_bit(), it is for "__u32 *" (which we
can not change). The common test_bit() is for "unsigned long *", in this
case, I guess it may cause issue under 64-bit environments.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
From: Chen Gang S [mailto:[email protected]]
> On 2/5/15 05:09, Marcel Holtmann wrote:
> > Hi Sergei,
> >
> >>>> -static inline int hci_test_bit(int nr, void *addr)
> >>>> +static inline int hci_test_bit(int nr, const void *addr)
> >>>> {
> >>>> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
> >>>> }
> >>
> >>> Is there a 'standard' function lurking that will do the above.
> >>> On x86 the cpus 'bit test' instruction will handle bit numbers
> >>> greater than the word size - so it can be a single instruction.
> >>
> >> Of course, there's test_bit().
> >
> > we did leave hci_test_bit in the code since there are some userspace facing
> > API that we can not change. Remember that the origin of this code is
> > from 2.4.6 kernel.
> >
> > So we can only change this if you can ensure not to break the userspace API.
> > So might want to write unit tests to ensure working HCI filter before even
> > considering touching this.
> >
>
> For me, we have to remain hci_test_bit(), it is for "__u32 *" (which we
> can not change). The common test_bit() is for "unsigned long *", in this
> case, I guess it may cause issue under 64-bit environments.
Except that half the time you are passing a 'long *' and you haven't
explained why this isn't broken on 64bit architectures.
Note that on LE systems the size of the accesses used to access a dense
bit array don't matter. This is not true of BE systems.
David
On 2/5/15 18:14, David Laight wrote:
> From: Chen Gang S [mailto:[email protected]]
>> On 2/5/15 05:09, Marcel Holtmann wrote:
>>> Hi Sergei,
>>>
>>>>>> -static inline int hci_test_bit(int nr, void *addr)
>>>>>> +static inline int hci_test_bit(int nr, const void *addr)
>>>>>> {
>>>>>> return *((__u32 *) addr + (nr >> 5)) & ((__u32) 1 << (nr & 31));
>>>>>> }
>>>>
>>>>> Is there a 'standard' function lurking that will do the above.
>>>>> On x86 the cpus 'bit test' instruction will handle bit numbers
>>>>> greater than the word size - so it can be a single instruction.
>>>>
>>>> Of course, there's test_bit().
>>>
>>> we did leave hci_test_bit in the code since there are some userspace facing
>>> API that we can not change. Remember that the origin of this code is
>>> from 2.4.6 kernel.
>>>
>>> So we can only change this if you can ensure not to break the userspace API.
>>> So might want to write unit tests to ensure working HCI filter before even
>>> considering touching this.
>>>
>>
>> For me, we have to remain hci_test_bit(), it is for "__u32 *" (which we
>> can not change). The common test_bit() is for "unsigned long *", in this
>> case, I guess it may cause issue under 64-bit environments.
>
> Except that half the time you are passing a 'long *' and you haven't
> explained why this isn't broken on 64bit architectures.
>
Maybe we are misunderstanding with each other (excuse me for my pool
English). What I want to say is:
- hci_test_bit() is OK (current implementation can not cause issue for
64-bit machine).
- But if we use test_bit(), I guess it will cause issue for 64-bit
machine.
> Note that on LE systems the size of the accesses used to access a dense
> bit array don't matter. This is not true of BE systems.
>
Yes, what you said above sounds reasonable to me, too.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed