-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com>
Sent: Tuesday, January 28, 2020 5:55 PM
To: Florian Fainelli <f.fainelli@gmail.com>
Cc: Madalin Bucur (OSS) <redacted>; davem@davemloft.net;
andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
ykaukab@suse.de
Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
indication

Hi Florian,

On Mon, 27 Jan 2020 at 19:44, Florian Fainelli [off-list ref]
wrote:

quoted

On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:

quoted

quoted

-----Original Message-----
From: Florian Fainelli <f.fainelli@gmail.com>
Sent: Wednesday, January 22, 2020 7:58 PM
To: Madalin Bucur (OSS) <redacted>;

davem@davemloft.net

quoted

quoted

quoted

Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
ykaukab@suse.de
Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add

rate_adaptation

quoted

quoted

quoted

indication

On 1/22/20 5:59 AM, Madalin Bucur wrote:

quoted

The AQR PHYs are able to perform rate adaptation between
the system interface and the line interfaces. When such
a PHY is deployed, the ethernet driver should not limit
the modes supported or advertised by the PHY. This patch
introduces the bit that allows checking for this feature
in the phy_device structure and its use for the Aquantia
PHYs.

Signed-off-by: Madalin Bucur <redacted>
---
 drivers/net/phy/aquantia_main.c | 3 +++
 include/linux/phy.h             | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c

b/drivers/net/phy/aquantia_main.c

quoted

index 975789d9349d..36fdd523b758 100644

--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c

@@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device

*phydev)

quoted

    u16 reg;
    int ret;

+   /* add here as this is called for all devices */
+   phydev->rate_adaptation = 1;

How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and

you

quoted

quoted

quoted

set that directly from the phy_driver entry? using the "flags"

bitmask

quoted

quoted

quoted

instead of adding another structure member to phy_device?

I've looked at the phydev->dev_flags use, it seemed to me that mostly

it

quoted

quoted

is used to convey configuration options towards the PHY.

You read me incorrectly, I am suggesting using the phy_driver::flags
member, not the phy_device::dev_flags entry, please re-consider your
position.

Whether the PHY performs rate adaptation is a dynamic property.
It will perform it at wire speeds lower than 2500Mbps (1000/100) when
system side is 2500Base-X, but not at wire speed 2500 & 2500Base-X,
and not at wire speed 1000 & USXGMII.
You can't really encode that in phydev->flags.

Vladimir, the patch adds a bit that indicates the PHY ability to perform
rate adaptation, not whether it is actually in use in a certain combination
of system interface and line interface modes. Please review the submission
again, I understand you have something slightly different in mind, but this
is just addressing a basic need of knowing whether there is a chance the
line side could work at other speeds than the system interface and allow it
to do so.
 

I was actually searching for a way to encode some more PHY capabilities:
- Does it work with in-band autoneg enabled?
- Does it work with in-band autoneg bypassed?
- Does it emit pause frames? <- Madalin got ahead of me on this one.

For the first 2, I want a mechanism for the PHY library to figure out
whether the MAC and the PHY should use in-band autoneg or not. If both
support in-band autoneg, that would be preferred. Otherwise,
out-of-band (MDIO) is preferred, and in-band autoneg is explicitly
disabled in both, if that is claimed to be supported. Otherwise,
report error to user. Yes, this deprecates "managed =
'in-band-status'" in the device tree, and that's for (what I think is)
the better. We can make "managed = 'in-band-status'" to just clear the
MAC's ability to support the "in-band autoneg bypassed" mode.

So I thought a function pointer in the phy driver would be better, and
more extensible.
Thoughts?

This is where you get when you try to implement anti-stupid devices, it gets
complicated fast and, most often, it gets in the way. Should someone change
a setting (pause settings) and experience adverse effects (excessive frame
loss), we should rely on his analytical abilities to connect the dots, imho.

Madalin

 

quoted

--
Florian

Regards,
-Vladimir