From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9079A222565 for ; Thu, 19 Dec 2024 09:41:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734601321; cv=none; b=Qm+abpRQhkWeOdB3Q+SakFP2cnCy8UvG5Fj2dpfhT3iiRLKn7aobTttDITaLoEzFyMZ3RMSkJynRyF/1ic4OYFWDzjxhWK1Vu82V3WuQatiIY/Up/K3Ve8Qnvx7aOBUNoljoLJj1TzYd7t0UkqXlCdobKpUPkjpKp90x/7d96tw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734601321; c=relaxed/simple; bh=hdp4spXp/2/zcBTurghgqS00k0uxvco5aCwl9iIT51U=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=WJA0XVNYfSbIhHHQwmrfI+amKNAUzUuTaENQxJfFTTHMXWOlIBKR6Icz22zbOQPOF+wFeLPAidppRn/BnuPBUUQ+NpqbXCAWmvcySQQ3cYmXCZPY1qFaSWimrOisrGdxaKE8mo22SQPocmqPRloKgGW6wzq1B3tL8bebR3tX2VU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=dB51tcvx; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="dB51tcvx" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1734601318; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/tOwvSdF+qmldOL8MB6NtY8sLxN453oRS9gZskeqpyo=; b=dB51tcvxJHOT0ZD8Hc7TvqXRz8XYnD1GEh+VsdfXpAGmsEoSNng3jr49WpaFudJvSD/oBp 0vG1fVKh54GwnDU4195rAqgARmahBR/SkHo5YtiXqBSWkG/Z+htic2qU1ICrp3OhjJ79g8 S4mEHngN4PonvWYXRcD8umRcmyQceYk= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-153-QKlJJ9MzMrmE945ljmVfPA-1; Thu, 19 Dec 2024 04:41:56 -0500 X-MC-Unique: QKlJJ9MzMrmE945ljmVfPA-1 X-Mimecast-MFC-AGG-ID: QKlJJ9MzMrmE945ljmVfPA Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-43627bb20b5so5254805e9.1 for ; Thu, 19 Dec 2024 01:41:56 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734601316; x=1735206116; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=/tOwvSdF+qmldOL8MB6NtY8sLxN453oRS9gZskeqpyo=; b=Bw9mbVFeiCJs33bHOLUQGsAlC9hlNUyVuA+cPT2zHQhhKr+QYRbKC6JnAZp2j/vK8l Zo/qOZop33Ze/WYzJ2MmFYtbyu1h+3G4rwpfv6G/MU2sRv2Cpj+eml7ly4ow21PnNCpv zxMxO4Dz92vII19QfzQx3W4clrzStQXxSaCwrBMUT+8x7bpdJ9RF0PvS2rjkMfREWE4p cKsVwtRvJJrhEJmN4hplRU9h89qH3OndarJzB1pwncL4OTkFKsFpfyrEdGQIWktK4P5W 5+ZHQvPFmkSv3NNPTyDFTjysiRonZfO9V+Ne9fdlkd7A/p3bMmWCuGZpW9n/owtcfuoN +KZw== X-Forwarded-Encrypted: i=1; AJvYcCXxHOBmosIKEz9Z25KurU+sRUYuW3bnAN9p1MX4Zbau4iw7VJaykNG++x2oZi9x3ehCcbFwIENJGr+uMLY=@vger.kernel.org X-Gm-Message-State: AOJu0Yw2n+CBr/1B5JncVrw0SyfSJClBfbr//WJvMOXLh/vjQSMGVJoR ukRydrUwg0WgJrj3gpOj1tIJQYvSBvesPUePGnFMW4Pxr1SJI++BkfY182gFMLy9SFGYXbByAHj BDhOQ8y79wx8LKS2zSXb5ULqzW9xTXgJla5yZwgasOwIrQKFg61jtAVOMOx7AKg== X-Gm-Gg: ASbGncsoNaFFST6jtMnKLyLlwNKcGVTs4vx1bivPiMgD0LxlE6Orh7ZgmaCnuUwKoGg ZZO7LTOp6k19MeT5qOEP2Wor0U1rBno3oq1+npceO71b+SEsPIUoID9ruPnJHjpdbEqvg97V/CW hrXir8dnhncYuYuPV8FrXLDtCZudFJqVFhCMQBC6dY0gb7qr8TbAOaphopVpWGoXidEUW59NzOY EOIEm8PV6yNv2ml6gQYd9KrOXDZ5D3XYo2CuyphrmORfeOGArhc3yOfM/xuDbC9Ksx5z8oh24wy ufjyv0W86g== X-Received: by 2002:a05:6000:144a:b0:385:ed16:c97 with SMTP id ffacd0b85a97d-388e4d9aab1mr4719039f8f.49.1734601315664; Thu, 19 Dec 2024 01:41:55 -0800 (PST) X-Google-Smtp-Source: AGHT+IF7EeWzAfoVVVhCPJj0ClUy5P2YL/B/JgQGOCnw/9vJ7tKY4RPxEfDlD1OKLpyVNJV6UqRj2w== X-Received: by 2002:a05:6000:144a:b0:385:ed16:c97 with SMTP id ffacd0b85a97d-388e4d9aab1mr4718992f8f.49.1734601315221; Thu, 19 Dec 2024 01:41:55 -0800 (PST) Received: from [192.168.88.24] (146-241-54-197.dyn.eolo.it. [146.241.54.197]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a1c847513sm1135038f8f.49.2024.12.19.01.41.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 19 Dec 2024 01:41:54 -0800 (PST) Message-ID: <8a789f23-a17a-456d-ba2a-de8207d65503@redhat.com> Date: Thu, 19 Dec 2024 10:41:53 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type To: Michal Swiatkowski , Jijie Shao Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, andrew+netdev@lunn.ch, horms@kernel.org, shenjian15@huawei.com, wangpeiyang1@huawei.com, liuyonglong@huawei.com, chenhao418@huawei.com, jonathan.cameron@huawei.com, shameerali.kolothum.thodi@huawei.com, salil.mehta@huawei.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org References: <20241217010839.1742227-1-shaojijie@huawei.com> <20241217010839.1742227-2-shaojijie@huawei.com> Content-Language: en-US From: Paolo Abeni In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 12/18/24 10:02, Michal Swiatkowski wrote: > On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote: >> From: Hao Lan >> >> When a reset type that is not supported by the driver is input, a reset >> pending flag bit of the HNAE3_NONE_RESET type is generated in >> reset_pending. The driver does not have a mechanism to clear this type >> of error. As a result, the driver considers that the reset is not >> complete. This patch provides a mechanism to clear the >> HNAE3_NONE_RESET flag and the parameter of >> hnae3_ae_ops.set_default_reset_request is verified. >> >> The error message: >> hns3 0000:39:01.0: cmd failed -16 >> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled! >> hns3 0000:39:01.0: failed to reset VF stack >> hns3 0000:39:01.0: failed to reset VF(4) >> hns3 0000:39:01.0: prepare reset(2) wait done >> hns3 0000:39:01.0 eth4: already uninitialized >> >> Use the crash tool to view struct hclgevf_dev: >> struct hclgevf_dev { >> ... >> default_reset_request = 0x20, >> reset_level = HNAE3_NONE_RESET, >> reset_pending = 0x100, >> reset_type = HNAE3_NONE_RESET, >> ... >> }; >> >> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops") >> Signed-off-by: Hao Lan >> Signed-off-by: Jijie Shao >> Signed-off-by: Paolo Abeni I haven't signed-off this patch. Still no need to repost (yet) for this if the following points are solved rapidly (as I may end-up merging the series and really adding my SoB), but please avoid this kind of issue in the future. >> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev) >> return false; >> } else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) { >> hdev->rst_stats.reset_fail_cnt++; >> - set_bit(hdev->reset_type, &hdev->reset_pending); >> + hclge_set_reset_pending(hdev, hdev->reset_type); > Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is > no reset? If yes, why in this case reset_fail_cnt++ is increasing? > > Maybe the check for NONE_RESET should be done in this else if check to > prevent reset_fail_cnt from increasing (and also solve the problem with > pending bit set) @Michal: I don't understand your comment above. hclge_reset_err_handle() handles attempted reset failures. I don't see it triggered when reset_type == HNAE3_NONE_RESET. >> @@ -4470,8 +4483,20 @@ static void hclge_reset_event(struct pci_dev *pdev, struct hnae3_handle *handle) >> static void hclge_set_def_reset_request(struct hnae3_ae_dev *ae_dev, >> enum hnae3_reset_type rst_type) >> { >> +#define HCLGE_SUPPORT_RESET_TYPE \ >> + (BIT(HNAE3_FLR_RESET) | BIT(HNAE3_FUNC_RESET) | \ >> + BIT(HNAE3_GLOBAL_RESET) | BIT(HNAE3_IMP_RESET)) >> + >> struct hclge_dev *hdev = ae_dev->priv; >> >> + if (!(BIT(rst_type) & HCLGE_SUPPORT_RESET_TYPE)) { >> + /* To prevent reset triggered by hclge_reset_event */ >> + set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request); >> + dev_warn(&hdev->pdev->dev, "unsupported reset type %d\n", >> + rst_type); >> + return; >> + } > Maybe (nit): > if (...) { > rst_type = > dev_warn(); > } > > set_bit(rst_type, ); > It is a little hard to follow with return in the if. @Michal: I personally find the patch code quite readable, do you have strong opinions here? >> set_bit(rst_type, &hdev->default_reset_request); >> } >> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c >> index 2f6ffb88e700..fd0abe37fdd7 100644 >> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c >> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c >> @@ -1393,6 +1393,17 @@ static int hclgevf_notify_roce_client(struct hclgevf_dev *hdev, >> return ret; >> } >> >> +static void hclgevf_set_reset_pending(struct hclgevf_dev *hdev, >> + enum hnae3_reset_type reset_type) >> +{ >> + /* When an incorrect reset type is executed, the get_reset_level >> + * function generates the HNAE3_NONE_RESET flag. As a result, this >> + * type do not need to pending. >> + */ >> + if (reset_type != HNAE3_NONE_RESET) >> + set_bit(reset_type, &hdev->reset_pending); >> +} > You already have a way to share the code between PF and VF, so please > move the same functions to common file in one direction up. AFAICS this can't be shared short of a large refactor not suitable for net as the functions eligible for sharing operate on different structs with different layout (hclgevf_dev vs hclge_dev). Currently all the shared code operates on shared structs. Cheers, Paolo