* [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
@ 2020-09-16 15:03 Srinivas Kandagatla
2020-09-16 15:20 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Srinivas Kandagatla @ 2020-09-16 15:03 UTC (permalink / raw)
To: davem, gregkh; +Cc: vkoul, linux-kernel, Srinivas Kandagatla
usage of apis like u32_replace_bits() without actually catching the return
value could hide problems without any warning!
Found this with recent usage of this api in SoundWire!
Having __must_check annotation would really catch this issues in future!
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
include/linux/bitfield.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 4e035aca6f7e..eb4f69253946 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -131,7 +131,7 @@ static __always_inline __##type type##_encode_bits(base v, base field) \
__field_overflow(); \
return to((v & field_mask(field)) * field_multiplier(field)); \
} \
-static __always_inline __##type type##_replace_bits(__##type old, \
+static __always_inline __must_check __##type type##_replace_bits(__##type old, \
base val, base field) \
{ \
return (old & ~to(field)) | type##_encode_bits(val, field); \
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
2020-09-16 15:03 [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check Srinivas Kandagatla
@ 2020-09-16 15:20 ` Greg KH
2020-09-16 15:33 ` Srinivas Kandagatla
0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2020-09-16 15:20 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: davem, vkoul, linux-kernel
On Wed, Sep 16, 2020 at 04:03:33PM +0100, Srinivas Kandagatla wrote:
> usage of apis like u32_replace_bits() without actually catching the return
> value could hide problems without any warning!
>
> Found this with recent usage of this api in SoundWire!
> Having __must_check annotation would really catch this issues in future!
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> include/linux/bitfield.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 4e035aca6f7e..eb4f69253946 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -131,7 +131,7 @@ static __always_inline __##type type##_encode_bits(base v, base field) \
> __field_overflow(); \
> return to((v & field_mask(field)) * field_multiplier(field)); \
> } \
> -static __always_inline __##type type##_replace_bits(__##type old, \
> +static __always_inline __must_check __##type type##_replace_bits(__##type old, \
> base val, base field) \
> { \
> return (old & ~to(field)) | type##_encode_bits(val, field); \
> --
> 2.21.0
>
Don't add __must_check to things that if merged will instantly cause
build warnings to the system, that's just rude :(
Fix up everything first, and then try to make this type of change.
But why does this function have to be checked?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
2020-09-16 15:20 ` Greg KH
@ 2020-09-16 15:33 ` Srinivas Kandagatla
2020-09-17 4:04 ` Vinod Koul
0 siblings, 1 reply; 5+ messages in thread
From: Srinivas Kandagatla @ 2020-09-16 15:33 UTC (permalink / raw)
To: Greg KH; +Cc: davem, vkoul, linux-kernel
On 16/09/2020 16:20, Greg KH wrote:
> On Wed, Sep 16, 2020 at 04:03:33PM +0100, Srinivas Kandagatla wrote:
>> usage of apis like u32_replace_bits() without actually catching the return
>> value could hide problems without any warning!
>>
>> Found this with recent usage of this api in SoundWire!
>> Having __must_check annotation would really catch this issues in future!
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> include/linux/bitfield.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>> index 4e035aca6f7e..eb4f69253946 100644
>> --- a/include/linux/bitfield.h
>> +++ b/include/linux/bitfield.h
>> @@ -131,7 +131,7 @@ static __always_inline __##type type##_encode_bits(base v, base field) \
>> __field_overflow(); \
>> return to((v & field_mask(field)) * field_multiplier(field)); \
>> } \
>> -static __always_inline __##type type##_replace_bits(__##type old, \
>> +static __always_inline __must_check __##type type##_replace_bits(__##type old, \
>> base val, base field) \
>> { \
>> return (old & ~to(field)) | type##_encode_bits(val, field); \
>> --
>> 2.21.0
>>
>
> Don't add __must_check to things that if merged will instantly cause
> build warnings to the system, that's just rude :(
Currently there are not many users for this api, found only one instance
in drivers/net/ipa/ipa_table.c which is also fixed with
https://lkml.org/lkml/2020/9/10/1062
>
> Fix up everything first, and then try to make this type of change.
>
> But why does this function have to be checked?
As this function would return updated value, this check is more to with
using the return value!
It is easy for someone to ignore this return value assuming that the new
value is already updated! So this check should help!
TBH, This is what happened when we(vkoul and me) tried use this api :-)
--srini
>
> thanks,
>
> greg k-h
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
2020-09-16 15:33 ` Srinivas Kandagatla
@ 2020-09-17 4:04 ` Vinod Koul
2020-09-17 5:24 ` Greg KH
0 siblings, 1 reply; 5+ messages in thread
From: Vinod Koul @ 2020-09-17 4:04 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: Greg KH, davem, linux-kernel
On 16-09-20, 16:33, Srinivas Kandagatla wrote:
>
>
> On 16/09/2020 16:20, Greg KH wrote:
> > On Wed, Sep 16, 2020 at 04:03:33PM +0100, Srinivas Kandagatla wrote:
> > > usage of apis like u32_replace_bits() without actually catching the return
> > > value could hide problems without any warning!
> > >
> > > Found this with recent usage of this api in SoundWire!
> > > Having __must_check annotation would really catch this issues in future!
> > >
> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > ---
> > > include/linux/bitfield.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > > index 4e035aca6f7e..eb4f69253946 100644
> > > --- a/include/linux/bitfield.h
> > > +++ b/include/linux/bitfield.h
> > > @@ -131,7 +131,7 @@ static __always_inline __##type type##_encode_bits(base v, base field) \
> > > __field_overflow(); \
> > > return to((v & field_mask(field)) * field_multiplier(field)); \
> > > } \
> > > -static __always_inline __##type type##_replace_bits(__##type old, \
> > > +static __always_inline __must_check __##type type##_replace_bits(__##type old, \
> > > base val, base field) \
> > > { \
> > > return (old & ~to(field)) | type##_encode_bits(val, field); \
> > > --
> > > 2.21.0
> > >
> >
> > Don't add __must_check to things that if merged will instantly cause
> > build warnings to the system, that's just rude :(
> Currently there are not many users for this api, found only one instance in
> drivers/net/ipa/ipa_table.c which is also fixed with
> https://lkml.org/lkml/2020/9/10/1062
>
> >
> > Fix up everything first, and then try to make this type of change.
> >
> > But why does this function have to be checked?
> As this function would return updated value, this check is more to with
> using the return value!
>
> It is easy for someone to ignore this return value assuming that the new
> value is already updated! So this check should help!
>
> TBH, This is what happened when we(vkoul and me) tried use this api :-)
So the only user of this has been moved to *p_replace_bits(), looking
back I think we should remove *_replace_bits (no users atm) and
duplicate of *p_replace_bits(). If not adding this patch would be
sensible thing to do
Somehow I feel former is a better idea ;-)
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
2020-09-17 4:04 ` Vinod Koul
@ 2020-09-17 5:24 ` Greg KH
0 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2020-09-17 5:24 UTC (permalink / raw)
To: Vinod Koul; +Cc: Srinivas Kandagatla, davem, linux-kernel
On Thu, Sep 17, 2020 at 09:34:59AM +0530, Vinod Koul wrote:
> On 16-09-20, 16:33, Srinivas Kandagatla wrote:
> >
> >
> > On 16/09/2020 16:20, Greg KH wrote:
> > > On Wed, Sep 16, 2020 at 04:03:33PM +0100, Srinivas Kandagatla wrote:
> > > > usage of apis like u32_replace_bits() without actually catching the return
> > > > value could hide problems without any warning!
> > > >
> > > > Found this with recent usage of this api in SoundWire!
> > > > Having __must_check annotation would really catch this issues in future!
> > > >
> > > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > > ---
> > > > include/linux/bitfield.h | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > > > index 4e035aca6f7e..eb4f69253946 100644
> > > > --- a/include/linux/bitfield.h
> > > > +++ b/include/linux/bitfield.h
> > > > @@ -131,7 +131,7 @@ static __always_inline __##type type##_encode_bits(base v, base field) \
> > > > __field_overflow(); \
> > > > return to((v & field_mask(field)) * field_multiplier(field)); \
> > > > } \
> > > > -static __always_inline __##type type##_replace_bits(__##type old, \
> > > > +static __always_inline __must_check __##type type##_replace_bits(__##type old, \
> > > > base val, base field) \
> > > > { \
> > > > return (old & ~to(field)) | type##_encode_bits(val, field); \
> > > > --
> > > > 2.21.0
> > > >
> > >
> > > Don't add __must_check to things that if merged will instantly cause
> > > build warnings to the system, that's just rude :(
> > Currently there are not many users for this api, found only one instance in
> > drivers/net/ipa/ipa_table.c which is also fixed with
> > https://lkml.org/lkml/2020/9/10/1062
> >
> > >
> > > Fix up everything first, and then try to make this type of change.
> > >
> > > But why does this function have to be checked?
> > As this function would return updated value, this check is more to with
> > using the return value!
> >
> > It is easy for someone to ignore this return value assuming that the new
> > value is already updated! So this check should help!
> >
> > TBH, This is what happened when we(vkoul and me) tried use this api :-)
>
> So the only user of this has been moved to *p_replace_bits(), looking
> back I think we should remove *_replace_bits (no users atm) and
> duplicate of *p_replace_bits(). If not adding this patch would be
> sensible thing to do
>
> Somehow I feel former is a better idea ;-)
Yes, please remove it if there is no users.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-17 5:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 15:03 [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check Srinivas Kandagatla
2020-09-16 15:20 ` Greg KH
2020-09-16 15:33 ` Srinivas Kandagatla
2020-09-17 4:04 ` Vinod Koul
2020-09-17 5:24 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).