| Message ID | 20210119150802.19997-12-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | baff4311c40ddae7bfc144dc628d4b11c19f0366 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ucc_geth improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: linuxppc-dev@lists.ozlabs.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Comparison to NULL could be written "!ug_info" |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit : > struct ucc_geth_info is somewhat large, and on systems with only one > or two UCC instances, that just wastes a few KB of memory. So > allocate and populate a chunk of memory at probe time instead of > initializing them all during driver init. > > Note that the existing "ug_info == NULL" check was dead code, as the > address of some static array element can obviously never be NULL. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > drivers/net/ethernet/freescale/ucc_geth.c | 32 +++++++++-------------- > 1 file changed, 12 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c > index 65ef7ae38912..67b93d60243e 100644 > --- a/drivers/net/ethernet/freescale/ucc_geth.c > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > @@ -157,8 +157,6 @@ static const struct ucc_geth_info ugeth_primary_info = { > .riscRx = QE_RISC_ALLOCATION_RISC1_AND_RISC2, > }; > > -static struct ucc_geth_info ugeth_info[8]; > - > #ifdef DEBUG > static void mem_disp(u8 *addr, int size) > { > @@ -3715,25 +3713,23 @@ static int ucc_geth_probe(struct platform_device* ofdev) > if ((ucc_num < 0) || (ucc_num > 7)) > return -ENODEV; > > - ug_info = &ugeth_info[ucc_num]; > - if (ug_info == NULL) { > - if (netif_msg_probe(&debug)) > - pr_err("[%d] Missing additional data!\n", ucc_num); > - return -ENODEV; > - } > + ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL); What about using devm_kmalloc() and avoid those kfree and associated goto ? > + if (ug_info == NULL) > + return -ENOMEM; > + memcpy(ug_info, &ugeth_primary_info, sizeof(*ug_info)); > > ug_info->uf_info.ucc_num = ucc_num; > > err = ucc_geth_parse_clock(np, "rx", &ug_info->uf_info.rx_clock); > if (err) > - return err; > + goto err_free_info; > err = ucc_geth_parse_clock(np, "tx", &ug_info->uf_info.tx_clock); > if (err) > - return err; > + goto err_free_info; > > err = of_address_to_resource(np, 0, &res); > if (err) > - return -EINVAL; > + goto err_free_info; > > ug_info->uf_info.regs = res.start; > ug_info->uf_info.irq = irq_of_parse_and_map(np, 0);
On 20/01/2021 08.02, Christophe Leroy wrote: > >> @@ -3715,25 +3713,23 @@ static int ucc_geth_probe(struct >> platform_device* ofdev) >> if ((ucc_num < 0) || (ucc_num > 7)) >> return -ENODEV; >> - ug_info = &ugeth_info[ucc_num]; >> - if (ug_info == NULL) { >> - if (netif_msg_probe(&debug)) >> - pr_err("[%d] Missing additional data!\n", ucc_num); >> - return -ENODEV; >> - } >> + ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL); > > What about using devm_kmalloc() and avoid those kfree and associated goto ? I already replied to that: I'd rather not mix kmalloc() and devm_kmalloc() as that makes it much harder to reason about the order in which stuff gets deallocated. But sure, if you insist. Rasmus
Le 20/01/2021 à 12:25, Rasmus Villemoes a écrit : > On 20/01/2021 08.02, Christophe Leroy wrote: >> >>> @@ -3715,25 +3713,23 @@ static int ucc_geth_probe(struct >>> platform_device* ofdev) >>> if ((ucc_num < 0) || (ucc_num > 7)) >>> return -ENODEV; >>> - ug_info = &ugeth_info[ucc_num]; >>> - if (ug_info == NULL) { >>> - if (netif_msg_probe(&debug)) >>> - pr_err("[%d] Missing additional data!\n", ucc_num); >>> - return -ENODEV; >>> - } >>> + ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL); >> >> What about using devm_kmalloc() and avoid those kfree and associated goto ? > > I already replied to that: I'd rather not mix kmalloc() and > devm_kmalloc() as that makes it much harder to reason about the order in > which stuff gets deallocated. But sure, if you insist. > I didn't remember I already did the same comment, sorry. Christophe
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index 65ef7ae38912..67b93d60243e 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -157,8 +157,6 @@ static const struct ucc_geth_info ugeth_primary_info = { .riscRx = QE_RISC_ALLOCATION_RISC1_AND_RISC2, }; -static struct ucc_geth_info ugeth_info[8]; - #ifdef DEBUG static void mem_disp(u8 *addr, int size) { @@ -3715,25 +3713,23 @@ static int ucc_geth_probe(struct platform_device* ofdev) if ((ucc_num < 0) || (ucc_num > 7)) return -ENODEV; - ug_info = &ugeth_info[ucc_num]; - if (ug_info == NULL) { - if (netif_msg_probe(&debug)) - pr_err("[%d] Missing additional data!\n", ucc_num); - return -ENODEV; - } + ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL); + if (ug_info == NULL) + return -ENOMEM; + memcpy(ug_info, &ugeth_primary_info, sizeof(*ug_info)); ug_info->uf_info.ucc_num = ucc_num; err = ucc_geth_parse_clock(np, "rx", &ug_info->uf_info.rx_clock); if (err) - return err; + goto err_free_info; err = ucc_geth_parse_clock(np, "tx", &ug_info->uf_info.tx_clock); if (err) - return err; + goto err_free_info; err = of_address_to_resource(np, 0, &res); if (err) - return -EINVAL; + goto err_free_info; ug_info->uf_info.regs = res.start; ug_info->uf_info.irq = irq_of_parse_and_map(np, 0); @@ -3746,7 +3742,7 @@ static int ucc_geth_probe(struct platform_device* ofdev) */ err = of_phy_register_fixed_link(np); if (err) - return err; + goto err_free_info; ug_info->phy_node = of_node_get(np); } @@ -3877,6 +3873,8 @@ static int ucc_geth_probe(struct platform_device* ofdev) of_phy_deregister_fixed_link(np); of_node_put(ug_info->tbi_node); of_node_put(ug_info->phy_node); +err_free_info: + kfree(ug_info); return err; } @@ -3893,6 +3891,7 @@ static int ucc_geth_remove(struct platform_device* ofdev) of_phy_deregister_fixed_link(np); of_node_put(ugeth->ug_info->tbi_node); of_node_put(ugeth->ug_info->phy_node); + kfree(ugeth->ug_info); free_netdev(dev); return 0; @@ -3921,17 +3920,10 @@ static struct platform_driver ucc_geth_driver = { static int __init ucc_geth_init(void) { - int i, ret; - if (netif_msg_drv(&debug)) pr_info(DRV_DESC "\n"); - for (i = 0; i < 8; i++) - memcpy(&(ugeth_info[i]), &ugeth_primary_info, - sizeof(ugeth_primary_info)); - - ret = platform_driver_register(&ucc_geth_driver); - return ret; + return platform_driver_register(&ucc_geth_driver); } static void __exit ucc_geth_exit(void)
struct ucc_geth_info is somewhat large, and on systems with only one or two UCC instances, that just wastes a few KB of memory. So allocate and populate a chunk of memory at probe time instead of initializing them all during driver init. Note that the existing "ug_info == NULL" check was dead code, as the address of some static array element can obviously never be NULL. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/net/ethernet/freescale/ucc_geth.c | 32 +++++++++-------------- 1 file changed, 12 insertions(+), 20 deletions(-)