[PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The pointer primary_channel is being assigned with a value that is never,
The assignment is redundant and can be removed.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/hv/channel_mgmt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index ffd7fffa5f83..f7bbb8dc4b0f 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
 
 	if (channel->primary_channel == NULL) {
 		list_del(&channel->listentry);
-
-		primary_channel = channel;
 	} else {
 		primary_channel = channel->primary_channel;
 		spin_lock_irqsave(&primary_channel->lock, flags);
-- 
2.25.1

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Julia Lawall]

On Tue, 14 Apr 2020, Colin King wrote:

> From: Colin Ian King <colin.king@canonical.com>
>
> The pointer primary_channel is being assigned with a value that is never,

never -> never used :)

> The assignment is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/hv/channel_mgmt.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>
>  	if (channel->primary_channel == NULL) {
>  		list_del(&channel->listentry);
> -
> -		primary_channel = channel;
>  	} else {
>  		primary_channel = channel->primary_channel;
>  		spin_lock_irqsave(&primary_channel->lock, flags);
> --
> 2.25.1
>
>

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Vitaly Kuznetsov]

Colin King <colin.king@canonical.com> writes:

> From: Colin Ian King <colin.king@canonical.com>
>
> The pointer primary_channel is being assigned with a value that is never,
> The assignment is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/hv/channel_mgmt.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>  
>  	if (channel->primary_channel == NULL) {
>  		list_del(&channel->listentry);
> -
> -		primary_channel = channel;
>  	} else {
>  		primary_channel = channel->primary_channel;
>  		spin_lock_irqsave(&primary_channel->lock, flags);

If I'm looking at the right source (5.7-rc1) it *is* beeing used:

	if (channel->primary_channel == NULL) {
		list_del(&channel->listentry);

		primary_channel = channel;
	} else {
		primary_channel = channel->primary_channel;
		spin_lock_irqsave(&primary_channel->lock, flags);
		list_del(&channel->sc_list);
		spin_unlock_irqrestore(&primary_channel->lock, flags);
	}

	/*
	 * We need to free the bit for init_vp_index() to work in the case
	 * of sub-channel, when we reload drivers like hv_netvsc.
	 */
	if (channel->affinity_policy == HV_LOCALIZED)
		cpumask_clear_cpu(channel->target_cpu,
				  &primary_channel->alloced_cpus_in_node);
                                   ^^^^^ HERE ^^^^^

-- 
Vitaly

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Colin Ian King]

On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
> Colin King <colin.king@canonical.com> writes:
> 
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The pointer primary_channel is being assigned with a value that is never,
>> The assignment is redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/hv/channel_mgmt.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
>> --- a/drivers/hv/channel_mgmt.c
>> +++ b/drivers/hv/channel_mgmt.c
>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>>  
>>  	if (channel->primary_channel == NULL) {
>>  		list_del(&channel->listentry);
>> -
>> -		primary_channel = channel;
>>  	} else {
>>  		primary_channel = channel->primary_channel;
>>  		spin_lock_irqsave(&primary_channel->lock, flags);
> 
> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
> 
> 	if (channel->primary_channel == NULL) {
> 		list_del(&channel->listentry);
> 
> 		primary_channel = channel;
> 	} else {
> 		primary_channel = channel->primary_channel;
> 		spin_lock_irqsave(&primary_channel->lock, flags);
> 		list_del(&channel->sc_list);
> 		spin_unlock_irqrestore(&primary_channel->lock, flags);
> 	}
> 
> 	/*
> 	 * We need to free the bit for init_vp_index() to work in the case
> 	 * of sub-channel, when we reload drivers like hv_netvsc.
> 	 */
> 	if (channel->affinity_policy == HV_LOCALIZED)
> 		cpumask_clear_cpu(channel->target_cpu,
> 				  &primary_channel->alloced_cpus_in_node);
>                                    ^^^^^ HERE ^^^^^
> 

I was basing my change on linux-next that has removed a hunk of code:

commit bcefa400900739310e8ef0cb34cbe029c404455c
Author: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Date:   Mon Apr 6 02:15:11 2020 +0200

    Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
logic

    The logic is unused since commit 509879bdb30b8 ("Drivers: hv: Introduce
    a policy for controlling channel affinity").

    This logic assumes that a channel target_cpu doesn't change during the
    lifetime of a channel, but this assumption is incompatible with the new
    functionality that allows changing the vCPU a channel will interrupt.

    Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
    Link:
https://lore.kernel.org/r/20200406001514.19876-9-parri.andrea@gmail.com
    Reviewed-by: Michael Kelley <mikelley@microsoft.com>
    Signed-off-by: Wei Liu <wei.liu@kernel.org>

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 7fb6eb647f14..476592b0bc00 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -433,14 +433,6 @@ void hv_process_channel_removal(struct
vmbus_channel *channel)
                spin_unlock_irqrestore(&primary_channel->lock, flags);
        }

-       /*
-        * We need to free the bit for init_vp_index() to work in the case
-        * of sub-channel, when we reload drivers like hv_netvsc.
-        */
-       if (channel->affinity_policy == HV_LOCALIZED)
-               cpumask_clear_cpu(channel->target_cpu,
-                                 &primary_channel->alloced_cpus_in_node);
-

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Vitaly Kuznetsov]

Colin Ian King <colin.king@canonical.com> writes:

> On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
>> Colin King <colin.king@canonical.com> writes:
>> 
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The pointer primary_channel is being assigned with a value that is never,
>>> The assignment is redundant and can be removed.
>>>
>>> Addresses-Coverity: ("Unused value")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>  drivers/hv/channel_mgmt.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
>>> --- a/drivers/hv/channel_mgmt.c
>>> +++ b/drivers/hv/channel_mgmt.c
>>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>>>  
>>>  	if (channel->primary_channel == NULL) {
>>>  		list_del(&channel->listentry);
>>> -
>>> -		primary_channel = channel;
>>>  	} else {
>>>  		primary_channel = channel->primary_channel;
>>>  		spin_lock_irqsave(&primary_channel->lock, flags);
>> 
>> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
>> 
>> 	if (channel->primary_channel == NULL) {
>> 		list_del(&channel->listentry);
>> 
>> 		primary_channel = channel;
>> 	} else {
>> 		primary_channel = channel->primary_channel;
>> 		spin_lock_irqsave(&primary_channel->lock, flags);
>> 		list_del(&channel->sc_list);
>> 		spin_unlock_irqrestore(&primary_channel->lock, flags);
>> 	}
>> 
>> 	/*
>> 	 * We need to free the bit for init_vp_index() to work in the case
>> 	 * of sub-channel, when we reload drivers like hv_netvsc.
>> 	 */
>> 	if (channel->affinity_policy == HV_LOCALIZED)
>> 		cpumask_clear_cpu(channel->target_cpu,
>> 				  &primary_channel->alloced_cpus_in_node);
>>                                    ^^^^^ HERE ^^^^^
>> 
>
> I was basing my change on linux-next that has removed a hunk of code:
>
> commit bcefa400900739310e8ef0cb34cbe029c404455c
> Author: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Date:   Mon Apr 6 02:15:11 2020 +0200
>
>     Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
> logic
>

Ah, please add the right 'Fixes:' tag then.

-- 
Vitaly

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Wei Liu]

On Wed, Apr 15, 2020 at 12:43:08PM +0200, Vitaly Kuznetsov wrote:
> Colin Ian King <colin.king@canonical.com> writes:
> 
> > On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
> >> Colin King <colin.king@canonical.com> writes:
> >> 
> >>> From: Colin Ian King <colin.king@canonical.com>
> >>>
> >>> The pointer primary_channel is being assigned with a value that is never,
> >>> The assignment is redundant and can be removed.
> >>>
> >>> Addresses-Coverity: ("Unused value")
> >>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >>> ---
> >>>  drivers/hv/channel_mgmt.c | 2 --
> >>>  1 file changed, 2 deletions(-)
> >>>
> >>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> >>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> >>> --- a/drivers/hv/channel_mgmt.c
> >>> +++ b/drivers/hv/channel_mgmt.c
> >>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
> >>>  
> >>>  	if (channel->primary_channel == NULL) {
> >>>  		list_del(&channel->listentry);
> >>> -
> >>> -		primary_channel = channel;
> >>>  	} else {
> >>>  		primary_channel = channel->primary_channel;
> >>>  		spin_lock_irqsave(&primary_channel->lock, flags);
> >> 
> >> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
> >> 
> >> 	if (channel->primary_channel == NULL) {
> >> 		list_del(&channel->listentry);
> >> 
> >> 		primary_channel = channel;
> >> 	} else {
> >> 		primary_channel = channel->primary_channel;
> >> 		spin_lock_irqsave(&primary_channel->lock, flags);
> >> 		list_del(&channel->sc_list);
> >> 		spin_unlock_irqrestore(&primary_channel->lock, flags);
> >> 	}
> >> 
> >> 	/*
> >> 	 * We need to free the bit for init_vp_index() to work in the case
> >> 	 * of sub-channel, when we reload drivers like hv_netvsc.
> >> 	 */
> >> 	if (channel->affinity_policy == HV_LOCALIZED)
> >> 		cpumask_clear_cpu(channel->target_cpu,
> >> 				  &primary_channel->alloced_cpus_in_node);
> >>                                    ^^^^^ HERE ^^^^^
> >> 
> >
> > I was basing my change on linux-next that has removed a hunk of code:
> >
> > commit bcefa400900739310e8ef0cb34cbe029c404455c
> > Author: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> > Date:   Mon Apr 6 02:15:11 2020 +0200
> >
> >     Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
> > logic
> >
> 
> Ah, please add the right 'Fixes:' tag then.

I don't think the Fixes tag is particularly useful in this instance.
Andrea's commit is not yet in Linus' tree. If I rebase hyper-next over
the next 2.5 months the tag is going to have a stale commit hash in it.

Wei.

> 
> -- 
> Vitaly
> 

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Dan Carpenter]

We have this discussion over and over.  I always say it helps to have
the commit mentioned in the commit message but it's not a Fixes tag.
So I think that the commit message should say something like
"commit 1234234 ("blah blah") removed some code so this variable isn't
used any more".  I think it helps the review process.  But then if we
mention the commit everyone says to use the Fixes tag.

It turns out if you leave out the commit entirely then people still
complain but a lot less frequently.  It shouldn't work that way but
reviewers are illogical.

regards,
dan carpenter

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Colin Ian King]

On 16/04/2020 11:02, Dan Carpenter wrote:
> We have this discussion over and over.  I always say it helps to have
> the commit mentioned in the commit message but it's not a Fixes tag.
> So I think that the commit message should say something like
> "commit 1234234 ("blah blah") removed some code so this variable isn't
> used any more".  I think it helps the review process.  But then if we
> mention the commit everyone says to use the Fixes tag.
> 
> It turns out if you leave out the commit entirely then people still
> complain but a lot less frequently.  It shouldn't work that way but
> reviewers are illogical.

Yep, to my knowledge the policy for this kind of commit is not described
in any documentation, so it's ambiguous. Either way, one can't win
without it being properly codified.

Colin

> 
> regards,
> dan carpenter
> 

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Wei Liu]

On Tue, Apr 14, 2020 at 04:23:43PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The pointer primary_channel is being assigned with a value that is never,
> The assignment is redundant and can be removed.
> 
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Thanks.

Now that the only user of primary_channel is within the else branch, we
can go one step further to move the definition of primary_channel there.

I can make the adjustment while committing this patch, as well as
updating the commit message.

Wei.

> ---
>  drivers/hv/channel_mgmt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>  
>  	if (channel->primary_channel == NULL) {
>  		list_del(&channel->listentry);
> -
> -		primary_channel = channel;
>  	} else {
>  		primary_channel = channel->primary_channel;
>  		spin_lock_irqsave(&primary_channel->lock, flags);
> -- 
> 2.25.1
> 

Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel

[Colin Ian King]

On 14/04/2020 21:24, Wei Liu wrote:
> On Tue, Apr 14, 2020 at 04:23:43PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The pointer primary_channel is being assigned with a value that is never,
>> The assignment is redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> 
> Thanks.
> 
> Now that the only user of primary_channel is within the else branch, we
> can go one step further to move the definition of primary_channel there.
> 
> I can make the adjustment while committing this patch, as well as
> updating the commit message.
> 
> Wei.

Thanks Wei,

Colin

> 
>> ---
>>  drivers/hv/channel_mgmt.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
>> --- a/drivers/hv/channel_mgmt.c
>> +++ b/drivers/hv/channel_mgmt.c
>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>>  
>>  	if (channel->primary_channel == NULL) {
>>  		list_del(&channel->listentry);
>> -
>> -		primary_channel = channel;
>>  	} else {
>>  		primary_channel = channel->primary_channel;
>>  		spin_lock_irqsave(&primary_channel->lock, flags);
>> -- 
>> 2.25.1
>>