Hello Jakub, On Mon, 15 Jul 2024 07:59:26 -0700 Jakub Kicinski <kuba@xxxxxxxxxx> wrote: Thanks for the review and sorry for the late reply. > On Tue, 09 Jul 2024 15:53:45 +0200 Kory Maincent wrote: > > + /* Get the hwtstamp config from netlink */ > > + if (tb[ETHTOOL_A_TSCONFIG_TX_TYPES]) { > > + ret = ethnl_parse_bitset(&req_tx_type, &mask, > > + __HWTSTAMP_TX_CNT, > > + tb[ETHTOOL_A_TSCONFIG_TX_TYPES], > > + ts_tx_type_names, info->extack); > > + if (ret < 0) > > + goto err_clock_put; > > + > > + /* Select only one tx type at a time */ > > + if (ffs(req_tx_type) != fls(req_tx_type)) { > > + ret = -EINVAL; > > + goto err_clock_put; > > + } > > + > > + hwtst_config.tx_type = ffs(req_tx_type) - 1; > > + } > > + if (tb[ETHTOOL_A_TSCONFIG_RX_FILTERS]) { > > + ret = ethnl_parse_bitset(&req_rx_filter, &mask, > > + __HWTSTAMP_FILTER_CNT, > > + tb[ETHTOOL_A_TSCONFIG_RX_FILTERS], > > + ts_rx_filter_names, info->extack); > > + if (ret < 0) > > + goto err_clock_put; > > + > > + /* Select only one rx filter at a time */ > > + if (ffs(req_rx_filter) != fls(req_rx_filter)) { > > + ret = -EINVAL; > > + goto err_clock_put; > > + } > > + > > + hwtst_config.rx_filter = ffs(req_rx_filter) - 1; > > + } > > + if (tb[ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS]) { > > + ret = nla_get_u32(tb[ETHTOOL_A_TSCONFIG_HWTSTAMP_FLAGS]); > > + if (ret < 0) > > + goto err_clock_put; > > + hwtst_config.flags = ret; > > + } > > We should be tracking mod on these, too. Separately from the provider > mod bit, let's not call the driver and send notification if nothing > changed. Ok > > + ret = net_hwtstamp_validate(&hwtst_config); > > + if (ret) > > + goto err_clock_put; > > + > > + /* Disable current time stamping if we try to enable another one */ > > + if (mod && (hwtst_config.tx_type || hwtst_config.rx_filter)) { > > + struct kernel_hwtstamp_config zero_config = {0}; > > + > > + ret = dev_set_hwtstamp_phylib(dev, &zero_config, > > info->extack); > > + if (ret < 0) > > + goto err_clock_put; > > + } > > + > > + /* Changed the selected hwtstamp source if needed */ > > + if (mod) { > > + struct hwtstamp_provider *__hwtstamp; > > + > > + __hwtstamp = rcu_replace_pointer_rtnl(dev->hwtstamp, > > hwtstamp); > > + if (__hwtstamp) > > + call_rcu(&__hwtstamp->rcu_head, > > + remove_hwtstamp_provider); > > + } > > + > > + ret = dev_set_hwtstamp_phylib(dev, &hwtst_config, info->extack); > > + if (ret < 0) > > + return ret; > > We can't unwind to old state here? Yes indeed we could unwind old state here. I will update it in next version. > Driver can change hwtst_config right? "upgrade" the rx_filter > to a broader one, IIRC. Shouldn't we reply to the set command with > the resulting configuration, in case it changed? Basically provide > the same info as the notification would. Yes, the driver does that. Indeed that's a good idea to report the resulting configuration. I will take a look at how I can do that. Regards, -- Köry Maincent, Bootlin Embedded Linux and kernel engineering https://bootlin.com