| Message ID | 1535082143-122281-4-git-send-email-lipeng321@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: hns: fix some bugs about speed and duplex change | expand |
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@huawei.com> Hi Peng Does the driver remove SUPPORTED_1000baseT_Half from phydev->supported? If you do that, the PHY should never negotiate this speed. Andrew
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@huawei.com> > 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. Thanks > > . >
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@huawei.com> > >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
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@huawei.com> >>> 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 > . >
> 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? I think it is not needed. And if it was needed, it would indicate there is a bug somewhere else. Andrew
On 2018/8/26 2:07, Andrew Lunn wrote: >> 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? > I think it is not needed. > > And if it was needed, it would indicate there is a bug somewhere else. Hi, Andrew It is a theoretical protect,we can remove this patch from patchset. Thanks. > > Andrew > > . >
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c index 09e4061..c1d062e 100644 --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c @@ -272,6 +272,11 @@ static int hns_gmac_adjust_link(void *mac_drv, enum mac_speed speed, { struct mac_driver *drv = (struct mac_driver *)mac_drv; + if (full_duplex == DUPLEX_HALF && speed == MAC_SPEED_1000) { + dev_err(drv->dev, "HW do not support 1000M half\n"); + return -EINVAL; + } + dsaf_set_dev_bit(drv, GMAC_DUPLEX_TYPE_REG, GMAC_DUPLEX_TYPE_B, !!full_duplex);
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@huawei.com> --- drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c | 5 +++++ 1 file changed, 5 insertions(+)