On Fri, Sep 14, 2018 at 6:16 AM, Christoph Hellwig hch@lst.de wrote:
An argument could be made to require that the ->kill() operation be set in the @pgmap arg rather than passed in separately. However, it helps code readability, tracking the lifetime of a given instance, to be able to grep the kill routine directly at the devm_memremap_pages() call site.
I generally do not like passing redundant argument, and I don't really see why this case is different. Or in other ways I'd like to make your above argument..
Logan had similar feedback, and now the chorus is getting louder. I personally like how I can do this with grep:
drivers/dax/pmem.c:114: addr = devm_memremap_pages(dev, &dax_pmem->pgmap, dax_pmem_percpu_kill); -- drivers/nvdimm/pmem.c:411: addr = devm_memremap_pages(dev, &pmem->pgmap, drivers/nvdimm/pmem.c-412- pmem_freeze_queue); -- drivers/nvdimm/pmem.c:425: addr = devm_memremap_pages(dev, &pmem->pgmap, drivers/nvdimm/pmem.c-426- pmem_freeze_queue); -- mm/hmm.c:1059: result = devm_memremap_pages(devmem->device, &devmem->pagemap, mm/hmm.c-1060- hmm_devmem_ref_kill); -- mm/hmm.c:1113: result = devm_memremap_pages(devmem->device, &devmem->pagemap, mm/hmm.c-1114- hmm_devmem_ref_kill);
...and see all of the kill() variants, but the redundancy will likely continue to bother folks.
Except for that the patch looks good to me.
Thanks, I'll fix it up to drop the redundant arg.