Re: [RFC next-next v2 3/5] net: marvell: prestera: Add ethtool interface support

[Jiri Pirko <jiri@resnulli.us>]

Fri, May 01, 2020 at 01:20:50AM CEST, vadym.kochan@plvision.eu wrote:
>The ethtool API provides support for the configuration of the following
>features: speed and duplex, auto-negotiation, MDI-x, forward error
>correction, port media type. The API also provides information about the
>port status, hardware and software statistic.  The following limitation

No double space.


>exists:
>
>    - port media type should be configured before speed setting
>    - ethtool –m option is not supported
>    - ethtool –p option is not supported

Those are some odd dashes...


>    - ethtool -r option is supported for RJ45 port only
>    - the following combination of parameters is not supported:
>
>          ethtool -s sw1pX port XX autoneg on
>
>    - forward error correction feature is supported only on SFP ports, 10G
>      speed
>
>    - auto-negotiation and MDI-x features are not supported on
>      Copper-to-Fiber SFP module
>

[...]


>+static const struct ethtool_ops ethtool_ops = {
>+	.get_drvinfo = prestera_port_get_drvinfo,
>+	.get_link_ksettings = prestera_port_get_link_ksettings,
>+	.set_link_ksettings = prestera_port_set_link_ksettings,
>+	.get_fecparam = prestera_port_get_fecparam,
>+	.set_fecparam = prestera_port_set_fecparam,
>+	.get_sset_count = prestera_port_get_sset_count,
>+	.get_strings = prestera_port_get_strings,
>+	.get_ethtool_stats = prestera_port_get_ethtool_stats,
>+	.get_link = ethtool_op_get_link,
>+	.nway_reset = prestera_port_nway_reset
>+};

I wonder, wouldn't it be better to put the ethtool bits into a separate
.c file. You have a separate .c file for less :)


>+
> static int prestera_port_create(struct prestera_switch *sw, u32 id)
> {
> 	struct prestera_port *port;
>@@ -264,6 +1023,7 @@ static int prestera_port_create(struct prestera_switch *sw, u32 id)
> 
> 	dev->features |= NETIF_F_NETNS_LOCAL;
> 	dev->netdev_ops = &netdev_ops;
>+	dev->ethtool_ops = &ethtool_ops;
> 
> 	netif_carrier_off(dev);
> 

[...]