net/mlx5: fix representors detection

  On systems where the required Netlink commands are not supported but
Mellanox OFED is installed, representors information must be retrieved
through sysfs.

Fixes: 4c10141488fe ("net/mlx5: add port representor awareness")

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx5/mlx5.c        |  7 +++--
 drivers/net/mlx5/mlx5.h        |  2 ++
 drivers/net/mlx5/mlx5_ethdev.c | 53 ++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 2 deletions(-)
  

Hi Nelio, 

Friday, July 13, 2018 4:54 PM, Nelio Laranjeiro:
> Subject: [PATCH] net/mlx5: fix representors detection
> 
> On systems where the required Netlink commands are not supported but
> Mellanox OFED is installed, representors information must be retrieved
> through sysfs.
> 
> Fixes: 4c10141488fe ("net/mlx5: add port representor awareness")
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  drivers/net/mlx5/mlx5.c        |  7 +++--
>  drivers/net/mlx5/mlx5.h        |  2 ++
>  drivers/net/mlx5/mlx5_ethdev.c | 53
> ++++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> df7f39844..8174947f3 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1306,7 +1306,8 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
>  	 * Netlink calls assuming kernel drivers are recent enough to
>  	 * support them.
>  	 *
> -	 * In the event of identification failure through Netlink, either:
> +	 * In the event of identification failure through Netlink, try again
> +	 * through sysfs, then either:
>  	 *
>  	 * 1. No device matches (n == 0), complain and bail out.
>  	 * 2. A single IB device matches (n == 1) and is not a representor, @@
> -1325,7 +1326,9 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
>  		if (nl_route < 0 ||
>  		    !list[i].ifindex ||
>  		    mlx5_nl_switch_info(nl_route, list[i].ifindex,
> -					&list[i].info)) {
> +					&list[i].info) ||
> +		    ((!list[i].info.representor && !list[i].info.master) &&
> +		     mlx5_sysfs_switch_info(list[i].ifindex, &list[i].info))) {

I find this logic a bit complex to be under a single if statement. Can you simplify it w/ local variables? 

>  			list[i].ifindex = 0;
>  			memset(&list[i].info, 0, sizeof(list[i].info));
>  			continue;
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> cc01310e0..fff88b1d6 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -254,6 +254,8 @@ eth_rx_burst_t mlx5_select_rx_function(struct
> rte_eth_dev *dev);  unsigned int mlx5_dev_to_port_id(const struct
> rte_device *dev,
>  				 uint16_t *port_list,
>  				 unsigned int port_list_n);
> +int mlx5_sysfs_switch_info(unsigned int ifindex,
> +			   struct mlx5_switch_info *info);
> 
>  /* mlx5_mac.c */
> 
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> b/drivers/net/mlx5/mlx5_ethdev.c index 05f66f7b6..101be4791 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1296,3 +1296,56 @@ mlx5_dev_to_port_id(const struct rte_device
> *dev, uint16_t *port_list,
>  	}
>  	return n;
>  }
> +
> +/**
> + * Get switch information associated with network interface.
> + *
> + * @param ifindex
> + *   Network interface index.
> + * @param[out] info
> + *   Switch information object, populated in case of success.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +int
> +mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info
> +*info) {
> +	char ifname[IF_NAMESIZE];
> +	FILE *file;
> +	struct mlx5_switch_info data = { .master = 0, };
> +	bool port_name_set = false;
> +	bool port_switch_id_set = false;
> +	char c;
> +
> +	if (!if_indextoname(ifindex, ifname)) {
> +		rte_errno = errno;
> +		return -rte_errno;
> +	}
> +
> +	MKSTR(phys_port_name, "/sys/class/net/%s/phys_port_name",
> +	      ifname);
> +	MKSTR(phys_switch_id, "/sys/class/net/%s/phys_switch_id",
> +	      ifname);
> +
> +	file = fopen(phys_port_name, "rb");
> +	if (file == NULL)
> +		goto error;

This goto is not correct. 
Representors will indeed be able to read, but PFs will not be able to open the phys_port_name (as there is none).
The phys_switch_id is accessible form both. 

I pasted a suggestion below[1], let me know what you think. 

> +	port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2
> &&
> +		c == '\n';
> +	fclose(file);
> +	file = fopen(phys_switch_id, "rb");
> +	if (file == NULL)
> +		goto error;
> +	port_switch_id_set =
> +		fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 &&
> +		c == '\n';
> +	fclose(file);
> +	data.master = port_switch_id_set && !port_name_set;
> +	data.representor = port_switch_id_set && port_name_set;
> +	*info = data;
> +	return 0;
> +error:
> +	rte_errno = errno;
> +	return -rte_errno;
> +}
> --
> 2.18.0

[1]
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index 6cfe02d..b64eb20 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1328,23 +1328,20 @@ struct ethtool_link_settings {
              ifname);
 
        file = fopen(phys_port_name, "rb");
-       if (file == NULL)
-               goto error;
-       port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2 &&
-               c == '\n';
-       fclose(file);
+       if (file) {
+               port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2 &&
+                               c == '\n';
+               fclose(file);
+       }
        file = fopen(phys_switch_id, "rb");
-       if (file == NULL)
-               goto error;
-       port_switch_id_set =
-               fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 &&
-               c == '\n';
-       fclose(file);
+       if (file) {
+               port_switch_id_set =
+                       fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 &&
+                       c == '\n';
+               fclose(file);
+       }
        data.master = port_switch_id_set && !port_name_set;
        data.representor = port_switch_id_set && port_name_set;
  

On Sun, Jul 22, 2018 at 11:18:34AM +0000, Shahaf Shuler wrote:
> Hi Nelio, 
> 
> Friday, July 13, 2018 4:54 PM, Nelio Laranjeiro:
> > Subject: [PATCH] net/mlx5: fix representors detection
> > 
> > On systems where the required Netlink commands are not supported but
> > Mellanox OFED is installed, representors information must be retrieved
> > through sysfs.
> > 
> > Fixes: 4c10141488fe ("net/mlx5: add port representor awareness")
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5.c        |  7 +++--
> >  drivers/net/mlx5/mlx5.h        |  2 ++
> >  drivers/net/mlx5/mlx5_ethdev.c | 53
> > ++++++++++++++++++++++++++++++++++
> >  3 files changed, 60 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > df7f39844..8174947f3 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -1306,7 +1306,8 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> > __rte_unused,
> >  	 * Netlink calls assuming kernel drivers are recent enough to
> >  	 * support them.
> >  	 *
> > -	 * In the event of identification failure through Netlink, either:
> > +	 * In the event of identification failure through Netlink, try again
> > +	 * through sysfs, then either:
> >  	 *
> >  	 * 1. No device matches (n == 0), complain and bail out.
> >  	 * 2. A single IB device matches (n == 1) and is not a representor, @@
> > -1325,7 +1326,9 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> > __rte_unused,
> >  		if (nl_route < 0 ||
> >  		    !list[i].ifindex ||
> >  		    mlx5_nl_switch_info(nl_route, list[i].ifindex,
> > -					&list[i].info)) {
> > +					&list[i].info) ||
> > +		    ((!list[i].info.representor && !list[i].info.master) &&
> > +		     mlx5_sysfs_switch_info(list[i].ifindex, &list[i].info))) {
> 
> I find this logic a bit complex to be under a single if statement. Can
> you simplify it w/ local variables? 

It won't be so easy has the second step depends on the result of the
mlx5_nl_switch_info() call.  To split this if statement into two, it
will be a full copy of the first block by replacing only the call to
mlx5_nl_switch_info().

> >  			list[i].ifindex = 0;
> >  			memset(&list[i].info, 0, sizeof(list[i].info));
> >  			continue;
> > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > cc01310e0..fff88b1d6 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -254,6 +254,8 @@ eth_rx_burst_t mlx5_select_rx_function(struct
> > rte_eth_dev *dev);  unsigned int mlx5_dev_to_port_id(const struct
> > rte_device *dev,
> >  				 uint16_t *port_list,
> >  				 unsigned int port_list_n);
> > +int mlx5_sysfs_switch_info(unsigned int ifindex,
> > +			   struct mlx5_switch_info *info);
> > 
> >  /* mlx5_mac.c */
> > 
> > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > b/drivers/net/mlx5/mlx5_ethdev.c index 05f66f7b6..101be4791 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -1296,3 +1296,56 @@ mlx5_dev_to_port_id(const struct rte_device
> > *dev, uint16_t *port_list,
> >  	}
> >  	return n;
> >  }
> > +
> > +/**
> > + * Get switch information associated with network interface.
> > + *
> > + * @param ifindex
> > + *   Network interface index.
> > + * @param[out] info
> > + *   Switch information object, populated in case of success.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +int
> > +mlx5_sysfs_switch_info(unsigned int ifindex, struct mlx5_switch_info
> > +*info) {
> > +	char ifname[IF_NAMESIZE];
> > +	FILE *file;
> > +	struct mlx5_switch_info data = { .master = 0, };
> > +	bool port_name_set = false;
> > +	bool port_switch_id_set = false;
> > +	char c;
> > +
> > +	if (!if_indextoname(ifindex, ifname)) {
> > +		rte_errno = errno;
> > +		return -rte_errno;
> > +	}
> > +
> > +	MKSTR(phys_port_name, "/sys/class/net/%s/phys_port_name",
> > +	      ifname);
> > +	MKSTR(phys_switch_id, "/sys/class/net/%s/phys_switch_id",
> > +	      ifname);
> > +
> > +	file = fopen(phys_port_name, "rb");
> > +	if (file == NULL)
> > +		goto error;
> 
> This goto is not correct. 
> Representors will indeed be able to read, but PFs will not be able to
> open the phys_port_name (as there is none).

From my testing, this is wrong, the PF can open the file, but it cannot
read (verified with strace).

> The phys_switch_id is accessible form both. 
> 
> I pasted a suggestion below[1], let me know what you think. 
> 
> > +	port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2
> > &&
> > +		c == '\n';
> > +	fclose(file);
> > +	file = fopen(phys_switch_id, "rb");
> > +	if (file == NULL)
> > +		goto error;
> > +	port_switch_id_set =
> > +		fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 &&
> > +		c == '\n';
> > +	fclose(file);
> > +	data.master = port_switch_id_set && !port_name_set;
> > +	data.representor = port_switch_id_set && port_name_set;
> > +	*info = data;
> > +	return 0;
> > +error:
> > +	rte_errno = errno;
> > +	return -rte_errno;
> > +}
> > --
> > 2.18.0
> 
> [1]
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 6cfe02d..b64eb20 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1328,23 +1328,20 @@ struct ethtool_link_settings {
>               ifname);
>  
>         file = fopen(phys_port_name, "rb");
> -       if (file == NULL)
> -               goto error;
> -       port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2 &&
> -               c == '\n';
> -       fclose(file);
> +       if (file) {
> +               port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2 &&
> +                               c == '\n';
> +               fclose(file);
> +       }
>         file = fopen(phys_switch_id, "rb");
> -       if (file == NULL)
> -               goto error;
> -       port_switch_id_set =
> -               fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 &&
> -               c == '\n';
> -       fclose(file);
> +       if (file) {
> +               port_switch_id_set =
> +                       fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 &&
> +                       c == '\n';
> +               fclose(file);
> +       }
>         data.master = port_switch_id_set && !port_name_set;
>         data.representor = port_switch_id_set && port_name_set;

Regards,