From: Dan Carpenter <hidden> Date: 2021-01-28 09:58:14
Hello Michał Mirosław,
The patch 9517b95bdc46: "Input: elants_i2c - add support for
eKTF3624" from Jan 24, 2021, leads to the following static checker
warning:
drivers/input/touchscreen/elants_i2c.c:966 elants_i2c_mt_event()
warn: should this be a bitwise negate mask?
drivers/input/touchscreen/elants_i2c.c
942 /* Note: all fingers have the same tool type */
943 tool_type = buf[FW_POS_TOOL_TYPE] & BIT(0) ?
944 MT_TOOL_FINGER : MT_TOOL_PALM;
945
946 for (i = 0; i < MAX_CONTACT_NUM && n_fingers; i++) {
947 if (finger_state & 1) {
948 unsigned int x, y, p, w;
949 u8 *pos;
950
951 pos = &buf[FW_POS_XY + i * 3];
952 x = (((u16)pos[0] & 0xf0) << 4) | pos[1];
953 y = (((u16)pos[0] & 0x0f) << 8) | pos[2];
954
955 /*
956 * eKTF3624 may have use "old" touch-report format,
957 * depending on a device and TS firmware version.
958 * For example, ASUS Transformer devices use the "old"
959 * format, while ASUS Nexus 7 uses the "new" formant.
960 */
961 if (packet_size == PACKET_SIZE_OLD &&
962 ts->chip_id == EKTF3624) {
963 w = buf[FW_POS_WIDTH + i / 2];
964 w >>= 4 * (~i & 1);
965 w |= w << 4;
966 w |= !w;
^^^^^^^^
This code is just very puzzling. I think it may actually be correct?
The boring and conventional way to write this would be to do it like so:
if (!w)
w = 1;
967 p = w;
968 } else {
969 p = buf[FW_POS_PRESSURE + i];
970 w = buf[FW_POS_WIDTH + i];
971 }
972
973 dev_dbg(&ts->client->dev, "i=%d x=%d y=%d p=%d w=%d\n",
974 i, x, y, p, w);
975
976 input_mt_slot(input, i);
977 input_mt_report_slot_state(input, tool_type, true);
978 touchscreen_report_pos(input, &ts->prop, x, y, true);
regards,
dan carpenter
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-01-28 13:08:01
On Thu, Jan 28, 2021 at 12:57:12PM +0300, Dan Carpenter wrote:
Hello Michał Mirosław,
The patch 9517b95bdc46: "Input: elants_i2c - add support for
eKTF3624" from Jan 24, 2021, leads to the following static checker
warning:
drivers/input/touchscreen/elants_i2c.c:966 elants_i2c_mt_event()
warn: should this be a bitwise negate mask?
drivers/input/touchscreen/elants_i2c.c
[...]
963 w = buf[FW_POS_WIDTH + i / 2];
964 w >>= 4 * (~i & 1);
965 w |= w << 4;
966 w |= !w;
^^^^^^^^
This code is just very puzzling. I think it may actually be correct?
The boring and conventional way to write this would be to do it like so:
if (!w)
w = 1;
It could also be written as:
w += !w;
or:
w += w == 0;
while avoiding conditional.
But, in this case, the warning is bogus. Because w | ~w == all-ones (always),
it might as well suggested to write:
w = -1;
or:
w = ~0;
making the code broken.
Best Regards
Michał Mirosław
From: Dan Carpenter <hidden> Date: 2021-01-28 14:38:27
On Thu, Jan 28, 2021 at 02:07:05PM +0100, Michał Mirosław wrote:
On Thu, Jan 28, 2021 at 12:57:12PM +0300, Dan Carpenter wrote:
quoted
Hello Michał Mirosław,
The patch 9517b95bdc46: "Input: elants_i2c - add support for
eKTF3624" from Jan 24, 2021, leads to the following static checker
warning:
drivers/input/touchscreen/elants_i2c.c:966 elants_i2c_mt_event()
warn: should this be a bitwise negate mask?
drivers/input/touchscreen/elants_i2c.c
[...]
quoted
963 w = buf[FW_POS_WIDTH + i / 2];
964 w >>= 4 * (~i & 1);
965 w |= w << 4;
966 w |= !w;
^^^^^^^^
This code is just very puzzling. I think it may actually be correct?
The boring and conventional way to write this would be to do it like so:
if (!w)
w = 1;
It could also be written as:
w += !w;
or:
w += w == 0;
while avoiding conditional.
Is there some kind of prize for avoiding if statements??
But, in this case, the warning is bogus. Because w | ~w == all-ones (always),
it might as well suggested to write:
w = -1;
or:
w = ~0;
making the code broken.
Yeah. The rule is just a simple heuristic of a logical negate used
with a bitwise operation. You're comment has prompted me to review
if this check is effective.
It turns out that it's not a super common thing so it doesn't lead to
many warnings whether they are false positives or real bugs. We did
find one bug last week (in linux-next):
5993e79398d3 ("drm/amdgpu: Fix masking binary not operator on two mask operations")
There are only three other warnings for this rule in the kernel:
drivers/pci/pcie/aer_inject.c:376 aer_inject() warn: should this be a bitwise negate mask?
drivers/pci/pcie/aer_inject.c:381 aer_inject() warn: should this be a bitwise negate mask?
drivers/net/wireless/realtek/rtlwifi/rtl8821ae/dm.c:2435 rtl8821ae_dm_refresh_basic_rate_mask() warn: should this be a bitwise negate mask?
I never reported any of these because they're in ancient code and I
couldn't figure out what it was trying to do.
drivers/pci/pcie/aer_inject.c
374 if (aer_mask_override) {
375 cor_mask_orig = cor_mask;
376 cor_mask &= !(einj->cor_status);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Is the bitwise ~ intended? Why is BIT(0) special? You would have to
know the PCIe hardware spec to say the answer for that. It's sort of
like BIT(0) is a magic number but invisible... :/
377 pci_write_config_dword(dev, pos_cap_err + PCI_ERR_COR_MASK,
378 cor_mask);
379
380 uncor_mask_orig = uncor_mask;
381 uncor_mask &= !(einj->uncor_status);
382 pci_write_config_dword(dev, pos_cap_err + PCI_ERR_UNCOR_MASK,
383 uncor_mask);
384 }
drivers/net/wireless/realtek/rtlwifi/rtl8821ae/dm.c
2415 static void rtl8821ae_dm_refresh_basic_rate_mask(struct ieee80211_hw *hw)
2416 {
2417 struct rtl_priv *rtlpriv = rtl_priv(hw);
2418 struct dig_t *dm_digtable = &rtlpriv->dm_digtable;
2419 struct rtl_mac *mac = &rtlpriv->mac80211;
2420 static u8 stage;
2421 u8 cur_stage = 0;
2422 u16 basic_rate = RRSR_1M | RRSR_2M | RRSR_5_5M | RRSR_11M | RRSR_6M;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The important thing to note here is BIT(0) is RRSR_1M.
2423
2424 if (mac->link_state < MAC80211_LINKED)
2425 cur_stage = 0;
2426 else if (dm_digtable->rssi_val_min < 25)
2427 cur_stage = 1;
2428 else if (dm_digtable->rssi_val_min > 30)
2429 cur_stage = 3;
2430 else
2431 cur_stage = 2;
2432
2433 if (cur_stage != stage) {
2434 if (cur_stage == 1) {
2435 basic_rate &= (!(basic_rate ^ mac->basic_rates));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here we set "basic_rate" to either 0 or RRSR_1M.
2436 rtlpriv->cfg->ops->set_hw_reg(hw,
2437 HW_VAR_BASIC_RATE, (u8 *)&basic_rate);
This can't possibly be correct but the the ->set_hw_reg() implementations
seem to have a work around where they take do:
basic_rate |= 0x01;
at the start of the function. Magic numbers again. *le bigger sigh*.
2438 } else if (cur_stage == 3 && (stage == 1 || stage == 2)) {
2439 rtlpriv->cfg->ops->set_hw_reg(hw,
2440 HW_VAR_BASIC_RATE, (u8 *)&mac->basic_rates);
2441 }
2442 }
2443 stage = cur_stage;
2444 }
regards,
dan carpenter
From: Michał Mirosław <mirq-linux@rere.qmqm.pl> Date: 2021-01-28 23:21:54
On Thu, Jan 28, 2021 at 05:37:26PM +0300, Dan Carpenter wrote:
On Thu, Jan 28, 2021 at 02:07:05PM +0100, Michał Mirosław wrote:
quoted
On Thu, Jan 28, 2021 at 12:57:12PM +0300, Dan Carpenter wrote:
quoted
Hello Michał Mirosław,
The patch 9517b95bdc46: "Input: elants_i2c - add support for
eKTF3624" from Jan 24, 2021, leads to the following static checker
warning:
drivers/input/touchscreen/elants_i2c.c:966 elants_i2c_mt_event()
warn: should this be a bitwise negate mask?
drivers/input/touchscreen/elants_i2c.c
[...]
quoted
963 w = buf[FW_POS_WIDTH + i / 2];
964 w >>= 4 * (~i & 1);
965 w |= w << 4;
966 w |= !w;
^^^^^^^^
This code is just very puzzling. I think it may actually be correct?
The boring and conventional way to write this would be to do it like so:
if (!w)
w = 1;
It could also be written as:
w += !w;
or:
w += w == 0;`
while avoiding conditional.
Is there some kind of prize for avoiding if statements??
Less LOC and an occassional puzzle, of course. :-) Considering your
examples below I can see where the check came from. I wouldn't object
to a patch changing the code or adding a comment on what it does (it
maps a selected nibble value (0..15) to a byte (1..255) spreading the
values evenly).
quoted
But, in this case, the warning is bogus. Because w | ~w == all-ones (always),
it might as well suggested to write:
w = -1;
or:
w = ~0;
making the code broken.
Yeah. The rule is just a simple heuristic of a logical negate used
with a bitwise operation.
[...]
Maybe it could differentiate between "|= !x" and "&= !x", the second one
being more suspicious? I must say that 'x | !x' idiom looks obvious
as other sigle-operator-letter misspellings ('x | ~x' and 'x || !x')
beg the question of why the constant wasn't used directly?
Best Regards
Michał Mirosław