[bug report] Input: elants_i2c - add support for eKTF3624

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:

18 more lines

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

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  {

45 more lines

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