diff mbox series

[iwl-next] ice: remove null checks before devm_kfree() calls

Message ID 20230530112549.20916-1-przemyslaw.kitszel@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [iwl-next] ice: remove null checks before devm_kfree() calls | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 10 maintainers not CCed: kuba@kernel.org hawk@kernel.org daniel@iogearbox.net john.fastabend@gmail.com bpf@vger.kernel.org davem@davemloft.net intel-wired-lan@lists.osuosl.org pabeni@redhat.com ast@kernel.org edumazet@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 182 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Przemek Kitszel May 30, 2023, 11:25 a.m. UTC 
We all know they are redundant.

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_common.c   |  6 +--
 drivers/net/ethernet/intel/ice/ice_controlq.c |  3 +-
 drivers/net/ethernet/intel/ice/ice_flow.c     | 23 ++--------
 drivers/net/ethernet/intel/ice/ice_lib.c      | 42 +++++++------------
 drivers/net/ethernet/intel/ice/ice_sched.c    | 11 ++---
 drivers/net/ethernet/intel/ice/ice_switch.c   | 19 +++------
 6 files changed, 29 insertions(+), 75 deletions(-)

Comments

Simon Horman May 30, 2023, 6:10 p.m. UTC | #1 
On Tue, May 30, 2023 at 01:25:49PM +0200, Przemek Kitszel wrote:
> We all know they are redundant.
> 
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> index ef103e47a8dc..85cca572c22a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> @@ -1303,23 +1303,6 @@ ice_flow_find_prof_id(struct ice_hw *hw, enum ice_block blk, u64 prof_id)
>  	return NULL;
>  }
>  
> -/**
> - * ice_dealloc_flow_entry - Deallocate flow entry memory
> - * @hw: pointer to the HW struct
> - * @entry: flow entry to be removed
> - */
> -static void
> -ice_dealloc_flow_entry(struct ice_hw *hw, struct ice_flow_entry *entry)
> -{
> -	if (!entry)
> -		return;
> -
> -	if (entry->entry)
> -		devm_kfree(ice_hw_to_dev(hw), entry->entry);
> -
> -	devm_kfree(ice_hw_to_dev(hw), entry);
> -}
> -
>  /**
>   * ice_flow_rem_entry_sync - Remove a flow entry
>   * @hw: pointer to the HW struct
> @@ -1335,7 +1318,8 @@ ice_flow_rem_entry_sync(struct ice_hw *hw, enum ice_block __always_unused blk,
>  
>  	list_del(&entry->l_entry);
>  
> -	ice_dealloc_flow_entry(hw, entry);
> +	devm_kfree(ice_hw_to_dev(hw), entry->entry);

Hi Przemek,

Previously entry was not dereferenced if it was NULL.
Now it is. Can that occur?

> +	devm_kfree(ice_hw_to_dev(hw), entry);
>  
>  	return 0;
>  }

...
Tony Nguyen May 30, 2023, 8:24 p.m. UTC | #2 
On 5/30/2023 4:25 AM, Przemek Kitszel wrote:

This wasn't received by IWL; you shouldn't be send sending to the 
bounces address, please use intel-wired-lan@lists.osuosl.org

Thanks,
Tony

> We all know they are redundant.
> 
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Przemek Kitszel May 30, 2023, 9:54 p.m. UTC | #3 
On 5/30/23 22:24, Tony Nguyen wrote:
> On 5/30/2023 4:25 AM, Przemek Kitszel wrote:
> 
> This wasn't received by IWL; you shouldn't be send sending to the 
> bounces address, please use intel-wired-lan@lists.osuosl.org
> 
> Thanks,
> Tony
> 
>> We all know they are redundant.
>>
>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 

Oh, sorry, I had bad address copy-pasted into my bashrc, updated now.

Should I repost?
Przemek Kitszel May 30, 2023, 10:06 p.m. UTC | #4 
On 5/30/23 20:10, Simon Horman wrote:
> On Tue, May 30, 2023 at 01:25:49PM +0200, Przemek Kitszel wrote:
>> We all know they are redundant.
>>
>> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
>> index ef103e47a8dc..85cca572c22a 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
>> @@ -1303,23 +1303,6 @@ ice_flow_find_prof_id(struct ice_hw *hw, enum ice_block blk, u64 prof_id)
>>   	return NULL;
>>   }
>>   
>> -/**
>> - * ice_dealloc_flow_entry - Deallocate flow entry memory
>> - * @hw: pointer to the HW struct
>> - * @entry: flow entry to be removed
>> - */
>> -static void
>> -ice_dealloc_flow_entry(struct ice_hw *hw, struct ice_flow_entry *entry)
>> -{
>> -	if (!entry)
>> -		return;
>> -
>> -	if (entry->entry)
>> -		devm_kfree(ice_hw_to_dev(hw), entry->entry);
>> -
>> -	devm_kfree(ice_hw_to_dev(hw), entry);
>> -}
>> -
>>   /**
>>    * ice_flow_rem_entry_sync - Remove a flow entry
>>    * @hw: pointer to the HW struct
>> @@ -1335,7 +1318,8 @@ ice_flow_rem_entry_sync(struct ice_hw *hw, enum ice_block __always_unused blk,

More context would include following:

         if (!entry)
                 return -EINVAL;


>>   
>>   	list_del(&entry->l_entry);
>>   
>> -	ice_dealloc_flow_entry(hw, entry);
>> +	devm_kfree(ice_hw_to_dev(hw), entry->entry);
> 
> Hi Przemek,
> 
> Previously entry was not dereferenced if it was NULL.
> Now it is. Can that occur?

The check is right above the default 3-line context provided by git, see 
above.

> 
>> +	devm_kfree(ice_hw_to_dev(hw), entry);
>>   
>>   	return 0;
>>   }
> 
> ...
Tony Nguyen May 30, 2023, 10:10 p.m. UTC | #5 
On 5/30/2023 2:54 PM, Przemek Kitszel wrote:>
> Oh, sorry, I had bad address copy-pasted into my bashrc, updated now.
> 
> Should I repost?

Yes please as IWL patchworks hasn't picked this up. However, since it 
did make it to netdev, I'd recommend waiting the 24 hours for any 
feedback from there.

Thanks,
Tony
Simon Horman May 31, 2023, 7:36 a.m. UTC | #6 
On Wed, May 31, 2023 at 12:06:15AM +0200, Przemek Kitszel wrote:
> On 5/30/23 20:10, Simon Horman wrote:
> > On Tue, May 30, 2023 at 01:25:49PM +0200, Przemek Kitszel wrote:
> > > We all know they are redundant.
> > > 
> > > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > > Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>
> > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > 
> > ...
> > 
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> > > index ef103e47a8dc..85cca572c22a 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> > > @@ -1303,23 +1303,6 @@ ice_flow_find_prof_id(struct ice_hw *hw, enum ice_block blk, u64 prof_id)
> > >   	return NULL;
> > >   }
> > > -/**
> > > - * ice_dealloc_flow_entry - Deallocate flow entry memory
> > > - * @hw: pointer to the HW struct
> > > - * @entry: flow entry to be removed
> > > - */
> > > -static void
> > > -ice_dealloc_flow_entry(struct ice_hw *hw, struct ice_flow_entry *entry)
> > > -{
> > > -	if (!entry)
> > > -		return;
> > > -
> > > -	if (entry->entry)
> > > -		devm_kfree(ice_hw_to_dev(hw), entry->entry);
> > > -
> > > -	devm_kfree(ice_hw_to_dev(hw), entry);
> > > -}
> > > -
> > >   /**
> > >    * ice_flow_rem_entry_sync - Remove a flow entry
> > >    * @hw: pointer to the HW struct
> > > @@ -1335,7 +1318,8 @@ ice_flow_rem_entry_sync(struct ice_hw *hw, enum ice_block __always_unused blk,
> 
> More context would include following:
> 
>         if (!entry)
>                 return -EINVAL;
> 
> 
> > >   	list_del(&entry->l_entry);
> > > -	ice_dealloc_flow_entry(hw, entry);
> > > +	devm_kfree(ice_hw_to_dev(hw), entry->entry);
> > 
> > Hi Przemek,
> > 
> > Previously entry was not dereferenced if it was NULL.
> > Now it is. Can that occur?
> 
> The check is right above the default 3-line context provided by git, see
> above.

Yes, right. Sorry for not checking that.
This does of course look good.

Reviewed-by: Simon Horman <simon.horman@corigine.com>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index eb2dc0983776..6acb40f3c202 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -814,8 +814,7 @@  static void ice_cleanup_fltr_mgmt_struct(struct ice_hw *hw)
 				devm_kfree(ice_hw_to_dev(hw), lst_itr);
 			}
 		}
-		if (recps[i].root_buf)
-			devm_kfree(ice_hw_to_dev(hw), recps[i].root_buf);
+		devm_kfree(ice_hw_to_dev(hw), recps[i].root_buf);
 	}
 	ice_rm_all_sw_replay_rule_info(hw);
 	devm_kfree(ice_hw_to_dev(hw), sw->recp_list);
@@ -1011,8 +1010,7 @@  static int ice_cfg_fw_log(struct ice_hw *hw, bool enable)
 	}
 
 out:
-	if (data)
-		devm_kfree(ice_hw_to_dev(hw), data);
+	devm_kfree(ice_hw_to_dev(hw), data);
 
 	return status;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_controlq.c b/drivers/net/ethernet/intel/ice/ice_controlq.c
index d2faf1baad2f..e4cb5055b999 100644
--- a/drivers/net/ethernet/intel/ice/ice_controlq.c
+++ b/drivers/net/ethernet/intel/ice/ice_controlq.c
@@ -339,8 +339,7 @@  do {									\
 		}							\
 	}								\
 	/* free the buffer info list */					\
-	if ((qi)->ring.cmd_buf)						\
-		devm_kfree(ice_hw_to_dev(hw), (qi)->ring.cmd_buf);	\
+	devm_kfree(ice_hw_to_dev(hw), (qi)->ring.cmd_buf);		\
 	/* free DMA head */						\
 	devm_kfree(ice_hw_to_dev(hw), (qi)->ring.dma_head);		\
 } while (0)
diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
index ef103e47a8dc..85cca572c22a 100644
--- a/drivers/net/ethernet/intel/ice/ice_flow.c
+++ b/drivers/net/ethernet/intel/ice/ice_flow.c
@@ -1303,23 +1303,6 @@  ice_flow_find_prof_id(struct ice_hw *hw, enum ice_block blk, u64 prof_id)
 	return NULL;
 }
 
-/**
- * ice_dealloc_flow_entry - Deallocate flow entry memory
- * @hw: pointer to the HW struct
- * @entry: flow entry to be removed
- */
-static void
-ice_dealloc_flow_entry(struct ice_hw *hw, struct ice_flow_entry *entry)
-{
-	if (!entry)
-		return;
-
-	if (entry->entry)
-		devm_kfree(ice_hw_to_dev(hw), entry->entry);
-
-	devm_kfree(ice_hw_to_dev(hw), entry);
-}
-
 /**
  * ice_flow_rem_entry_sync - Remove a flow entry
  * @hw: pointer to the HW struct
@@ -1335,7 +1318,8 @@  ice_flow_rem_entry_sync(struct ice_hw *hw, enum ice_block __always_unused blk,
 
 	list_del(&entry->l_entry);
 
-	ice_dealloc_flow_entry(hw, entry);
+	devm_kfree(ice_hw_to_dev(hw), entry->entry);
+	devm_kfree(ice_hw_to_dev(hw), entry);
 
 	return 0;
 }
@@ -1662,8 +1646,7 @@  ice_flow_add_entry(struct ice_hw *hw, enum ice_block blk, u64 prof_id,
 
 out:
 	if (status && e) {
-		if (e->entry)
-			devm_kfree(ice_hw_to_dev(hw), e->entry);
+		devm_kfree(ice_hw_to_dev(hw), e->entry);
 		devm_kfree(ice_hw_to_dev(hw), e);
 	}
 
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index e8142bea2eb2..c3722c68af99 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -321,31 +321,19 @@  static void ice_vsi_free_arrays(struct ice_vsi *vsi)
 
 	dev = ice_pf_to_dev(pf);
 
-	if (vsi->af_xdp_zc_qps) {
-		bitmap_free(vsi->af_xdp_zc_qps);
-		vsi->af_xdp_zc_qps = NULL;
-	}
+	bitmap_free(vsi->af_xdp_zc_qps);
+	vsi->af_xdp_zc_qps = NULL;
 	/* free the ring and vector containers */
-	if (vsi->q_vectors) {
-		devm_kfree(dev, vsi->q_vectors);
-		vsi->q_vectors = NULL;
-	}
-	if (vsi->tx_rings) {
-		devm_kfree(dev, vsi->tx_rings);
-		vsi->tx_rings = NULL;
-	}
-	if (vsi->rx_rings) {
-		devm_kfree(dev, vsi->rx_rings);
-		vsi->rx_rings = NULL;
-	}
-	if (vsi->txq_map) {
-		devm_kfree(dev, vsi->txq_map);
-		vsi->txq_map = NULL;
-	}
-	if (vsi->rxq_map) {
-		devm_kfree(dev, vsi->rxq_map);
-		vsi->rxq_map = NULL;
-	}
+	devm_kfree(dev, vsi->q_vectors);
+	vsi->q_vectors = NULL;
+	devm_kfree(dev, vsi->tx_rings);
+	vsi->tx_rings = NULL;
+	devm_kfree(dev, vsi->rx_rings);
+	vsi->rx_rings = NULL;
+	devm_kfree(dev, vsi->txq_map);
+	vsi->txq_map = NULL;
+	devm_kfree(dev, vsi->rxq_map);
+	vsi->rxq_map = NULL;
 }
 
 /**
@@ -902,10 +890,8 @@  static void ice_rss_clean(struct ice_vsi *vsi)
 
 	dev = ice_pf_to_dev(pf);
 
-	if (vsi->rss_hkey_user)
-		devm_kfree(dev, vsi->rss_hkey_user);
-	if (vsi->rss_lut_user)
-		devm_kfree(dev, vsi->rss_lut_user);
+	devm_kfree(dev, vsi->rss_hkey_user);
+	devm_kfree(dev, vsi->rss_lut_user);
 
 	ice_vsi_clean_rss_flow_fld(vsi);
 	/* remove RSS replay list */
diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
index b7682de0ae05..b664d60fd037 100644
--- a/drivers/net/ethernet/intel/ice/ice_sched.c
+++ b/drivers/net/ethernet/intel/ice/ice_sched.c
@@ -358,10 +358,7 @@  void ice_free_sched_node(struct ice_port_info *pi, struct ice_sched_node *node)
 				node->sibling;
 	}
 
-	/* leaf nodes have no children */
-	if (node->children)
-		devm_kfree(ice_hw_to_dev(hw), node->children);
-
+	devm_kfree(ice_hw_to_dev(hw), node->children);
 	kfree(node->name);
 	xa_erase(&pi->sched_node_ids, node->id);
 	devm_kfree(ice_hw_to_dev(hw), node);
@@ -859,10 +856,8 @@  void ice_sched_cleanup_all(struct ice_hw *hw)
 	if (!hw)
 		return;
 
-	if (hw->layer_info) {
-		devm_kfree(ice_hw_to_dev(hw), hw->layer_info);
-		hw->layer_info = NULL;
-	}
+	devm_kfree(ice_hw_to_dev(hw), hw->layer_info);
+	hw->layer_info = NULL;
 
 	ice_sched_clear_port(hw->port_info);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index d69efd33beee..49be0d2532eb 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -1636,21 +1636,16 @@  ice_save_vsi_ctx(struct ice_hw *hw, u16 vsi_handle, struct ice_vsi_ctx *vsi)
  */
 static void ice_clear_vsi_q_ctx(struct ice_hw *hw, u16 vsi_handle)
 {
-	struct ice_vsi_ctx *vsi;
+	struct ice_vsi_ctx *vsi = ice_get_vsi_ctx(hw, vsi_handle);
 	u8 i;
 
-	vsi = ice_get_vsi_ctx(hw, vsi_handle);
 	if (!vsi)
 		return;
 	ice_for_each_traffic_class(i) {
-		if (vsi->lan_q_ctx[i]) {
-			devm_kfree(ice_hw_to_dev(hw), vsi->lan_q_ctx[i]);
-			vsi->lan_q_ctx[i] = NULL;
-		}
-		if (vsi->rdma_q_ctx[i]) {
-			devm_kfree(ice_hw_to_dev(hw), vsi->rdma_q_ctx[i]);
-			vsi->rdma_q_ctx[i] = NULL;
-		}
+		devm_kfree(ice_hw_to_dev(hw), vsi->lan_q_ctx[i]);
+		vsi->lan_q_ctx[i] = NULL;
+		devm_kfree(ice_hw_to_dev(hw), vsi->rdma_q_ctx[i]);
+		vsi->rdma_q_ctx[i] = NULL;
 	}
 }
 
@@ -5486,9 +5481,7 @@  ice_add_adv_recipe(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
 		devm_kfree(ice_hw_to_dev(hw), fvit);
 	}
 
-	if (rm->root_buf)
-		devm_kfree(ice_hw_to_dev(hw), rm->root_buf);
-
+	devm_kfree(ice_hw_to_dev(hw), rm->root_buf);
 	kfree(rm);
 
 err_free_lkup_exts: