2024-04-26 10:16:40

by Jijie Shao

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

From: Peiyang Wang <[email protected]>

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")
Signed-off-by: Peiyang Wang <[email protected]>
Signed-off-by: Jijie Shao <[email protected]>
---
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 */
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

2024-04-26 14:28:13

by Simon Horman

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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jijie Shao <[email protected]>
> ---
> 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 <[email protected]>

> 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
>
>