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

On 2018/8/24 21:28, Andrew Lunn wrote:

On Fri, Aug 24, 2018 at 02:39:36PM +0800, lipeng (Y) wrote:

quoted

On 2018/8/24 11:41, Andrew Lunn wrote:

quoted

On Fri, Aug 24, 2018 at 11:42:23AM +0800, Peng Li wrote:

quoted

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 <redacted>

Hi Peng

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

    Andrew

Hi, Andrew,

The driver has removed SUPPORTED_1000baseT_Half from

phydev->supported.

the code is :
#define MAC_GMAC_SUPPORTED \
	(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.

	 Andrew

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?

Thanks

.