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
