netdev - Re: [PATCH net-next 2/2] net: stmmac: Add support for DWMAC5 and implement Safety Features

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3196a467-cfe7-fb2f-0ae6-f243b3aeebd7@synopsys.com>
Date:   Thu, 1 Feb 2018 15:18:33 +0000
From:   Jose Abreu <Jose.Abreu@...opsys.com>
To:     Andrew Lunn <andrew@...n.ch>, Joao Pinto <Joao.Pinto@...opsys.com>
CC:     Jose Abreu <Jose.Abreu@...opsys.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "Giuseppe Cavallaro" <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>
Subject: Re: [PATCH net-next 2/2] net: stmmac: Add support for DWMAC5 and
 implement Safety Features

Hi Andrew,

On 01-02-2018 15:02, Andrew Lunn wrote:
>> +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.

Yes, I will add.

>
>> +
>> +static bool dwmac5_handle_mac_err(struct net_device *ndev,
>> +		void __iomem *ioaddr, bool correctable) {
>> +	u32 value;
>> +
>> +	value = readl(ioaddr + MAC_DPP_FSM_INT_STATUS);
>> +	writel(value, ioaddr + MAC_DPP_FSM_INT_STATUS);
>> +
>> +	dwmac5_log_error(ndev, value, correctable, "MAC", dwmac5_mac_errors);
>> +	return !correctable;
>> +}
> Returning !correctable in all these functions seems pointless.  None
> of the handlers change its value. So just make these void functions.

Ok.

>
>>  static void stmmac_global_err(struct stmmac_priv *priv)  {
>> +	netif_carrier_off(priv->dev);
>>  	set_bit(STMMAC_RESET_REQUESTED, &priv->state);
>>  	stmmac_service_event_schedule(priv);
>>  }
> This should be in a separate patch, with an explanation why it is
> needed.

Yes it should belong in the first patch of this series.

Best Regards,
Jose Miguel Abreu

>
> 	Andrew