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. _______________________________________________ ewg mailing list ewg@lists.openfabrics.org http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ewg