| Message ID | 9b879c8d-4c28-4748-acf6-18dc69d8ebdf@web.de (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fman: Use common error handling code in dtsec_init() | expand |
> -----Original Message----- > From: Markus Elfring <Markus.Elfring@web.de> > Sent: Tuesday, February 27, 2024 3:15 PM > To: netdev@vger.kernel.org; kernel-janitors@vger.kernel.org; David S. Miller > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org>; Madalin Bucur <madalin.bucur@nxp.com>; Paolo > Abeni <pabeni@redhat.com>; Sean Anderson <sean.anderson@seco.com> > Cc: LKML <linux-kernel@vger.kernel.org> > Subject: [PATCH] net: fman: Use common error handling code in dtsec_init() > > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Tue, 27 Feb 2024 14:05:25 +0100 > > Adjust jump targets so that a bit of exception handling can be better > reused at the end of this function implementation. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > .../net/ethernet/freescale/fman/fman_dtsec.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c > b/drivers/net/ethernet/freescale/fman/fman_dtsec.c > index 3088da7adf0f..1de22400fd89 100644 > --- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c > +++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c > @@ -1279,9 +1279,8 @@ static int dtsec_init(struct fman_mac *dtsec) > dtsec->max_speed, dtsec->addr, dtsec->exceptions, > dtsec->tbidev->addr); > if (err) { > - free_init_resources(dtsec); > pr_err("DTSEC version doesn't support this i/f mode\n"); > - return err; > + goto free_resources; > } > > /* Configure the TBI PHY Control Register */ > @@ -1296,23 +1295,21 @@ static int dtsec_init(struct fman_mac *dtsec) > err = fman_set_mac_max_frame(dtsec->fm, dtsec->mac_id, > max_frm_ln); > if (err) { > pr_err("Setting max frame length failed\n"); > - free_init_resources(dtsec); > - return -EINVAL; > + err = -EINVAL; > + goto free_resources; > } > > dtsec->multicast_addr_hash = > alloc_hash_table(EXTENDED_HASH_TABLE_SIZE); > if (!dtsec->multicast_addr_hash) { > - free_init_resources(dtsec); > pr_err("MC hash table is failed\n"); > - return -ENOMEM; > + goto e_nomem; > } > > dtsec->unicast_addr_hash = > alloc_hash_table(DTSEC_HASH_TABLE_SIZE); > if (!dtsec->unicast_addr_hash) { > - free_init_resources(dtsec); > pr_err("UC hash table is failed\n"); > - return -ENOMEM; > + goto e_nomem; > } > > /* register err intr handler for dtsec to FPM (err) */ > @@ -1326,6 +1323,12 @@ static int dtsec_init(struct fman_mac *dtsec) > dtsec->dtsec_drv_param = NULL; > > return 0; > + > +e_nomem: > + err = -ENOMEM; > +free_resources: > + free_init_resources(dtsec); > + return err; > } > > static int dtsec_free(struct fman_mac *dtsec) > -- > 2.43.2 Looks good Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
Tue, Feb 27, 2024 at 02:14:52PM CET, Markus.Elfring@web.de wrote: >From: Markus Elfring <elfring@users.sourceforge.net> >Date: Tue, 27 Feb 2024 14:05:25 +0100 > >Adjust jump targets so that a bit of exception handling can be better >reused at the end of this function implementation. > >Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> Reviewed-by: Jiri Pirko <jiri@nvidia.com> Nit, next time please indicate the target tree in the patch subject prefix: [patch net-next] xxx
On Tue, 27 Feb 2024 14:14:52 +0100 Markus Elfring wrote: > Adjust jump targets so that a bit of exception handling can be better > reused at the end of this function implementation. Okay, but.. > .../net/ethernet/freescale/fman/fman_dtsec.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) ..you've added more lines than you've removed so what's the point.
Wed, Feb 28, 2024 at 03:46:57AM CET, kuba@kernel.org wrote: >On Tue, 27 Feb 2024 14:14:52 +0100 Markus Elfring wrote: >> Adjust jump targets so that a bit of exception handling can be better >> reused at the end of this function implementation. > >Okay, but.. > >> .../net/ethernet/freescale/fman/fman_dtsec.c | 19 +++++++++++-------- >> 1 file changed, 11 insertions(+), 8 deletions(-) > >..you've added more lines than you've removed so what's the point. To have cleaner error path? Not always lines of code is the correct indicator of patch quality :) >-- >pw-bot: reject >
>> Adjust jump targets so that a bit of exception handling can be better >> reused at the end of this function implementation. > > Okay, but.. > >> .../net/ethernet/freescale/fman/fman_dtsec.c | 19 +++++++++++-------- >> 1 file changed, 11 insertions(+), 8 deletions(-) > > ..you've added more lines than you've removed so what's the point. * Can the change acceptance grow any more for attempts to improve error handling another bit? * Would you like to fiddle with scope-based resource management? https://elixir.bootlin.com/linux/v6.8-rc6/source/include/linux/cleanup.h#L8 See also: Article by Jonathan Corbet from 2023-06-15 https://lwn.net/Articles/934679/ > -- > pw-bot: reject Can such an reaction be reconsidered once more? Regards, Markus
On Wed, Feb 28, 2024 at 08:51:49AM +0100, Jiri Pirko wrote: > Wed, Feb 28, 2024 at 03:46:57AM CET, kuba@kernel.org wrote: > >On Tue, 27 Feb 2024 14:14:52 +0100 Markus Elfring wrote: > >> Adjust jump targets so that a bit of exception handling can be better > >> reused at the end of this function implementation. > > > >Okay, but.. > > > >> .../net/ethernet/freescale/fman/fman_dtsec.c | 19 +++++++++++-------- > >> 1 file changed, 11 insertions(+), 8 deletions(-) > > > >..you've added more lines than you've removed so what's the point. > > To have cleaner error path? Not always lines of code is the correct > indicator of patch quality :) > I really don't like those goto e_nomem type of things. When you're laddering gotos you should do that kind of thing before the gotos so that when people add new gotos it doesn't make a mess. It's the same for unlocks, do that before the goto unless it matches a lock at the very start of the function. Or if you're doing a goto from inside a loop then clean up the partial iteration through the loop before the goto. regards, dan carpenter
diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c b/drivers/net/ethernet/freescale/fman/fman_dtsec.c index 3088da7adf0f..1de22400fd89 100644 --- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c +++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c @@ -1279,9 +1279,8 @@ static int dtsec_init(struct fman_mac *dtsec) dtsec->max_speed, dtsec->addr, dtsec->exceptions, dtsec->tbidev->addr); if (err) { - free_init_resources(dtsec); pr_err("DTSEC version doesn't support this i/f mode\n"); - return err; + goto free_resources; } /* Configure the TBI PHY Control Register */ @@ -1296,23 +1295,21 @@ static int dtsec_init(struct fman_mac *dtsec) err = fman_set_mac_max_frame(dtsec->fm, dtsec->mac_id, max_frm_ln); if (err) { pr_err("Setting max frame length failed\n"); - free_init_resources(dtsec); - return -EINVAL; + err = -EINVAL; + goto free_resources; } dtsec->multicast_addr_hash = alloc_hash_table(EXTENDED_HASH_TABLE_SIZE); if (!dtsec->multicast_addr_hash) { - free_init_resources(dtsec); pr_err("MC hash table is failed\n"); - return -ENOMEM; + goto e_nomem; } dtsec->unicast_addr_hash = alloc_hash_table(DTSEC_HASH_TABLE_SIZE); if (!dtsec->unicast_addr_hash) { - free_init_resources(dtsec); pr_err("UC hash table is failed\n"); - return -ENOMEM; + goto e_nomem; } /* register err intr handler for dtsec to FPM (err) */ @@ -1326,6 +1323,12 @@ static int dtsec_init(struct fman_mac *dtsec) dtsec->dtsec_drv_param = NULL; return 0; + +e_nomem: + err = -ENOMEM; +free_resources: + free_init_resources(dtsec); + return err; } static int dtsec_free(struct fman_mac *dtsec)