Message-ID: <7cd0e092-0896-4dc5-66a9-7213e92b3060@gmail.com>
Date:   Wed, 13 May 2020 14:39:21 -0700
From:   Doug Berger <opendmb@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] net: ethernet: introduce phy_set_pause

On 5/13/2020 2:42 AM, Russell King - ARM Linux admin wrote:
> On Mon, May 11, 2020 at 05:24:09PM -0700, Doug Berger wrote:
>> This commit introduces the phy_set_pause function to the phylib as
>> a helper to support the set_pauseparam ethtool method.
>>
>> It is hoped that the new behavior introduced by this function will
>> be widely embraced and the phy_set_sym_pause and phy_set_asym_pause
>> functions can be deprecated. Those functions are retained for all
>> existing users and for any desenting opinions on my interpretation
>> of the functionality.
>>
>> Signed-off-by: Doug Berger <opendmb@...il.com>
>> ---
>>  drivers/net/phy/phy_device.c | 31 +++++++++++++++++++++++++++++++
>>  include/linux/phy.h          |  1 +
>>  2 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 48ab9efa0166..e6dafb3c3e5f 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -2614,6 +2614,37 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx)
>>  EXPORT_SYMBOL(phy_set_asym_pause);
>>  
>>  /**
>> + * phy_set_pause - Configure Pause and Asym Pause with autoneg
>> + * @phydev: target phy_device struct
>> + * @rx: Receiver Pause is supported
>> + * @tx: Transmit Pause is supported
>> + * @autoneg: Auto neg should be used
>> + *
>> + * Description: Configure advertised Pause support depending on if
>> + * receiver pause and pause auto neg is supported. Generally called
>> + * from the set_pauseparam ethtool_ops.
>> + *
>> + * Note: Since pause is really a MAC level function it should be
>> + * notified via adjust_link to update its pause functions.
>> + */
>> +void phy_set_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg)
>> +{
>> +	linkmode_set_pause(phydev->advertising, tx, rx, autoneg);
>> +
>> +	/* Reset the state of an already running link to force a new
>> +	 * link up event when advertising doesn't change or when PHY
>> +	 * autoneg is disabled.
>> +	 */
>> +	mutex_lock(&phydev->lock);
>> +	if (phydev->state == PHY_RUNNING)
>> +		phydev->state = PHY_UP;
>> +	mutex_unlock(&phydev->lock);
> 
> I wonder about this - will drivers cope with having two link-up events
> via adjust_link without a corresponding link-down event?  What if they
> touch registers that are only supposed to be touched while the link is
> down?  Obviously, drivers have to opt-in to this interface, so it may
> be okay provided we don't get wholesale changes.
I too wonder about this. That's why I brought it up in the cover letter
to this set. I would prefer a cleaner service interface for this kind of
behavior for the reasons described in the cover letter, but thought this
might be acceptable.

>> +
>> +	phy_start_aneg(phydev);
> 
> Should we be making that conditional on something changing and autoneg
> being enabled, like phy_set_asym_pause() does?  There is no point
> interrupting an established link if the advertisement didn't change.
Again this is described in the cover letter but repeated here:
"The third introduces the phy_set_pause() function based on the existing
phy_set_asym_pause() implementation. One aberration here is the direct
manipulation of the phy state machine to allow a new link up event to
notify the MAC that the pause parameters may have changed. This is a
convenience to simplify the MAC driver by allowing one implementation
of the pause configuration logic to be located in its adjust_link
callback. Otherwise, the MAC driver would need to handle configuring
the pause parameters for an already active PHY link which would likely
require additional synchronization logic to protect the logic from
asynchronous changes in the PHY state.

The logic in phy_set_asym_pause() that looks for a change in
advertising is not particularly helpful here since now a change from
tx=1 rx=1 to tx=0 rx=1 no longer changes the advertising if autoneg is
enabled so phy_start_aneg() would not be called. I took the alternate
approach of unconditionally calling phy_start_aneg() since it
accommodates both manual and autoneg configured links. The "aberrant"
logic allows manually configured and autonegotiated links that don't
change their advertised parameters to receive an adjust_link call to
act on pause parameters that have no effect on the PHY layer.

It seemed excessive to bring the PHY down and back up when nogotiation
is not necessary, but that could be an alternative approach. I am
certainly open to any suggestions on how to improve that portion of
the code if it is controversial and a consensus can be reached."

>> +}
>> +EXPORT_SYMBOL(phy_set_pause);
>> +
>> +/**
>>   * phy_validate_pause - Test if the PHY/MAC support the pause configuration
>>   * @phydev: phy_device struct
>>   * @pp: requested pause configuration
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 5d8ff5428010..71e484424e68 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -1403,6 +1403,7 @@ void phy_support_asym_pause(struct phy_device *phydev);
>>  void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx,
>>  		       bool autoneg);
>>  void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
>> +void phy_set_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg);
>>  bool phy_validate_pause(struct phy_device *phydev,
>>  			struct ethtool_pauseparam *pp);
>>  void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
>> -- 
>> 2.7.4
>>
>>
> 

Powered by blists - more mailing lists