On Mon, 4 Mar 2024 19:27:33 -0800
Jakub Kicinski <kuba@xxxxxxxxxx> 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
