Re: [PATCH net-next v9 12/13] net: ethtool: tsinfo: Add support for hwtstamp provider and get/set hwtstamp config

["Köry Maincent" <kory.maincent@bootlin.com>]

On Mon, 4 Mar 2024 19:27:33 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Mon, 26 Feb 2024 14:40:03 +0100 Kory Maincent wrote:
> > diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> > index b3f45c307301..37071929128a 100644
> > --- a/net/ethtool/common.c
> > +++ b/net/ethtool/common.c
> > @@ -426,6 +426,7 @@ const char sof_timestamping_names[][ETH_GSTRING_LEN] = {
> >  	[const_ilog2(SOF_TIMESTAMPING_OPT_TX_SWHW)]  = "option-tx-swhw",
> >  	[const_ilog2(SOF_TIMESTAMPING_BIND_PHC)]     = "bind-phc",
> >  	[const_ilog2(SOF_TIMESTAMPING_OPT_ID_TCP)]   = "option-id-tcp",
> > +	[const_ilog2(SOF_TIMESTAMPING_GHWTSTAMP)]    = "get-hwtstamp",  
> 
> What is this new SOF_TIMESTAMPING_GHWTSTAMP? If there's 
> a good reason for it to exist it should be documented in
> Documentation/networking/timestamping.rst

/o\ Sorry I totally forgot about documentation here!

> > +const struct nla_policy ethnl_tsinfo_get_policy[ETHTOOL_A_TSINFO_MAX + 1]
> > = { [ETHTOOL_A_TSINFO_HEADER]		=
> >  		NLA_POLICY_NESTED(ethnl_header_policy),
> > +	[ETHTOOL_A_TSINFO_TIMESTAMPING] = { .type = NLA_NESTED },
> > +	[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST] = { .type = NLA_NESTED
> > },  
> 
> link the policy by NLA_POLICY_NESTED() so that user space can inspect
> the sub-layers via the control family.

Ok thanks!

> > +
> > +	if (!hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX] ||
> > +	    !hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_QUALIFIER])
> > +		return -EINVAL;  
> 
> NL_REQ_ATTR_CHECK()

ok.

> 
> > +	ret =
> > nla_get_u32(hwtst_tb[ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX]);
> > +	if (ret < 0)
> > +		return -EINVAL;  
> 
> How's the get_u32 going to return a negative value?
> That's the purpose of this check?
> The policy should contain the max expected value - NLA_POLICY_MAX().

Right I will use more NLA_POLICY_* to check the values in next version.

> >  		return ret;
> > -	ret = __ethtool_get_ts_info(dev, &data->ts_info);
> > +
> > +	if (!netif_device_present(dev)) {  
> 
>  ethnl_ops_begin() checks for presence

Ok thanks!

> 
> > +	if (req->hwtst.index != -1) {
> > +		struct hwtstamp_provider hwtstamp;
> > +
> > +		hwtstamp.ptp = ptp_clock_get_by_index(req->hwtst.index);
> > +		if (!hwtstamp.ptp) {
> > +			ret = -ENODEV;
> > +			goto out;
> > +		}
> > +		hwtstamp.qualifier = req->hwtst.qualifier;
> > +
> > +		ret = ethtool_get_ts_info_by_phc(dev, &data->ts_info,
> > +						 &hwtstamp);
> > +	} else {
> > +		ret = __ethtool_get_ts_info(dev, &data->ts_info);  
> 
> Not sure I grok why we need 3 forms of getting the tstamp config.
> 
> Please make sure to always update
> Documentation/networking/ethtool-netlink.rst
> when extending ethtool-nl.

Yes sorry I forgot!
The three cases are:
- get hwtstamp config like ioctl SIOCGHWTSTAMP
- get tsinfo of the current hwtstamp
- get tsinfo of a specific hwtstamp

> > +	if (ts_info->phc_index >= 0) {
> > +		/* _TSINFO_HWTSTAMP_PROVIDER_NEST */
> > +		len += nla_total_size(sizeof(u32) * 2);  
> 
> That translates to two raw u32s into a single attribute.
> Is that what you mean?

Oh right that's not what I want. Thanks you!
This is better:
len += 2 * nla_total_size(sizeof(u32));

> > +	if (ts_info->phc_index >= 0) {
> > +		ret = nla_put_u32(skb, ETHTOOL_A_TSINFO_PHC_INDEX,
> > +				  ts_info->phc_index);
> > +		if (ret)
> > +			return -EMSGSIZE;
> > +
> > +		nest = nla_nest_start(skb,
> > ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_NEST);
> > +		if (!nest)
> > +			return -EMSGSIZE;
> > +
> > +		ret = nla_put_u32(skb,
> > +				  ETHTOOL_A_TSINFO_HWTSTAMP_PROVIDER_INDEX,
> > +				  ts_info->phc_index);  
> 
> You can assume nla_put_u32 only returns EMSGSIZE, so doing:
> 
> if (nla_put_u32(....) ||
>     nla_put_u32(....))
> 	return -EMSGSIZE;
> 
> is generally considered to be fine.

Ok.

> > +
> > +		/* Does the hwtstamp supported in the netdev topology */
> > +		if (mod) {
> > +			hwtstamp.ptp = ptp_clock_get_by_index(phc_index);  
> 
> This just returns a pointer without any refcounting, right?
> What guarantees the ptp object doesn't disappear?

Could the ptp object disappears within rtnlock?
Maybe I should add refcounting.

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