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.
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
