On Mon, May 04, 2020 at 12:10:29PM +0200, Oleksij Rempel wrote:

quoted

quoted

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ac2784192472f..42dda9d2082ee 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c

@@ -1768,6 +1768,90 @@ int genphy_setup_forced(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_setup_forced);
 
+static int genphy_setup_master_slave(struct phy_device *phydev)
+{
+	u16 ctl = 0;
+
+	if (!phydev->is_gigabit_capable)
+		return 0;

Why did you revert to silently ignoring requests in this case?

genphy_setup_forced() is called by __genphy_config_aneg() and this can
be called by a PHY driver after configuring master slave mode locally by
PHY driver. See tja11xx patch. Same can be potentially done in the phy/realtek.c
driver.

Currently my imagination is not caffeanized enough to
provide a better solution. Do you have ideas?

If we have the check in ethnl_update_linkmodes(), we shouldn't really
get here so I believe we can leave this part as it is.

quoted

quoted

 static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
 				  struct ethtool_link_ksettings *ksettings,
 				  bool *mod)
 {
 	struct ethtool_link_settings *lsettings = &ksettings->base;
 	bool req_speed, req_duplex;
+	const struct nlattr *master_slave_cfg;
 	int ret;
 
+	master_slave_cfg = tb[ETHTOOL_A_LINKMODES_MASTER_SLAVE_CFG];
+	if (master_slave_cfg) {
+		u8 cfg = nla_get_u8(master_slave_cfg);
+		if (!ethnl_validate_master_slave_cfg(cfg)) {
+			GENL_SET_ERR_MSG(info, "LINKMODES_MASTER_SLAVE_CFG contains not valid value");
+			return -EOPNOTSUPP;
+		}
+	}

Please set also the "bad attribute" in extack, it may help
non-interactive clients.

Also, it would be nice to report error if client wants to set master/slave but
driver does not support it. How about this?

	if (master_slave_cfg) {
		u8 cfg = nla_get_u8(master_slave_cfg);

		if (lsettings->master_slave_cfg == MASTER_SLAVE_CFG_UNSUPPORTED) {
			NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
					    "master/slave configuration not supported by device");
			return -EOPNOTSUPP;
		}
		if (!ethnl_validate_master_slave_cfg(cfg)) {
			NL_SET_ERR_MSG_ATTR(info->extack, master_slave_cfg,
					    "master/slave value is invalid");
			return -EOPNOTSUPP;
		}
	}

looks good. thx!

quoted

Do you plan to allow handling master/slave also via ioctl()?

no.

quoted

If yes, we should
also add the sanity checks to ioctl code path. If not, we should prevent
passing non-zero values from userspace to driver.

What is the best place to add this sanity check?

If there is no plan to allow handling master/slave via ioctl, the best
option would IMHO be zeroing both fields in ethtool_get_link_ksettings()
right before the call to store_link_ksettings_for_user() and either
zeroing master_slave_cfg in ethtool_set_link_ksettings() after the call
load_link_ksettings_from_user(), or checking that it's zero (i.e. that
userspace left it untouched).

Michal