* [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).