Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 10 Apr 2024 10:36:16 +0200 | Subject | Re: [PATCH net-next v8 2/3] net: ethernet: ti: Add desc_infos member to struct k3_cppi_desc_pool | From | Julien Panis <> |
| |
On 4/10/24 02:39, Jakub Kicinski wrote: > On Mon, 08 Apr 2024 11:38:03 +0200 Julien Panis wrote: >> goto gen_pool_create_fail; >> } >> >> + pool->desc_infos = kcalloc(pool->num_desc, >> + sizeof(*pool->desc_infos), GFP_KERNEL); >> + if (!pool->desc_infos) { >> + ret = -ENOMEM; >> + dev_err(pool->dev, >> + "pool descriptor infos alloc failed %d\n", ret); > Please don't add errors on mem alloc failures. They just bloat the > kernel, there will be a rather large OOM splat in the logs if GFP_KERNEL > allocation fails. > >> + kfree_const(pool_name); >> + goto gen_pool_desc_infos_alloc_fail; >> + } >> + >> pool->gen_pool->name = pool_name; > If you add the new allocation after this line, I think you wouldn't > have to free pool_name under the if () explicitly.
Hello Jakub,
Thanks for these suggestions, I'll implement them in next version.
Also, about mem alloc failures, shouldn't we free 'pool' on kstrdup_const() error at the beginning of k3_cppi_desc_pool_create_name() ? I mean, it's not visible in my patch but I now wonder if this was done properly even before I modify the file.
Currently, we have: pool_name = kstrdup_const(...) if (!pool_name) return ERR_PTR(ret);
Shouldnt we have instead: pool_name = kstrdup_const(...) if (!pool_name) goto gen_pool_create_fail; (maybe label to be renamed) ..so that 'pool' can be freed before returning error.
Julien
| ![\](/images/icornerr.gif) |