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. > + 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? > + return 1; 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.