Re: [PATCH, net-next, v3, 1/2] Three virtual devices (ibmveth, virtio_net, and netvsc) all have similar code to set/get link settings and validate ethtool command. To eliminate duplication of code, it is factored out into core/ethtool.c.

[Cristobal Forno <cforno12@linux.vnet.ibm.com>]

Thanks for your suggestion Micheal and Thomas. This will be included in 
the next version of the patch series.

-Cris Forno

On 20/12/2019 01:04, Michal Kubecek wrote:
> On Thu, Dec 19, 2019 at 07:26:14PM -0600, Thomas Falcon wrote:
>> On 12/19/19 4:36 PM, Michal Kubecek wrote:
>>> On Thu, Dec 19, 2019 at 01:40:56PM -0600, Cris Forno wrote:
> [...]
>>>> @@ -579,6 +579,32 @@ static int load_link_ksettings_from_user(struct ethtool_link_ksettings *to,
>>>>    	return 0;
>>>>    }
>>>> +/* Check if the user is trying to change anything besides speed/duplex */
>>>> +static bool
>>>> +ethtool_virtdev_validate_cmd(const struct ethtool_link_ksettings *cmd)
>>>> +{
>>>> +	struct ethtool_link_ksettings diff1 = *cmd;
>>>> +	struct ethtool_link_ksettings diff2 = {};
>>>> +
>>>> +	/* cmd is always set so we need to clear it, validate the port type
>>>> +	 * and also without autonegotiation we can ignore advertising
>>>> +	 */
>>>> +	diff1.base.speed = 0;
>>>> +	diff2.base.port = PORT_OTHER;
>>>> +	ethtool_link_ksettings_zero_link_mode(&diff1, advertising);
>>>> +	diff1.base.duplex = 0;
>>>> +	diff1.base.cmd = 0;
>>>> +	diff1.base.link_mode_masks_nwords = 0;
>>>> +
>>>> +	return !memcmp(&diff1.base, &diff2.base, sizeof(diff1.base)) &&
>>>> +		bitmap_empty(diff1.link_modes.supported,
>>>> +			     __ETHTOOL_LINK_MODE_MASK_NBITS) &&
>>>> +		bitmap_empty(diff1.link_modes.advertising,
>>>> +			     __ETHTOOL_LINK_MODE_MASK_NBITS) &&
>>> Isn't this condition always true? You zeroed the advertising bitmap
>>> above. Could you just omit this part and clearing of advertising above?
>>>
>>>> +		bitmap_empty(diff1.link_modes.lp_advertising,
>>>> +			     __ETHTOOL_LINK_MODE_MASK_NBITS);
>>>> +}
>>> Another idea: instead of zeroing parts of diff1, you could copy these
>>> members from *cmd to diff2 and compare cmd->base with diff2.base. You
>>> could then drop diff1. And you wouldn't even need whole struct
>>> ethtool_link_ksettings for diff2 as you only compare embedded struct
>>> ethtool_link_settings (and check two bitmaps in cmd->link_modes).
>> If I understand your suggestion correctly, then the validate function might
>> look something like this?
>>
>> /* Check if the user is trying to change anything besides speed/duplex */
>> static bool
>> ethtool_virtdev_validate_cmd(const struct ethtool_link_ksettings *cmd)
>> {
>>      struct ethtool_link_settings base2 = {};
>>
>>      base2.speed = cmd->base.speed;
>>      base2.port = PORT_OTHER;
>>      base2.duplex = cmd->base.duplex;
>>      base2.cmd = cmd->base.cmd;
>>      base2.link_mode_masks_nwords = cmd->base.link_mode_masks_nwords;
>>
>>      return !memcmp(&base2, cmd->base, sizeof(base2)) &&
>>          bitmap_empty(cmd->link_modes.supported,
>>                   __ETHTOOL_LINK_MODE_MASK_NBITS) &&
>>          bitmap_empty(cmd->link_modes.lp_advertising,
>>                   __ETHTOOL_LINK_MODE_MASK_NBITS);
>> }
> Yes, that is what I wanted to suggest (the second argument of memcmp()
> should be "&cmd->base", I think).
>
> Michal