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

[Dan Carpenter <dan.carpenter@oracle.com>]

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:
> > 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.

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