| Message ID | 20170606235407.6478-1-colin.king@canonical.com |
|---|---|
| State | Rejected |
| Delegated to: | Jeff Kirsher |
| Headers | show |
> -----Original Message----- > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On > Behalf Of Colin King > Sent: Tuesday, June 6, 2017 4:54 PM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; intel-wired- > lan@lists.osuosl.org; netdev@vger.kernel.org > Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [Intel-wired-lan] [PATCH] i40evf: remove redundant null check on > key > > From: Colin Ian King <colin.king@canonical.com> > > key has previously been null checked so the subsequent null check is > redundant as key can never be null at that point, so remove it. > > Detected by CoverityScan, CID#1357164 ("Logically dead code") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
This patch isn't right... On Wed, Jun 07, 2017 at 12:54:07AM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > key has previously been null checked so the subsequent null check > is redundant as key can never be null at that point, so remove it. > Actually, it's the reverse. "key" is always NULL. Probably the || should be a &&? regards, dan carpenter
On Sat, Jun 10, 2017 at 4:33 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > This patch isn't right... > > On Wed, Jun 07, 2017 at 12:54:07AM +0100, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> key has previously been null checked so the subsequent null check >> is redundant as key can never be null at that point, so remove it. >> > > Actually, it's the reverse. "key" is always NULL. Probably the || > should be a &&? > > regards, > dan carpenter Actually the original code and the patched version are still both broken, but it is more broken with the patch. With this change I am pretty sure we will kernel panic if we use the ethtool ioctl for ETHTOOL_SRXFHINDIR, or don't update the key when updating other fields in the flow hash. So the original logic here looks like a bad copy of code from igb. There it doesn't support updating the key so if key is set we are supposed to be returning an error since key update isn't currently supported. So the check for key at the start of this function should probably be dropped instead of the second check. From what I can tell the original code prevents key from ever being updated since if key is non-null it means we want to update the key. - Alex
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c index 9bb2cc7dd4e4..838e57c6e176 100644 --- a/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c +++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c @@ -732,9 +732,7 @@ static int i40evf_set_rxfh(struct net_device *netdev, const u32 *indir, if (!indir) return 0; - if (key) { - memcpy(adapter->rss_key, key, adapter->rss_key_size); - } + memcpy(adapter->rss_key, key, adapter->rss_key_size); /* Each 32 bits pointed by 'indir' is stored with a lut entry */ for (i = 0; i < adapter->rss_lut_size; i++)