[PATCH net-next] net/mlx4: remove many unnecessary NULL values

[PATCH net-next] net/mlx4: remove many unnecessary NULL values

[Ruan Jinjie]

Ther are many pointers assigned first, which need not to be initialized, so
remove the NULL assignment.

Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 10 +++++-----
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  |  4 ++--
 drivers/net/ethernet/mellanox/mlx4/main.c       | 12 ++++++------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 7d45f1d55f79..164a13272faa 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1467,8 +1467,8 @@ static int add_ip_rule(struct mlx4_en_priv *priv,
 		       struct list_head *list_h)
 {
 	int err;
-	struct mlx4_spec_list *spec_l2 = NULL;
-	struct mlx4_spec_list *spec_l3 = NULL;
+	struct mlx4_spec_list *spec_l2;
+	struct mlx4_spec_list *spec_l3;
 	struct ethtool_usrip4_spec *l3_mask = &cmd->fs.m_u.usr_ip4_spec;
 
 	spec_l3 = kzalloc(sizeof(*spec_l3), GFP_KERNEL);
@@ -1505,9 +1505,9 @@ static int add_tcp_udp_rule(struct mlx4_en_priv *priv,
 			     struct list_head *list_h, int proto)
 {
 	int err;
-	struct mlx4_spec_list *spec_l2 = NULL;
-	struct mlx4_spec_list *spec_l3 = NULL;
-	struct mlx4_spec_list *spec_l4 = NULL;
+	struct mlx4_spec_list *spec_l2;
+	struct mlx4_spec_list *spec_l3;
+	struct mlx4_spec_list *spec_l4;
 	struct ethtool_tcpip4_spec *l4_mask = &cmd->fs.m_u.tcp_ip4_spec;
 
 	spec_l2 = kzalloc(sizeof(*spec_l2), GFP_KERNEL);
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index e11bc0ac880e..403604ceebc8 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -291,7 +291,7 @@ mlx4_en_filter_alloc(struct mlx4_en_priv *priv, int rxq_index, __be32 src_ip,
 		     __be32 dst_ip, u8 ip_proto, __be16 src_port,
 		     __be16 dst_port, u32 flow_id)
 {
-	struct mlx4_en_filter *filter = NULL;
+	struct mlx4_en_filter *filter;
 
 	filter = kzalloc(sizeof(struct mlx4_en_filter), GFP_ATOMIC);
 	if (!filter)
@@ -2935,7 +2935,7 @@ static void mlx4_en_bond_work(struct work_struct *work)
 static int mlx4_en_queue_bond_work(struct mlx4_en_priv *priv, int is_bonded,
 				   u8 v2p_p1, u8 v2p_p2)
 {
-	struct mlx4_en_bond *bond = NULL;
+	struct mlx4_en_bond *bond;
 
 	bond = kzalloc(sizeof(*bond), GFP_ATOMIC);
 	if (!bond)
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 61286b0d9b0c..9320b57cf788 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -864,7 +864,7 @@ static void mlx4_slave_destroy_special_qp_cap(struct mlx4_dev *dev)
 
 static int mlx4_slave_special_qp_cap(struct mlx4_dev *dev)
 {
-	struct mlx4_func_cap *func_cap = NULL;
+	struct mlx4_func_cap *func_cap;
 	struct mlx4_caps *caps = &dev->caps;
 	int i, err = 0;
 
@@ -908,9 +908,9 @@ static int mlx4_slave_cap(struct mlx4_dev *dev)
 {
 	int			   err;
 	u32			   page_size;
-	struct mlx4_dev_cap	   *dev_cap = NULL;
-	struct mlx4_func_cap	   *func_cap = NULL;
-	struct mlx4_init_hca_param *hca_param = NULL;
+	struct mlx4_dev_cap	   *dev_cap;
+	struct mlx4_func_cap	   *func_cap;
+	struct mlx4_init_hca_param *hca_param;
 
 	hca_param = kzalloc(sizeof(*hca_param), GFP_KERNEL);
 	func_cap = kzalloc(sizeof(*func_cap), GFP_KERNEL);
@@ -2294,8 +2294,8 @@ static int mlx4_init_fw(struct mlx4_dev *dev)
 static int mlx4_init_hca(struct mlx4_dev *dev)
 {
 	struct mlx4_priv	  *priv = mlx4_priv(dev);
-	struct mlx4_init_hca_param *init_hca = NULL;
-	struct mlx4_dev_cap	  *dev_cap = NULL;
+	struct mlx4_init_hca_param *init_hca;
+	struct mlx4_dev_cap	  *dev_cap;
 	struct mlx4_adapter	   adapter;
 	struct mlx4_profile	   profile;
 	u64 icm_size;
-- 
2.34.1

Re: [PATCH net-next] net/mlx4: remove many unnecessary NULL values

[Simon Horman]

On Tue, Aug 01, 2023 at 08:34:22PM +0800, Ruan Jinjie wrote:
> Ther are many pointers assigned first, which need not to be initialized, so
> remove the NULL assignment.

How about something like:

Don't initialise local variables to NULL which are always
set to other values elsewhere in the same function.

> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>

...

> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c

...

> @@ -2294,8 +2294,8 @@ static int mlx4_init_fw(struct mlx4_dev *dev)
>  static int mlx4_init_hca(struct mlx4_dev *dev)
>  {
>  	struct mlx4_priv	  *priv = mlx4_priv(dev);
> -	struct mlx4_init_hca_param *init_hca = NULL;
> -	struct mlx4_dev_cap	  *dev_cap = NULL;
> +	struct mlx4_init_hca_param *init_hca;
> +	struct mlx4_dev_cap	  *dev_cap;
>  	struct mlx4_adapter	   adapter;
>  	struct mlx4_profile	   profile;
>  	u64 icm_size;

This last hunk doesn't seem correct, as it doesn't
seem these aren't always initialised elsewhere in the function
before being passed to kfree().

> -- 
> 2.34.1
> 
> 

Re: [PATCH net-next] net/mlx4: remove many unnecessary NULL values

[Jesse Brandeburg]

On 8/1/2023 5:34 AM, Ruan Jinjie wrote:
> Ther are many pointers assigned first, which need not to be initialized, so
> remove the NULL assignment.
> 
> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>

Thanks for your patch, make sure you're always explaining "why" you're
making a change in your commit message.

but see below please.

> ---
>  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 10 +++++-----
>  drivers/net/ethernet/mellanox/mlx4/en_netdev.c  |  4 ++--
>  drivers/net/ethernet/mellanox/mlx4/main.c       | 12 ++++++------
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index 7d45f1d55f79..164a13272faa 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -1467,8 +1467,8 @@ static int add_ip_rule(struct mlx4_en_priv *priv,
>  		       struct list_head *list_h)
>  {
>  	int err;
> -	struct mlx4_spec_list *spec_l2 = NULL;
> -	struct mlx4_spec_list *spec_l3 = NULL;
> +	struct mlx4_spec_list *spec_l2;
> +	struct mlx4_spec_list *spec_l3;

What sequence of commands did you use to identify this set of things to
change? That would be useful data for the commit message.

gcc with -Wunused-something?
cppcheck?

I've sent these types of patches before, but they've been rejected as
churn if they don't fix a clear W=1 or C=2 warning.

Did you run the above and see these issues?


>  	struct ethtool_usrip4_spec *l3_mask = &cmd->fs.m_u.usr_ip4_spec;
>  
>  	spec_l3 = kzalloc(sizeof(*spec_l3), GFP_KERNEL);
> @@ -1505,9 +1505,9 @@ static int add_tcp_udp_rule(struct mlx4_en_priv *priv,
>  			     struct list_head *list_h, int proto)
>  {
>  	int err;
> -	struct mlx4_spec_list *spec_l2 = NULL;
> -	struct mlx4_spec_list *spec_l3 = NULL;
> -	struct mlx4_spec_list *spec_l4 = NULL;
> +	struct mlx4_spec_list *spec_l2;
> +	struct mlx4_spec_list *spec_l3;
> +	struct mlx4_spec_list *spec_l4;
>  	struct ethtool_tcpip4_spec *l4_mask = &cmd->fs.m_u.tcp_ip4_spec;
>  
>  	spec_l2 = kzalloc(sizeof(*spec_l2), GFP_KERNEL);

I suggest if you want to have these kind of changes committed you spend
more time to make a detailed commit message and explain what's going on
for the change as otherwise it's not going to be accepted.

Re: [PATCH net-next] net/mlx4: remove many unnecessary NULL values

[Ruan Jinjie]

On 2023/8/1 23:40, Simon Horman wrote:
> On Tue, Aug 01, 2023 at 08:34:22PM +0800, Ruan Jinjie wrote:
>> Ther are many pointers assigned first, which need not to be initialized, so
>> remove the NULL assignment.
> 
> How about something like:
> 
> Don't initialise local variables to NULL which are always
> set to other values elsewhere in the same function.

I think these NULL assignments can also be removed.

> 
>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> 
> ...
> 
>> @@ -2294,8 +2294,8 @@ static int mlx4_init_fw(struct mlx4_dev *dev)
>>  static int mlx4_init_hca(struct mlx4_dev *dev)
>>  {
>>  	struct mlx4_priv	  *priv = mlx4_priv(dev);
>> -	struct mlx4_init_hca_param *init_hca = NULL;
>> -	struct mlx4_dev_cap	  *dev_cap = NULL;
>> +	struct mlx4_init_hca_param *init_hca;
>> +	struct mlx4_dev_cap	  *dev_cap;
>>  	struct mlx4_adapter	   adapter;
>>  	struct mlx4_profile	   profile;
>>  	u64 icm_size;
> 
> This last hunk doesn't seem correct, as it doesn't
> seem these aren't always initialised elsewhere in the function
> before being passed to kfree().

Yes, this kind of situation the NULL assignments should not be removed,I'll
reserch it more.

> 
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH net-next] net/mlx4: remove many unnecessary NULL values

[Ruan Jinjie]

On 2023/8/2 1:55, Jesse Brandeburg wrote:
> On 8/1/2023 5:34 AM, Ruan Jinjie wrote:
>> Ther are many pointers assigned first, which need not to be initialized, so
>> remove the NULL assignment.
>>
>> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com>
> 
> Thanks for your patch, make sure you're always explaining "why" you're
> making a change in your commit message.

Thank you for your sincere advice! I'll notice the issue in future patches.

> 
> but see below please.
> 
>> ---
>>  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 10 +++++-----
>>  drivers/net/ethernet/mellanox/mlx4/en_netdev.c  |  4 ++--
>>  drivers/net/ethernet/mellanox/mlx4/main.c       | 12 ++++++------
>>  3 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
>> index 7d45f1d55f79..164a13272faa 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
>> @@ -1467,8 +1467,8 @@ static int add_ip_rule(struct mlx4_en_priv *priv,
>>  		       struct list_head *list_h)
>>  {
>>  	int err;
>> -	struct mlx4_spec_list *spec_l2 = NULL;
>> -	struct mlx4_spec_list *spec_l3 = NULL;
>> +	struct mlx4_spec_list *spec_l2;
>> +	struct mlx4_spec_list *spec_l3;
> 
> What sequence of commands did you use to identify this set of things to
> change? That would be useful data for the commit message.
> 
> gcc with -Wunused-something?
> cppcheck?

Just code walk-through and some keyword match such as malloc and NULL
assignment.

> 
> I've sent these types of patches before, but they've been rejected as
> churn if they don't fix a clear W=1 or C=2 warning.
> 
> Did you run the above and see these issues?
> 
> 
>>  	struct ethtool_usrip4_spec *l3_mask = &cmd->fs.m_u.usr_ip4_spec;
>>  
>>  	spec_l3 = kzalloc(sizeof(*spec_l3), GFP_KERNEL);
>> @@ -1505,9 +1505,9 @@ static int add_tcp_udp_rule(struct mlx4_en_priv *priv,
>>  			     struct list_head *list_h, int proto)
>>  {
>>  	int err;
>> -	struct mlx4_spec_list *spec_l2 = NULL;
>> -	struct mlx4_spec_list *spec_l3 = NULL;
>> -	struct mlx4_spec_list *spec_l4 = NULL;
>> +	struct mlx4_spec_list *spec_l2;
>> +	struct mlx4_spec_list *spec_l3;
>> +	struct mlx4_spec_list *spec_l4;
>>  	struct ethtool_tcpip4_spec *l4_mask = &cmd->fs.m_u.tcp_ip4_spec;
>>  
>>  	spec_l2 = kzalloc(sizeof(*spec_l2), GFP_KERNEL);
> 
> I suggest if you want to have these kind of changes committed you spend
> more time to make a detailed commit message and explain what's going on
> for the change as otherwise it's not going to be accepted.

Thank you very much! I'll give more explain in future patches.

> 
> 
>