| Message ID | 20201205191744.7847-15-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ethernet: ucc_geth: assorted fixes and simplifications | expand |
Le 05/12/2020 à 20:17, 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 a06744d8b4af..273342233bba 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) > { > @@ -3714,25 +3712,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); Could we use dev_kmalloc() instead, to avoid the freeing on the wait out and the err_free_info: path ? > + 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); > @@ -3745,7 +3741,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); > } > > @@ -3876,6 +3872,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; > } > @@ -3886,6 +3884,7 @@ static int ucc_geth_remove(struct platform_device* ofdev) > struct ucc_geth_private *ugeth = netdev_priv(dev); > struct device_node *np = ofdev->dev.of_node; > > + kfree(ugeth->ug_info); > ucc_geth_memclean(ugeth); > if (of_phy_is_fixed_link(np)) > of_phy_deregister_fixed_link(np); > @@ -3920,17 +3919,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) >
On 08/12/2020 16.13, Christophe Leroy wrote: > > > Le 05/12/2020 à 20:17, Rasmus Villemoes a écrit : >> @@ -3714,25 +3712,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); > > Could we use dev_kmalloc() instead, to avoid the freeing on the wait out > and the err_free_info: path ? Perhaps, but I don't think mixing ordinary kmalloc() with devm_ versions in the same driver is a good idea - IIRC there are at least some rules to obey if one does that, but I don't remember and can't find what they are. Rasmus
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index a06744d8b4af..273342233bba 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) { @@ -3714,25 +3712,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); @@ -3745,7 +3741,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); } @@ -3876,6 +3872,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; } @@ -3886,6 +3884,7 @@ static int ucc_geth_remove(struct platform_device* ofdev) struct ucc_geth_private *ugeth = netdev_priv(dev); struct device_node *np = ofdev->dev.of_node; + kfree(ugeth->ug_info); ucc_geth_memclean(ugeth); if (of_phy_is_fixed_link(np)) of_phy_deregister_fixed_link(np); @@ -3920,17 +3919,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(-)