On Tue, May 11, 2021 at 11:22:12AM +0300, Leon Romanovsky wrote:
> @@ -2139,6 +2197,8 @@ static int cm_req_handler(struct cm_work *work)
> sa_path_set_dmac(&work->path[0],
> cm_id_priv->av.ah_attr.roce.dmac);
> work->path[0].hop_limit = grh->hop_limit;
> +
> + cm_destroy_av(&cm_id_priv->av);
> ret = cm_init_av_by_path(&work->path[0], gid_attr, &cm_id_priv->av);
> if (ret) {
> int err;
Why add cm_destroy_av() here? The cm_id_priv was freshly created at
the top of this function and hasn't left the stack frame yet?
> @@ -4419,12 +4486,19 @@ static void cm_remove_one(struct ib_device *ib_device, void *client_data)
> * after that we can call the unregister_mad_agent
> */
> flush_workqueue(cm.wq);
> - ib_unregister_mad_agent(port->mad_agent);
> + /*
> + * The above ensures no call paths from the work are running,
> + * the remaining paths all take the unregistration lock
"unregistration lock" is "mad_agent_lock"
> + */
> + spin_lock(&cm_dev->mad_agent_lock);
> + port->mad_agent = NULL;
> + spin_unlock(&cm_dev->mad_agent_lock);
> + ib_unregister_mad_agent(mad_agent);
> cm_remove_port_fs(port);
> - kfree(port);
> }
>
> - kfree(cm_dev);
> + /* All touches can only be on call path from the work */
Not sure anymore what this comment means, the work was flushed? I
think it is saying all touches can only be on a place outside the
work.
Other than these little details it all looks OK
Jason
