Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
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 > > > > --- > > > > 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
Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
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 > > > --- > > > 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
[PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
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 --- 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
Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
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 > --- > 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
Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
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 --- 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