2020-05-06 03:05:14

by Zou Wei

Subject: [PATCH -next] iwlwifi: pcie: Use bitwise instead of arithmetic operator for flags

This silences the following coccinelle warning:

"WARNING: sum of probable bitmasks, consider |"

Reported-by: Hulk Robot <[email protected]>
Signed-off-by: Samuel Zou <[email protected]>
---
drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index a0daae0..6d9bf9f 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -109,9 +109,9 @@ void iwl_trans_pcie_dump_regs(struct iwl_trans *trans)

/* Alloc a max size buffer */
alloc_size = PCI_ERR_ROOT_ERR_SRC + 4 + PREFIX_LEN;
- alloc_size = max_t(u32, alloc_size, PCI_DUMP_SIZE + PREFIX_LEN);
- alloc_size = max_t(u32, alloc_size, PCI_MEM_DUMP_SIZE + PREFIX_LEN);
- alloc_size = max_t(u32, alloc_size, PCI_PARENT_DUMP_SIZE + PREFIX_LEN);
+ alloc_size = max_t(u32, alloc_size, PCI_DUMP_SIZE | PREFIX_LEN);
+ alloc_size = max_t(u32, alloc_size, PCI_MEM_DUMP_SIZE | PREFIX_LEN);
+ alloc_size = max_t(u32, alloc_size, PCI_PARENT_DUMP_SIZE | PREFIX_LEN);

buf = kmalloc(alloc_size, GFP_ATOMIC);
if (!buf)
--
2.6.2

2020-05-06 03:20:46

by Joe Perches

Subject: Re: [PATCH -next] iwlwifi: pcie: Use bitwise instead of arithmetic operator for flags

On Wed, 2020-05-06 at 11:07 +0800, Samuel Zou wrote:
> This silences the following coccinelle warning:
>
> "WARNING: sum of probable bitmasks, consider |"

I suggest instead ignoring bad and irrelevant warnings.

PREFIX_LEN is 32 not 0x20 or BIT(5)
PCI_DUMP_SIZE is 352

> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
[]
> @@ -109,9 +109,9 @@ void iwl_trans_pcie_dump_regs(struct iwl_trans *trans)
>
> /* Alloc a max size buffer */
> alloc_size = PCI_ERR_ROOT_ERR_SRC + 4 + PREFIX_LEN;
> - alloc_size = max_t(u32, alloc_size, PCI_DUMP_SIZE + PREFIX_LEN);
> - alloc_size = max_t(u32, alloc_size, PCI_MEM_DUMP_SIZE + PREFIX_LEN);
> - alloc_size = max_t(u32, alloc_size, PCI_PARENT_DUMP_SIZE + PREFIX_LEN);
> + alloc_size = max_t(u32, alloc_size, PCI_DUMP_SIZE | PREFIX_LEN);
> + alloc_size = max_t(u32, alloc_size, PCI_MEM_DUMP_SIZE | PREFIX_LEN);
> + alloc_size = max_t(u32, alloc_size, PCI_PARENT_DUMP_SIZE | PREFIX_LEN);
>
> buf = kmalloc(alloc_size, GFP_ATOMIC);
> if (!buf)

2020-05-06 13:52:36

by Luciano Coelho

Subject: Re: [PATCH -next] iwlwifi: pcie: Use bitwise instead of arithmetic operator for flags

On Tue, 2020-05-05 at 20:19 -0700, Joe Perches wrote:
> On Wed, 2020-05-06 at 11:07 +0800, Samuel Zou wrote:
> > This silences the following coccinelle warning:
> >
> > "WARNING: sum of probable bitmasks, consider |"
>
> I suggest instead ignoring bad and irrelevant warnings.
>
> PREFIX_LEN is 32 not 0x20 or BIT(5)
> PCI_DUMP_SIZE is 352
>
> > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
> []
> > @@ -109,9 +109,9 @@ void iwl_trans_pcie_dump_regs(struct iwl_trans *trans)
> >
> > /* Alloc a max size buffer */
> > alloc_size = PCI_ERR_ROOT_ERR_SRC + 4 + PREFIX_LEN;
> > - alloc_size = max_t(u32, alloc_size, PCI_DUMP_SIZE + PREFIX_LEN);
> > - alloc_size = max_t(u32, alloc_size, PCI_MEM_DUMP_SIZE + PREFIX_LEN);
> > - alloc_size = max_t(u32, alloc_size, PCI_PARENT_DUMP_SIZE + PREFIX_LEN);
> > + alloc_size = max_t(u32, alloc_size, PCI_DUMP_SIZE | PREFIX_LEN);
> > + alloc_size = max_t(u32, alloc_size, PCI_MEM_DUMP_SIZE | PREFIX_LEN);
> > + alloc_size = max_t(u32, alloc_size, PCI_PARENT_DUMP_SIZE | PREFIX_LEN);
> >
> > buf = kmalloc(alloc_size, GFP_ATOMIC);
> > if (!buf)

Yeah, those macros are clearly not bitmasks. I'm dropping this patch.

--
Cheers,
Luca.

2020-05-07 04:12:00

by Zou Wei

Subject: Re: [PATCH -next] iwlwifi: pcie: Use bitwise instead of arithmetic operator for flags

Both of you are right.
I neglected, and this patch is wrong.

Thanks.

On 2020/5/6 23:15, Joe Perches wrote:
> On Wed, 2020-05-06 at 16:51 +0300, Luciano Coelho wrote:
>> On Tue, 2020-05-05 at 20:19 -0700, Joe Perches wrote:
>>> On Wed, 2020-05-06 at 11:07 +0800, Samuel Zou wrote:
>>>> This silences the following coccinelle warning:
>>>>
>>>> "WARNING: sum of probable bitmasks, consider |"
>>>
>>> I suggest instead ignoring bad and irrelevant warnings.
>>>
>>> PREFIX_LEN is 32 not 0x20 or BIT(5)
>>> PCI_DUMP_SIZE is 352
>>>
>>>> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
>>> []
>>>> @@ -109,9 +109,9 @@ void iwl_trans_pcie_dump_regs(struct iwl_trans *trans)
>>>>
>>>> /* Alloc a max size buffer */
>>>> alloc_size = PCI_ERR_ROOT_ERR_SRC + 4 + PREFIX_LEN;
>>>> - alloc_size = max_t(u32, alloc_size, PCI_DUMP_SIZE + PREFIX_LEN);
>>>> - alloc_size = max_t(u32, alloc_size, PCI_MEM_DUMP_SIZE + PREFIX_LEN);
>>>> - alloc_size = max_t(u32, alloc_size, PCI_PARENT_DUMP_SIZE + PREFIX_LEN);
>>>> + alloc_size = max_t(u32, alloc_size, PCI_DUMP_SIZE | PREFIX_LEN);
>>>> + alloc_size = max_t(u32, alloc_size, PCI_MEM_DUMP_SIZE | PREFIX_LEN);
>>>> + alloc_size = max_t(u32, alloc_size, PCI_PARENT_DUMP_SIZE | PREFIX_LEN);
>>>>
>>>> buf = kmalloc(alloc_size, GFP_ATOMIC);
>>>> if (!buf)
>>
>> Yeah, those macros are clearly not bitmasks. I'm dropping this patch.
>
> Can the cocci script that generated this warning
>
> scripts/coccinelle/misc/orplus.cocci
>
> be dropped or improved to validate the likelihood that
> the defines or constants used are more likely than
> not are bit values?
>
> Maybe these should be defined as hex or BIT or BIT_ULL
> or GENMASK or the like?
>
>
> Right now it seems it just tests for two constants.
>
>
>
> .
>