| Message ID | 1574130708-30767-1-git-send-email-tanhuazhong@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: hns3: fix a wrong reset interrupt status mask | expand |
From: Huazhong Tan <tanhuazhong@huawei.com> Date: Tue, 19 Nov 2019 10:31:48 +0800 > According to hardware user manual, bits5~7 in register > HCLGE_MISC_VECTOR_INT_STS means reset interrupts status, > but HCLGE_RESET_INT_M is defined as bits0~2 now. So it > will make hclge_reset_err_handle() read the wrong reset > interrupt status. > > This patch fixes it and prints out the register value. > > Fixes: 2336f19d7892 ("net: hns3: check reset interrupt status when reset fails") > Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com> Fix exactly _one_ thing or else you make your patch hard to review. The bug is that the bits are wrong, just fix the bits! > > + u32 msix_sts_reg; > + > + msix_sts_reg = hclge_read_dev(&hdev->hw, HCLGE_MISC_VECTOR_INT_STS); > + > if (hdev->reset_pending) { Now you are reading a register, and potentially clearing status bits and causing other side effects, that would not happen in this code path where hdev->reset_pending is true. Don't do stuff like this! If you want to add code to print out the register value, that is a separate patch, for net-next, and it must be done properly. In that you should only read the register in the same code paths you do previously. Otherwise you must _clearly_ explain why reading the register value in new code paths is OK, and the side effects will not potentially cause problems for the pending reset operation. It is still going to be a net-next improvement only. Thank you.
On 2019/11/20 7:36, David Miller wrote: > From: Huazhong Tan <tanhuazhong@huawei.com> > Date: Tue, 19 Nov 2019 10:31:48 +0800 > >> According to hardware user manual, bits5~7 in register >> HCLGE_MISC_VECTOR_INT_STS means reset interrupts status, >> but HCLGE_RESET_INT_M is defined as bits0~2 now. So it >> will make hclge_reset_err_handle() read the wrong reset >> interrupt status. >> >> This patch fixes it and prints out the register value. >> >> Fixes: 2336f19d7892 ("net: hns3: check reset interrupt status when reset fails") >> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com> > > Fix exactly _one_ thing or else you make your patch hard to review. > > The bug is that the bits are wrong, just fix the bits! > ok, thanks >> >> + u32 msix_sts_reg; >> + >> + msix_sts_reg = hclge_read_dev(&hdev->hw, HCLGE_MISC_VECTOR_INT_STS); >> + >> if (hdev->reset_pending) { > > Now you are reading a register, and potentially clearing status bits and > causing other side effects, that would not happen in this code path > where hdev->reset_pending is true. > > Don't do stuff like this! > > If you want to add code to print out the register value, that is a > separate patch, for net-next, and it must be done properly. In that > you should only read the register in the same code paths you do > previously. Otherwise you must _clearly_ explain why reading the > register value in new code paths is OK, and the side effects will > not potentially cause problems for the pending reset operation. It > is still going to be a net-next improvement only. > > Thank you. I will separate it, and resend the fix to -net. Thanks. > > . >
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c index c052bb3..731cda0 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c @@ -3555,14 +3555,18 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev) { #define MAX_RESET_FAIL_CNT 5 + u32 msix_sts_reg; + + msix_sts_reg = hclge_read_dev(&hdev->hw, HCLGE_MISC_VECTOR_INT_STS); + if (hdev->reset_pending) { dev_info(&hdev->pdev->dev, "Reset pending %lu\n", hdev->reset_pending); return true; - } else if (hclge_read_dev(&hdev->hw, HCLGE_MISC_VECTOR_INT_STS) & - HCLGE_RESET_INT_M) { + } else if (msix_sts_reg & HCLGE_RESET_INT_M) { dev_info(&hdev->pdev->dev, - "reset failed because new reset interrupt\n"); + "fail to reset, new reset interrupt is 0x%x\n", + msix_sts_reg); hclge_clear_reset_cause(hdev); return false; } else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) { diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h index 59b8243..615cde1 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h @@ -166,7 +166,7 @@ enum HLCGE_PORT_TYPE { #define HCLGE_GLOBAL_RESET_BIT 0 #define HCLGE_CORE_RESET_BIT 1 #define HCLGE_IMP_RESET_BIT 2 -#define HCLGE_RESET_INT_M GENMASK(2, 0) +#define HCLGE_RESET_INT_M GENMASK(7, 5) #define HCLGE_FUN_RST_ING 0x20C00 #define HCLGE_FUN_RST_ING_B 0
According to hardware user manual, bits5~7 in register HCLGE_MISC_VECTOR_INT_STS means reset interrupts status, but HCLGE_RESET_INT_M is defined as bits0~2 now. So it will make hclge_reset_err_handle() read the wrong reset interrupt status. This patch fixes it and prints out the register value. Fixes: 2336f19d7892 ("net: hns3: check reset interrupt status when reset fails") Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com> --- drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 10 +++++++--- drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-)