On Fri, Jul 19, 2019 at 11:41:28AM -0700, Shannon Nelson wrote:

On 7/18/19 7:40 PM, Andrew Lunn wrote:

quoted

On Thu, Jul 18, 2019 at 05:12:07PM -0700, Shannon Nelson wrote:

quoted

On 7/17/19 8:28 PM, Andrew Lunn wrote:

quoted

On Fri, Jul 12, 2019 at 10:16:31PM -0700, Shannon Nelson wrote:

quoted

On 7/8/19 7:14 PM, Andrew Lunn wrote:

quoted

quoted

+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.

Because this is fairly smart FW, it handles this as expected.  I can add
this as another comment in the code.

Hi Shannon

Looking at the code, i don't see how it can handle this
correctly. Please look at my comments, particularly the meaning of
pause->autoneg and describe how the firmware does the right thing,
given what information it is passed.

I guess I'm not sure how much better I can answer your question. Like
several other devices, we don't support selecting pause->autoneg.  The
firmware manages the PHY itself, the driver doesn't have direct access to
the PHY.  The driver passes the tx and rx pause info down to the firmware in
a command request.  The NIC firmware does an autoneg if autoneg is enabled
on the port.

Hi Shannon

Thanks. That was the information i was looking for.

Please return -EOPNOTSUPP if pause->autoneg indicates autoneg results
should not be used. Your firmware does not support it, so the driver
should error out. Also the get function should use a hard coded value
for pause->autoneg.

If you ever fix your firmware, you can add full support for pause
configuration.

Thanks
	Andrew