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] [day] [month] [year] [list]

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

Hello

any news or other advice?

In that case, I'll resend the patches (v3) with the modifications
suggested by Ben.

Regards
Peppe

On 8/31/2011 7:52 AM, Giuseppe CAVALLARO wrote:
> 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
> 

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