netdev - Re: [PATCH, net-next, v5, 2/2] net/ethtool: Introduce link_ksettings API for virtual network devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200221181622.GD5607@unicorn.suse.cz>
Date:   Fri, 21 Feb 2020 19:16:22 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Cris Forno <cforno12@...ux.vnet.ibm.com>, mst@...hat.com,
        jasowang@...hat.com, haiyangz@...rosoft.com,
        sthemmin@...rosoft.com, sashal@...nel.org, tlfalcon@...ux.ibm.com,
        davem@...emloft.net, willemdebruijn.kernel@...il.com,
        kuba@...nel.org
Subject: Re: [PATCH, net-next, v5, 2/2] net/ethtool: Introduce link_ksettings
 API for virtual network devices

On Tue, Feb 18, 2020 at 11:52:27AM -0600, Cris Forno wrote:
> With get/set link settings functions in core/ethtool.c, ibmveth,
> netvsc, and virtio now use the core's helper function.
> 
> Funtionality changes that pertain to ibmveth driver include:
> 
>   1. Changed the initial hardcoded link speed to 1GB.
> 
>   2. Added support for allowing a user to change the reported link
>   speed via ethtool.
> 
> Changes to the netvsc driver include:
> 
>   1. When netvsc_get_link_ksettings is called, it will defer to the VF
>   device if it exists to pull accelerated networking values, otherwise
>   pull default or user-defined values.
> 
>   2. Similarly, if netvsc_set_link_ksettings called and a VF device
>   exists, the real values of speed and duplex are changed.
> 
> Signed-off-by: Cris Forno <cforno12@...ux.vnet.ibm.com>
> ---
[...]
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 65e12cb..f733ec5 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
[...]
> @@ -1187,18 +1193,19 @@ static int netvsc_set_link_ksettings(struct net_device *dev,
>  				     const struct ethtool_link_ksettings *cmd)
>  {
>  	struct net_device_context *ndc = netdev_priv(dev);
> -	u32 speed;
> +	struct net_device *vf_netdev = rtnl_dereference(ndc->vf_netdev);
>  
> -	speed = cmd->base.speed;
> -	if (!ethtool_validate_speed(speed) ||
> -	    !ethtool_validate_duplex(cmd->base.duplex) ||
> -	    !netvsc_validate_ethtool_ss_cmd(cmd))
> -		return -EINVAL;
> +	if (vf_netdev) {
> +		if (!vf_netdev->ethtool_ops->set_link_ksettings)
> +			return -EOPNOTSUPP;
>  
> -	ndc->speed = speed;
> -	ndc->duplex = cmd->base.duplex;
> +		return vf_netdev->ethtool_ops->set_link_ksettings(vf_netdev,
> +								  cmd);
> +	}
>  
> -	return 0;
> +	return ethtool_virtdev_set_link_ksettings(dev, cmd,
> +						  &ndc->speed, &ndc->duplex,
> +						  &netvsc_validate_ethtool_ss_cmd);
>  }

I may be missing something obvious but I cannot see how does
netvsc_validate_ethtool_ss_cmd() differ from ethtool_virtdev_validate_cmd().
If it does, it would be probably worth a comment at the function.

Michal Kubecek