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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190709021426.GA5835@lunn.ch>
Date:   Tue, 9 Jul 2019 04:14:26 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Shannon Nelson <snelson@...sando.io>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 13/19] ionic: Add initial ethtool support

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