[v2] net/mlx5: fix wrong representor port link status
Checks
Commit Message
Current code uses PF links status for representor port, not the representor
interface itself. This caused wrong representor port link status when
toggling linterface up or down.
Fixes: 5a4b8e2612c5 ("net/mlx5: probe all port representors")
Cc: adrien.mazarguil@6wind.com
Signed-off-by: Xueming Li <xuemingl@mellanox.com>
---
drivers/net/mlx5/mlx5_ethdev.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
Comments
> On Sep 13, 2018, at 11:27 PM, Xueming Li <xuemingl@mellanox.com> wrote:
>
> Current code uses PF links status for representor port, not the representor
> interface itself. This caused wrong representor port link status when
> toggling linterface up or down.
>
> Fixes: 5a4b8e2612c5 ("net/mlx5: probe all port representors")
Wrong commit SHA.
Please always check it by running
./devtools/check-git-log.sh -$n
./devtools/checkpatches.sh -n$n
> Cc: adrien.mazarguil@6wind.com
>
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> ---
> drivers/net/mlx5/mlx5_ethdev.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 34c5b95..7391ab8 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -627,7 +627,7 @@ struct ethtool_link_settings {
> int link_speed = 0;
> int ret;
>
> - ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 1);
> + ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 0);
> if (ret) {
> DRV_LOG(WARNING, "port %u ioctl(SIOCGIFFLAGS) failed: %s",
> dev->data->port_id, strerror(rte_errno));
> @@ -636,6 +636,7 @@ struct ethtool_link_settings {
> memset(&dev_link, 0, sizeof(dev_link));
> dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
> (ifr.ifr_flags & IFF_RUNNING));
> + memset(&ifr, 0, sizeof(ifr));
> ifr.ifr_data = (void *)&edata;
It would be enough to be done like:
ifr = {
.ifr_data = (void *)&edata,
};
And please do the same for dev_link even though it isn't relevant in the patch.
> ret = mlx5_ifreq(dev, SIOCETHTOOL, &ifr, 1);
> if (ret) {
> @@ -666,8 +667,9 @@ struct ethtool_link_settings {
> ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
> dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> ETH_LINK_SPEED_FIXED);
> - if ((dev_link.link_speed && !dev_link.link_status) ||
> - (!dev_link.link_speed && dev_link.link_status)) {
> + if (!priv->representor &&
> + ((dev_link.link_speed && !dev_link.link_status) ||
> + (!dev_link.link_speed && dev_link.link_status))) {
What does this change mean?
Is it allowed for representors?
> rte_errno = EAGAIN;
> return -rte_errno;
> }
> @@ -698,7 +700,7 @@ struct ethtool_link_settings {
> uint64_t sc;
> int ret;
>
> - ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 1);
> + ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 0);
> if (ret) {
> DRV_LOG(WARNING, "port %u ioctl(SIOCGIFFLAGS) failed: %s",
> dev->data->port_id, strerror(rte_errno));
> @@ -707,6 +709,7 @@ struct ethtool_link_settings {
> memset(&dev_link, 0, sizeof(dev_link));
> dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
> (ifr.ifr_flags & IFF_RUNNING));
> + memset(&ifr, 0, sizeof(ifr));
Same here for dev_link and ifr.
Thanks,
Yongseok
> ifr.ifr_data = (void *)&gcmd;
> ret = mlx5_ifreq(dev, SIOCETHTOOL, &ifr, 1);
> if (ret) {
> @@ -775,8 +778,9 @@ struct ethtool_link_settings {
> ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
> dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> ETH_LINK_SPEED_FIXED);
> - if ((dev_link.link_speed && !dev_link.link_status) ||
> - (!dev_link.link_speed && dev_link.link_status)) {
> + if (!priv->representor &&
> + ((dev_link.link_speed && !dev_link.link_status) ||
> + (!dev_link.link_speed && dev_link.link_status))) {
> rte_errno = EAGAIN;
> return -rte_errno;
> }
> --
> 1.8.3.1
>
> -----Original Message-----
> From: Yongseok Koh
> Sent: Saturday, September 15, 2018 12:43 AM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org; Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Subject: Re: [PATCH v2] net/mlx5: fix wrong representor port link status
>
>
> > On Sep 13, 2018, at 11:27 PM, Xueming Li <xuemingl@mellanox.com> wrote:
> >
> > Current code uses PF links status for representor port, not the
> > representor interface itself. This caused wrong representor port link
> > status when toggling linterface up or down.
> >
> > Fixes: 5a4b8e2612c5 ("net/mlx5: probe all port representors")
>
> Wrong commit SHA.
> Please always check it by running
> ./devtools/check-git-log.sh -$n
> ./devtools/checkpatches.sh -n$n
>
> > Cc: adrien.mazarguil@6wind.com
> >
> > Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> > ---
> > drivers/net/mlx5/mlx5_ethdev.c | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > b/drivers/net/mlx5/mlx5_ethdev.c index 34c5b95..7391ab8 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -627,7 +627,7 @@ struct ethtool_link_settings {
> > int link_speed = 0;
> > int ret;
> >
> > - ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 1);
> > + ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 0);
> > if (ret) {
> > DRV_LOG(WARNING, "port %u ioctl(SIOCGIFFLAGS) failed: %s",
> > dev->data->port_id, strerror(rte_errno)); @@ -636,6 +636,7 @@
> > struct ethtool_link_settings {
> > memset(&dev_link, 0, sizeof(dev_link));
> > dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
> > (ifr.ifr_flags & IFF_RUNNING));
> > + memset(&ifr, 0, sizeof(ifr));
> > ifr.ifr_data = (void *)&edata;
>
> It would be enough to be done like:
>
> ifr = {
> .ifr_data = (void *)&edata,
> };
>
> And please do the same for dev_link even though it isn't relevant in the patch.
>
> > ret = mlx5_ifreq(dev, SIOCETHTOOL, &ifr, 1);
> > if (ret) {
> > @@ -666,8 +667,9 @@ struct ethtool_link_settings {
> > ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
> > dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> > ETH_LINK_SPEED_FIXED);
> > - if ((dev_link.link_speed && !dev_link.link_status) ||
> > - (!dev_link.link_speed && dev_link.link_status)) {
> > + if (!priv->representor &&
> > + ((dev_link.link_speed && !dev_link.link_status) ||
> > + (!dev_link.link_speed && dev_link.link_status))) {
>
> What does this change mean?
> Is it allowed for representors?
Performance the check only if not representor port.
For representor port, the status not consistent here due to speed info retrieved from PF.
>
> > rte_errno = EAGAIN;
> > return -rte_errno;
> > }
> > @@ -698,7 +700,7 @@ struct ethtool_link_settings {
> > uint64_t sc;
> > int ret;
> >
> > - ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 1);
> > + ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 0);
> > if (ret) {
> > DRV_LOG(WARNING, "port %u ioctl(SIOCGIFFLAGS) failed: %s",
> > dev->data->port_id, strerror(rte_errno)); @@ -707,6 +709,7 @@
> > struct ethtool_link_settings {
> > memset(&dev_link, 0, sizeof(dev_link));
> > dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
> > (ifr.ifr_flags & IFF_RUNNING));
> > + memset(&ifr, 0, sizeof(ifr));
>
> Same here for dev_link and ifr.
>
> Thanks,
> Yongseok
>
> > ifr.ifr_data = (void *)&gcmd;
> > ret = mlx5_ifreq(dev, SIOCETHTOOL, &ifr, 1);
> > if (ret) {
> > @@ -775,8 +778,9 @@ struct ethtool_link_settings {
> > ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
> > dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> > ETH_LINK_SPEED_FIXED);
> > - if ((dev_link.link_speed && !dev_link.link_status) ||
> > - (!dev_link.link_speed && dev_link.link_status)) {
> > + if (!priv->representor &&
> > + ((dev_link.link_speed && !dev_link.link_status) ||
> > + (!dev_link.link_speed && dev_link.link_status))) {
> > rte_errno = EAGAIN;
> > return -rte_errno;
> > }
> > --
> > 1.8.3.1
> >
@@ -627,7 +627,7 @@ struct ethtool_link_settings {
int link_speed = 0;
int ret;
- ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 1);
+ ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 0);
if (ret) {
DRV_LOG(WARNING, "port %u ioctl(SIOCGIFFLAGS) failed: %s",
dev->data->port_id, strerror(rte_errno));
@@ -636,6 +636,7 @@ struct ethtool_link_settings {
memset(&dev_link, 0, sizeof(dev_link));
dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
(ifr.ifr_flags & IFF_RUNNING));
+ memset(&ifr, 0, sizeof(ifr));
ifr.ifr_data = (void *)&edata;
ret = mlx5_ifreq(dev, SIOCETHTOOL, &ifr, 1);
if (ret) {
@@ -666,8 +667,9 @@ struct ethtool_link_settings {
ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
ETH_LINK_SPEED_FIXED);
- if ((dev_link.link_speed && !dev_link.link_status) ||
- (!dev_link.link_speed && dev_link.link_status)) {
+ if (!priv->representor &&
+ ((dev_link.link_speed && !dev_link.link_status) ||
+ (!dev_link.link_speed && dev_link.link_status))) {
rte_errno = EAGAIN;
return -rte_errno;
}
@@ -698,7 +700,7 @@ struct ethtool_link_settings {
uint64_t sc;
int ret;
- ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 1);
+ ret = mlx5_ifreq(dev, SIOCGIFFLAGS, &ifr, 0);
if (ret) {
DRV_LOG(WARNING, "port %u ioctl(SIOCGIFFLAGS) failed: %s",
dev->data->port_id, strerror(rte_errno));
@@ -707,6 +709,7 @@ struct ethtool_link_settings {
memset(&dev_link, 0, sizeof(dev_link));
dev_link.link_status = ((ifr.ifr_flags & IFF_UP) &&
(ifr.ifr_flags & IFF_RUNNING));
+ memset(&ifr, 0, sizeof(ifr));
ifr.ifr_data = (void *)&gcmd;
ret = mlx5_ifreq(dev, SIOCETHTOOL, &ifr, 1);
if (ret) {
@@ -775,8 +778,9 @@ struct ethtool_link_settings {
ETH_LINK_HALF_DUPLEX : ETH_LINK_FULL_DUPLEX);
dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
ETH_LINK_SPEED_FIXED);
- if ((dev_link.link_speed && !dev_link.link_status) ||
- (!dev_link.link_speed && dev_link.link_status)) {
+ if (!priv->representor &&
+ ((dev_link.link_speed && !dev_link.link_status) ||
+ (!dev_link.link_speed && dev_link.link_status))) {
rte_errno = EAGAIN;
return -rte_errno;
}