Intel-Wired-Lan Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [bug report] igc: Add NVM support
Date: Wed, 17 Mar 2021 16:23:39 +0300	[thread overview]
Message-ID: <YFIC27QOoIaVkDYL@mwanda> (raw)

Hello Sasha Neftin,

The patch ab4056126813: "igc: Add NVM support" from Oct 11, 2018,
leads to the following static checker warning:

	drivers/net/ethernet/intel/igc/igc_i225.c:235 igc_write_nvm_srwr()
	warn: loop overwrites return value 'ret_val'

drivers/net/ethernet/intel/igc/igc_i225.c
   218  static s32 igc_write_nvm_srwr(struct igc_hw *hw, u16 offset, u16 words,
   219                                u16 *data)
   220  {
   221          struct igc_nvm_info *nvm = &hw->nvm;
   222          s32 ret_val = -IGC_ERR_NVM;
   223          u32 attempts = 100000;
   224          u32 i, k, eewr = 0;
   225  
   226          /* A check for invalid values:  offset too large, too many words,
   227           * too many words for the offset, and not enough words.
   228           */
   229          if (offset >= nvm->word_size || (words > (nvm->word_size - offset)) ||
   230              words == 0) {
   231                  hw_dbg("nvm parameter(s) out of bounds\n");
   232                  goto out;

I really don't care for "goto out;" labels.  They add a level of
misdirection and ambiguity.  This should be "return -EINVAL;" instead
of "return -IGC_ERR_NVM;".  Eventually it gets propogated back to the
user via dev_ethtool() and it becomes -EPERM to the user.

   233          }
   234  
   235          for (i = 0; i < words; i++) {
   236                  eewr = ((offset + i) << IGC_NVM_RW_ADDR_SHIFT) |
   237                          (data[i] << IGC_NVM_RW_REG_DATA) |
   238                          IGC_NVM_RW_REG_START;
   239  
   240                  wr32(IGC_SRWR, eewr);
   241  
   242                  for (k = 0; k < attempts; k++) {
   243                          if (IGC_NVM_RW_REG_DONE &
   244                              rd32(IGC_SRWR)) {
   245                                  ret_val = 0;
   246                                  break;
   247                          }
   248                          udelay(5);
   249                  }
   250  
   251                  if (ret_val) {
   252                          hw_dbg("Shadow RAM write EEWR timed out\n");
   253                          break;
   254                  }

If there is a read error on subsequent iterations through the loop then
this code will return success.

   255          }
   256  
   257  out:
   258          return ret_val;
   259  }

regards,
dan carpenter

                 reply	other threads:[~2021-03-17 13:23 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

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=YFIC27QOoIaVkDYL@mwanda \
    --to=dan.carpenter@oracle.com \
    --cc=intel-wired-lan@osuosl.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