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

[Andrew Lunn]

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