On 30.04.2019 07:06, Heiner Kallweit wrote:

On 29.04.2019 23:52, Andrew Lunn wrote:

quoted

quoted

@@ -2078,6 +2089,11 @@ EXPORT_SYMBOL(phy_set_sym_pause);
 void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(oldadv);
+	bool asym_pause_supported;
+
+	asym_pause_supported =
+		linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				  phydev->supported);
 
 	linkmode_copy(oldadv, phydev->advertising);
 

@@ -2086,14 +2102,14 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx)
 	linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
 			   phydev->advertising);
 
-	if (rx) {
+	if (rx && asym_pause_supported) {
 		linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
 				 phydev->advertising);
 		linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
 				 phydev->advertising);
 	}
 
-	if (tx)
+	if (tx && asym_pause_supported)
 		linkmode_change_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
 				    phydev->advertising);

Hi Heiner

Hi Andrew,

quoted

If the PHY only supports Pause, not Asym Pause, i wounder if we should
fall back to Pause here?

I wasn't sure about whether a silent fallback is the expected behavior.
Also open is whether we can rely on a set_pause callback having called
phy_validate_pause() before. Another option could be to change the
return type to int and return an error like -EOPNOTSUPP if the requested
mode isn't supported.

Most drivers call phy_validate_pause() first, this seems to be the
expected pattern. Therefore I will remove patch 2. However there seems to
be an error in phy_validate_pause(), the fix I will submit separately.

quoted

     Andrew

Heiner