diff mbox series

[net-next,v2,11/12] net: devlink: expose phys port name

Message ID 20190326120307.2953-12-jiri@resnulli.us
State Superseded
Delegated to: David Miller
Headers show
Series net: call for phys_port_name into devlink directly if possible | expand

Commit Message

Jiri Pirko March 26, 2019, 12:03 p.m. UTC 
From: Jiri Pirko <jiri@mellanox.com>

Currently, it is exposed via rtnetlink. But it is relevant to devlink
ports, so expose it here too.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- new patch
---
 include/uapi/linux/devlink.h |  2 +
 net/core/devlink.c           | 86 ++++++++++++++++++++++--------------
 2 files changed, 56 insertions(+), 32 deletions(-)

Comments

Jakub Kicinski March 26, 2019, 8:35 p.m. UTC | #1 
On Tue, 26 Mar 2019 13:03:06 +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Currently, it is exposed via rtnetlink. But it is relevant to devlink
> ports, so expose it here too.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Why?  User space has access to all the components which are used to
generate the name in a typed, structured form.  That's strictly
superior to the string netdev string.

Do you have some use in mind for the new attribute?

> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index 5bb4ea67d84f..2d0365e45141 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -332,6 +332,8 @@ enum devlink_attr {
>  	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
>  	DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,	/* string */
>  
> +	DEVLINK_ATTR_PORT_PHYS_NAME,		/* string */
> +
>  	/* add new attributes above here, update the policy in devlink.c */
>  
>  	__DEVLINK_ATTR_MAX,
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 6bbd07e3861e..356d7ee7c404 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -508,6 +508,57 @@ static void devlink_notify(struct devlink *devlink, enum devlink_command cmd)
>  				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
>  }
>  
> +static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
> +					     char *name, size_t len)
> +{
> +	struct devlink_port_attrs *attrs = &devlink_port->attrs;
> +	int n = 0;
> +
> +	if (!attrs->set)
> +		return -EOPNOTSUPP;
> +
> +	switch (attrs->flavour) {
> +	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
> +		if (!attrs->split)
> +			n = snprintf(name, len, "p%u", attrs->port_number);
> +		else
> +			n = snprintf(name, len, "p%us%u", attrs->port_number,
> +				     attrs->split_subport_number);
> +		break;
> +	case DEVLINK_PORT_FLAVOUR_CPU:
> +	case DEVLINK_PORT_FLAVOUR_DSA:
> +		/* As CPU and DSA ports do not have a netdevice associated
> +		 * case should not ever happen.
> +		 */
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	if (n >= len)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int devlink_nl_port_phys_port_name_put(struct sk_buff *msg,
> +					      struct devlink_port *devlink_port)
> +{
> +	struct devlink_port_attrs *attrs = &devlink_port->attrs;
> +	char phys_name[IFNAMSIZ];
> +	int err;
> +
> +	if (attrs->flavour != DEVLINK_PORT_FLAVOUR_PHYSICAL)
> +		return 0;
> +
> +	err = __devlink_port_phys_port_name_get(devlink_port,
> +						phys_name, sizeof(phys_name));
> +	if (err)
> +		return err;
> +	if (nla_put_string(msg, DEVLINK_ATTR_PORT_PHYS_NAME, phys_name))
> +		return -EMSGSIZE;
> +	return 0;
> +}
> +
>  static int devlink_nl_port_attrs_put(struct sk_buff *msg,
>  				     struct devlink_port *devlink_port)
>  {
> @@ -526,6 +577,9 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
>  	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_SUBPORT_NUMBER,
>  			attrs->split_subport_number))
>  		return -EMSGSIZE;
> +	if (devlink_nl_port_phys_port_name_put(msg, devlink_port))
> +		return -EMSGSIZE;
> +
>  	return 0;
>  }
>  
> @@ -5414,38 +5468,6 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
>  }
>  EXPORT_SYMBOL_GPL(devlink_port_attrs_set);
>  
> -static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
> -					     char *name, size_t len)
> -{
> -	struct devlink_port_attrs *attrs = &devlink_port->attrs;
> -	int n = 0;
> -
> -	if (!attrs->set)
> -		return -EOPNOTSUPP;
> -
> -	switch (attrs->flavour) {
> -	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
> -		if (!attrs->split)
> -			n = snprintf(name, len, "p%u", attrs->port_number);
> -		else
> -			n = snprintf(name, len, "p%us%u", attrs->port_number,
> -				     attrs->split_subport_number);
> -		break;
> -	case DEVLINK_PORT_FLAVOUR_CPU:
> -	case DEVLINK_PORT_FLAVOUR_DSA:
> -		/* As CPU and DSA ports do not have a netdevice associated
> -		 * case should not ever happen.
> -		 */
> -		WARN_ON(1);
> -		return -EINVAL;
> -	}
> -
> -	if (n >= len)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
>  			u32 size, u16 ingress_pools_count,
>  			u16 egress_pools_count, u16 ingress_tc_count,
Jiri Pirko March 27, 2019, 7:45 a.m. UTC | #2 
Tue, Mar 26, 2019 at 09:35:58PM CET, jakub.kicinski@netronome.com wrote:
>On Tue, 26 Mar 2019 13:03:06 +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Currently, it is exposed via rtnetlink. But it is relevant to devlink
>> ports, so expose it here too.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>Why?  User space has access to all the components which are used to
>generate the name in a typed, structured form.  That's strictly

Nope. It does has access only to flavour. Not to port number for
example. So I thought either expose that or the generated name.

>superior to the string netdev string.
>
>Do you have some use in mind for the new attribute?
>
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index 5bb4ea67d84f..2d0365e45141 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -332,6 +332,8 @@ enum devlink_attr {
>>  	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
>>  	DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,	/* string */
>>  
>> +	DEVLINK_ATTR_PORT_PHYS_NAME,		/* string */
>> +
>>  	/* add new attributes above here, update the policy in devlink.c */
>>  
>>  	__DEVLINK_ATTR_MAX,
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 6bbd07e3861e..356d7ee7c404 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -508,6 +508,57 @@ static void devlink_notify(struct devlink *devlink, enum devlink_command cmd)
>>  				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
>>  }
>>  
>> +static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
>> +					     char *name, size_t len)
>> +{
>> +	struct devlink_port_attrs *attrs = &devlink_port->attrs;
>> +	int n = 0;
>> +
>> +	if (!attrs->set)
>> +		return -EOPNOTSUPP;
>> +
>> +	switch (attrs->flavour) {
>> +	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
>> +		if (!attrs->split)
>> +			n = snprintf(name, len, "p%u", attrs->port_number);
>> +		else
>> +			n = snprintf(name, len, "p%us%u", attrs->port_number,
>> +				     attrs->split_subport_number);
>> +		break;
>> +	case DEVLINK_PORT_FLAVOUR_CPU:
>> +	case DEVLINK_PORT_FLAVOUR_DSA:
>> +		/* As CPU and DSA ports do not have a netdevice associated
>> +		 * case should not ever happen.
>> +		 */
>> +		WARN_ON(1);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (n >= len)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int devlink_nl_port_phys_port_name_put(struct sk_buff *msg,
>> +					      struct devlink_port *devlink_port)
>> +{
>> +	struct devlink_port_attrs *attrs = &devlink_port->attrs;
>> +	char phys_name[IFNAMSIZ];
>> +	int err;
>> +
>> +	if (attrs->flavour != DEVLINK_PORT_FLAVOUR_PHYSICAL)
>> +		return 0;
>> +
>> +	err = __devlink_port_phys_port_name_get(devlink_port,
>> +						phys_name, sizeof(phys_name));
>> +	if (err)
>> +		return err;
>> +	if (nla_put_string(msg, DEVLINK_ATTR_PORT_PHYS_NAME, phys_name))
>> +		return -EMSGSIZE;
>> +	return 0;
>> +}
>> +
>>  static int devlink_nl_port_attrs_put(struct sk_buff *msg,
>>  				     struct devlink_port *devlink_port)
>>  {
>> @@ -526,6 +577,9 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
>>  	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_SUBPORT_NUMBER,
>>  			attrs->split_subport_number))
>>  		return -EMSGSIZE;
>> +	if (devlink_nl_port_phys_port_name_put(msg, devlink_port))
>> +		return -EMSGSIZE;
>> +
>>  	return 0;
>>  }
>>  
>> @@ -5414,38 +5468,6 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_port_attrs_set);
>>  
>> -static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
>> -					     char *name, size_t len)
>> -{
>> -	struct devlink_port_attrs *attrs = &devlink_port->attrs;
>> -	int n = 0;
>> -
>> -	if (!attrs->set)
>> -		return -EOPNOTSUPP;
>> -
>> -	switch (attrs->flavour) {
>> -	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
>> -		if (!attrs->split)
>> -			n = snprintf(name, len, "p%u", attrs->port_number);
>> -		else
>> -			n = snprintf(name, len, "p%us%u", attrs->port_number,
>> -				     attrs->split_subport_number);
>> -		break;
>> -	case DEVLINK_PORT_FLAVOUR_CPU:
>> -	case DEVLINK_PORT_FLAVOUR_DSA:
>> -		/* As CPU and DSA ports do not have a netdevice associated
>> -		 * case should not ever happen.
>> -		 */
>> -		WARN_ON(1);
>> -		return -EINVAL;
>> -	}
>> -
>> -	if (n >= len)
>> -		return -EINVAL;
>> -
>> -	return 0;
>> -}
>> -
>>  int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
>>  			u32 size, u16 ingress_pools_count,
>>  			u16 egress_pools_count, u16 ingress_tc_count,
>
Jiri Pirko March 27, 2019, 11:56 a.m. UTC | #3 
Wed, Mar 27, 2019 at 08:45:38AM CET, jiri@resnulli.us wrote:
>Tue, Mar 26, 2019 at 09:35:58PM CET, jakub.kicinski@netronome.com wrote:
>>On Tue, 26 Mar 2019 13:03:06 +0100, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>> 
>>> Currently, it is exposed via rtnetlink. But it is relevant to devlink
>>> ports, so expose it here too.
>>> 
>>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>>
>>Why?  User space has access to all the components which are used to
>>generate the name in a typed, structured form.  That's strictly
>
>Nope. It does has access only to flavour. Not to port number for
>example. So I thought either expose that or the generated name.

To correct myself, all is exposed by kernel, just not implemented in
userspace. I thought that generated port name would be good to have.
Not really good idea to generate it again in userspace. Anyway, I'll
leave this patch out for now, I can send it separatelly later on.


>
>>superior to the string netdev string.
>>
>>Do you have some use in mind for the new attribute?
>>
>>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>> index 5bb4ea67d84f..2d0365e45141 100644
>>> --- a/include/uapi/linux/devlink.h
>>> +++ b/include/uapi/linux/devlink.h
>>> @@ -332,6 +332,8 @@ enum devlink_attr {
>>>  	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
>>>  	DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,	/* string */
>>>  
>>> +	DEVLINK_ATTR_PORT_PHYS_NAME,		/* string */
>>> +
>>>  	/* add new attributes above here, update the policy in devlink.c */
>>>  
>>>  	__DEVLINK_ATTR_MAX,
>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>> index 6bbd07e3861e..356d7ee7c404 100644
>>> --- a/net/core/devlink.c
>>> +++ b/net/core/devlink.c
>>> @@ -508,6 +508,57 @@ static void devlink_notify(struct devlink *devlink, enum devlink_command cmd)
>>>  				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
>>>  }
>>>  
>>> +static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
>>> +					     char *name, size_t len)
>>> +{
>>> +	struct devlink_port_attrs *attrs = &devlink_port->attrs;
>>> +	int n = 0;
>>> +
>>> +	if (!attrs->set)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	switch (attrs->flavour) {
>>> +	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
>>> +		if (!attrs->split)
>>> +			n = snprintf(name, len, "p%u", attrs->port_number);
>>> +		else
>>> +			n = snprintf(name, len, "p%us%u", attrs->port_number,
>>> +				     attrs->split_subport_number);
>>> +		break;
>>> +	case DEVLINK_PORT_FLAVOUR_CPU:
>>> +	case DEVLINK_PORT_FLAVOUR_DSA:
>>> +		/* As CPU and DSA ports do not have a netdevice associated
>>> +		 * case should not ever happen.
>>> +		 */
>>> +		WARN_ON(1);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (n >= len)
>>> +		return -EINVAL;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int devlink_nl_port_phys_port_name_put(struct sk_buff *msg,
>>> +					      struct devlink_port *devlink_port)
>>> +{
>>> +	struct devlink_port_attrs *attrs = &devlink_port->attrs;
>>> +	char phys_name[IFNAMSIZ];
>>> +	int err;
>>> +
>>> +	if (attrs->flavour != DEVLINK_PORT_FLAVOUR_PHYSICAL)
>>> +		return 0;
>>> +
>>> +	err = __devlink_port_phys_port_name_get(devlink_port,
>>> +						phys_name, sizeof(phys_name));
>>> +	if (err)
>>> +		return err;
>>> +	if (nla_put_string(msg, DEVLINK_ATTR_PORT_PHYS_NAME, phys_name))
>>> +		return -EMSGSIZE;
>>> +	return 0;
>>> +}
>>> +
>>>  static int devlink_nl_port_attrs_put(struct sk_buff *msg,
>>>  				     struct devlink_port *devlink_port)
>>>  {
>>> @@ -526,6 +577,9 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
>>>  	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_SUBPORT_NUMBER,
>>>  			attrs->split_subport_number))
>>>  		return -EMSGSIZE;
>>> +	if (devlink_nl_port_phys_port_name_put(msg, devlink_port))
>>> +		return -EMSGSIZE;
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -5414,38 +5468,6 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
>>>  }
>>>  EXPORT_SYMBOL_GPL(devlink_port_attrs_set);
>>>  
>>> -static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
>>> -					     char *name, size_t len)
>>> -{
>>> -	struct devlink_port_attrs *attrs = &devlink_port->attrs;
>>> -	int n = 0;
>>> -
>>> -	if (!attrs->set)
>>> -		return -EOPNOTSUPP;
>>> -
>>> -	switch (attrs->flavour) {
>>> -	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
>>> -		if (!attrs->split)
>>> -			n = snprintf(name, len, "p%u", attrs->port_number);
>>> -		else
>>> -			n = snprintf(name, len, "p%us%u", attrs->port_number,
>>> -				     attrs->split_subport_number);
>>> -		break;
>>> -	case DEVLINK_PORT_FLAVOUR_CPU:
>>> -	case DEVLINK_PORT_FLAVOUR_DSA:
>>> -		/* As CPU and DSA ports do not have a netdevice associated
>>> -		 * case should not ever happen.
>>> -		 */
>>> -		WARN_ON(1);
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	if (n >= len)
>>> -		return -EINVAL;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>  int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
>>>  			u32 size, u16 ingress_pools_count,
>>>  			u16 egress_pools_count, u16 ingress_tc_count,
>>
diff mbox series

Patch

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 5bb4ea67d84f..2d0365e45141 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -332,6 +332,8 @@  enum devlink_attr {
 	DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,	/* string */
 	DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,	/* string */
 
+	DEVLINK_ATTR_PORT_PHYS_NAME,		/* string */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6bbd07e3861e..356d7ee7c404 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -508,6 +508,57 @@  static void devlink_notify(struct devlink *devlink, enum devlink_command cmd)
 				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
 }
 
+static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
+					     char *name, size_t len)
+{
+	struct devlink_port_attrs *attrs = &devlink_port->attrs;
+	int n = 0;
+
+	if (!attrs->set)
+		return -EOPNOTSUPP;
+
+	switch (attrs->flavour) {
+	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
+		if (!attrs->split)
+			n = snprintf(name, len, "p%u", attrs->port_number);
+		else
+			n = snprintf(name, len, "p%us%u", attrs->port_number,
+				     attrs->split_subport_number);
+		break;
+	case DEVLINK_PORT_FLAVOUR_CPU:
+	case DEVLINK_PORT_FLAVOUR_DSA:
+		/* As CPU and DSA ports do not have a netdevice associated
+		 * case should not ever happen.
+		 */
+		WARN_ON(1);
+		return -EINVAL;
+	}
+
+	if (n >= len)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int devlink_nl_port_phys_port_name_put(struct sk_buff *msg,
+					      struct devlink_port *devlink_port)
+{
+	struct devlink_port_attrs *attrs = &devlink_port->attrs;
+	char phys_name[IFNAMSIZ];
+	int err;
+
+	if (attrs->flavour != DEVLINK_PORT_FLAVOUR_PHYSICAL)
+		return 0;
+
+	err = __devlink_port_phys_port_name_get(devlink_port,
+						phys_name, sizeof(phys_name));
+	if (err)
+		return err;
+	if (nla_put_string(msg, DEVLINK_ATTR_PORT_PHYS_NAME, phys_name))
+		return -EMSGSIZE;
+	return 0;
+}
+
 static int devlink_nl_port_attrs_put(struct sk_buff *msg,
 				     struct devlink_port *devlink_port)
 {
@@ -526,6 +577,9 @@  static int devlink_nl_port_attrs_put(struct sk_buff *msg,
 	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_SUBPORT_NUMBER,
 			attrs->split_subport_number))
 		return -EMSGSIZE;
+	if (devlink_nl_port_phys_port_name_put(msg, devlink_port))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
@@ -5414,38 +5468,6 @@  void devlink_port_attrs_set(struct devlink_port *devlink_port,
 }
 EXPORT_SYMBOL_GPL(devlink_port_attrs_set);
 
-static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
-					     char *name, size_t len)
-{
-	struct devlink_port_attrs *attrs = &devlink_port->attrs;
-	int n = 0;
-
-	if (!attrs->set)
-		return -EOPNOTSUPP;
-
-	switch (attrs->flavour) {
-	case DEVLINK_PORT_FLAVOUR_PHYSICAL:
-		if (!attrs->split)
-			n = snprintf(name, len, "p%u", attrs->port_number);
-		else
-			n = snprintf(name, len, "p%us%u", attrs->port_number,
-				     attrs->split_subport_number);
-		break;
-	case DEVLINK_PORT_FLAVOUR_CPU:
-	case DEVLINK_PORT_FLAVOUR_DSA:
-		/* As CPU and DSA ports do not have a netdevice associated
-		 * case should not ever happen.
-		 */
-		WARN_ON(1);
-		return -EINVAL;
-	}
-
-	if (n >= len)
-		return -EINVAL;
-
-	return 0;
-}
-
 int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
 			u32 size, u16 ingress_pools_count,
 			u16 egress_pools_count, u16 ingress_tc_count,