-----"Jason Gunthorpe" <jgg@xxxxxxxx> wrote: -----
>To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>From: "Jason Gunthorpe" <jgg@xxxxxxxx>
>Date: 01/31/2019 06:22PM
>Cc: linux-rdma@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v4 03/13] SIW network and RDMA core interface
>
>On Thu, Jan 31, 2019 at 03:24:01PM +0000, Bernard Metzler wrote:
>
>> >> +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.
>
>If it is important it should be done during module unload and link
>removal, if it is not important, it shouldn't be done :)
If siw catches a NETDEV_UNREGISTER event, it shall first
close all related connection endpoints before the ib_device
can go away.
Following your new device management design, I would now
first schedule local work to cleanup those siw private
resources, and later ib_unregister_device_queued(),
or is it OK to do that local cleanup stuff within the
dealloc_driver() method?
Thanks!
Bernard
>
>The netdev will not fully unregister until the IB device unregisters
>because of the set_netdev thing.
>
>rxe and siw may need to support 'device disassociate' to be fully
>correct.
>
>> >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 ;) ).
>
>It is good, take all of these tools with a grain of salt, they are
>sort of 90% useful
>
>I run every patch though checkpatch as I apply it.
>
>Jason
>
>
