From patchwork Sun Sep 16 19:17:26 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 184143 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id B3C4C2C007F for ; Mon, 17 Sep 2012 05:17:39 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751528Ab2IPTRh (ORCPT ); Sun, 16 Sep 2012 15:17:37 -0400 Received: from mail-bk0-f46.google.com ([209.85.214.46]:63468 "EHLO mail-bk0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751055Ab2IPTRg (ORCPT ); Sun, 16 Sep 2012 15:17:36 -0400 Received: by bkwj10 with SMTP id j10so2133700bkw.19 for ; Sun, 16 Sep 2012 12:17:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; bh=K9p7jruBWpBfSof0s+znIuH6bzDYIzMGZcna+dnkUns=; b=06qp5nDORwzsTuhlzdMIPPTFyqPkCcyMKTM/JeEpbde7myQQpXoyIm4Sa4bt6g7q+w lelAYnewgwX9iBtfPqFYgUpbxYySqBJ+gUaxqq+V4Zj1BrX9xwpJUA4UzEblNtm/s5Lw r1m199W0OMOaJAkcKhf+vCX2dC0EZnKJD80aKNv6/FZB9CJ1DnDRG77PhM2A4d4CTAfH 4zx2cqMoBcqyl/yqqljFEOuUurcJFs2K73nrHcA//xh6SsYn80ZGf8cC3dDm3w6jBrYi TzqkvRLFplSVS5zSXmrP2hcksOhkq/VjIjPtctmDMM4iqDcMWJjvS97LvmJA1FqWZYoi yk3w== Received: by 10.204.154.202 with SMTP id p10mr3708138bkw.105.1347823055056; Sun, 16 Sep 2012 12:17:35 -0700 (PDT) Received: from [172.28.91.118] ([172.28.91.118]) by mx.google.com with ESMTPS id s26sm4028386bks.13.2012.09.16.12.17.28 (version=SSLv3 cipher=OTHER); Sun, 16 Sep 2012 12:17:33 -0700 (PDT) Subject: Re: [PATCH net-next 1/2] net: provide a default dev->ethtool_ops From: Eric Dumazet To: Ben Hutchings Cc: David Miller , netdev , Maciej =?UTF-8?Q?=C5=BBenczykowski?= In-Reply-To: <1347716447.13258.101.camel@deadeye.wl.decadent.org.uk> References: <1347607441.8555.265.camel@edumazet-glaptop> <1347716447.13258.101.camel@deadeye.wl.decadent.org.uk> Date: Sun, 16 Sep 2012 21:17:26 +0200 Message-ID: <1347823046.26523.44.camel@edumazet-glaptop> Mime-Version: 1.0 X-Mailer: Evolution 2.28.3 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Eric Dumazet On Sat, 2012-09-15 at 14:40 +0100, Ben Hutchings wrote: > > strcpy(dev->name, name); > > dev->group = INIT_NETDEV_GROUP; > > + if (!dev->ethtool_ops) { > > + static const struct ethtool_ops default_ethtool_ops; > > + > > + dev->ethtool_ops = &default_ethtool_ops; > > + } > > This block has a blank line in it, so I think it needs a blank line > either side to make the visual grouping of code right. Alternately you > could pull the variable out of the block. > Blank line is preferred after a variable declaration in a function. I dont feel the need to make this variable visible outside of this function yet. But if you feel it, no problem. > [...] > > @@ -1410,8 +1409,9 @@ static int ethtool_get_module_eeprom(struct net_device *dev, > > modinfo.eeprom_len); > > } > > > > -/* The main entry point in this file. Called from net/core/dev.c */ > > - > > +/* The main entry point in this file. Called from net/core/dev.c > > + * with RTNL held. > > + */ > > Good point but an unrelated change. Its related, because I wanted to make clear (at least for me) why assuming dev->ethtool_ops was not NULL at this point was valid. > > > int dev_ethtool(struct net *net, struct ifreq *ifr) > > { > > struct net_device *dev = __dev_get_by_name(net, ifr->ifr_name); > > @@ -1419,25 +1419,15 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) > > u32 ethcmd; > > int rc; > > u32 old_features; > > + const struct ethtool_ops *ops; > > > > if (!dev || !netif_device_present(dev)) > > return -ENODEV; > > > > + ops = dev->ethtool_ops; > [...] > > Introducing this local variable is a useful cleanup but again should be > a separate change. Its a patch meant for net-next, and a cleanup. I could understand your point if we had to backport this to stable trees, buts its not the case ? I really dont care, so if you really prefer I dont cleanup ethtool.c, so be it. Some functions test dev->ethtool_ops is NULL, others lack this test. This just makes no sense to me, maybe there is something I missed. Thanks [PATCH v2 net-next] net: provide a default dev->ethtool_ops Instead of forcing device drivers to provide empty ethtool_ops or tweak net/core/ethtool.c again, we could provide a generic ethtool_ops. This occurred to me when I wanted to add GSO support to GRE tunnels. ethtool -k support should be generic for all drivers. Signed-off-by: Eric Dumazet Cc: Ben Hutchings Cc: Maciej Żenczykowski Reviewed-by: Ben Hutchings --- net/core/dev.c | 4 ++++ net/core/ethtool.c | 12 ------------ 2 files changed, 4 insertions(+), 12 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/net/core/dev.c b/net/core/dev.c index dcc673d..2bcb02c 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5959,6 +5959,8 @@ struct netdev_queue *dev_ingress_queue_create(struct net_device *dev) return queue; } +static const struct ethtool_ops default_ethtool_ops; + /** * alloc_netdev_mqs - allocate network device * @sizeof_priv: size of private data to allocate space for @@ -6046,6 +6048,8 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name, strcpy(dev->name, name); dev->group = INIT_NETDEV_GROUP; + if (!dev->ethtool_ops) + dev->ethtool_ops = &default_ethtool_ops; return dev; free_all: diff --git a/net/core/ethtool.c b/net/core/ethtool.c index cbf033d..4d64cc2 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1426,18 +1426,6 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) if (copy_from_user(ðcmd, useraddr, sizeof(ethcmd))) return -EFAULT; - if (!dev->ethtool_ops) { - /* A few commands do not require any driver support, - * are unprivileged, and do not change anything, so we - * can take a shortcut to them. */ - if (ethcmd == ETHTOOL_GDRVINFO) - return ethtool_get_drvinfo(dev, useraddr); - else if (ethcmd == ETHTOOL_GET_TS_INFO) - return ethtool_get_ts_info(dev, useraddr); - else - return -EOPNOTSUPP; - } - /* Allow some commands to be done by anyone */ switch (ethcmd) { case ETHTOOL_GSET: