Re: [PATCH v3 net-next 13/19] ionic: Add initial ethtool support

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote:
> On 7/8/19 7:14 PM, Andrew Lunn wrote:
> >>+static int ionic_set_pauseparam(struct net_device *netdev,
> >>+				struct ethtool_pauseparam *pause)
> >>+{
> >>+	struct lif *lif = netdev_priv(netdev);
> >>+	struct ionic *ionic = lif->ionic;
> >>+	struct ionic_dev *idev = &lif->ionic->idev;
> >>+
> >>+	u32 requested_pause;
> >>+	u32 cur_autoneg;
> >>+	int err;
> >>+
> >>+	cur_autoneg = idev->port_info->config.an_enable ? AUTONEG_ENABLE :
> >>+								AUTONEG_DISABLE;
> >>+	if (pause->autoneg != cur_autoneg) {
> >>+		netdev_info(netdev, "Please use 'ethtool -s ...' to change autoneg\n");
> >>+		return -EOPNOTSUPP;
> >>+	}
> >>+
> >>+	/* change both at the same time */
> >>+	requested_pause = PORT_PAUSE_TYPE_LINK;
> >>+	if (pause->rx_pause)
> >>+		requested_pause |= IONIC_PAUSE_F_RX;
> >>+	if (pause->tx_pause)
> >>+		requested_pause |= IONIC_PAUSE_F_TX;
> >>+
> >>+	if (requested_pause == idev->port_info->config.pause_type)
> >>+		return 0;
> >>+
> >>+	idev->port_info->config.pause_type = requested_pause;
> >>+
> >>+	mutex_lock(&ionic->dev_cmd_lock);
> >>+	ionic_dev_cmd_port_pause(idev, requested_pause);
> >>+	err = ionic_dev_cmd_wait(ionic, devcmd_timeout);
> >>+	mutex_unlock(&ionic->dev_cmd_lock);
> >>+	if (err)
> >>+		return err;
> >Hi Shannon
> >
> >I've no idea what the firmware black box is doing, but this looks
> >wrong.
> >
> >pause->autoneg is about if the results of auto-neg should be used or
> >not. If false, just configure the MAC with the pause settings and you
> >are done. If the interface is being forced, so autoneg in general is
> >disabled, just configure the MAC and you are done.
> >
> >If pause->autoneg is true and the interface is using auto-neg as a
> >whole, you pass the pause values to the PHY for it to advertise and
> >trigger an auto-neg. Once autoneg has completed, and the resolved
> >settings are available, the MAC is configured with the resolved
> >values.
> >
> >Looking at this code, i don't see any difference between configuring
> >the MAC or configuring the PHY. I would expect pause->autoneg to be
> >part of requested_pause somehow, so the firmware knows what is should
> >do.
> >
> >	Andrew
> 
> In this device there's actually very little the driver can do to directly
> configure the mac or phy besides passing through to the firmware what the
> user has requested - that happens here for the pause values, and in
> ionic_set_link_ksettings() for autoneg.  The firmware is managing the port
> based on these requests with the help of internally configured rules defined
> in a customer setting.

I get that. But the firmware needs to conform to what Linux
expects. And what i see here does not conform. That is why i gave a
bit of detail in my reply.

What exactly does the firmware do? Once we know that, we can figure
out when the driver should return -EOPNOTSUPP because of firmware
limitations, and what it can configure and should return 0.

    Andrew