Re: [PATCH, net-next, v3, 1/2] ethtool: Factored out similar ethtool link settings for virtual devices to core

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

Thanks for your suggestions Willlem. I have a question on one of your 
suggestions for the ethtool_virtdev_get_link_ksettings function below.

On 22/12/2019 15:19, Willem de Bruijn wrote:
> On Thu, Dec 19, 2019 at 3:54 PM Cris Forno <cforno12@linux.vnet.ibm.com> wrote:
>> 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.
>>
>> Signed-off-by: Cris Forno <cforno12@linux.vnet.ibm.com>
>> ---
>>   include/linux/ethtool.h |  2 ++
>>   net/core/ethtool.c      | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 60 insertions(+)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index 95991e43..1b0417b 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -394,6 +394,8 @@ struct ethtool_ops {
>>                                            struct ethtool_coalesce *);
>>          int     (*set_per_queue_coalesce)(struct net_device *, u32,
>>                                            struct ethtool_coalesce *);
>> +       bool    (*virtdev_validate_link_ksettings)(const struct
>> +                                                  ethtool_link_ksettings *);
>>          int     (*get_link_ksettings)(struct net_device *,
>>                                        struct ethtool_link_ksettings *);
>>          int     (*set_link_ksettings)(struct net_device *,
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index cd9bc67..4091a94 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -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)
> If called from other modules like drivers/net/virtio_net.ko, these
> functions cannot be static, need a declaration in
> include/linux/ethtool.h and an EXPORT_SYMBOL_GPL.
>
> Also return type should be on the same line.
Good point, I will incorporate this into the next version of the series.
>> +{
>> +       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) &&
>> +               bitmap_empty(diff1.link_modes.lp_advertising,
>> +                            __ETHTOOL_LINK_MODE_MASK_NBITS);
>> +}
>> +
>>   /* convert a kernel internal ethtool_link_ksettings to
>>    * ethtool_link_usettings in user space. return 0 on success, errno on
>>    * error.
>> @@ -660,6 +686,17 @@ static int ethtool_get_link_ksettings(struct net_device *dev,
>>          return store_link_ksettings_for_user(useraddr, &link_ksettings);
>>   }
>>
>> +static int
>> +ethtool_virtdev_get_link_ksettings(struct net_device *dev,
>> +                                  struct ethtool_link_ksettings *cmd,
>> +                                  u32 *speed, u8 *duplex)
> No need to pass by reference, really. Indeed, the virtio_net caller
> passes vi->speed and vi->duplex instead of &vi->speed and &vi->duplex.
Agreed.
>
> More fundamentally, these three assignments are simple enough that I
> don't think a helper actually simplifies anything here.
>
Although the function is simple, it does achieve the goal of this 
version of the patch series which is to eliminate duplication of code 
throughout the virtual devices. I think it's best to leave it like this, 
but I am open to more suggestions.

-Cris Forno

>
>> +{
>> +       cmd->base.speed = *speed;
>> +       cmd->base.duplex = *duplex;
>> +       cmd->base.port = PORT_OTHER;
>> +       return 0;
>> +}
>> +
>>   /* Update device ethtool_link_settings. */
>>   static int ethtool_set_link_ksettings(struct net_device *dev,
>>                                        void __user *useraddr)
>> @@ -696,6 +733,27 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
>>          return dev->ethtool_ops->set_link_ksettings(dev, &link_ksettings);
>>   }
>>
>> +static int
>> +ethtool_virtdev_set_link_ksettings(struct net_device *dev,
>> +                                  const struct ethtool_link_ksettings *cmd,
>> +                                  u32 *dev_speed, u8 *dev_duplex)
>> +{
>> +       u32 speed;
>> +       u8 duplex;
>> +
>> +       speed = cmd->base.speed;
>> +       duplex = cmd->base.duplex;
>> +       /* don't allow custom speed and duplex */
>> +       if (!ethtool_validate_speed(speed) ||
>> +           !ethtool_validate_duplex(duplex) ||
>> +           !dev->ethtool_ops->virtdev_validate_link_ksettings(cmd))
>> +               return -EINVAL;
>> +       *dev_speed = speed;
>> +       *dev_duplex = duplex;
>> +
>> +       return 0;
>> +}
>> +
>>   /* Query device for its ethtool_cmd settings.
>>    *
>>    * Backward compatibility note: for compatibility with legacy ethtool, this is
>> --
>> 1.8.3.1
>>