On Mon, May 04, 2020 at 12:10:29PM +0200, Oleksij Rempel wrote:
> > > 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.
> > > 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!
>
> >
> > Do you plan to allow handling master/slave also via ioctl()?
>
> no.
>
> > 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
