On 12/18/2018 2:40 PM, Jason Gunthorpe wrote:
> On Fri, Dec 14, 2018 at 08:05:57AM -0800, Steve Wise wrote:
>> Add support for the RDMA_NLDEV_CMD_NEWLINK/DELLINK messages which allow
>> dynamically adding new RXE links. Deprecate the old module options
>> for now.
>>
>> Cc: Moni Shoua <monis@xxxxxxxxxxxx>
>> Reviewed-by: Yanjun Zhu <yanjun.zhu@xxxxxxxxxx>
>> Signed-off-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
>> drivers/infiniband/sw/rxe/rxe.c | 39 +++++++++++++++++++++++++++++++++--
>> drivers/infiniband/sw/rxe/rxe.h | 2 +-
>> drivers/infiniband/sw/rxe/rxe_net.c | 4 ++--
>> drivers/infiniband/sw/rxe/rxe_net.h | 2 +-
>> drivers/infiniband/sw/rxe/rxe_param.h | 3 ++-
>> drivers/infiniband/sw/rxe/rxe_sysfs.c | 6 +++---
>> drivers/infiniband/sw/rxe/rxe_verbs.c | 4 ++--
>> drivers/infiniband/sw/rxe/rxe_verbs.h | 2 +-
>> 8 files changed, 49 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
>> index 91ad339fd6e1..07fea762f0ea 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"
>> @@ -301,7 +302,7 @@ void rxe_set_mtu(struct rxe_dev *rxe, unsigned int ndev_mtu)
>> /* called by ifc layer to create new rxe device.
>> * The caller should allocate memory for rxe by calling ib_alloc_device.
>> */
>> -int rxe_add(struct rxe_dev *rxe, unsigned int mtu)
>> +int rxe_add(struct rxe_dev *rxe, unsigned int mtu, const char *ibdev_name)
>> {
>> int err;
>>
>> @@ -311,9 +312,39 @@ int rxe_add(struct rxe_dev *rxe, unsigned int mtu)
>>
>> rxe_set_mtu(rxe, mtu);
>>
>> - return rxe_register_device(rxe);
>> + return rxe_register_device(rxe, ibdev_name);
>> }
>>
>> +static int rxe_newlink(const char *ibdev_name, struct net_device *ndev)
>> +{
>> + struct rxe_dev *rxe = rxe_get_dev_from_net(ndev);
>> + int err = 0;
>> +
>> + if (rxe) {
>> + pr_err("already configured on %s\n", ndev->name);
>> + err = -EEXIST;
>> + ib_device_put(&rxe->ib_dev);
>> + goto err;
>> + }
>> +
>> + rxe = rxe_net_add(ibdev_name, ndev);
>> + if (!rxe) {
>> + pr_err("failed to add %s\n", ndev->name);
>> + err = -EINVAL;
>> + goto err;
>> + }
>> +
>> + rxe_set_port_state(ndev, rxe);
> Hurm. So there is another pre-existing problem here..
>
> ib_register_device creates a device and that immediately becomes
> available to be unregistered - ie this can race with a netdev notifier
> tearing down the netdev.
>
> So, rxe_register_device/net_add can't return a pointer and we can't
> call rxe_set_port_state like this.
>
> rxe_set_port_state has to be done before registration or as part of
> some kind of core code callback setting up the device, or we have to
> do more stuff with refcounting.
ib_register_device() could take a kref on the ib_device before it
becomes available to be unregistered. Then the callers could be
required to ib_device_put() at the end of these sorts of operations. Maybe?
> Otherwise I think the two netlink patches are looking great now. It is
> unfortunate Parav discovered my patch is all busted up. :(
