Re: [patch net-next-2.6 v2] net: consolidate and fix ethtool_ops->get_settings calling

Fri, Sep 02, 2011 at 08:46:01PM CEST, bhutchings@solarflare.com wrote:

On Fri, 2011-09-02 at 14:26 +0200, Jiri Pirko wrote:

quoted

This patch does several things:
- introduces __ethtool_get_settings which is called from ethtool code and
  from dev_ethtool_get_settings() as well.
- dev_ethtool_get_settings() becomes rtnl wrapper for
  __ethtool_get_settings()

[...]

I don't like this locking change.  Most other dev_*() functions require
the caller to hold RTNL, and it will break any OOT module calling
dev_ethtool_get_settings() without producing any warning at compile
time.  Why not put an ASSERT_RTNL() in it instead?

Hmm. Okay, then I would remove dev_ethtool_get_settings() from
net/core/dev.c and only put __ethtool_get_settings() to
net/core/ethtool.c. Makes more sense to me to have it there...
ASSERT_RTNL woudl be good there as well.

The rest of this looks fine.

Ben. 

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.