From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BA028C47258 for ; Fri, 26 Jan 2024 02:09:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=W9GORzynnvYZpKN7xh+O3Lba1DKKvKP4gi/dChskDQ8=; b=Y56f3UzhxVetz1 bA6W6SBX8shHY+DR8jGBImEUO9G50/CsisrdhxOcm6xzdZylLxVcxQUcbWuWfiEcVqjzwgSLUadDm q8oqDKJUtuQX3u/8GhjwYqZnlRG9AEVm1LPXvRUNqMUWKDO6gY0xYMgKwo1R8VEXMawCJ2mvlnsWH 4w3+lyu1HKM5jCrPd+sjWY9cip6jAKWTZaO5qGwnBuFFc5RgQ1DF8sQk8crQ9ikHALotajAQ7VGcx 4HVBSi14K76JOiYQNl6FfNFnj5gZeS7Bj2oGip4mrpIh2rweNZOca9/OZMpb+FGl7l/xyM6zoQor7 Rhw6SnHLwLy/mqAbeMpA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rTBeQ-00000002pU9-1ifR; Fri, 26 Jan 2024 02:09:02 +0000 Received: from mail-pg1-x536.google.com ([2607:f8b0:4864:20::536]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rTBeO-00000002pTm-0CYK for linux-arm-kernel@lists.infradead.org; Fri, 26 Jan 2024 02:09:01 +0000 Received: by mail-pg1-x536.google.com with SMTP id 41be03b00d2f7-5ce07cf1e5dso4252483a12.2 for ; Thu, 25 Jan 2024 18:08:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1706234939; x=1706839739; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=Br6VJLagRb3AfSDX41gJAp6j1fOegiaHY5+r3X1kYKU=; b=ZA4su+ZdaP9GMvLOtCciVPoypjfkMJl0rxqlcBkfN/4aqEHVM7K2BSbqxvCWuzuSbx RJc0idWvDi79GGbL/qb0buoGXU/Bh4DH+a3Q/fhvNopbJhH+f62UxT+ZMHK6dDjJRbnA 2qq6e37jrlR5RLh2zuGhrYBnykz2XzBrXbL/1GIAIgB0b/FseMZaMml9/SgJRSQglsYU KkZgwFtBsLsr7mpJ88SPDI+61kKLrLdWLShRblWJrfmfixDZL7o+0zoDsTlU+Pa4g+DR 8Tswsklc1K3ZJPphM8P8yaeO2CALVau4ZHl8mtjtC8y/uWOqSMDEyJiDxRNDachf84I3 KuzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706234939; x=1706839739; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Br6VJLagRb3AfSDX41gJAp6j1fOegiaHY5+r3X1kYKU=; b=qsiOJw4bSzChAibNHt/cZHOUjrVSYDvCfTI+8ZefH+Ss0MSkfvPQXq1fadmMFFRP09 xw8zZdhZtX/59LShirl0iI32Ua75QXI7Q/ZMg3bxvZjDVA+E1g9q+FAdC22R4GHXXid7 g8qBWUhC01nZXbUGsZsW2LygHDaMflHfwZzRxi3TBqgvhfiicx16q/Hmbo2o8YmlAym3 /DLijxhQ/gl7s/rAV5wiVGEHIg7hkA5Mc9HpFmG9ssqxogsBOwa2rC64flXUDND5deck Ghcwi/EcfcEl0X3HiSic943xd1OPNJUq7/+rj69zIT0OD4DjXGXf8rTa8GW8WmeGCXiT mEAw== X-Gm-Message-State: AOJu0YyNN8DMfiBUg1Bt2SK0p9IjKD4cCTirSZp1mKKwKW5sUG26mk+O lklYjv5lx+uCcEh3ZJw2wMr9izSSqQuFc28d7J+4Vf3DdieU5yZH X-Google-Smtp-Source: AGHT+IFpXDfDdLfol3GnedT7BVcJU3Lg5lPS/W1n8oFL0fn5cIeJRsE+SQKAsKBssVzbjlUXZm9qHA== X-Received: by 2002:a05:6a20:6a03:b0:19a:404b:86c3 with SMTP id p3-20020a056a206a0300b0019a404b86c3mr631310pzk.70.1706234938867; Thu, 25 Jan 2024 18:08:58 -0800 (PST) Received: from localhost ([129.146.253.192]) by smtp.gmail.com with ESMTPSA id m13-20020a17090ade0d00b0028ddfb484bfsm2251209pjv.49.2024.01.25.18.08.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Jan 2024 18:08:58 -0800 (PST) Date: Fri, 26 Jan 2024 10:08:47 +0800 From: Furong Xu <0x1207@gmail.com> To: Serge Semin Cc: "David S. Miller" , Alexandre Torgue , Jose Abreu , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Maxime Coquelin , Joao Pinto , Simon Horman , netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, xfr@outlook.com, rock.xu@nio.com Subject: Re: [PATCH net] net: stmmac: xgmac: fix safety error descriptions Message-ID: <20240126100847.0000370e@gmail.com> In-Reply-To: <4coefc2fqtc2eoereds3rf7vudci5l7ahme2wydocjepk2wrwy@ncgwl3j3koyu> References: <20240123085037.939471-1-0x1207@gmail.com> <20240125103454.0000312a@gmail.com> <4coefc2fqtc2eoereds3rf7vudci5l7ahme2wydocjepk2wrwy@ncgwl3j3koyu> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.34; x86_64-w64-mingw32) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240125_180900_137512_B9CCC72B X-CRM114-Status: GOOD ( 47.68 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 25 Jan 2024 16:48:23 +0300 Serge Semin wrote: > On Thu, Jan 25, 2024 at 10:34:54AM +0800, Furong Xu wrote: > > On Wed, 24 Jan 2024 17:25:27 +0300 > > Serge Semin wrote: > > > > > On Tue, Jan 23, 2024 at 04:50:37PM +0800, Furong Xu wrote: > > > > Commit 56e58d6c8a56 ("net: stmmac: Implement Safety Features in > > > > XGMAC core") prints safety error descriptions when safety error assert, > > > > but missed some special errors, and mixed correctable errors and > > > > uncorrectable errors together. > > > > This patch complete the error code list and print the type of errors. > > > > > > The XGMAC ECC Safety code has likely been just copied from the DW GMAC > > > v5 (DW QoS Eth) part. So this change is partly relevant to that code too. I > > > can't confirm that the special errors support is relevant to the DW > > > QoS Eth too (it likely is though), so what about splitting this patch > > > up into two: > > > 1. Elaborate the errors description for DW GMAC v5 and DW XGMAC. > > > 2. Add new ECC safety errors support. > > > ? > > > > > > On the other hand if we were sure that both DW QoS Eth and XGMAC > > > safety features implementation match the ideal solution would be to > > > refactor out the common code into a dedicated module. > > > > > > -Serge(y) > > > > > > > > Checked XGMAC Version 3.20a and DW QoS Eth Version 5.20a, the safety error > > code definitions are not identical at all, they do have some differences, > > about more than 20 bits of status register are different. > > I think we should just leave them in individual implementations. > > For some reason you answered to the last part of my comment and > completely ignored the first part which was the main point of my > message. > > Regarding the Safety Feature support implemented in QoS Eth and XGMAC > STMMAC modules. You were wrong in using the statement "at all". Except > the optional events enable/disable procedure introduced in the commit > 5ac712dcdfef ("net: stmmac: enable platform specific safety > features"), there aren't many differences: at least the errors > handling and report are identical, MTL and DMA error flags match, even > MTL and DMA ECC/Safety IRQ flags match. The only difference is in the > MTL/MAC DPP (Data Parity Protection) part which can be easily factored > out based on the device ID should we attempt to refactor the safety > feature code. See the attached html-diff for more details of what > match and what is different. > > Anyway I am not insisting on the refactoring. That was just a > proposal, a more preferred alternative to simultaneously patching two > parts of the drivers looking very much alike. Such refactoring would > improve the code maintainability. The main point of my comment was to > extend your patch for DW QoS Eth safety feature implementation too > since some of the changes you introduced were useful for it too, and > in splitting the patch up since your patch added new flags support > which was unrelated change. Thus your patch would turned into the > two-patches patchset like this: > [Patch 1] would provide an elaborated errors description for both DW > QOS Eth (GMAC v5.x) and DW XGMAC. > [Patch 2] would introduce the new ECC safety errors support. > > See my further comments about the respective changes. > > > > > > > > > > > Fixes: 56e58d6c8a56 ("net: stmmac: Implement Safety Features in XGMAC core") > > > > Signed-off-by: Furong Xu <0x1207@gmail.com> > > > > --- > > > > .../ethernet/stmicro/stmmac/dwxgmac2_core.c | 36 +++++++++---------- > > > > 1 file changed, 18 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > > > index eb48211d9b0e..ad812484059e 100644 > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c > > > > @@ -748,29 +748,29 @@ static void dwxgmac3_handle_mac_err(struct net_device *ndev, > > > > } > > > > > > > > static const struct dwxgmac3_error_desc dwxgmac3_mtl_errors[32]= { > > > > > - { true, "TXCES", "MTL TX Memory Error" }, > > > > + { true, "TXCES", "MTL TX Memory Correctable Error" }, > > Applicable for both IP-cores > [Patch 1] +QoS, +XGMAC > please apply this change to dwmac5.c too. > > > > > { true, "TXAMS", "MTL TX Memory Address Mismatch Error" }, > > > > > - { true, "TXUES", "MTL TX Memory Error" }, > > > > + { true, "TXUES", "MTL TX Memory Uncorrectable Error" }, > > [Patch 1] +QoS, +XGMAC > ditto > > > > > { false, "UNKNOWN", "Unknown Error" }, /* 3 */ > > > > > - { true, "RXCES", "MTL RX Memory Error" }, > > > > + { true, "RXCES", "MTL RX Memory Correctable Error" }, > > [Patch 1] +QoS, +XGMAC > ditto > > > > > { true, "RXAMS", "MTL RX Memory Address Mismatch Error" }, > > > > > - { true, "RXUES", "MTL RX Memory Error" }, > > > > + { true, "RXUES", "MTL RX Memory Uncorrectable Error" }, > > [Patch 1] +QoS, +XGMAC > ditto > > > > > { false, "UNKNOWN", "Unknown Error" }, /* 7 */ > > > > > - { true, "ECES", "MTL EST Memory Error" }, > > > > + { true, "ECES", "MTL EST Memory Correctable Error" }, > > [Patch 1] +QoS, +XGMAC > ditto > > > > > { true, "EAMS", "MTL EST Memory Address Mismatch Error" }, > > > > > - { true, "EUES", "MTL EST Memory Error" }, > > > > + { true, "EUES", "MTL EST Memory Uncorrectable Error" }, > > [Patch 1] +QoS, +XGMAC > ditto > > > > > { false, "UNKNOWN", "Unknown Error" }, /* 11 */ > > > > > - { true, "RPCES", "MTL RX Parser Memory Error" }, > > > > + { true, "RPCES", "MTL RX Parser Memory Correctable Error" }, > > [Patch 1] +QoS, +XGMAC > ditto > > > > > { true, "RPAMS", "MTL RX Parser Memory Address Mismatch Error" }, > > > > > - { true, "RPUES", "MTL RX Parser Memory Error" }, > > > > + { true, "RPUES", "MTL RX Parser Memory Uncorrectable Error" }, > > [Patch 1] +QoS, +XGMAC > ditto > > > > > { false, "UNKNOWN", "Unknown Error" }, /* 15 */ > > > > > - { false, "UNKNOWN", "Unknown Error" }, /* 16 */ > > > > - { false, "UNKNOWN", "Unknown Error" }, /* 17 */ > > > > - { false, "UNKNOWN", "Unknown Error" }, /* 18 */ > > > > + { true, "SCES", "MTL SGF GCL Memory Correctable Error" }, > > > > + { true, "SAMS", "MTL SGF GCL Memory Address Mismatch Error" }, > > > > + { true, "SUES", "MTL SGF GCL Memory Uncorrectable Error" }, > > > > { false, "UNKNOWN", "Unknown Error" }, /* 19 */ > > > > - { false, "UNKNOWN", "Unknown Error" }, /* 20 */ > > > > - { false, "UNKNOWN", "Unknown Error" }, /* 21 */ > > > > - { false, "UNKNOWN", "Unknown Error" }, /* 22 */ > > > > + { true, "RXFCES", "MTL RXF Memory Correctable Error" }, > > > > + { true, "RXFAMS", "MTL RXF Memory Address Mismatch Error" }, > > > > + { true, "RXFUES", "MTL RXF Memory Uncorrectable Error" }, > > This introduces the new flags support. Please move this change into a > separate patch (Patch 2): > [Patch 2] +XGMAC only. > > My DW QoS Eth v5.10a databook doesn't have these flags defined. If > your 5.20a HW-manual have them described then please add them for DW > QoS Eth too. > > > > > { false, "UNKNOWN", "Unknown Error" }, /* 23 */ > > > > { false, "UNKNOWN", "Unknown Error" }, /* 24 */ > > > > { false, "UNKNOWN", "Unknown Error" }, /* 25 */ > > > > @@ -796,13 +796,13 @@ static void dwxgmac3_handle_mtl_err(struct net_device *ndev, > > > > } > > > > > > > > static const struct dwxgmac3_error_desc dwxgmac3_dma_errors[32]= { > > > > > - { true, "TCES", "DMA TSO Memory Error" }, > > > > + { true, "TCES", "DMA TSO Memory Correctable Error" }, > > Applicable for both IP-cores > [Patch 1] +QoS, +XGMAC > please apply this change to dwmac5.c too. > > > > > { true, "TAMS", "DMA TSO Memory Address Mismatch Error" }, > > > > > - { true, "TUES", "DMA TSO Memory Error" }, > > > > + { true, "TUES", "DMA TSO Memory Uncorrectable Error" }, > > [Patch 1] +QoS, +XGMAC > ditto > > > > > { false, "UNKNOWN", "Unknown Error" }, /* 3 */ > > > > > - { true, "DCES", "DMA DCACHE Memory Error" }, > > > > + { true, "DCES", "DMA DCACHE Memory Correctable Error" }, > > > > { true, "DAMS", "DMA DCACHE Address Mismatch Error" }, > > > > - { true, "DUES", "DMA DCACHE Memory Error" }, > > > > + { true, "DUES", "DMA DCACHE Memory Uncorrectable Error" }, > > AFAICS applicable for XGMAC only > [Patch 1] +XGMAC only. > Once again, My DW QoS Eth v5.10a databook doesn't have these flags > defined. So if your DW QoS Eth 5.20a HW-manual do have them described > please add them for DW QoS Eth with the elaborated description in the > framework of the Patch 2. > > -Serge(y) > > > > > { false, "UNKNOWN", "Unknown Error" }, /* 7 */ > > > > { false, "UNKNOWN", "Unknown Error" }, /* 8 */ > > > > { false, "UNKNOWN", "Unknown Error" }, /* 9 */ > > > > -- > > > > 2.34.1 > > > > > > > > > > Hi Serge: Thanks for your detailed explanation, new refactoring will be sent to net-next. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel