OK, one major issue with this patch and a few minor nits.
First, the major issue is that I don't see anything in the patch that
changes the code in ehca_mem_notifier() in ehca_main.c:
case MEM_GOING_ONLINE:
case MEM_GOING_OFFLINE:
/* only ok if no hca is attached to the lpar */
spin_lock_irqsave(&shca_list_lock, flags);
if (list_empty(&shca_list)) {
spin_unlock_irqrestore(&shca_list_lock, flags);
return NOTIFY_OK;
} else {
spin_unlock_irqrestore(&shca_list_lock, flags);
if (printk_timed_ratelimit(&ehca_dmem_warn_time,
30 * 1000))
ehca_gen_err("DMEM operations are not allowed"
"in conjunction with eHCA");
return NOTIFY_BAD;
}
But your patch description says:
> This patch implements toleration of dynamic memory operations....
But it seems you're still going to hit the same NOTIFY_BAD case above
after your patch. So something doesn't compute for me. Could you
explain more?
Second, a nit:
> +#define EHCA_REG_MR 0
> +#define EHCA_REG_BUSMAP_MR (~0)
and you pass these as the reg_busmap parm in:
> int ehca_reg_mr(struct ehca_shca *shca,
> struct ehca_mr *e_mr,
> u64 *iova_start,
> @@ -991,7 +1031,8 @@
> struct ehca_pd *e_pd,
> struct ehca_mr_pginfo *pginfo,
> u32 *lkey, /*OUT*/
> - u32 *rkey) /*OUT*/
> + u32 *rkey, /*OUT*/
> + int reg_busmap)
and test it as:
> + if (reg_busmap)
> + ret = ehca_reg_bmap_mr_rpages(shca, e_mr, pginfo);
> + else
> + ret = ehca_reg_mr_rpages(shca, e_mr, pginfo);
So the ~0 for true looks a bit odd. One option would be to make
reg_busmap a bool, since that's how you're using it, but then you lose
the nice self-documenting macro where you call things.
So I think it would be cleaner to do something like
enum ehca_reg_type {
EHCA_REG_MR,
EHCA_REG_BUSMAP_MR
};
and make the "int reg_busmap" parameter into "enum ehca_reg_type reg_type"
and have the code become
+ if (reg_type == EHCA_REG_BUSMAP_MR)
+ ret = ehca_reg_bmap_mr_rpages(shca, e_mr, pginfo);
+ else if (reg_type == EHCA_REG_MR)
+ ret = ehca_reg_mr_rpages(shca, e_mr, pginfo);
+ else
+ ret = -EINVAL
or something like that.
> +struct ib_dma_mapping_ops ehca_dma_mapping_ops = {
> + .mapping_error = ehca_dma_mapping_error,
> + .map_single = ehca_dma_map_single,
> + .unmap_single = ehca_dma_unmap_single,
> + .map_page = ehca_dma_map_page,
> + .unmap_page = ehca_dma_unmap_page,
> + .map_sg = ehca_dma_map_sg,
> + .unmap_sg = ehca_dma_unmap_sg,
> + .dma_address = ehca_dma_address,
> + .dma_len = ehca_dma_len,
> + .sync_single_for_cpu = ehca_dma_sync_single_for_cpu,
> + .sync_single_for_device = ehca_dma_sync_single_for_device,
> + .alloc_coherent = ehca_dma_alloc_coherent,
> + .free_coherent = ehca_dma_free_coherent,
> +};
I always think structures like this are easier to read if you align the
'=' signs. But no big deal.
> + ret = ehca_create_busmap();
> + if (ret) {
> + ehca_gen_err("Cannot create busmap.");
> + goto module_init2;
> + }
> +
> ret = ibmebus_register_driver(&ehca_driver);
> if (ret) {
> ehca_gen_err("Cannot register eHCA device driver");
> ret = -EINVAL;
> - goto module_init2;
> + goto module_init3;
> }
>
> ret = register_memory_notifier(&ehca_mem_nb);
> if (ret) {
> ehca_gen_err("Failed registering memory add/remove notifier");
> - goto module_init3;
> + goto module_init4;
Having to renumber unrelated things is when something changes is why I
don't like this style of error path labels. But I think it's well and
truly too late to fix that in ehca.
- R.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
