-----"Jason Gunthorpe" <jgg@xxxxxxxx> wrote: -----
>To: bmt@xxxxxxxxxxxxxx
>From: "Jason Gunthorpe" <jgg@xxxxxxxx>
>Date: 01/31/2019 05:55AM
>Cc: linux-rdma@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v4 03/13] SIW network and RDMA core interface
>
>On Wed, Jan 30, 2019 at 06:21:26PM +0100, bmt@xxxxxxxxxxxxxx wrote:
>
>> +static int siw_modify_port(struct ib_device *base_dev, u8 port,
>int mask,
>> + struct ib_port_modify *props)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>
>Drivers should not unconditionally return EOPNOTSUPP, just don't set
>the ops pointer. For all of these cases.
OK, I did that when some older RDMA midlayer version
unconditionally called those functions, even when NULL.
Will remove that.
>
>> +static struct net_device *siw_get_netdev(struct ib_device
>*base_dev, u8 port)
>> +{
>> + struct siw_device *sdev = siw_dev_base2siw(base_dev);
>> +
>> + if (!sdev->netdev)
>> + return NULL;
>> +
>> + dev_hold(sdev->netdev);
>> +
>> + return sdev->netdev;
>> +}
>
>Since this is based on my series it should not provid get_netdev, but
>use the new set_netdev flow.
OK, I will look into that and fix.
>> +static void siw_unregistered(struct ib_device *base_dev)
>> +{
>> + struct siw_device *sdev = siw_dev_base2siw(base_dev);
>> +
>> + siw_device_cleanup(sdev);
>> + siw_device_destroy(sdev);
>> +}
>> +
>> +static struct siw_device *siw_device_create(struct net_device
>*netdev)
>> +{
>> + struct siw_device *sdev;
>> + struct ib_device *base_dev;
>> + struct device *parent = netdev->dev.parent;
>> +
>> + sdev = (struct siw_device *)ib_alloc_device(sizeof(*sdev));
>> + if (!sdev)
>> + goto out;
>> +
>> + base_dev = &sdev->base_dev;
>> +
>> + base_dev->driver_unregister = siw_unregistered;
>
>This is some weird branch this is based on .. This is in ops in the
>latest revision of my series
OK., I think I have to rebase.
>
>> + base_dev->phys_port_cnt = 1;
>> +
>> + base_dev->dev.parent = parent;
>> + base_dev->dev.dma_ops = &dma_virt_ops;
>> +
>> + base_dev->num_comp_vectors = num_possible_cpus();
>> + base_dev->ops.query_device = siw_query_device;
>> + base_dev->ops.query_port = siw_query_port;
>> + base_dev->ops.get_port_immutable = siw_get_port_immutable;
>> + base_dev->ops.get_netdev = siw_get_netdev;
>> + base_dev->ops.query_qp = siw_query_qp;
>> + base_dev->ops.modify_port = siw_modify_port;
>> + base_dev->ops.query_pkey = siw_query_pkey;
>> + base_dev->ops.query_gid = siw_query_gid;
>> + base_dev->ops.alloc_ucontext = siw_alloc_ucontext;
>> + base_dev->ops.dealloc_ucontext = siw_dealloc_ucontext;
>> + base_dev->ops.mmap = siw_mmap;
>> + base_dev->ops.alloc_pd = siw_alloc_pd;
>> + base_dev->ops.dealloc_pd = siw_dealloc_pd;
>> + base_dev->ops.create_ah = siw_create_ah;
>> + base_dev->ops.destroy_ah = siw_destroy_ah;
>> + base_dev->ops.create_qp = siw_create_qp;
>> + base_dev->ops.modify_qp = siw_verbs_modify_qp;
>> + base_dev->ops.destroy_qp = siw_destroy_qp;
>> + base_dev->ops.create_cq = siw_create_cq;
>> + base_dev->ops.destroy_cq = siw_destroy_cq;
>> + base_dev->ops.resize_cq = NULL;
>> + base_dev->ops.poll_cq = siw_poll_cq;
>> + base_dev->ops.get_dma_mr = siw_get_dma_mr;
>> + base_dev->ops.reg_user_mr = siw_reg_user_mr;
>> + base_dev->ops.dereg_mr = siw_dereg_mr;
>> + base_dev->ops.alloc_mr = siw_alloc_mr;
>> + base_dev->ops.map_mr_sg = siw_map_mr_sg;
>> + base_dev->ops.dealloc_mw = NULL;
>> +
>> + base_dev->ops.create_srq = siw_create_srq;
>> + base_dev->ops.modify_srq = siw_modify_srq;
>> + base_dev->ops.query_srq = siw_query_srq;
>> + base_dev->ops.destroy_srq = siw_destroy_srq;
>> + base_dev->ops.post_srq_recv = siw_post_srq_recv;
>> +
>> + base_dev->ops.attach_mcast = NULL;
>> + base_dev->ops.detach_mcast = NULL;
>> + base_dev->ops.process_mad = siw_no_mad;
>> +
>> + base_dev->ops.req_notify_cq = siw_req_notify_cq;
>> + base_dev->ops.post_send = siw_post_send;
>> + base_dev->ops.post_recv = siw_post_receive;
>> +
>> + base_dev->ops.drain_sq = siw_verbs_sq_flush;
>> + base_dev->ops.drain_rq = siw_verbs_rq_flush;
>
>Nope. Use ops properly like all other drivers.
Ups. was not aware of ib_set_device_ops(). Will fix...
>
>> +static void siw_netdev_unregistered(struct work_struct *work)
>> +{
>> + struct siw_device *sdev = container_of(work, struct siw_device,
>> + netdev_unregister);
>> +
>> + struct siw_qp_attrs qp_attrs;
>> + struct list_head *pos, *tmp;
>> +
>> + memset(&qp_attrs, 0, sizeof(qp_attrs));
>> + qp_attrs.state = SIW_QP_STATE_ERROR;
>> +
>> + /*
>> + * Mark all current QP's of this device dead
>> + */
>> + list_for_each_safe(pos, tmp, &sdev->qp_list) {
>> + struct siw_qp *qp = list_entry(pos, struct siw_qp, devq);
>> +
>> + down_write(&qp->state_lock);
>> + (void) siw_qp_modify(qp, &qp_attrs, SIW_QP_ATTR_STATE);
>> + up_write(&qp->state_lock);
>> + }
>
>Don't put code around ib_unregister. If you need to do things use a
>callback at the right moment in the unregister flow. You might need
>to
>add a callback (see my latest series)
I thought by having that executed in a work queue context only
we'd be safe. I have to bring down all related QP's if a network device
suddenly disappears... Looks like I have to better understand the
intended unregister flow.
>
>> + ib_unregister_device_and_put(&sdev->base_dev);
>
>A 'put' probably should not be held into a WQ, this is a good way to
>deadlock.
>
>This shared pattern with rxe also needs to move into the core code, I
>think.
>
>> +}
>> +
>> +static int siw_netdev_event(struct notifier_block *nb, unsigned
>long event,
>> + void *arg)
>> +{
>> + struct net_device *netdev = netdev_notifier_info_to_dev(arg);
>> + struct siw_device *sdev;
>> +
>> + dev_dbg(&netdev->dev, "siw: event %lu\n", event);
>> +
>> + if (dev_net(netdev) != &init_net)
>> + return NOTIFY_OK;
>> +
>> + sdev = siw_dev_from_netdev(netdev);
>
>It seems like obfuscation to wrapper ib_device_get_by_netdev() like
>this..
OK, let me better make that explicit then.
>
>> + pr_info("SoftiWARP attached\n");
>
>Lets not print stuff like this during module load
>
OK.
>You should also largely run the whole thing through clang-format
>
Will do.
I used sparse and checkpatch so far. I thought that would
make it for kernel patches... (obviously lots of
RDMA midlayer code made it even w/o those checks ;) ).
Thanks a lot!
Bernard.
