Date: Fri, 21 Jun 2024 10:54:08 +0200
From: Kory Maincent <kory.maincent@...tlin.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>, Broadcom internal
 kernel review list <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn
 <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Richard
 Cochran <richardcochran@...il.com>, Radu Pirea
 <radu-nicolae.pirea@....nxp.com>, Jay Vosburgh <j.vosburgh@...il.com>, Andy
 Gospodarek <andy@...yhouse.net>, Nicolas Ferre
 <nicolas.ferre@...rochip.com>, Claudiu Beznea <claudiu.beznea@...on.dev>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, Jonathan Corbet
 <corbet@....net>, Horatiu Vultur <horatiu.vultur@...rochip.com>,
 UNGLinuxDriver@...rochip.com, Simon Horman <horms@...nel.org>, Vladimir
 Oltean <vladimir.oltean@....com>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, Maxime Chevallier
 <maxime.chevallier@...tlin.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>
Subject: Re: [PATCH net-next v15 13/14] net: ethtool: tsinfo: Add support
 for hwtstamp provider and get/set hwtstamp config

On Mon, 17 Jun 2024 18:43:31 -0700
Jakub Kicinski <kuba@...nel.org> wrote:

Thanks for your review!

> On Wed, 12 Jun 2024 17:04:13 +0200 Kory Maincent wrote:
> > Enhance 'get' command to retrieve tsinfo of hwtstamp providers within a
> > network topology and read current hwtstamp configuration.
> > 
> > Introduce support for ETHTOOL_MSG_TSINFO_SET ethtool netlink socket to
> > configure hwtstamp of a PHC provider. Note that simultaneous hwtstamp
> > isn't supported; configuring a new one disables the previous setting.
> > 
> > Also, add support for a specific dump command to retrieve all hwtstamp
> > providers within the network topology, with added functionality for
> > filtered dump to target a single interface.  
> 
> Could you split this up, a little bit? It's rather large for a core
> change.

Ok I will do so.
 
> >  Desired behavior is passed into the kernel and to a specific device by
> > -calling ioctl(SIOCSHWTSTAMP) with a pointer to a struct ifreq whose
> > -ifr_data points to a struct hwtstamp_config. The tx_type and
> > -rx_filter are hints to the driver what it is expected to do. If
> > -the requested fine-grained filtering for incoming packets is not
> > +calling the tsinfo netlink socket ETHTOOL_MSG_TSINFO_SET.
> > +The ETHTOOL_A_TSINFO_TX_TYPES, ETHTOOL_A_TSINFO_RX_FILTERS and
> > +ETHTOOL_A_TSINFO_HWTSTAMP_FLAGS netlink attributes are then used to set the
> > +struct hwtstamp_config accordingly.  
> 
> nit: EHTOOL_A* defines in `` `` quotes?

Ack.

> 
> > +		if (hwtstamp && ptp_clock_phydev(hwtstamp->ptp) == phydev)
> > {
> > +			rcu_assign_pointer(dev->hwtstamp, NULL);
> > +			synchronize_rcu();
> >  			kfree(hwtstamp);  
> 
> Could you add an rcu_head to this struct and use kfree_rcu()
> similarly later use an rcu call to do the dismantle?
> synchronize_rcu() can be slow.

Ack. I might need to use call_rcu() as I have to call ptp_clock_put also before
the kfree.
 
> > +const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1]
> > = { [ETHTOOL_A_TSINFO_HEADER]		=
> >  		NLA_POLICY_NESTED(ethnl_header_policy_stats),
> > +	[ETHTOOL_A_TSINFO_GHWTSTAMP] =
> > +		NLA_POLICY_MAX(NLA_U8, 1),  
> 
> I think this can be an NLA_FLAG, but TBH I'm also confused about 
> the semantics. Can you explain what it does from user perspective?

As I described it in the documentation it replaces SIOCGHWTSTAMP:
"Any process can read the actual configuration by requesting tsinfo netlink
socket ETHTOOL_MSG_TSINFO_GET with ETHTOOL_MSG_TSINFO_GHWTSTAMP netlink
attribute set.

The legacy usage is to pass this structure to ioctl(SIOCGHWTSTAMP) in the       
same way as the ioctl(SIOCSHWTSTAMP).  However, this has not been implemented   
in all drivers."

The aim is to get rid of ioctls.

Indeed NLA_FLAG is the right type I should use.
 
> > +	[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER] =
> > +		NLA_POLICY_NESTED(ethnl_tsinfo_hwtstamp_provider_policy),
> >  };
> >  
> > +static int tsinfo_parse_hwtstamp_provider(const struct nlattr *nest,
> > +					  struct hwtst_provider *hwtst,
> > +					  struct netlink_ext_ack *extack,
> > +					  bool *mod)
> > +{
> > +	struct nlattr
> > *tb[ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy)];  
> 
> Could you find a more sensible name for this policy?

I am not a naming expert but "hwtstamp_provider" is the struct name I have used
to describe hwtstamp index + qualifier and the prefix of the netlink nested
attribute, so IMHO it fits well.
Have you another proposition to clarify what you would expect?
 
> > +	int ret;
> > +
> > +	ret = nla_parse_nested(tb,
> > +
> > ARRAY_SIZE(ethnl_tsinfo_hwtstamp_provider_policy) - 1,
> > +			       nest,
> > +			       ethnl_tsinfo_hwtstamp_provider_policy,
> > extack);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (NL_REQ_ATTR_CHECK(extack, nest, tb,
> > ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX) ||
> > +	    NL_REQ_ATTR_CHECK(extack, nest, tb,
> > ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER))  
> 
> nit: wrap at 80 chars, if you can, please

Yes indeed, thanks!

> >  	struct tsinfo_reply_data *data = TSINFO_REPDATA(reply_base);
> > +	struct tsinfo_req_info *req = TSINFO_REQINFO(req_base);
> >  	struct net_device *dev = reply_base->dev;
> >  	int ret;
> >  
> >  	ret = ethnl_ops_begin(dev);
> >  	if (ret < 0)
> >  		return ret;
> > +
> > +	if (req->get_hwtstamp) {
> > +		struct kernel_hwtstamp_config cfg = {};
> > +
> > +		if (!dev->netdev_ops->ndo_hwtstamp_get) {
> > +			ret = -EOPNOTSUPP;
> > +			goto out;
> > +		}
> > +
> > +		ret = dev_get_hwtstamp_phylib(dev, &cfg);
> > +		data->hwtst_config.tx_type = BIT(cfg.tx_type);
> > +		data->hwtst_config.rx_filter = BIT(cfg.rx_filter);
> > +		data->hwtst_config.flags = BIT(cfg.flags);
> > +		goto out;  
> 
> This is wrong AFAICT, everything up to this point was a nit pick ;)
> Please take a look at 89e281ebff72e6, I think you're reintroducing a
> form of the same bug. If ETHTOOL_FLAG_STATS was set, you gotta run stats
> init.
> 
> Perhaps you can move the stats getting up, and turn this code into if
> / else if / else, without the goto.

Indeed thanks for spotting the issue.

> > +
> > +	if (ret == -EMSGSIZE && skb->len)
> > +		return skb->len;
> > +	return ret;  
> 
> You can just return ret without the if converting to skb->len
> af_netlink will handle the EMSGSIZE errors in the same way.

Alright, thanks.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Powered by blists - more mailing lists