On Sat, 9 Nov 2024 01:43:30 +0000
Vadim Fedorenko [off-list ref] wrote:

quoted

+	ret = net_hwtstamp_validate(&hwtst_config);
+	if (ret)
+		goto err_clock_put;
+
+	if (mod) {
+		struct kernel_hwtstamp_config zero_config = {0};
+		struct hwtstamp_provider *__hwtstamp;
+
+		/* Disable current time stamping if we try to enable
+		 * another one
+		 */
+		ret = dev_set_hwtstamp_phylib(dev, &zero_config,
info->extack);  

		
_hwtst_config is still inited to 0 here, maybe it can be used to avoid
another stack allocation?

You are right, thanks!

quoted

+		if (ret < 0)
+			goto err_clock_put;
+
+		/* Change the selected hwtstamp source */
+		__hwtstamp = rcu_replace_pointer_rtnl(dev->hwtstamp,
hwtstamp);
+		if (__hwtstamp)
+			call_rcu(&__hwtstamp->rcu_head,
+				 remove_hwtstamp_provider);
+	} else {
+		/* Get current hwtstamp config if we are not changing the
+		 * hwtstamp source
+		 */
+		ret = dev_get_hwtstamp_phylib(dev, &_hwtst_config);  

This may be tricky whithout ifr set properly. But it should force
drivers to be converted.

It is the point, it even return not supported error if ndo_hwtstamp_set/get are
not defined.
 

quoted

+		if (ret < 0 && ret != -EOPNOTSUPP)
+			goto err_clock_put;
+	}
+
+	if (memcmp(&hwtst_config, &_hwtst_config, sizeof(hwtst_config))) {
 

better to use kernel_hwtstamp_config_changed() helper here

Oh yes, thanks for pointing it out.

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