| Message ID | 20240223-am65-cpsw-xdp-basic-v8-2-f3421b58da09@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add minimal XDP support to TI AM65 CPSW Ethernet driver | expand |
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.
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
On Wed, 10 Apr 2024 10:36:16 +0200 Julien Panis wrote: > 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. Yes, it uses managed memory (devm_*) but prueth (I didn't check other callers) calls it from .ndo_open. So while not technically a full leak we can accumulate infinite memory by repeatedly failing here :S
diff --git a/drivers/net/ethernet/ti/k3-cppi-desc-pool.c b/drivers/net/ethernet/ti/k3-cppi-desc-pool.c index 414bcac9dcc6..e79d039fdaa5 100644 --- a/drivers/net/ethernet/ti/k3-cppi-desc-pool.c +++ b/drivers/net/ethernet/ti/k3-cppi-desc-pool.c @@ -22,6 +22,7 @@ struct k3_cppi_desc_pool { size_t mem_size; size_t num_desc; struct gen_pool *gen_pool; + void **desc_infos; }; void k3_cppi_desc_pool_destroy(struct k3_cppi_desc_pool *pool) @@ -37,6 +38,8 @@ void k3_cppi_desc_pool_destroy(struct k3_cppi_desc_pool *pool) dma_free_coherent(pool->dev, pool->mem_size, pool->cpumem, pool->dma_addr); + kfree(pool->desc_infos); + gen_pool_destroy(pool->gen_pool); /* frees pool->name */ } EXPORT_SYMBOL_GPL(k3_cppi_desc_pool_destroy); @@ -72,6 +75,16 @@ k3_cppi_desc_pool_create_name(struct device *dev, size_t size, 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); + kfree_const(pool_name); + goto gen_pool_desc_infos_alloc_fail; + } + pool->gen_pool->name = pool_name; pool->cpumem = dma_alloc_coherent(pool->dev, pool->mem_size, @@ -94,6 +107,8 @@ k3_cppi_desc_pool_create_name(struct device *dev, size_t size, dma_free_coherent(pool->dev, pool->mem_size, pool->cpumem, pool->dma_addr); dma_alloc_fail: + kfree(pool->desc_infos); +gen_pool_desc_infos_alloc_fail: gen_pool_destroy(pool->gen_pool); /* frees pool->name */ gen_pool_create_fail: devm_kfree(pool->dev, pool); @@ -144,5 +159,19 @@ void *k3_cppi_desc_pool_cpuaddr(const struct k3_cppi_desc_pool *pool) } EXPORT_SYMBOL_GPL(k3_cppi_desc_pool_cpuaddr); +void k3_cppi_desc_pool_desc_info_set(struct k3_cppi_desc_pool *pool, + int desc_idx, void *info) +{ + pool->desc_infos[desc_idx] = info; +} +EXPORT_SYMBOL_GPL(k3_cppi_desc_pool_desc_info_set); + +void *k3_cppi_desc_pool_desc_info(const struct k3_cppi_desc_pool *pool, + int desc_idx) +{ + return pool->desc_infos[desc_idx]; +} +EXPORT_SYMBOL_GPL(k3_cppi_desc_pool_desc_info); + MODULE_LICENSE("GPL"); MODULE_DESCRIPTION("TI K3 CPPI5 descriptors pool API"); diff --git a/drivers/net/ethernet/ti/k3-cppi-desc-pool.h b/drivers/net/ethernet/ti/k3-cppi-desc-pool.h index 3c6aed0bed71..851d352b338b 100644 --- a/drivers/net/ethernet/ti/k3-cppi-desc-pool.h +++ b/drivers/net/ethernet/ti/k3-cppi-desc-pool.h @@ -28,5 +28,9 @@ void k3_cppi_desc_pool_free(struct k3_cppi_desc_pool *pool, void *addr); size_t k3_cppi_desc_pool_avail(struct k3_cppi_desc_pool *pool); size_t k3_cppi_desc_pool_desc_size(const struct k3_cppi_desc_pool *pool); void *k3_cppi_desc_pool_cpuaddr(const struct k3_cppi_desc_pool *pool); +void k3_cppi_desc_pool_desc_info_set(struct k3_cppi_desc_pool *pool, + int desc_idx, void *info); +void *k3_cppi_desc_pool_desc_info(const struct k3_cppi_desc_pool *pool, + int desc_idx); #endif /* K3_CPPI_DESC_POOL_H_ */
This patch introduces a member and the related accessors which can be used to store descriptor specific additional information. This member can store, for instance, an ID to differentiate a skb TX buffer type from a xdpf TX buffer type. Signed-off-by: Julien Panis <jpanis@baylibre.com> --- drivers/net/ethernet/ti/k3-cppi-desc-pool.c | 29 +++++++++++++++++++++++++++++ drivers/net/ethernet/ti/k3-cppi-desc-pool.h | 4 ++++ 2 files changed, 33 insertions(+)