netdev - Re: [PATCH V2 net 4/7] net: hns3: use appropriate barrier function after setting a bit value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240426142553.GB513047@kernel.org>
Date: Fri, 26 Apr 2024 15:25:53 +0100
From: Simon Horman <horms@...nel.org>
To: Jijie Shao <shaojijie@...wei.com>
Cc: yisen.zhuang@...wei.com, salil.mehta@...wei.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	jiri@...nulli.us, shenjian15@...wei.com, wangjie125@...wei.com,
	liuyonglong@...wei.com, chenhao418@...wei.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 net 4/7] net: hns3: use appropriate barrier function
 after setting a bit value

On Fri, Apr 26, 2024 at 06:00:42PM +0800, Jijie Shao wrote:
> From: Peiyang Wang <wangpeiyang1@...wei.com>
> 
> There is a memory barrier in followed case. When set the port down,
> hclgevf_set_timmer will set DOWN in state. Meanwhile, the service task has
> different behaviour based on whether the state is DOWN. Thus, to make sure
> service task see DOWN, use smp_mb__after_atomic after calling set_bit().
> 
>           CPU0                        CPU1
> ========================== ===================================
> hclgevf_set_timer_task()    hclgevf_periodic_service_task()
>   set_bit(DOWN,state)         test_bit(DOWN,state)
> 
> pf also has this issue.
> 
> Fixes: ff200099d271 ("net: hns3: remove unnecessary work in hclgevf_main")
> Fixes: 1c6dfe6fc6f7 ("net: hns3: remove mailbox and reset work in hclge_main")

FWIIW, I think it is fine to fix both problems in one patch
because both the cited patches were included in the same release - v5.6.
(Actually, they seem to be consecutive patches in git history.)

> Signed-off-by: Peiyang Wang <wangpeiyang1@...wei.com>
> Signed-off-by: Jijie Shao <shaojijie@...wei.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c   | 3 +--
>  drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index a068cd745eb4..6eda73f1e6ad 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -7954,8 +7954,7 @@ static void hclge_set_timer_task(struct hnae3_handle *handle, bool enable)
>  		/* Set the DOWN flag here to disable link updating */
>  		set_bit(HCLGE_STATE_DOWN, &hdev->state);
>  
> -		/* flush memory to make sure DOWN is seen by service task */
> -		smp_mb__before_atomic();
> +		smp_mb__after_atomic(); /* flush memory to make sure DOWN is seen by service task */

If you need to post a v2 for some other reason, please consider reworking
this comment so lines are no longer than 80 columns wide. The previous form
where the comment was on it's own line looks good to me.

Likewise below.

In any case, this patch looks good to me.

Reviewed-by: Simon Horman <horms@...nel.org>

>  		hclge_flush_link_update(hdev);
>  	}
>  }
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> index b57111252d07..08db8e84be4e 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> @@ -2181,8 +2181,7 @@ static void hclgevf_set_timer_task(struct hnae3_handle *handle, bool enable)
>  	} else {
>  		set_bit(HCLGEVF_STATE_DOWN, &hdev->state);
>  
> -		/* flush memory to make sure DOWN is seen by service task */
> -		smp_mb__before_atomic();
> +		smp_mb__after_atomic(); /* flush memory to make sure DOWN is seen by service task */
>  		hclgevf_flush_link_update(hdev);
>  	}
>  }
> -- 
> 2.30.0
> 
>