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

[Giuseppe CAVALLARO <peppe.cavallaro@st.com>]

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.
>