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.
> +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.
> +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
> + 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.
> +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)
> + 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..
> + pr_info("SoftiWARP attached\n");
Lets not print stuff like this during module load
You should also largely run the whole thing through clang-format
Jason
