Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 10 Oct 2019 20:17:43 +0200
From:   Michal Kubecek <mkubecek@suse.cz>
To:     netdev@vger.kernel.org
Cc:     Jiri Pirko <jiri@resnulli.us>, David Miller <davem@davemloft.net>,
        Jakub Kicinski <jakub.kicinski@netronome.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        John Linville <linville@tuxdriver.com>,
        Stephen Hemminger <stephen@networkplumber.org>,
        Johannes Berg <johannes@sipsolutions.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v7 13/17] ethtool: add standard notification
 handler
Message-ID: <20191010181743.GF22163@unicorn.suse.cz>
References: <cover.1570654310.git.mkubecek@suse.cz>
 <ac2fef494116db9d4679f4501f1be6a7898ef724.1570654310.git.mkubecek@suse.cz>
 <20191010152559.GA2994@nanopsycho>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191010152559.GA2994@nanopsycho>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Oct 10, 2019 at 05:25:59PM +0200, Jiri Pirko wrote:
> Wed, Oct 09, 2019 at 10:59:40PM CEST, mkubecek@suse.cz wrote:
> >+static void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd)
> >+{
> >+	return genlmsg_put(skb, 0, ++ethnl_bcast_seq, &ethtool_genl_family, 0,
> >+			   cmd);
> >+}
> >+
> >+static int ethnl_multicast(struct sk_buff *skb, struct net_device *dev)
> >+{
> >+	return genlmsg_multicast_netns(&ethtool_genl_family, dev_net(dev), skb,
> >+				       0, ETHNL_MCGRP_MONITOR, GFP_KERNEL);
> >+}
> 
> No need for these 2 helpers. Just put the code directly into
> ethnl_std_notify() and make the code easier to read.

In later patches (not submitted yet), these two will be also called by
other notification handlers.

> >+static const struct get_request_ops *ethnl_std_notify_to_ops(unsigned int cmd)
> >+{
> >+	WARN_ONCE(1, "unexpected notification type %u\n", cmd);
> >+	return NULL;
> >+}
> 
> Why this isn't a table similar to get_requests ?

It's a relic of earlier version before splitting the complex message
types when the table was rather sparse. I'll change it to a lookup table
to make it consistent with the rest of the code.

> >+
> >+/* generic notification handler */
> >+static void ethnl_std_notify(struct net_device *dev, unsigned int cmd,
> 
> Better "common" comparing to "standard", I believe.

That's similar to ethnl_std_parse(), the idea is that this is the
standard handler for notifications which are triggered without
additional data and the message is the same as reply to corresponding
"GET" request (which is generated by the standard ethnl_get_doit()
handler). Notifications for actions and notifications for SET commands
which cannot be generated this standard way will have to use their own
(nonstandard) handler.

Michal