diff mbox series

[RFC,WIP,2/2] rdma_rxe: use netlink messages to add/delete links

Message ID 4df04403a9c875e30fdd96be95072e8b7b857f50.1537896647.git.swise@opengridcomputing.com (mailing list archive)
State RFC
Headers show
Series *** SUBJECT HERE *** | expand

Commit Message

Steve Wise Sept. 14, 2018, 6:43 p.m. UTC
Signed-off-by: Steve Wise <swise@opengridcomputing.com>
---
 drivers/infiniband/sw/rxe/rxe.c       | 61 +++++++++++++++++++++++++++++++++++
 drivers/infiniband/sw/rxe/rxe_net.c   |  3 +-
 drivers/infiniband/sw/rxe/rxe_net.h   |  2 +-
 drivers/infiniband/sw/rxe/rxe_sysfs.c |  6 ++--
 drivers/infiniband/sw/rxe/rxe_verbs.c |  1 -
 5 files changed, 67 insertions(+), 6 deletions(-)

Comments

Jason Gunthorpe Sept. 25, 2018, 8:18 p.m. UTC | #1
On Fri, Sep 14, 2018 at 11:43:55AM -0700, Steve Wise wrote:
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>  drivers/infiniband/sw/rxe/rxe.c       | 61 +++++++++++++++++++++++++++++++++++
>  drivers/infiniband/sw/rxe/rxe_net.c   |  3 +-
>  drivers/infiniband/sw/rxe/rxe_net.h   |  2 +-
>  drivers/infiniband/sw/rxe/rxe_sysfs.c |  6 ++--
>  drivers/infiniband/sw/rxe/rxe_verbs.c |  1 -
>  5 files changed, 67 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
> index 10999fa69281..83f52954c300 100644
> +++ b/drivers/infiniband/sw/rxe/rxe.c
> @@ -31,6 +31,7 @@
>   * SOFTWARE.
>   */
>  
> +#include <rdma/rdma_netlink.h>
>  #include <net/addrconf.h>
>  #include "rxe.h"
>  #include "rxe_loc.h"
> @@ -340,6 +341,64 @@ void rxe_remove(struct rxe_dev *rxe)
>  	rxe_dev_put(rxe);
>  }
>  
> +static int rxe_newlink(char *ibdev_name, char *ndev_name)

const char *ndev_name

> +{
> +	struct net_device *ndev = NULL;
> +	struct rxe_dev *rxe;
> +	int err = 0;
> +
> +	ndev = dev_get_by_name(&init_net, ndev_name);
> +	if (!ndev) {
> +		pr_err("interface %s not found\n", ndev_name);
> +		err = -ENODEV;
> +		goto err;
> +	}
> +
> +	if (net_to_rxe(ndev)) {
> +		pr_err("already configured on %s\n", ndev_name);
> +		err = -EEXIST;
> +		goto err;
> +	}
> +
> +	rxe = rxe_net_add(ibdev_name, ndev);
> +	if (!rxe) {
> +		pr_err("failed to add %s\n", ndev_name);
> +		err = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (netif_running(ndev) && netif_carrier_ok(ndev))
> +		rxe_port_up(rxe);
> +	else
> +		rxe_port_down(rxe);
> +	pr_info("added %s to %s\n", rxe->ib_dev.name, ndev->name);
> +err:
> +	if (ndev)
> +		dev_put(ndev);
> +	return err;
> +}
> +
> +static int rxe_dellink(char *ibdev_name)
> +{
> +	struct rxe_dev *rxe;
> +
> +	rxe = get_rxe_by_name(ibdev_name);
> +	if (!rxe) {
> +		pr_err("not configured on %s\n", ibdev_name);
> +		return -ENODEV;
> +	}
> +
> +	list_del(&rxe->list);
> +	rxe_remove(rxe);
> +	return 0;
> +}
> +
> +static struct rdma_link_ops rxe_link_ops = {
> +	.type = "rxe",
> +	.newlink = rxe_newlink,
> +	.dellink = rxe_dellink,
> +};
> +
>  static int __init rxe_module_init(void)
>  {
>  	int err;
> @@ -355,12 +414,14 @@ static int __init rxe_module_init(void)
>  	if (err)
>  		return err;
>  
> +	rdma_link_register(&rxe_link_ops);
>  	pr_info("loaded\n");
>  	return 0;
>  }
>  
>  static void __exit rxe_module_exit(void)
>  {
> +	rdma_link_unregister(&rxe_link_ops);
>  	rxe_remove_all();
>  	rxe_net_exit();
>  	rxe_cache_exit();
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 8094cbaa54a9..0c520999c8d4 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -565,7 +565,7 @@ enum rdma_link_layer rxe_link_layer(struct rxe_dev *rxe, unsigned int port_num)
>  	return IB_LINK_LAYER_ETHERNET;
>  }
>  
> -struct rxe_dev *rxe_net_add(struct net_device *ndev)
> +struct rxe_dev *rxe_net_add(char *ibdev_name, struct net_device *ndev)
>  {
>  	int err;
>  	struct rxe_dev *rxe = NULL;
> @@ -575,6 +575,7 @@ struct rxe_dev *rxe_net_add(struct net_device *ndev)
>  		return NULL;
>  
>  	rxe->ndev = ndev;
> +	strlcpy(rxe->ib_dev.name, ibdev_name, IB_DEVICE_NAME_MAX);

I'm trying to delete these, don't add new ones :)

The name needs to be passed to register. Maybe check that it doesn't
have % in it in the core code too.

>  	err = rxe_add(rxe, ndev->mtu);
>  	if (err) {
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.h b/drivers/infiniband/sw/rxe/rxe_net.h
> index 106c586dbb26..5e646039d393 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_net.h
> @@ -43,7 +43,7 @@ struct rxe_recv_sockets {
>  	struct socket *sk6;
>  };
>  
> -struct rxe_dev *rxe_net_add(struct net_device *ndev);
> +struct rxe_dev *rxe_net_add(char *ibdev_name, struct net_device *ndev);
>  
>  int rxe_net_init(void);
>  void rxe_net_exit(void);
> diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> index d5ed7571128f..1f82019ff8f3 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> @@ -97,7 +97,7 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
>  		goto err;
>  	}
>  
> -	rxe = rxe_net_add(ndev);
> +	rxe = rxe_net_add(NULL, ndev);

Doesn't this crash?

Jason
Leon Romanovsky Sept. 26, 2018, 8:45 a.m. UTC | #2
On Fri, Sep 14, 2018 at 11:43:55AM -0700, Steve Wise wrote:
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> ---
>  drivers/infiniband/sw/rxe/rxe.c       | 61 +++++++++++++++++++++++++++++++++++
>  drivers/infiniband/sw/rxe/rxe_net.c   |  3 +-
>  drivers/infiniband/sw/rxe/rxe_net.h   |  2 +-
>  drivers/infiniband/sw/rxe/rxe_sysfs.c |  6 ++--
>  drivers/infiniband/sw/rxe/rxe_verbs.c |  1 -
>  5 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
> index 10999fa69281..83f52954c300 100644
> --- a/drivers/infiniband/sw/rxe/rxe.c
> +++ b/drivers/infiniband/sw/rxe/rxe.c
> @@ -31,6 +31,7 @@
>   * SOFTWARE.
>   */
>
> +#include <rdma/rdma_netlink.h>
>  #include <net/addrconf.h>
>  #include "rxe.h"
>  #include "rxe_loc.h"
> @@ -340,6 +341,64 @@ void rxe_remove(struct rxe_dev *rxe)
>  	rxe_dev_put(rxe);
>  }
>
> +static int rxe_newlink(char *ibdev_name, char *ndev_name)
> +{
> +	struct net_device *ndev = NULL;
> +	struct rxe_dev *rxe;
> +	int err = 0;
> +
> +	ndev = dev_get_by_name(&init_net, ndev_name);

I would like to see this code in general nldex.c and it should respect
namespaces.

> +	if (!ndev) {
> +		pr_err("interface %s not found\n", ndev_name);
> +		err = -ENODEV;
> +		goto err;
> +	}
> +
> +	if (net_to_rxe(ndev)) {
> +		pr_err("already configured on %s\n", ndev_name);

Something general, not for this series, but we need to start to use
extack to deliver errors and not dmesg.

> +		err = -EEXIST;
> +		goto err;
> +	}
> +
> +	rxe = rxe_net_add(ibdev_name, ndev);
> +	if (!rxe) {
> +		pr_err("failed to add %s\n", ndev_name);
> +		err = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (netif_running(ndev) && netif_carrier_ok(ndev))
> +		rxe_port_up(rxe);
> +	else
> +		rxe_port_down(rxe);
> +	pr_info("added %s to %s\n", rxe->ib_dev.name, ndev->name);
> +err:
> +	if (ndev)
> +		dev_put(ndev);
> +	return err;
> +}
> +
> +static int rxe_dellink(char *ibdev_name)
> +{
> +	struct rxe_dev *rxe;
> +
> +	rxe = get_rxe_by_name(ibdev_name);
> +	if (!rxe) {
> +		pr_err("not configured on %s\n", ibdev_name);
> +		return -ENODEV;
> +	}
> +
> +	list_del(&rxe->list);
> +	rxe_remove(rxe);
> +	return 0;
> +}
> +
> +static struct rdma_link_ops rxe_link_ops = {
> +	.type = "rxe",
> +	.newlink = rxe_newlink,
> +	.dellink = rxe_dellink,
> +};
> +
>  static int __init rxe_module_init(void)
>  {
>  	int err;
> @@ -355,12 +414,14 @@ static int __init rxe_module_init(void)
>  	if (err)
>  		return err;
>
> +	rdma_link_register(&rxe_link_ops);
>  	pr_info("loaded\n");
>  	return 0;
>  }
>
>  static void __exit rxe_module_exit(void)
>  {
> +	rdma_link_unregister(&rxe_link_ops);
>  	rxe_remove_all();
>  	rxe_net_exit();
>  	rxe_cache_exit();
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 8094cbaa54a9..0c520999c8d4 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -565,7 +565,7 @@ enum rdma_link_layer rxe_link_layer(struct rxe_dev *rxe, unsigned int port_num)
>  	return IB_LINK_LAYER_ETHERNET;
>  }
>
> -struct rxe_dev *rxe_net_add(struct net_device *ndev)
> +struct rxe_dev *rxe_net_add(char *ibdev_name, struct net_device *ndev)
>  {
>  	int err;
>  	struct rxe_dev *rxe = NULL;
> @@ -575,6 +575,7 @@ struct rxe_dev *rxe_net_add(struct net_device *ndev)
>  		return NULL;
>
>  	rxe->ndev = ndev;
> +	strlcpy(rxe->ib_dev.name, ibdev_name, IB_DEVICE_NAME_MAX);
>
>  	err = rxe_add(rxe, ndev->mtu);
>  	if (err) {
> diff --git a/drivers/infiniband/sw/rxe/rxe_net.h b/drivers/infiniband/sw/rxe/rxe_net.h
> index 106c586dbb26..5e646039d393 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.h
> +++ b/drivers/infiniband/sw/rxe/rxe_net.h
> @@ -43,7 +43,7 @@ struct rxe_recv_sockets {
>  	struct socket *sk6;
>  };
>
> -struct rxe_dev *rxe_net_add(struct net_device *ndev);
> +struct rxe_dev *rxe_net_add(char *ibdev_name, struct net_device *ndev);
>
>  int rxe_net_init(void);
>  void rxe_net_exit(void);
> diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> index d5ed7571128f..1f82019ff8f3 100644
> --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> @@ -97,7 +97,7 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
>  		goto err;
>  	}
>
> -	rxe = rxe_net_add(ndev);
> +	rxe = rxe_net_add(NULL, ndev);
>  	if (!rxe) {
>  		pr_err("failed to add %s\n", intf);
>  		err = -EINVAL;
> @@ -152,6 +152,6 @@ static int rxe_param_set_remove(const char *val, const struct kernel_param *kp)
>  };
>
>  module_param_cb(add, &rxe_add_ops, NULL, 0200);
> -MODULE_PARM_DESC(add, "Create RXE device over network interface");
> +MODULE_PARM_DESC(add, "DEPRECATED.  Create RXE device over network interface");
>  module_param_cb(remove, &rxe_remove_ops, NULL, 0200);
> -MODULE_PARM_DESC(remove, "Remove RXE device over network interface");
> +MODULE_PARM_DESC(remove, "DEPRECATED.  Remove RXE device over network interface");
> diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
> index f5b1e0ad6142..d7e39408c3af 100644
> --- a/drivers/infiniband/sw/rxe/rxe_verbs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
> @@ -1159,7 +1159,6 @@ int rxe_register_device(struct rxe_dev *rxe)
>  	struct ib_device *dev = &rxe->ib_dev;
>  	struct crypto_shash *tfm;
>
> -	strlcpy(dev->name, "rxe%d", IB_DEVICE_NAME_MAX);
>  	strlcpy(dev->node_desc, "rxe", sizeof(dev->node_desc));
>
>  	dev->owner = THIS_MODULE;
> --
> 1.8.3.1
>
Steve Wise Sept. 26, 2018, 3:41 p.m. UTC | #3
On 9/26/2018 3:45 AM, Leon Romanovsky wrote:

>> +	if (!ndev) {
>> +		pr_err("interface %s not found\n", ndev_name);
>> +		err = -ENODEV;
>> +		goto err;
>> +	}
>> +
>> +	if (net_to_rxe(ndev)) {
>> +		pr_err("already configured on %s\n", ndev_name);
> 
> Something general, not for this series, but we need to start to use
> extack to deliver errors and not dmesg.

extack is being used for the NEWLINK/DELLINK flow:

rdma link add rxe_enp4s0f0 type rxe dev foo0
link add: No such device

nltrace snipit:

netlink recv(3):
Setting msg proto to 20
--------------------------   BEGIN NETLINK MESSAGE
---------------------------
  [NETLINK HEADER] 16 octets
    .nlmsg_len = 76
    .type = 2 <ERROR>
    .flags = 0 <>
    .seq = 1537976444
    .port = 1718
  [ERRORMSG] 20 octets
    .error = -19 "No such device"
  [ORIGINAL MESSAGE] 16 octets
    .nlmsg_len = 16
    .type = 5135 <0x140f>
    .flags = 5 <REQUEST,ACK>
    .seq = 1537976444
    .port = 0
---------------------------  END NETLINK MESSAGE
---------------------------
Steve Wise Sept. 26, 2018, 3:54 p.m. UTC | #4
On 9/25/2018 3:18 PM, Jason Gunthorpe wrote:
> On Fri, Sep 14, 2018 at 11:43:55AM -0700, Steve Wise wrote:
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>>  drivers/infiniband/sw/rxe/rxe.c       | 61 +++++++++++++++++++++++++++++++++++
>>  drivers/infiniband/sw/rxe/rxe_net.c   |  3 +-
>>  drivers/infiniband/sw/rxe/rxe_net.h   |  2 +-
>>  drivers/infiniband/sw/rxe/rxe_sysfs.c |  6 ++--
>>  drivers/infiniband/sw/rxe/rxe_verbs.c |  1 -
>>  5 files changed, 67 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
>> index 10999fa69281..83f52954c300 100644
>> +++ b/drivers/infiniband/sw/rxe/rxe.c
>> @@ -31,6 +31,7 @@
>>   * SOFTWARE.
>>   */
>>  
>> +#include <rdma/rdma_netlink.h>
>>  #include <net/addrconf.h>
>>  #include "rxe.h"
>>  #include "rxe_loc.h"
>> @@ -340,6 +341,64 @@ void rxe_remove(struct rxe_dev *rxe)
>>  	rxe_dev_put(rxe);
>>  }
>>  
>> +static int rxe_newlink(char *ibdev_name, char *ndev_name)
> 
> const char *ndev_name

Right.

> 
>> +{
>> +	struct net_device *ndev = NULL;
>> +	struct rxe_dev *rxe;
>> +	int err = 0;
>> +
>> +	ndev = dev_get_by_name(&init_net, ndev_name);
>> +	if (!ndev) {
>> +		pr_err("interface %s not found\n", ndev_name);
>> +		err = -ENODEV;
>> +		goto err;
>> +	}
>> +
>> +	if (net_to_rxe(ndev)) {
>> +		pr_err("already configured on %s\n", ndev_name);
>> +		err = -EEXIST;
>> +		goto err;
>> +	}
>> +
>> +	rxe = rxe_net_add(ibdev_name, ndev);
>> +	if (!rxe) {
>> +		pr_err("failed to add %s\n", ndev_name);
>> +		err = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	if (netif_running(ndev) && netif_carrier_ok(ndev))
>> +		rxe_port_up(rxe);
>> +	else
>> +		rxe_port_down(rxe);
>> +	pr_info("added %s to %s\n", rxe->ib_dev.name, ndev->name);
>> +err:
>> +	if (ndev)
>> +		dev_put(ndev);
>> +	return err;
>> +}
>> +
>> +static int rxe_dellink(char *ibdev_name)
>> +{
>> +	struct rxe_dev *rxe;
>> +
>> +	rxe = get_rxe_by_name(ibdev_name);
>> +	if (!rxe) {
>> +		pr_err("not configured on %s\n", ibdev_name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	list_del(&rxe->list);
>> +	rxe_remove(rxe);
>> +	return 0;
>> +}
>> +
>> +static struct rdma_link_ops rxe_link_ops = {
>> +	.type = "rxe",
>> +	.newlink = rxe_newlink,
>> +	.dellink = rxe_dellink,
>> +};
>> +
>>  static int __init rxe_module_init(void)
>>  {
>>  	int err;
>> @@ -355,12 +414,14 @@ static int __init rxe_module_init(void)
>>  	if (err)
>>  		return err;
>>  
>> +	rdma_link_register(&rxe_link_ops);
>>  	pr_info("loaded\n");
>>  	return 0;
>>  }
>>  
>>  static void __exit rxe_module_exit(void)
>>  {
>> +	rdma_link_unregister(&rxe_link_ops);
>>  	rxe_remove_all();
>>  	rxe_net_exit();
>>  	rxe_cache_exit();
>> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
>> index 8094cbaa54a9..0c520999c8d4 100644
>> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
>> @@ -565,7 +565,7 @@ enum rdma_link_layer rxe_link_layer(struct rxe_dev *rxe, unsigned int port_num)
>>  	return IB_LINK_LAYER_ETHERNET;
>>  }
>>  
>> -struct rxe_dev *rxe_net_add(struct net_device *ndev)
>> +struct rxe_dev *rxe_net_add(char *ibdev_name, struct net_device *ndev)
>>  {
>>  	int err;
>>  	struct rxe_dev *rxe = NULL;
>> @@ -575,6 +575,7 @@ struct rxe_dev *rxe_net_add(struct net_device *ndev)
>>  		return NULL;
>>  
>>  	rxe->ndev = ndev;
>> +	strlcpy(rxe->ib_dev.name, ibdev_name, IB_DEVICE_NAME_MAX);
> 
> I'm trying to delete these, don't add new ones :)
> 
> The name needs to be passed to register. Maybe check that it doesn't
> have % in it in the core code too.

Yea ok.  I haven't rebased this on your or leon's work in the dev name
area...  The next version will.

> 
>>  	err = rxe_add(rxe, ndev->mtu);
>>  	if (err) {
>> diff --git a/drivers/infiniband/sw/rxe/rxe_net.h b/drivers/infiniband/sw/rxe/rxe_net.h
>> index 106c586dbb26..5e646039d393 100644
>> +++ b/drivers/infiniband/sw/rxe/rxe_net.h
>> @@ -43,7 +43,7 @@ struct rxe_recv_sockets {
>>  	struct socket *sk6;
>>  };
>>  
>> -struct rxe_dev *rxe_net_add(struct net_device *ndev);
>> +struct rxe_dev *rxe_net_add(char *ibdev_name, struct net_device *ndev);
>>  
>>  int rxe_net_init(void);
>>  void rxe_net_exit(void);
>> diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
>> index d5ed7571128f..1f82019ff8f3 100644
>> +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
>> @@ -97,7 +97,7 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
>>  		goto err;
>>  	}
>>  
>> -	rxe = rxe_net_add(ndev);
>> +	rxe = rxe_net_add(NULL, ndev);
> 
> Doesn't this crash?

It's a deterrent to discourage the use of the module options. :)

Steve.
Leon Romanovsky Sept. 26, 2018, 4:53 p.m. UTC | #5
On Wed, Sep 26, 2018 at 10:41:50AM -0500, Steve Wise wrote:
>
>
> On 9/26/2018 3:45 AM, Leon Romanovsky wrote:
>
> >> +	if (!ndev) {
> >> +		pr_err("interface %s not found\n", ndev_name);
> >> +		err = -ENODEV;
> >> +		goto err;
> >> +	}
> >> +
> >> +	if (net_to_rxe(ndev)) {
> >> +		pr_err("already configured on %s\n", ndev_name);
> >
> > Something general, not for this series, but we need to start to use
> > extack to deliver errors and not dmesg.
>
> extack is being used for the NEWLINK/DELLINK flow:
>
> rdma link add rxe_enp4s0f0 type rxe dev foo0
> link add: No such device
>
> nltrace snipit:
>
> netlink recv(3):
> Setting msg proto to 20
> --------------------------   BEGIN NETLINK MESSAGE
> ---------------------------
>   [NETLINK HEADER] 16 octets
>     .nlmsg_len = 76
>     .type = 2 <ERROR>
>     .flags = 0 <>
>     .seq = 1537976444
>     .port = 1718
>   [ERRORMSG] 20 octets
>     .error = -19 "No such device"
>   [ORIGINAL MESSAGE] 16 octets
>     .nlmsg_len = 16
>     .type = 5135 <0x140f>
>     .flags = 5 <REQUEST,ACK>
>     .seq = 1537976444
>     .port = 0
> ---------------------------  END NETLINK MESSAGE
> ---------------------------
>

Ohh, great, can we return "already configured on .." error via extack
and on not print it with pr_err()?

Thanks
Leon Romanovsky Sept. 26, 2018, 5:02 p.m. UTC | #6
On Wed, Sep 26, 2018 at 11:57:52AM -0500, Steve Wise wrote:
>
>
> On 9/26/2018 11:53 AM, Leon Romanovsky wrote:
> > On Wed, Sep 26, 2018 at 10:41:50AM -0500, Steve Wise wrote:
> >>
> >> On 9/26/2018 3:45 AM, Leon Romanovsky wrote:
> >>
> >>>> +	if (!ndev) {
> >>>> +		pr_err("interface %s not found\n", ndev_name);
> >>>> +		err = -ENODEV;
> >>>> +		goto err;
> >>>> +	}
> >>>> +
> >>>> +	if (net_to_rxe(ndev)) {
> >>>> +		pr_err("already configured on %s\n", ndev_name);
> >>> Something general, not for this series, but we need to start to use
> >>> extack to deliver errors and not dmesg.
> >> extack is being used for the NEWLINK/DELLINK flow:
> >>
> >> rdma link add rxe_enp4s0f0 type rxe dev foo0
> >> link add: No such device
> >>
> >> nltrace snipit:
> >>
> >> netlink recv(3):
> >> Setting msg proto to 20
> >> --------------------------   BEGIN NETLINK MESSAGE
> >> ---------------------------
> >>   [NETLINK HEADER] 16 octets
> >>     .nlmsg_len = 76
> >>     .type = 2 <ERROR>
> >>     .flags = 0 <>
> >>     .seq = 1537976444
> >>     .port = 1718
> >>   [ERRORMSG] 20 octets
> >>     .error = -19 "No such device"
> >>   [ORIGINAL MESSAGE] 16 octets
> >>     .nlmsg_len = 16
> >>     .type = 5135 <0x140f>
> >>     .flags = 5 <REQUEST,ACK>
> >>     .seq = 1537976444
> >>     .port = 0
> >> ---------------------------  END NETLINK MESSAGE
> >> ---------------------------
> >>
> > Ohh, great, can we return "already configured on .." error via extack
> > and on not print it with pr_err()?
>
> I'm not sure yet how this works.  I just see that if the nldev_newlink()
> returns a negative errno, it ends up in this ERROR nlmsg and the error
> string is automatically added.  Still learning this stuff. :)

See net/code/devlink.c code, they added support to return error messages
through extack not too long ago.

BTW, it is not related to this series, just general comment :)

>
>
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
index 10999fa69281..83f52954c300 100644
--- a/drivers/infiniband/sw/rxe/rxe.c
+++ b/drivers/infiniband/sw/rxe/rxe.c
@@ -31,6 +31,7 @@ 
  * SOFTWARE.
  */
 
+#include <rdma/rdma_netlink.h>
 #include <net/addrconf.h>
 #include "rxe.h"
 #include "rxe_loc.h"
@@ -340,6 +341,64 @@  void rxe_remove(struct rxe_dev *rxe)
 	rxe_dev_put(rxe);
 }
 
+static int rxe_newlink(char *ibdev_name, char *ndev_name)
+{
+	struct net_device *ndev = NULL;
+	struct rxe_dev *rxe;
+	int err = 0;
+
+	ndev = dev_get_by_name(&init_net, ndev_name);
+	if (!ndev) {
+		pr_err("interface %s not found\n", ndev_name);
+		err = -ENODEV;
+		goto err;
+	}
+
+	if (net_to_rxe(ndev)) {
+		pr_err("already configured on %s\n", ndev_name);
+		err = -EEXIST;
+		goto err;
+	}
+
+	rxe = rxe_net_add(ibdev_name, ndev);
+	if (!rxe) {
+		pr_err("failed to add %s\n", ndev_name);
+		err = -EINVAL;
+		goto err;
+	}
+
+	if (netif_running(ndev) && netif_carrier_ok(ndev))
+		rxe_port_up(rxe);
+	else
+		rxe_port_down(rxe);
+	pr_info("added %s to %s\n", rxe->ib_dev.name, ndev->name);
+err:
+	if (ndev)
+		dev_put(ndev);
+	return err;
+}
+
+static int rxe_dellink(char *ibdev_name)
+{
+	struct rxe_dev *rxe;
+
+	rxe = get_rxe_by_name(ibdev_name);
+	if (!rxe) {
+		pr_err("not configured on %s\n", ibdev_name);
+		return -ENODEV;
+	}
+
+	list_del(&rxe->list);
+	rxe_remove(rxe);
+	return 0;
+}
+
+static struct rdma_link_ops rxe_link_ops = {
+	.type = "rxe",
+	.newlink = rxe_newlink,
+	.dellink = rxe_dellink,
+};
+
 static int __init rxe_module_init(void)
 {
 	int err;
@@ -355,12 +414,14 @@  static int __init rxe_module_init(void)
 	if (err)
 		return err;
 
+	rdma_link_register(&rxe_link_ops);
 	pr_info("loaded\n");
 	return 0;
 }
 
 static void __exit rxe_module_exit(void)
 {
+	rdma_link_unregister(&rxe_link_ops);
 	rxe_remove_all();
 	rxe_net_exit();
 	rxe_cache_exit();
diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index 8094cbaa54a9..0c520999c8d4 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -565,7 +565,7 @@  enum rdma_link_layer rxe_link_layer(struct rxe_dev *rxe, unsigned int port_num)
 	return IB_LINK_LAYER_ETHERNET;
 }
 
-struct rxe_dev *rxe_net_add(struct net_device *ndev)
+struct rxe_dev *rxe_net_add(char *ibdev_name, struct net_device *ndev)
 {
 	int err;
 	struct rxe_dev *rxe = NULL;
@@ -575,6 +575,7 @@  struct rxe_dev *rxe_net_add(struct net_device *ndev)
 		return NULL;
 
 	rxe->ndev = ndev;
+	strlcpy(rxe->ib_dev.name, ibdev_name, IB_DEVICE_NAME_MAX);
 
 	err = rxe_add(rxe, ndev->mtu);
 	if (err) {
diff --git a/drivers/infiniband/sw/rxe/rxe_net.h b/drivers/infiniband/sw/rxe/rxe_net.h
index 106c586dbb26..5e646039d393 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.h
+++ b/drivers/infiniband/sw/rxe/rxe_net.h
@@ -43,7 +43,7 @@  struct rxe_recv_sockets {
 	struct socket *sk6;
 };
 
-struct rxe_dev *rxe_net_add(struct net_device *ndev);
+struct rxe_dev *rxe_net_add(char *ibdev_name, struct net_device *ndev);
 
 int rxe_net_init(void);
 void rxe_net_exit(void);
diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
index d5ed7571128f..1f82019ff8f3 100644
--- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
+++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
@@ -97,7 +97,7 @@  static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
 		goto err;
 	}
 
-	rxe = rxe_net_add(ndev);
+	rxe = rxe_net_add(NULL, ndev);
 	if (!rxe) {
 		pr_err("failed to add %s\n", intf);
 		err = -EINVAL;
@@ -152,6 +152,6 @@  static int rxe_param_set_remove(const char *val, const struct kernel_param *kp)
 };
 
 module_param_cb(add, &rxe_add_ops, NULL, 0200);
-MODULE_PARM_DESC(add, "Create RXE device over network interface");
+MODULE_PARM_DESC(add, "DEPRECATED.  Create RXE device over network interface");
 module_param_cb(remove, &rxe_remove_ops, NULL, 0200);
-MODULE_PARM_DESC(remove, "Remove RXE device over network interface");
+MODULE_PARM_DESC(remove, "DEPRECATED.  Remove RXE device over network interface");
diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c
index f5b1e0ad6142..d7e39408c3af 100644
--- a/drivers/infiniband/sw/rxe/rxe_verbs.c
+++ b/drivers/infiniband/sw/rxe/rxe_verbs.c
@@ -1159,7 +1159,6 @@  int rxe_register_device(struct rxe_dev *rxe)
 	struct ib_device *dev = &rxe->ib_dev;
 	struct crypto_shash *tfm;
 
-	strlcpy(dev->name, "rxe%d", IB_DEVICE_NAME_MAX);
 	strlcpy(dev->node_desc, "rxe", sizeof(dev->node_desc));
 
 	dev->owner = THIS_MODULE;