| Message ID | 0b2972cb-03b2-40c7-a728-6ebe2512637f@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bcmasp: Use common error handling code in bcmasp_probe() | expand |
On 05.11.2023 17:33, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sun, 5 Nov 2023 17:24:01 +0100 > > Add a jump target so that a bit of exception handling can be better > reused at the end of this function. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/net/ethernet/broadcom/asp2/bcmasp.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c > index 29b04a274d07..675437e44b94 100644 > --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c > +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c > @@ -1304,9 +1304,8 @@ static int bcmasp_probe(struct platform_device *pdev) > intf = bcmasp_interface_create(priv, intf_node, i); > if (!intf) { > dev_err(dev, "Cannot create eth interface %d\n", i); > - bcmasp_remove_intfs(priv); > of_node_put(intf_node); > - goto of_put_exit; > + goto remove_intfs; > } > list_add_tail(&intf->list, &priv->intfs); > i++; > @@ -1331,8 +1330,7 @@ static int bcmasp_probe(struct platform_device *pdev) > netdev_err(intf->ndev, > "failed to register net_device: %d\n", ret); > priv->destroy_wol(priv); > - bcmasp_remove_intfs(priv); > - goto of_put_exit; > + goto remove_intfs; > } > count++; > } > @@ -1342,6 +1340,10 @@ static int bcmasp_probe(struct platform_device *pdev) > of_put_exit: > of_node_put(ports_node); > return ret; > + > +remove_intfs: > + bcmasp_remove_intfs(priv); > + goto of_put_exit; Why is it at the end of the function? Just move it above of_put_exit and it will naturally go to the of_node_put call. No need for "goto of_put_exit". > } > > static void bcmasp_remove(struct platform_device *pdev) > -- > 2.42.0 > >
… >> Add a jump target so that a bit of exception handling can be better >> reused at the end of this function. … >> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c >> @@ -1304,9 +1304,8 @@ static int bcmasp_probe(struct platform_device *pdev) >> intf = bcmasp_interface_create(priv, intf_node, i); >> if (!intf) { >> dev_err(dev, "Cannot create eth interface %d\n", i); >> - bcmasp_remove_intfs(priv); >> of_node_put(intf_node); >> - goto of_put_exit; >> + goto remove_intfs; >> } >> list_add_tail(&intf->list, &priv->intfs); >> i++; >> @@ -1331,8 +1330,7 @@ static int bcmasp_probe(struct platform_device *pdev) >> netdev_err(intf->ndev, >> "failed to register net_device: %d\n", ret); >> priv->destroy_wol(priv); >> - bcmasp_remove_intfs(priv); >> - goto of_put_exit; >> + goto remove_intfs; >> } >> count++; >> } >> @@ -1342,6 +1340,10 @@ static int bcmasp_probe(struct platform_device *pdev) >> of_put_exit: >> of_node_put(ports_node); >> return ret; >> + >> +remove_intfs: >> + bcmasp_remove_intfs(priv); >> + goto of_put_exit; > > Why is it at the end of the function? Just move it above of_put_exit and it will naturally > go to the of_node_put call. No need for "goto of_put_exit". I got the impression that the call of the function “bcmasp_remove_intfs” belongs only to exceptional cases in the shown control flow. Regards, Markus
On 06.11.2023 14:55, Markus Elfring wrote: > … >>> Add a jump target so that a bit of exception handling can be better >>> reused at the end of this function. > … >>> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c >>> @@ -1304,9 +1304,8 @@ static int bcmasp_probe(struct platform_device *pdev) >>> intf = bcmasp_interface_create(priv, intf_node, i); >>> if (!intf) { >>> dev_err(dev, "Cannot create eth interface %d\n", i); >>> - bcmasp_remove_intfs(priv); >>> of_node_put(intf_node); >>> - goto of_put_exit; >>> + goto remove_intfs; >>> } >>> list_add_tail(&intf->list, &priv->intfs); >>> i++; >>> @@ -1331,8 +1330,7 @@ static int bcmasp_probe(struct platform_device *pdev) >>> netdev_err(intf->ndev, >>> "failed to register net_device: %d\n", ret); >>> priv->destroy_wol(priv); >>> - bcmasp_remove_intfs(priv); >>> - goto of_put_exit; >>> + goto remove_intfs; >>> } >>> count++; >>> } >>> @@ -1342,6 +1340,10 @@ static int bcmasp_probe(struct platform_device *pdev) >>> of_put_exit: >>> of_node_put(ports_node); >>> return ret; >>> + >>> +remove_intfs: >>> + bcmasp_remove_intfs(priv); >>> + goto of_put_exit; >> >> Why is it at the end of the function? Just move it above of_put_exit and it will naturally >> go to the of_node_put call. No need for "goto of_put_exit". > > I got the impression that the call of the function “bcmasp_remove_intfs” belongs only > to exceptional cases in the shown control flow. Ah, yes, you're right. If we move it above of_put_exit as I suggested then it'll be executed in successful path as well. Makes sense now Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > > Regards, > Markus
On Sun, 5 Nov 2023 17:33:46 +0100 Markus Elfring wrote: > Add a jump target so that a bit of exception handling can be better > reused at the end of this function. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/net/ethernet/broadcom/asp2/bcmasp.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) The diffstat proves otherwise. Please don't send such patches to networking.
>> Add a jump target so that a bit of exception handling can be better >> reused at the end of this function. … >> --- >> drivers/net/ethernet/broadcom/asp2/bcmasp.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) > > The diffstat proves otherwise. > Please don't send such patches to networking. How does this feedback fit to a change possibility which was reviewed by Wojciech Drewek yesterday? Regards, Markus
On Mon, Nov 6, 2023 at 10:38 PM Markus Elfring <Markus.Elfring@web.de> wrote: > > >> Add a jump target so that a bit of exception handling can be better > >> reused at the end of this function. > … > >> --- > >> drivers/net/ethernet/broadcom/asp2/bcmasp.c | 10 ++++++---- > >> 1 file changed, 6 insertions(+), 4 deletions(-) > > > > The diffstat proves otherwise. > > Please don't send such patches to networking. > > How does this feedback fit to a change possibility which was reviewed by > Wojciech Drewek yesterday? > > Regards, > Markus We are making the code harder to follow with these changes. Also adding more lines than removing. Don't think this patch is an improvement IMHO. NAK on my end. Thanks, Justin
On 11/7/23 10:48, Justin Chen wrote: > On Mon, Nov 6, 2023 at 10:38 PM Markus Elfring <Markus.Elfring@web.de> wrote: >> >>>> Add a jump target so that a bit of exception handling can be better >>>> reused at the end of this function. >> … >>>> --- >>>> drivers/net/ethernet/broadcom/asp2/bcmasp.c | 10 ++++++---- >>>> 1 file changed, 6 insertions(+), 4 deletions(-) >>> >>> The diffstat proves otherwise. >>> Please don't send such patches to networking. >> >> How does this feedback fit to a change possibility which was reviewed by >> Wojciech Drewek yesterday? >> >> Regards, >> Markus > > We are making the code harder to follow with these changes. Also > adding more lines than removing. Don't think this patch is an > improvement IMHO. NAK on my end. Likewise, at the very least, why not have the remove_intfs label immediately above the of_put_exit one so then it just falls through, and then obviously update the return path to drop the reference count and return success?
diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c index 29b04a274d07..675437e44b94 100644 --- a/drivers/net/ethernet/broadcom/asp2/bcmasp.c +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp.c @@ -1304,9 +1304,8 @@ static int bcmasp_probe(struct platform_device *pdev) intf = bcmasp_interface_create(priv, intf_node, i); if (!intf) { dev_err(dev, "Cannot create eth interface %d\n", i); - bcmasp_remove_intfs(priv); of_node_put(intf_node); - goto of_put_exit; + goto remove_intfs; } list_add_tail(&intf->list, &priv->intfs); i++; @@ -1331,8 +1330,7 @@ static int bcmasp_probe(struct platform_device *pdev) netdev_err(intf->ndev, "failed to register net_device: %d\n", ret); priv->destroy_wol(priv); - bcmasp_remove_intfs(priv); - goto of_put_exit; + goto remove_intfs; } count++; } @@ -1342,6 +1340,10 @@ static int bcmasp_probe(struct platform_device *pdev) of_put_exit: of_node_put(ports_node); return ret; + +remove_intfs: + bcmasp_remove_intfs(priv); + goto of_put_exit; } static void bcmasp_remove(struct platform_device *pdev)