From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Miller Subject: Re: [PATCH net-next 2/2] net: stmmac: Add support for DWMAC5 and implement Safety Features Date: Thu, 01 Feb 2018 10:09:18 -0500 (EST) Message-ID: <20180201.100918.842039702040652176.davem@davemloft.net> References: <20180201150209.GB16818@lunn.ch> Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: Joao.Pinto@synopsys.com, Jose.Abreu@synopsys.com, netdev@vger.kernel.org, peppe.cavallaro@st.com, alexandre.torgue@st.com To: andrew@lunn.ch Return-path: Received: from shards.monkeyblade.net ([184.105.139.130]:44324 "EHLO shards.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751531AbeBAPJU (ORCPT ); Thu, 1 Feb 2018 10:09:20 -0500 In-Reply-To: <20180201150209.GB16818@lunn.ch> Sender: netdev-owner@vger.kernel.org List-ID: From: Andrew Lunn Date: Thu, 1 Feb 2018 16:02:09 +0100 >> +static void dwmac5_log_error(struct net_device *ndev, u32 value, bool corr, >> + const char *module_name, const char **errors_str) { >> + unsigned long loc, mask; >> + >> + mask = value; >> + for_each_set_bit(loc, &mask, 32) { >> + netdev_err(ndev, "Found %s error in %s: '%s'\n", corr ? >> + "correctable" : "uncorrectable", module_name, >> + errors_str[loc]); >> + } >> +} > > How about also adding ethtool -S stats. You have a text string, so all > you need to add is a counter. And i expect statistics are looked at > more than dmesg output. I agree. Perhaps for extremely catastrophic errors (those which require a complete chip reset, for example), statistics are really the way to go. Also, all of these functions are not style properly. The openning curly braces of a function belong on a separate line of their own, not at the end of the arguments. The braces for structure assignment blocks have a similar problem, the final closing brace and semicolon should be on a line by itself.