[openib-general] Re: [PATCH] SRP: don't use TX IU after freeing it

[Roland Dreier]

OK, a few trivial comments:

    > +struct srp_device_data {
    > +	struct list_head       *dev_list;
    > +	struct ib_pd	       *pd;
    > +	struct ib_mr	       *mr;
    > +	struct ib_fmr_pool     *fmr_pool;
    > +};

Why put a pointer to struct list_head here instead of just a struct
list_head?  If you just used the struct, then you wouldn't need this:

    > +	srp_data->dev_list = kmalloc(sizeof *srp_data->dev_list, GFP_KERNEL);
    > +	if (!srp_data->dev_list)
    > +		goto free_params_attr;

    > @@ -94,10 +115,14 @@ struct srp_request {
    >  	struct scsi_cmnd       *scmnd;
    >  	struct srp_iu	       *cmd;
    >  	struct srp_iu	       *tsk_mgmt;
    > +	DECLARE_PCI_UNMAP_ADDR(direct_mapping)
    >  	struct completion	done;
    >  	short			next;
    >  	u8			cmd_done;
    >  	u8			tsk_status;
    > +	struct srp_fmr	       *fmr_arr;
    > +	u16			fmr_cnt;
    > +	u16			in_use;
    >  };

I can't find anywhere that the in_use flag is used.

    > +static int srp_map_fmr(struct srp_target_port *target, struct scatterlist *scat,
    > +		       int sg_cnt, struct srp_request *req)

    [...]

    > +	return -ENOMEM;

    > +			} else if (fmr_cnt <= 0) {

fmr_cnt is unsigned so I think this is going to get you in trouble.
Might as well make fmr_cnt a plain int to make things simpler.

Also, it might be good to try and add some more comments explaining
srp_map_fmr() -- it would definitely help me review.

 - R.