* [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-14 15:23 ` Colin King
0 siblings, 0 replies; 20+ messages in thread
From: Colin King @ 2020-04-14 15:23 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv
Cc: kernel-janitors, linux-kernel
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-14 15:23 ` Colin King
@ 2020-04-14 15:42 ` Julia Lawall
-1 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2020-04-14 15:42 UTC (permalink / raw)
To: Colin King
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, kernel-janitors, linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-14 15:42 ` Julia Lawall
0 siblings, 0 replies; 20+ messages in thread
From: Julia Lawall @ 2020-04-14 15:42 UTC (permalink / raw)
To: Colin King
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, kernel-janitors, linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-14 15:23 ` Colin King
@ 2020-04-14 16:51 ` Vitaly Kuznetsov
-1 siblings, 0 replies; 20+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-14 16:51 UTC (permalink / raw)
To: Colin King
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-14 16:51 ` Vitaly Kuznetsov
0 siblings, 0 replies; 20+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-14 16:51 UTC (permalink / raw)
To: Colin King
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-14 16:51 ` Vitaly Kuznetsov
@ 2020-04-14 16:58 ` Colin Ian King
-1 siblings, 0 replies; 20+ messages in thread
From: Colin Ian King @ 2020-04-14 16:58 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
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);
-
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-14 16:58 ` Colin Ian King
0 siblings, 0 replies; 20+ messages in thread
From: Colin Ian King @ 2020-04-14 16:58 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
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);
-
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-14 16:58 ` Colin Ian King
@ 2020-04-15 10:43 ` Vitaly Kuznetsov
-1 siblings, 0 replies; 20+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-15 10:43 UTC (permalink / raw)
To: Colin Ian King
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-15 10:43 ` Vitaly Kuznetsov
0 siblings, 0 replies; 20+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-15 10:43 UTC (permalink / raw)
To: Colin Ian King
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-15 10:43 ` Vitaly Kuznetsov
@ 2020-04-15 14:37 ` Wei Liu
-1 siblings, 0 replies; 20+ messages in thread
From: Wei Liu @ 2020-04-15 14:37 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Colin Ian King, kernel-janitors, linux-kernel,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-15 14:37 ` Wei Liu
0 siblings, 0 replies; 20+ messages in thread
From: Wei Liu @ 2020-04-15 14:37 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Colin Ian King, kernel-janitors, linux-kernel,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-15 14:37 ` Wei Liu
@ 2020-04-16 10:02 ` Dan Carpenter
-1 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2020-04-16 10:02 UTC (permalink / raw)
To: Wei Liu
Cc: Vitaly Kuznetsov, Colin Ian King, kernel-janitors, linux-kernel,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-16 10:02 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2020-04-16 10:02 UTC (permalink / raw)
To: Wei Liu
Cc: Vitaly Kuznetsov, Colin Ian King, kernel-janitors, linux-kernel,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-16 10:02 ` Dan Carpenter
@ 2020-04-16 10:08 ` Colin Ian King
-1 siblings, 0 replies; 20+ messages in thread
From: Colin Ian King @ 2020-04-16 10:08 UTC (permalink / raw)
To: Dan Carpenter, Wei Liu
Cc: Vitaly Kuznetsov, kernel-janitors, linux-kernel,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-16 10:08 ` Colin Ian King
0 siblings, 0 replies; 20+ messages in thread
From: Colin Ian King @ 2020-04-16 10:08 UTC (permalink / raw)
To: Dan Carpenter, Wei Liu
Cc: Vitaly Kuznetsov, kernel-janitors, linux-kernel,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-14 15:23 ` Colin King
@ 2020-04-14 20:24 ` Wei Liu
-1 siblings, 0 replies; 20+ messages in thread
From: Wei Liu @ 2020-04-14 20:24 UTC (permalink / raw)
To: Colin King
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, kernel-janitors, linux-kernel
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-14 20:24 ` Wei Liu
0 siblings, 0 replies; 20+ messages in thread
From: Wei Liu @ 2020-04-14 20:24 UTC (permalink / raw)
To: Colin King
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, kernel-janitors, linux-kernel
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
2020-04-14 20:24 ` Wei Liu
@ 2020-04-14 21:25 ` Colin Ian King
-1 siblings, 0 replies; 20+ messages in thread
From: Colin Ian King @ 2020-04-14 21:25 UTC (permalink / raw)
To: Wei Liu
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv, kernel-janitors, linux-kernel
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
>>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
@ 2020-04-14 21:25 ` Colin Ian King
0 siblings, 0 replies; 20+ messages in thread
From: Colin Ian King @ 2020-04-14 21:25 UTC (permalink / raw)
To: Wei Liu
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv, kernel-janitors, linux-kernel
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
>>
^ permalink raw reply [flat|nested] 20+ messages in thread