Re: [PATCH] net: bcmasp: Use common error handling code in bcmasp_probe()

Re: [PATCH] net: bcmasp: Use common error handling code in bcmasp_probe()

[Wojciech Drewek]

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
> 
> 

Re: [PATCH] net: bcmasp: Use common error handling code in bcmasp_probe()

[Wojciech Drewek]

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

Re: [PATCH] net: bcmasp: Use common error handling code in bcmasp_probe()

[Jakub Kicinski]

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.

Re: net: bcmasp: Use common error handling code in bcmasp_probe()

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: net: bcmasp: Use common error handling code in bcmasp_probe()

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

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?
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH v2] net: bcmasp: Use common error handling code in bcmasp_probe()

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 2350 bytes --]

On Wed, Nov 15, 2023 at 1:11 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 15 Nov 2023 09:38:56 +0100
>
> Add a jump target so that a bit of exception handling can be better reused
> in this function implementation.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>
> v2:
> Special expectations were expressed for the previous patch size.
>
> * Jakub Kicinski
>   https://lore.kernel.org/netdev/20231106145806.669875f4@kernel.org/
>
> * Justin Chen
>   https://lore.kernel.org/netdev/CALSSxFYRgPwEq+QhCOYPqrtae8RvL=jTOcz4mk3vbe+Fc0QwbQ@mail.gmail.com/
>
> * Florian Fainelli
>   https://lore.kernel.org/netdev/4053e838-e5cf-4450-8067-21bdec989d1b@broadcom.com/
>
>
> Thus another change variant can eventually be integrated.
>
>
>  drivers/net/ethernet/broadcom/asp2/bcmasp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp.c b/drivers/net/ethernet/broadcom/asp2/bcmasp.c
> index 29b04a274d07..7d6c15732d9f 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,6 +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);
> +remove_intfs:
>                         bcmasp_remove_intfs(priv);
>                         goto of_put_exit;
>                 }
> --
> 2.42.1
>
nak. Doesn't save any lines of code. Doesn't make things clearer or
easier to follow. This doesn't seem like an improvement to me.

Justin

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]