From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: davem@davemloft.net, vkoul@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] bitfield.h: annotate type_replace_bits functions with __must_check
Date: Wed, 16 Sep 2020 16:33:49 +0100 [thread overview]
Message-ID: <66657ed6-b8ef-7e8d-b77c-9fd65453f53e@linaro.org> (raw)
In-Reply-To: <20200916152059.GA3018065@kroah.com>
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
>
next prev parent reply other threads:[~2020-09-16 15:56 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-09-17 4:04 ` Vinod Koul
2020-09-17 5:24 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=66657ed6-b8ef-7e8d-b77c-9fd65453f53e@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=davem@davemloft.net \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).