Re: [PATCH NET 3/3] net: hns: add configuration constraints for 1000M half

From: lipeng (Y)
Date: Fri Aug 24 2018 - 21:22:26 EST

On 2018/8/24 21:28, Andrew Lunn wrote:
On Fri, Aug 24, 2018 at 02:39:36PM +0800, lipeng (Y) wrote:

On 2018/8/24 11:41, Andrew Lunn wrote:
On Fri, Aug 24, 2018 at 11:42:23AM +0800, Peng Li wrote:
Hisilicon hip05 and hip06 board network card do not support
1000M half configuration. Driver can not config gmac as
1000M half.

Signed-off-by: Peng Li <lipeng321@xxxxxxxxxx>
Hi Peng

Does the driver remove SUPPORTED_1000baseT_Half from
phydev->supported? If you do that, the PHY should never negotiate
this speed.

Hi, Andrew,

The driver has removed SUPPORTED_1000baseT_Half from


the code is :
(SUPPORTED_10baseT_Half \
| SUPPORTED_10baseT_Full \
| SUPPORTED_100baseT_Half \
| SUPPORTED_100baseT_Full \
| SUPPORTED_Autoneg)
h->if_support = MAC_GMAC_SUPPORTED;
h->if_support |= SUPPORTED_1000baseT_Full;
phydev->supported &= h->if_support;

As gmac do not support 1000M half, we add this patch to
make sure that no users can set 1000M half in any case.
Well, not quite. What this patch does is protect the hardware when it
is asked to change to an unsupported mode. This patch has nothing to
do with user APIs.

The user API for setting link modes is hns_nic_set_link_ksettings().

What you do have is

if (speed == SPEED_1000 && cmd->base.duplex == DUPLEX_HALF)
return -EINVAL;

which should stop somebody setting up a fixed speed link at 1000Half.

But you don't do anything with cmd->link_modes.advertising. However,
when you call phy_ethtool_ksettings_set(), it gets AND'ed with
phydev->supported, which you have already removed 1000Half from.

So is this a patch for a theoretical problem? Or have you seen it
happen? If it did happen, how did the user configure it to cause this
problem? That user API needs to prevent it.

Hi, Andrew,

HWÂ do not support 1000M half, and the driver has done the 2 things:
1, remove SUPPORTED_1000baseT_Half from phydev->supported.
2, add the protect in hns_nic_set_link_ksettings().

This patch is a theoretical protect, and the problem does not really happen.

I think you really get the point, do you think we need this patch?