Message ID | 01eba3c86137eb348f8cce69f500222bd7c72c57.1635058203.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
---|---|
State | Not Applicable |
Delegated to: | Lorenzo Pieralisi |
Headers | show |
Series | PCI: endpoint: Use 'bitmap_zalloc()' when applicable | expand |
Hi Christophe, > 'mem->bitmap' is a bitmap. So use 'bitmap_zalloc()' to simplify code, > improve the semantic and avoid some open-coded arithmetic in allocator > arguments. > > Also change the corresponding 'kfree()' into 'bitmap_free()' to keep > consistency. Thank you! > Finally, while at it, axe the useless 'bitmap' variable and use > 'mem->bitmap' directly. Personally, I would keep the bitmap variable - this might be what Bjorn would also prefer, as I believe he prefers not to store what is a "failed" state of sorts in a target variable directly, if memory serves me right. [...] > @@ -49,10 +49,8 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, > unsigned int num_windows) > { > struct pci_epc_mem *mem = NULL; > - unsigned long *bitmap = NULL; > unsigned int page_shift; > size_t page_size; > - int bitmap_size; > int pages; > int ret; > int i; > @@ -72,7 +70,6 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, > page_size = PAGE_SIZE; > page_shift = ilog2(page_size); > pages = windows[i].size >> page_shift; > - bitmap_size = BITS_TO_LONGS(pages) * sizeof(long); > > mem = kzalloc(sizeof(*mem), GFP_KERNEL); > if (!mem) { > @@ -81,8 +78,8 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, > goto err_mem; > } > > - bitmap = kzalloc(bitmap_size, GFP_KERNEL); > - if (!bitmap) { > + mem->bitmap = bitmap_zalloc(pages, GFP_KERNEL); > + if (!mem->bitmap) { > ret = -ENOMEM; > kfree(mem); > i--; > @@ -92,7 +89,6 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, > mem->window.phys_base = windows[i].phys_base; > mem->window.size = windows[i].size; > mem->window.page_size = page_size; > - mem->bitmap = bitmap; > mem->pages = pages; > mutex_init(&mem->lock); > epc->windows[i] = mem; > @@ -106,7 +102,7 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, > err_mem: > for (; i >= 0; i--) { > mem = epc->windows[i]; > - kfree(mem->bitmap); > + bitmap_free(mem->bitmap); > kfree(mem); > } > kfree(epc->windows); > @@ -145,7 +141,7 @@ void pci_epc_mem_exit(struct pci_epc *epc) > > for (i = 0; i < epc->num_windows; i++) { > mem = epc->windows[i]; > - kfree(mem->bitmap); > + bitmap_free(mem->bitmap); > kfree(mem); > } > kfree(epc->windows); Thank you! Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Krzysztof
Le 06/11/2021 à 23:01, Krzysztof Wilczyński a écrit : > Hi Christophe, > >> 'mem->bitmap' is a bitmap. So use 'bitmap_zalloc()' to simplify code, >> improve the semantic and avoid some open-coded arithmetic in allocator >> arguments. >> >> Also change the corresponding 'kfree()' into 'bitmap_free()' to keep >> consistency. > > Thank you! > >> Finally, while at it, axe the useless 'bitmap' variable and use >> 'mem->bitmap' directly. > > Personally, I would keep the bitmap variable - this might be what Bjorn > would also prefer, as I believe he prefers not to store what is a "failed" > state of sorts in a target variable directly, if memory serves me right. Hi, mostly a mater of taste. On another similar patch I got the answer in [1] :). 'mem' is kzalloc'ed, so in case of failure, here we are just replacing NULL by NULL. Let me know the preferred style here and if I should send a V2. CJ [1]; https://lore.kernel.org/kernel-janitors/20211028164437.GA4045120@p14s/ > > [...] >> @@ -49,10 +49,8 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, >> unsigned int num_windows) >> { >> struct pci_epc_mem *mem = NULL; >> - unsigned long *bitmap = NULL; >> unsigned int page_shift; >> size_t page_size; >> - int bitmap_size; >> int pages; >> int ret; >> int i; >> @@ -72,7 +70,6 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, >> page_size = PAGE_SIZE; >> page_shift = ilog2(page_size); >> pages = windows[i].size >> page_shift; >> - bitmap_size = BITS_TO_LONGS(pages) * sizeof(long); >> >> mem = kzalloc(sizeof(*mem), GFP_KERNEL); >> if (!mem) { >> @@ -81,8 +78,8 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, >> goto err_mem; >> } >> >> - bitmap = kzalloc(bitmap_size, GFP_KERNEL); >> - if (!bitmap) { >> + mem->bitmap = bitmap_zalloc(pages, GFP_KERNEL); >> + if (!mem->bitmap) { >> ret = -ENOMEM; >> kfree(mem); >> i--; >> @@ -92,7 +89,6 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, >> mem->window.phys_base = windows[i].phys_base; >> mem->window.size = windows[i].size; >> mem->window.page_size = page_size; >> - mem->bitmap = bitmap; >> mem->pages = pages; >> mutex_init(&mem->lock); >> epc->windows[i] = mem; >> @@ -106,7 +102,7 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, >> err_mem: >> for (; i >= 0; i--) { >> mem = epc->windows[i]; >> - kfree(mem->bitmap); >> + bitmap_free(mem->bitmap); >> kfree(mem); >> } >> kfree(epc->windows); >> @@ -145,7 +141,7 @@ void pci_epc_mem_exit(struct pci_epc *epc) >> >> for (i = 0; i < epc->num_windows; i++) { >> mem = epc->windows[i]; >> - kfree(mem->bitmap); >> + bitmap_free(mem->bitmap); >> kfree(mem); >> } >> kfree(epc->windows); > > Thank you! > > Reviewed-by: Krzysztof Wilczyński <kw@linux.com> > > Krzysztof >
diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c index a97b56a6d2db..b15a264f19af 100644 --- a/drivers/pci/endpoint/pci-epc-mem.c +++ b/drivers/pci/endpoint/pci-epc-mem.c @@ -49,10 +49,8 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, unsigned int num_windows) { struct pci_epc_mem *mem = NULL; - unsigned long *bitmap = NULL; unsigned int page_shift; size_t page_size; - int bitmap_size; int pages; int ret; int i; @@ -72,7 +70,6 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, page_size = PAGE_SIZE; page_shift = ilog2(page_size); pages = windows[i].size >> page_shift; - bitmap_size = BITS_TO_LONGS(pages) * sizeof(long); mem = kzalloc(sizeof(*mem), GFP_KERNEL); if (!mem) { @@ -81,8 +78,8 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, goto err_mem; } - bitmap = kzalloc(bitmap_size, GFP_KERNEL); - if (!bitmap) { + mem->bitmap = bitmap_zalloc(pages, GFP_KERNEL); + if (!mem->bitmap) { ret = -ENOMEM; kfree(mem); i--; @@ -92,7 +89,6 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, mem->window.phys_base = windows[i].phys_base; mem->window.size = windows[i].size; mem->window.page_size = page_size; - mem->bitmap = bitmap; mem->pages = pages; mutex_init(&mem->lock); epc->windows[i] = mem; @@ -106,7 +102,7 @@ int pci_epc_multi_mem_init(struct pci_epc *epc, err_mem: for (; i >= 0; i--) { mem = epc->windows[i]; - kfree(mem->bitmap); + bitmap_free(mem->bitmap); kfree(mem); } kfree(epc->windows); @@ -145,7 +141,7 @@ void pci_epc_mem_exit(struct pci_epc *epc) for (i = 0; i < epc->num_windows; i++) { mem = epc->windows[i]; - kfree(mem->bitmap); + bitmap_free(mem->bitmap); kfree(mem); } kfree(epc->windows);
'mem->bitmap' is a bitmap. So use 'bitmap_zalloc()' to simplify code, improve the semantic and avoid some open-coded arithmetic in allocator arguments. Also change the corresponding 'kfree()' into 'bitmap_free()' to keep consistency. Finally, while at it, axe the useless 'bitmap' variable and use 'mem->bitmap' directly. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/pci/endpoint/pci-epc-mem.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)