netdev - Re: [PATCH 4/9] stmmac: add MMC support exported via ethtool (v2)

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]

Date:	Wed, 31 Aug 2011 07:52:40 +0200
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 4/9] stmmac: add MMC support exported via ethtool (v2)

Hello Ben

On 8/30/2011 5:26 PM, Ben Hutchings wrote:
> On Tue, 2011-08-30 at 16:20 +0200, Giuseppe CAVALLARO wrote:
>> This patch adds the MMC management counters support.
>> MMC module is an extension of the register address
>> space and all the hardware counters can be accessed
>> via ethtoo -S ethX.
>>
>> Note that, the MMC interrupts remain masked and the logic
>> to handle this kind of interrupt will be added later (if
>> actually useful).
> [...]
>>  static void stmmac_ethtool_getdrvinfo(struct net_device *dev,
>>  				      struct ethtool_drvinfo *info)
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(dev);
>>  
>> -	if (!priv->plat->has_gmac)
>> -		strcpy(info->driver, MAC100_ETHTOOL_NAME);
>> -	else
>> +	info->n_stats = STMMAC_STATS_LEN;
>> +
>> +	if (likely(priv->plat->has_gmac)) {
> 
> Using likely() and unlikely() in ethtool operations seems like a
> pointless optimisation.

I agree with you that ethtool part is not critical and likely/unlikely
could be not used at all.

I had added them because today, AFAIK, the driver works on several
platforms with the GMAC and continues to support MAC10/100 (that I test
in my lab). I had thought that the likely/unlikely in ethtool can help
to emit instructions that causes optimize branch prediction to favor the
"likely" GMAC side.

At any rate, no problem to rework the patch deleting them or create a
new one on top of these.

> 
>>  		strcpy(info->driver, GMAC_ETHTOOL_NAME);
>> +		info->n_stats += STMMAC_MMC_STATS_LEN;
>> +	} else
>> +		strcpy(info->driver, MAC100_ETHTOOL_NAME);
>>  
>>  	strcpy(info->version, DRV_MODULE_VERSION);
>>  	info->fw_version[0] = '\0';
>> -	info->n_stats = STMMAC_STATS_LEN;
>>  }
> [...]
> 
> Don't bother initialising ethtool_drvinfo::n_stats.  The ethtool core
> will do it by calling your get_sset_count implementation.

Ok! thanks I missed this.

As above, let me know if I have to resent the patches or create a new
patch for this fix too.

In the end, any other advice?

Regards
Peppe
> 
> Ben.
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html