diff mbox series

[net-next,v2,04/14] ice: fix set pause param autoneg check

Message ID 20190819161708.3763-5-jeffrey.t.kirsher@intel.com
State Changes Requested
Delegated to: David Miller
Headers show
Series 100GbE Intel Wired LAN Driver Updates 2019-08-19 | expand

Commit Message

Kirsher, Jeffrey T Aug. 19, 2019, 4:16 p.m. UTC 
From: Paul Greenwalt <paul.greenwalt@intel.com>

When ETHTOOL_GLINKSETTINGS is defined get pause param pause->autoneg
reports SW configured setting, however when not defined get pause param
pause->autoneg reports the link status. Set pause param needs to compare
pause->autoneg with the same source as get pause param to block the user
from changing autoneg with the set pause param option, or the user
may be incorrectly blocked from changing Rx|Tx pause settings.

Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 28 +++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski Aug. 19, 2019, 11:11 p.m. UTC | #1 
On Mon, 19 Aug 2019 09:16:58 -0700, Jeff Kirsher wrote:
> +	pcaps = devm_kzalloc(&vsi->back->pdev->dev, sizeof(*pcaps),
> +			     GFP_KERNEL);
> +	if (!pcaps)
> +		return -ENOMEM;
> +
> +	/* Get current PHY config */
> +	status = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_SW_CFG, pcaps,
> +				     NULL);
> +	if (status) {
> +		devm_kfree(&vsi->back->pdev->dev, pcaps);
> +		return -EIO;
> +	}
> +
> +	is_an = ((pcaps->caps & ICE_AQC_PHY_AN_MODE) ?
> +			AUTONEG_ENABLE : AUTONEG_DISABLE);
> +
> +	devm_kfree(&vsi->back->pdev->dev, pcaps);

Is it just me or is this use of devm_k*alloc absolutely pointless?
David Miller Aug. 19, 2019, 11:59 p.m. UTC | #2 
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Mon, 19 Aug 2019 16:11:42 -0700

> On Mon, 19 Aug 2019 09:16:58 -0700, Jeff Kirsher wrote:
>> +	pcaps = devm_kzalloc(&vsi->back->pdev->dev, sizeof(*pcaps),
>> +			     GFP_KERNEL);
>> +	if (!pcaps)
>> +		return -ENOMEM;
>> +
>> +	/* Get current PHY config */
>> +	status = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_SW_CFG, pcaps,
>> +				     NULL);
>> +	if (status) {
>> +		devm_kfree(&vsi->back->pdev->dev, pcaps);
>> +		return -EIO;
>> +	}
>> +
>> +	is_an = ((pcaps->caps & ICE_AQC_PHY_AN_MODE) ?
>> +			AUTONEG_ENABLE : AUTONEG_DISABLE);
>> +
>> +	devm_kfree(&vsi->back->pdev->dev, pcaps);
> 
> Is it just me or is this use of devm_k*alloc absolutely pointless?

Yeah it looks like an overzealous use of these interfaces to me too.
David Miller Aug. 20, 2019, 1:31 a.m. UTC | #3 
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Mon, 19 Aug 2019 09:16:58 -0700

> +	/* Get pause param reports configured and negotiated flow control pause
> +	 * when ETHTOOL_GLINKSETTINGS is defined. Since ETHTOOL_GLINKSETTINGS is
> +	 * defined get pause param pause->autoneg reports SW configured setting,
> +	 * so compare pause->autoneg with SW configured to prevent the user from
> +	 * using set pause param to chance autoneg.
> +	 */
> +	pcaps = devm_kzalloc(&vsi->back->pdev->dev, sizeof(*pcaps),
> +			     GFP_KERNEL);

Just in case it isn't clear, please use plain kzalloc/kfree in this code.

Thank you.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index d3ba535bd65a..d02b72e72423 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -2856,6 +2856,7 @@  static int
 ice_set_pauseparam(struct net_device *netdev, struct ethtool_pauseparam *pause)
 {
 	struct ice_netdev_priv *np = netdev_priv(netdev);
+	struct ice_aqc_get_phy_caps_data *pcaps;
 	struct ice_link_status *hw_link_info;
 	struct ice_pf *pf = np->vsi->back;
 	struct ice_dcbx_cfg *dcbx_cfg;
@@ -2866,6 +2867,7 @@  ice_set_pauseparam(struct net_device *netdev, struct ethtool_pauseparam *pause)
 	u8 aq_failures;
 	bool link_up;
 	int err = 0;
+	u32 is_an;
 
 	pi = vsi->port_info;
 	hw_link_info = &pi->phy.link_info;
@@ -2880,7 +2882,31 @@  ice_set_pauseparam(struct net_device *netdev, struct ethtool_pauseparam *pause)
 		return -EOPNOTSUPP;
 	}
 
-	if (pause->autoneg != (hw_link_info->an_info & ICE_AQ_AN_COMPLETED)) {
+	/* Get pause param reports configured and negotiated flow control pause
+	 * when ETHTOOL_GLINKSETTINGS is defined. Since ETHTOOL_GLINKSETTINGS is
+	 * defined get pause param pause->autoneg reports SW configured setting,
+	 * so compare pause->autoneg with SW configured to prevent the user from
+	 * using set pause param to chance autoneg.
+	 */
+	pcaps = devm_kzalloc(&vsi->back->pdev->dev, sizeof(*pcaps),
+			     GFP_KERNEL);
+	if (!pcaps)
+		return -ENOMEM;
+
+	/* Get current PHY config */
+	status = ice_aq_get_phy_caps(pi, false, ICE_AQC_REPORT_SW_CFG, pcaps,
+				     NULL);
+	if (status) {
+		devm_kfree(&vsi->back->pdev->dev, pcaps);
+		return -EIO;
+	}
+
+	is_an = ((pcaps->caps & ICE_AQC_PHY_AN_MODE) ?
+			AUTONEG_ENABLE : AUTONEG_DISABLE);
+
+	devm_kfree(&vsi->back->pdev->dev, pcaps);
+
+	if (pause->autoneg != is_an) {
 		netdev_info(netdev, "To change autoneg please use: ethtool -s <dev> autoneg <on|off>\n");
 		return -EOPNOTSUPP;
 	}