[PATCH] net/sonic: Fix some resource leaks in error handling paths

[PATCH] net/sonic: Fix some resource leaks in error handling paths

[Christophe JAILLET]

A call to 'dma_alloc_coherent()' is hidden in 'sonic_alloc_descriptors()'.

This is correctly freed in the remove function, but not in the error
handling path of the probe function.
Fix it and add the missing 'dma_free_coherent()' call.

While at it, rename a label in order to be slightly more informative and
split some too long lines.

This patch is similar to commit 10e3cc180e64 ("net/sonic: Fix a resource leak in an error handling path in 'jazz_sonic_probe()'")
which was for 'jazzsonic.c'.

Suggested-by: Finn Thain <fthain@telegraphics.com.au>
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Only macsonic has been compile tested. I don't have the needed setup to
compile xtsonic
---
 drivers/net/ethernet/natsemi/macsonic.c | 17 +++++++++++++----
 drivers/net/ethernet/natsemi/xtsonic.c  |  7 +++++--
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/natsemi/macsonic.c b/drivers/net/ethernet/natsemi/macsonic.c
index 1b5559aacb38..38d86c712bbc 100644
--- a/drivers/net/ethernet/natsemi/macsonic.c
+++ b/drivers/net/ethernet/natsemi/macsonic.c
@@ -506,10 +506,14 @@ static int mac_sonic_platform_probe(struct platform_device *pdev)
 
 	err = register_netdev(dev);
 	if (err)
-		goto out;
+		goto undo_probe1;
 
 	return 0;
 
+undo_probe1:
+	dma_free_coherent(lp->device,
+			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+			  lp->descriptors, lp->descriptors_laddr);
 out:
 	free_netdev(dev);
 
@@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
 	struct sonic_local* lp = netdev_priv(dev);
 
 	unregister_netdev(dev);
-	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
-	                  lp->descriptors, lp->descriptors_laddr);
+	dma_free_coherent(lp->device,
+			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+			  lp->descriptors, lp->descriptors_laddr);
 	free_netdev(dev);
 
 	return 0;
@@ -584,12 +589,16 @@ static int mac_sonic_nubus_probe(struct nubus_board *board)
 
 	err = register_netdev(ndev);
 	if (err)
-		goto out;
+		goto undo_probe1;
 
 	nubus_set_drvdata(board, ndev);
 
 	return 0;
 
+undo_probe1:
+	dma_free_coherent(lp->device,
+			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+			  lp->descriptors, lp->descriptors_laddr);
 out:
 	free_netdev(ndev);
 	return err;
diff --git a/drivers/net/ethernet/natsemi/xtsonic.c b/drivers/net/ethernet/natsemi/xtsonic.c
index dda9ec7d9cee..a917f1a830fc 100644
--- a/drivers/net/ethernet/natsemi/xtsonic.c
+++ b/drivers/net/ethernet/natsemi/xtsonic.c
@@ -229,11 +229,14 @@ int xtsonic_probe(struct platform_device *pdev)
 	sonic_msg_init(dev);
 
 	if ((err = register_netdev(dev)))
-		goto out1;
+		goto undo_probe1;
 
 	return 0;
 
-out1:
+undo_probe1:
+	dma_free_coherent(lp->device,
+			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+			  lp->descriptors, lp->descriptors_laddr);
 	release_region(dev->base_addr, SONIC_MEM_SIZE);
 out:
 	free_netdev(dev);
-- 
2.25.1

[PATCH] net/sonic: Fix some resource leaks in error handling paths

[Christophe JAILLET]

A call to 'dma_alloc_coherent()' is hidden in 'sonic_alloc_descriptors()'.

This is correctly freed in the remove function, but not in the error
handling path of the probe function.
Fix it and add the missing 'dma_free_coherent()' call.

While at it, rename a label in order to be slightly more informative and
split some too long lines.

This patch is similar to commit 10e3cc180e64 ("net/sonic: Fix a resource leak in an error handling path in 'jazz_sonic_probe()'")
which was for 'jazzsonic.c'.

Suggested-by: Finn Thain <fthain@telegraphics.com.au>
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Only macsonic has been compile tested. I don't have the needed setup to
compile xtsonic
---
 drivers/net/ethernet/natsemi/macsonic.c | 17 +++++++++++++----
 drivers/net/ethernet/natsemi/xtsonic.c  |  7 +++++--
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/natsemi/macsonic.c b/drivers/net/ethernet/natsemi/macsonic.c
index 1b5559aacb38..38d86c712bbc 100644
--- a/drivers/net/ethernet/natsemi/macsonic.c
+++ b/drivers/net/ethernet/natsemi/macsonic.c
@@ -506,10 +506,14 @@ static int mac_sonic_platform_probe(struct platform_device *pdev)
 
 	err = register_netdev(dev);
 	if (err)
-		goto out;
+		goto undo_probe1;
 
 	return 0;
 
+undo_probe1:
+	dma_free_coherent(lp->device,
+			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+			  lp->descriptors, lp->descriptors_laddr);
 out:
 	free_netdev(dev);
 
@@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
 	struct sonic_local* lp = netdev_priv(dev);
 
 	unregister_netdev(dev);
-	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
-	                  lp->descriptors, lp->descriptors_laddr);
+	dma_free_coherent(lp->device,
+			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+			  lp->descriptors, lp->descriptors_laddr);
 	free_netdev(dev);
 
 	return 0;
@@ -584,12 +589,16 @@ static int mac_sonic_nubus_probe(struct nubus_board *board)
 
 	err = register_netdev(ndev);
 	if (err)
-		goto out;
+		goto undo_probe1;
 
 	nubus_set_drvdata(board, ndev);
 
 	return 0;
 
+undo_probe1:
+	dma_free_coherent(lp->device,
+			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+			  lp->descriptors, lp->descriptors_laddr);
 out:
 	free_netdev(ndev);
 	return err;
diff --git a/drivers/net/ethernet/natsemi/xtsonic.c b/drivers/net/ethernet/natsemi/xtsonic.c
index dda9ec7d9cee..a917f1a830fc 100644
--- a/drivers/net/ethernet/natsemi/xtsonic.c
+++ b/drivers/net/ethernet/natsemi/xtsonic.c
@@ -229,11 +229,14 @@ int xtsonic_probe(struct platform_device *pdev)
 	sonic_msg_init(dev);
 
 	if ((err = register_netdev(dev)))
-		goto out1;
+		goto undo_probe1;
 
 	return 0;
 
-out1:
+undo_probe1:
+	dma_free_coherent(lp->device,
+			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
+			  lp->descriptors, lp->descriptors_laddr);
 	release_region(dev->base_addr, SONIC_MEM_SIZE);
 out:
 	free_netdev(dev);
-- 
2.25.1

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Fri, 8 May 2020, Christophe JAILLET wrote:

> A call to 'dma_alloc_coherent()' is hidden in 
> 'sonic_alloc_descriptors()'.
> 
> This is correctly freed in the remove function, but not in the error 
> handling path of the probe function. Fix it and add the missing 
> 'dma_free_coherent()' call.
> 
> While at it, rename a label in order to be slightly more informative and 
> split some too long lines.
> 
> This patch is similar to commit 10e3cc180e64 ("net/sonic: Fix a resource 
> leak in an error handling path in 'jazz_sonic_probe()'") which was for 
> 'jazzsonic.c'.
> 
> Suggested-by: Finn Thain <fthain@telegraphics.com.au>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks.

Reviewed-by: Finn Thain <fthain@telegraphics.com.au>

> ---
> Only macsonic has been compile tested. I don't have the needed setup to
> compile xtsonic

I compile tested xtsonic.c. I didn't use an xtensa toolchain as there's 
probably no need: the new code already appears elsewhere in that file.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Fri, 8 May 2020, Christophe JAILLET wrote:

> A call to 'dma_alloc_coherent()' is hidden in 
> 'sonic_alloc_descriptors()'.
> 
> This is correctly freed in the remove function, but not in the error 
> handling path of the probe function. Fix it and add the missing 
> 'dma_free_coherent()' call.
> 
> While at it, rename a label in order to be slightly more informative and 
> split some too long lines.
> 
> This patch is similar to commit 10e3cc180e64 ("net/sonic: Fix a resource 
> leak in an error handling path in 'jazz_sonic_probe()'") which was for 
> 'jazzsonic.c'.
> 
> Suggested-by: Finn Thain <fthain@telegraphics.com.au>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks.

Reviewed-by: Finn Thain <fthain@telegraphics.com.au>

> ---
> Only macsonic has been compile tested. I don't have the needed setup to
> compile xtsonic

I compile tested xtsonic.c. I didn't use an xtensa toolchain as there's 
probably no need: the new code already appears elsewhere in that file.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Jakub Kicinski]

On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
> Only macsonic has been compile tested. I don't have the needed setup to
> compile xtsonic

Well, we gotta do that before we apply the patch :S

Does the driver actually depend on some platform stuff, or can we 
do this:

diff --git a/drivers/net/ethernet/natsemi/Kconfig b/drivers/net/ethernet/natsemi/Kconfig
@@ -58,7 +58,7 @@ config NS83820
 
 config XTENSA_XT2000_SONIC
        tristate "Xtensa XT2000 onboard SONIC Ethernet support"
-       depends on XTENSA_PLATFORM_XT2000
+       depends on XTENSA_PLATFORM_XT2000 || COMPILE_TEST
        ---help---
          This is the driver for the onboard card of the Xtensa XT2000 board.
 
?

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Jakub Kicinski]

On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
> Only macsonic has been compile tested. I don't have the needed setup to
> compile xtsonic

Well, we gotta do that before we apply the patch :S

Does the driver actually depend on some platform stuff, or can we 
do this:

diff --git a/drivers/net/ethernet/natsemi/Kconfig b/drivers/net/ethernet/natsemi/Kconfig
@@ -58,7 +58,7 @@ config NS83820
 
 config XTENSA_XT2000_SONIC
        tristate "Xtensa XT2000 onboard SONIC Ethernet support"
-       depends on XTENSA_PLATFORM_XT2000
+       depends on XTENSA_PLATFORM_XT2000 || COMPILE_TEST
        ---help---
          This is the driver for the onboard card of the Xtensa XT2000 board.
 
?

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Jakub Kicinski]

On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
> @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
>  	struct sonic_local* lp = netdev_priv(dev);
>  
>  	unregister_netdev(dev);
> -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> -	                  lp->descriptors, lp->descriptors_laddr);
> +	dma_free_coherent(lp->device,
> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> +			  lp->descriptors, lp->descriptors_laddr);
>  	free_netdev(dev);
>  
>  	return 0;

This is a white-space only change, right? Since this is a fix we should
avoid making cleanups which are not strictly necessary.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Jakub Kicinski]

On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
> @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
>  	struct sonic_local* lp = netdev_priv(dev);
>  
>  	unregister_netdev(dev);
> -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> -	                  lp->descriptors, lp->descriptors_laddr);
> +	dma_free_coherent(lp->device,
> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> +			  lp->descriptors, lp->descriptors_laddr);
>  	free_netdev(dev);
>  
>  	return 0;

This is a white-space only change, right? Since this is a fix we should
avoid making cleanups which are not strictly necessary.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Fri, 8 May 2020, Jakub Kicinski wrote:

> On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
> > Only macsonic has been compile tested. I don't have the needed setup to
> > compile xtsonic
> 
> Well, we gotta do that before we apply the patch :S
> 

I've compiled xtsonic.c with this patch.

> Does the driver actually depend on some platform stuff,

xtsonic.c looks portable enough but it has some asm includes that I 
haven't looked at. It is really a question for the arch maintainers.

>  or can we do this:
> 
> diff --git a/drivers/net/ethernet/natsemi/Kconfig b/drivers/net/ethernet/natsemi/Kconfig
> @@ -58,7 +58,7 @@ config NS83820
>  
>  config XTENSA_XT2000_SONIC
>         tristate "Xtensa XT2000 onboard SONIC Ethernet support"
> -       depends on XTENSA_PLATFORM_XT2000
> +       depends on XTENSA_PLATFORM_XT2000 || COMPILE_TEST
>         ---help---
>           This is the driver for the onboard card of the Xtensa XT2000 board.
>  
> ?
> 

That's effectively what I did to compile test xtsonic.c (I removed the 
line to get the same effect).

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Fri, 8 May 2020, Jakub Kicinski wrote:

> On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
> > Only macsonic has been compile tested. I don't have the needed setup to
> > compile xtsonic
> 
> Well, we gotta do that before we apply the patch :S
> 

I've compiled xtsonic.c with this patch.

> Does the driver actually depend on some platform stuff,

xtsonic.c looks portable enough but it has some asm includes that I 
haven't looked at. It is really a question for the arch maintainers.

>  or can we do this:
> 
> diff --git a/drivers/net/ethernet/natsemi/Kconfig b/drivers/net/ethernet/natsemi/Kconfig
> @@ -58,7 +58,7 @@ config NS83820
>  
>  config XTENSA_XT2000_SONIC
>         tristate "Xtensa XT2000 onboard SONIC Ethernet support"
> -       depends on XTENSA_PLATFORM_XT2000
> +       depends on XTENSA_PLATFORM_XT2000 || COMPILE_TEST
>         ---help---
>           This is the driver for the onboard card of the Xtensa XT2000 board.
>  
> ?
> 

That's effectively what I did to compile test xtsonic.c (I removed the 
line to get the same effect).

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Jakub Kicinski]

On Sat, 9 May 2020 11:57:44 +1000 (AEST) Finn Thain wrote:
> On Fri, 8 May 2020, Jakub Kicinski wrote:
> > On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:  
> > > Only macsonic has been compile tested. I don't have the needed setup to
> > > compile xtsonic  
> > 
> > Well, we gotta do that before we apply the patch :S
> >   
> 
> I've compiled xtsonic.c with this patch.
> 
> > Does the driver actually depend on some platform stuff,  
> 
> xtsonic.c looks portable enough but it has some asm includes that I 
> haven't looked at. It is really a question for the arch maintainers.

I see.

> >  or can we do this:
> > 
> > diff --git a/drivers/net/ethernet/natsemi/Kconfig b/drivers/net/ethernet/natsemi/Kconfig
> > @@ -58,7 +58,7 @@ config NS83820
> >  
> >  config XTENSA_XT2000_SONIC
> >         tristate "Xtensa XT2000 onboard SONIC Ethernet support"
> > -       depends on XTENSA_PLATFORM_XT2000
> > +       depends on XTENSA_PLATFORM_XT2000 || COMPILE_TEST
> >         ---help---
> >           This is the driver for the onboard card of the Xtensa XT2000 board.
> >  
> > ?
> >   
> 
> That's effectively what I did to compile test xtsonic.c (I removed the 
> line to get the same effect).

Thank you, that should do!

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Jakub Kicinski]

On Sat, 9 May 2020 11:57:44 +1000 (AEST) Finn Thain wrote:
> On Fri, 8 May 2020, Jakub Kicinski wrote:
> > On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:  
> > > Only macsonic has been compile tested. I don't have the needed setup to
> > > compile xtsonic  
> > 
> > Well, we gotta do that before we apply the patch :S
> >   
> 
> I've compiled xtsonic.c with this patch.
> 
> > Does the driver actually depend on some platform stuff,  
> 
> xtsonic.c looks portable enough but it has some asm includes that I 
> haven't looked at. It is really a question for the arch maintainers.

I see.

> >  or can we do this:
> > 
> > diff --git a/drivers/net/ethernet/natsemi/Kconfig b/drivers/net/ethernet/natsemi/Kconfig
> > @@ -58,7 +58,7 @@ config NS83820
> >  
> >  config XTENSA_XT2000_SONIC
> >         tristate "Xtensa XT2000 onboard SONIC Ethernet support"
> > -       depends on XTENSA_PLATFORM_XT2000
> > +       depends on XTENSA_PLATFORM_XT2000 || COMPILE_TEST
> >         ---help---
> >           This is the driver for the onboard card of the Xtensa XT2000 board.
> >  
> > ?
> >   
> 
> That's effectively what I did to compile test xtsonic.c (I removed the 
> line to get the same effect).

Thank you, that should do!

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

> While at it, rename a label in order to be slightly more informative and
> split some too long lines.

Would you like to add the tag “Fixes” to the change description?


…
> +++ b/drivers/net/ethernet/natsemi/macsonic.c
> @@ -506,10 +506,14 @@ static int mac_sonic_platform_probe(struct platform_device *pdev)
>
>  	err = register_netdev(dev);
>  	if (err)
> -		goto out;
> +		goto undo_probe1;
>
>  	return 0;
>
> +undo_probe1:
> +	dma_free_coherent(lp->device,
> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> +			  lp->descriptors, lp->descriptors_laddr);
>  out:
…

How do you think about the possibility to use the label “free_dma”?

Regards,
Markus

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

> While at it, rename a label in order to be slightly more informative and
> split some too long lines.

Would you like to add the tag “Fixes” to the change description?


…
> +++ b/drivers/net/ethernet/natsemi/macsonic.c
> @@ -506,10 +506,14 @@ static int mac_sonic_platform_probe(struct platform_device *pdev)
>
>  	err = register_netdev(dev);
>  	if (err)
> -		goto out;
> +		goto undo_probe1;
>
>  	return 0;
>
> +undo_probe1:
> +	dma_free_coherent(lp->device,
> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> +			  lp->descriptors, lp->descriptors_laddr);
>  out:
…

How do you think about the possibility to use the label “free_dma”?

Regards,
Markus

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Christophe JAILLET]

Le 09/05/2020 à 03:54, Jakub Kicinski a écrit :
> On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
>> @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
>>   	struct sonic_local* lp = netdev_priv(dev);
>>   
>>   	unregister_netdev(dev);
>> -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>> -	                  lp->descriptors, lp->descriptors_laddr);
>> +	dma_free_coherent(lp->device,
>> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>> +			  lp->descriptors, lp->descriptors_laddr);
>>   	free_netdev(dev);
>>   
>>   	return 0;
> This is a white-space only change, right? Since this is a fix we should
> avoid making cleanups which are not strictly necessary.
>
Right.

The reason of this clean-up is that I wanted to avoid a checkpatch 
warning with the proposed patch and I felt that having the same layout 
in the error handling path of the probe function and in the remove 
function was clearer.
So I updated also the remove function.

Fell free to ignore this hunk if not desired. I will not sent a V2 only 
for that.

CJ

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Christophe JAILLET]

Le 09/05/2020 à 03:54, Jakub Kicinski a écrit :
> On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
>> @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
>>   	struct sonic_local* lp = netdev_priv(dev);
>>   
>>   	unregister_netdev(dev);
>> -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>> -	                  lp->descriptors, lp->descriptors_laddr);
>> +	dma_free_coherent(lp->device,
>> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>> +			  lp->descriptors, lp->descriptors_laddr);
>>   	free_netdev(dev);
>>   
>>   	return 0;
> This is a white-space only change, right? Since this is a fix we should
> avoid making cleanups which are not strictly necessary.
>
Right.

The reason of this clean-up is that I wanted to avoid a checkpatch 
warning with the proposed patch and I felt that having the same layout 
in the error handling path of the probe function and in the remove 
function was clearer.
So I updated also the remove function.

Fell free to ignore this hunk if not desired. I will not sent a V2 only 
for that.

CJ

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Jakub Kicinski]

On Sat, 9 May 2020 18:47:08 +0200 Christophe JAILLET wrote:
> Le 09/05/2020 à 03:54, Jakub Kicinski a écrit :
> > On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:  
> >> @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
> >>   	struct sonic_local* lp = netdev_priv(dev);
> >>   
> >>   	unregister_netdev(dev);
> >> -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> >> -	                  lp->descriptors, lp->descriptors_laddr);
> >> +	dma_free_coherent(lp->device,
> >> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> >> +			  lp->descriptors, lp->descriptors_laddr);
> >>   	free_netdev(dev);
> >>   
> >>   	return 0;  
> > This is a white-space only change, right? Since this is a fix we should
> > avoid making cleanups which are not strictly necessary.
>
> Right.
> 
> The reason of this clean-up is that I wanted to avoid a checkpatch 
> warning with the proposed patch and I felt that having the same layout 
> in the error handling path of the probe function and in the remove 
> function was clearer.
> So I updated also the remove function.

I understand the motivation is good.

> Fell free to ignore this hunk if not desired. I will not sent a V2 only 
> for that.

That's not how it works. Busy maintainers don't have time to hand edit
patches. I'm not applying this to the networking tree and I'm tossing it
from patchwork. Please address the basic feedback.

Thank you.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Jakub Kicinski]

On Sat, 9 May 2020 18:47:08 +0200 Christophe JAILLET wrote:
> Le 09/05/2020 à 03:54, Jakub Kicinski a écrit :
> > On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:  
> >> @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
> >>   	struct sonic_local* lp = netdev_priv(dev);
> >>   
> >>   	unregister_netdev(dev);
> >> -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> >> -	                  lp->descriptors, lp->descriptors_laddr);
> >> +	dma_free_coherent(lp->device,
> >> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> >> +			  lp->descriptors, lp->descriptors_laddr);
> >>   	free_netdev(dev);
> >>   
> >>   	return 0;  
> > This is a white-space only change, right? Since this is a fix we should
> > avoid making cleanups which are not strictly necessary.
>
> Right.
> 
> The reason of this clean-up is that I wanted to avoid a checkpatch 
> warning with the proposed patch and I felt that having the same layout 
> in the error handling path of the probe function and in the remove 
> function was clearer.
> So I updated also the remove function.

I understand the motivation is good.

> Fell free to ignore this hunk if not desired. I will not sent a V2 only 
> for that.

That's not how it works. Busy maintainers don't have time to hand edit
patches. I'm not applying this to the networking tree and I'm tossing it
from patchwork. Please address the basic feedback.

Thank you.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Christophe JAILLET]

Le 09/05/2020 à 20:13, Jakub Kicinski a écrit :
> On Sat, 9 May 2020 18:47:08 +0200 Christophe JAILLET wrote:
>> Le 09/05/2020 à 03:54, Jakub Kicinski a écrit :
>>> On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
>>>> @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
>>>>    	struct sonic_local* lp = netdev_priv(dev);
>>>>    
>>>>    	unregister_netdev(dev);
>>>> -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>>>> -	                  lp->descriptors, lp->descriptors_laddr);
>>>> +	dma_free_coherent(lp->device,
>>>> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>>>> +			  lp->descriptors, lp->descriptors_laddr);
>>>>    	free_netdev(dev);
>>>>    
>>>>    	return 0;
>>> This is a white-space only change, right? Since this is a fix we should
>>> avoid making cleanups which are not strictly necessary.
>> Right.
>>
>> The reason of this clean-up is that I wanted to avoid a checkpatch
>> warning with the proposed patch and I felt that having the same layout
>> in the error handling path of the probe function and in the remove
>> function was clearer.
>> So I updated also the remove function.
> I understand the motivation is good.
>
>> Fell free to ignore this hunk if not desired. I will not sent a V2 only
>> for that.
> That's not how it works. Busy maintainers don't have time to hand edit
> patches. I'm not applying this to the networking tree and I'm tossing it
> from patchwork. Please address the basic feedback.
>
> Thank you.
>
Hi,

that's not the way you would like it to work.
It happens that some maintainers make some small adjustments in the 
commit message or the patch itself.

The patch is good enough for me. If you can not accept the additional 
small clean-up, or don't have time to tweak it by yourself, or by anyone 
else, please, just reject it.
The issue I propose to fix is minor and unlikely to happen anyway.

If anyone else cares to update the proposal, please do.


I don't want to discuss your motivation, I understand them.

But please, do also understand mine and do not require too futile things 
from hobbyists.

Spending time only to remove a CR because it does not match your quality 
standard or your expectation of what a patch is, is of no interest for me.
That's why I told I would not send a V2.


It is up to you to accept it as-is, update it or reject it, according to 
the value you think this patch has.

Hoping for your understanding and sorry for wasting your time.

Best regards,
CJ

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Christophe JAILLET]

Le 09/05/2020 à 20:13, Jakub Kicinski a écrit :
> On Sat, 9 May 2020 18:47:08 +0200 Christophe JAILLET wrote:
>> Le 09/05/2020 à 03:54, Jakub Kicinski a écrit :
>>> On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
>>>> @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
>>>>    	struct sonic_local* lp = netdev_priv(dev);
>>>>    
>>>>    	unregister_netdev(dev);
>>>> -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>>>> -	                  lp->descriptors, lp->descriptors_laddr);
>>>> +	dma_free_coherent(lp->device,
>>>> +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
>>>> +			  lp->descriptors, lp->descriptors_laddr);
>>>>    	free_netdev(dev);
>>>>    
>>>>    	return 0;
>>> This is a white-space only change, right? Since this is a fix we should
>>> avoid making cleanups which are not strictly necessary.
>> Right.
>>
>> The reason of this clean-up is that I wanted to avoid a checkpatch
>> warning with the proposed patch and I felt that having the same layout
>> in the error handling path of the probe function and in the remove
>> function was clearer.
>> So I updated also the remove function.
> I understand the motivation is good.
>
>> Fell free to ignore this hunk if not desired. I will not sent a V2 only
>> for that.
> That's not how it works. Busy maintainers don't have time to hand edit
> patches. I'm not applying this to the networking tree and I'm tossing it
> from patchwork. Please address the basic feedback.
>
> Thank you.
>
Hi,

that's not the way you would like it to work.
It happens that some maintainers make some small adjustments in the 
commit message or the patch itself.

The patch is good enough for me. If you can not accept the additional 
small clean-up, or don't have time to tweak it by yourself, or by anyone 
else, please, just reject it.
The issue I propose to fix is minor and unlikely to happen anyway.

If anyone else cares to update the proposal, please do.


I don't want to discuss your motivation, I understand them.

But please, do also understand mine and do not require too futile things 
from hobbyists.

Spending time only to remove a CR because it does not match your quality 
standard or your expectation of what a patch is, is of no interest for me.
That's why I told I would not send a V2.


It is up to you to accept it as-is, update it or reject it, according to 
the value you think this patch has.

Hoping for your understanding and sorry for wasting your time.

Best regards,
CJ

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Joe Perches]

On Sat, 2020-05-09 at 11:13 -0700, Jakub Kicinski wrote:
> On Sat, 9 May 2020 18:47:08 +0200 Christophe JAILLET wrote:
> > Le 09/05/2020 à 03:54, Jakub Kicinski a écrit :
> > > On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:  
> > > > @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
> > > >   	struct sonic_local* lp = netdev_priv(dev);
> > > >   
> > > >   	unregister_netdev(dev);
> > > > -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > > > -	                  lp->descriptors, lp->descriptors_laddr);
> > > > +	dma_free_coherent(lp->device,
> > > > +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > > > +			  lp->descriptors, lp->descriptors_laddr);
> > > >   	free_netdev(dev);
> > > >   
> > > >   	return 0;  
> > > This is a white-space only change, right? Since this is a fix we should
> > > avoid making cleanups which are not strictly necessary.
> > 
> > Right.
> > 
> > The reason of this clean-up is that I wanted to avoid a checkpatch 
> > warning with the proposed patch and I felt that having the same layout 
> > in the error handling path of the probe function and in the remove 
> > function was clearer.
> > So I updated also the remove function.
> 
> I understand the motivation is good.

David, maybe I missed some notification about Jakub's role.

What is Jakub's role in relation to the networking tree?

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Joe Perches]

On Sat, 2020-05-09 at 11:13 -0700, Jakub Kicinski wrote:
> On Sat, 9 May 2020 18:47:08 +0200 Christophe JAILLET wrote:
> > Le 09/05/2020 à 03:54, Jakub Kicinski a écrit :
> > > On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:  
> > > > @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
> > > >   	struct sonic_local* lp = netdev_priv(dev);
> > > >   
> > > >   	unregister_netdev(dev);
> > > > -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > > > -	                  lp->descriptors, lp->descriptors_laddr);
> > > > +	dma_free_coherent(lp->device,
> > > > +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > > > +			  lp->descriptors, lp->descriptors_laddr);
> > > >   	free_netdev(dev);
> > > >   
> > > >   	return 0;  
> > > This is a white-space only change, right? Since this is a fix we should
> > > avoid making cleanups which are not strictly necessary.
> > 
> > Right.
> > 
> > The reason of this clean-up is that I wanted to avoid a checkpatch 
> > warning with the proposed patch and I felt that having the same layout 
> > in the error handling path of the probe function and in the remove 
> > function was clearer.
> > So I updated also the remove function.
> 
> I understand the motivation is good.

David, maybe I missed some notification about Jakub's role.

What is Jakub's role in relation to the networking tree?

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Sat, 09 May 2020 15:42:36 -0700

> David, maybe I missed some notification about Jakub's role.
> 
> What is Jakub's role in relation to the networking tree?

He is the co-maintainer of the networking tree and you should respect
his actions and feedback as if it were coming from me.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Sat, 09 May 2020 15:42:36 -0700

> David, maybe I missed some notification about Jakub's role.
> 
> What is Jakub's role in relation to the networking tree?

He is the co-maintainer of the networking tree and you should respect
his actions and feedback as if it were coming from me.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Joe Perches]

On Sat, 2020-05-09 at 16:32 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Sat, 09 May 2020 15:42:36 -0700
> 
> > David, maybe I missed some notification about Jakub's role.
> > 
> > What is Jakub's role in relation to the networking tree?
> 
> He is the co-maintainer of the networking tree and you should respect
> his actions and feedback as if it were coming from me.

If he's committing drivers then presumably then
he should be added to this section as well.

NETWORKING DRIVERS
M:	"David S. Miller" <davem@davemloft.net>
L:	netdev@vger.kernel.org
S:	Odd Fixes

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Joe Perches]

On Sat, 2020-05-09 at 16:32 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Sat, 09 May 2020 15:42:36 -0700
> 
> > David, maybe I missed some notification about Jakub's role.
> > 
> > What is Jakub's role in relation to the networking tree?
> 
> He is the co-maintainer of the networking tree and you should respect
> his actions and feedback as if it were coming from me.

If he's committing drivers then presumably then
he should be added to this section as well.

NETWORKING DRIVERS
M:	"David S. Miller" <davem@davemloft.net>
L:	netdev@vger.kernel.org
S:	Odd Fixes

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Sat, 9 May 2020, Markus Elfring wrote:

> > While at it, rename a label in order to be slightly more informative and
> > split some too long lines.
> 
> Would you like to add the tag 'Fixes' to the change description?
> 

Sorry but I don't follow your reasoning here. Are you saying that this 
needs to be pushed out to -stable branches? If so, stable-kernel-rules.rst 
would seem to disagree as the bug is theoretical and isn't bothering 
people.

Is there a way to add a Fixes tag that would not invoke the -stable 
process? And was that what you had in mind?

> 
> > +++ b/drivers/net/ethernet/natsemi/macsonic.c
> > @@ -506,10 +506,14 @@ static int mac_sonic_platform_probe(struct platform_device *pdev)
> >
> >  	err = register_netdev(dev);
> >  	if (err)
> > -		goto out;
> > +		goto undo_probe1;
> >
> >  	return 0;
> >
> > +undo_probe1:
> > +	dma_free_coherent(lp->device,
> > +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > +			  lp->descriptors, lp->descriptors_laddr);
> >  out:
> How do you think about the possibility to use the label 'free_dma'?

I think 'undo_probe1' is both descriptive and consistent with commit 
10e3cc180e64 ("net/sonic: Fix a resource leak in an error handling path in 
'jazz_sonic_probe()'").

Your suggestion, 'free_dma' is also good. But coming up with good 
alternatives is easy. If every good alternative would be considered there 
would be no obvious way to get a patch merged.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Sat, 9 May 2020, Markus Elfring wrote:

> > While at it, rename a label in order to be slightly more informative and
> > split some too long lines.
> 
> Would you like to add the tag 'Fixes' to the change description?
> 

Sorry but I don't follow your reasoning here. Are you saying that this 
needs to be pushed out to -stable branches? If so, stable-kernel-rules.rst 
would seem to disagree as the bug is theoretical and isn't bothering 
people.

Is there a way to add a Fixes tag that would not invoke the -stable 
process? And was that what you had in mind?

> 
> > +++ b/drivers/net/ethernet/natsemi/macsonic.c
> > @@ -506,10 +506,14 @@ static int mac_sonic_platform_probe(struct platform_device *pdev)
> >
> >  	err = register_netdev(dev);
> >  	if (err)
> > -		goto out;
> > +		goto undo_probe1;
> >
> >  	return 0;
> >
> > +undo_probe1:
> > +	dma_free_coherent(lp->device,
> > +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > +			  lp->descriptors, lp->descriptors_laddr);
> >  out:
> How do you think about the possibility to use the label 'free_dma'?

I think 'undo_probe1' is both descriptive and consistent with commit 
10e3cc180e64 ("net/sonic: Fix a resource leak in an error handling path in 
'jazz_sonic_probe()'").

Your suggestion, 'free_dma' is also good. But coming up with good 
alternatives is easy. If every good alternative would be considered there 
would be no obvious way to get a patch merged.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Fri, 8 May 2020, Jakub Kicinski wrote:

> On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
> > @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
> >  	struct sonic_local* lp = netdev_priv(dev);
> >  
> >  	unregister_netdev(dev);
> > -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > -	                  lp->descriptors, lp->descriptors_laddr);
> > +	dma_free_coherent(lp->device,
> > +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > +			  lp->descriptors, lp->descriptors_laddr);
> >  	free_netdev(dev);
> >  
> >  	return 0;
> 
> This is a white-space only change, right? Since this is a fix we should
> avoid making cleanups which are not strictly necessary.
> 

I think it is harmless if it doesn't create any merge conflicts. Any merge 
conflict created by the whitespace change would have happened anyway, 
because all of the changes in this patch are very closely related. That's 
why I was happy to put a 'reviewed-by' tag on this.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Fri, 8 May 2020, Jakub Kicinski wrote:

> On Fri,  8 May 2020 19:25:57 +0200 Christophe JAILLET wrote:
> > @@ -527,8 +531,9 @@ static int mac_sonic_platform_remove(struct platform_device *pdev)
> >  	struct sonic_local* lp = netdev_priv(dev);
> >  
> >  	unregister_netdev(dev);
> > -	dma_free_coherent(lp->device, SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > -	                  lp->descriptors, lp->descriptors_laddr);
> > +	dma_free_coherent(lp->device,
> > +			  SIZEOF_SONIC_DESC * SONIC_BUS_SCALE(lp->dma_bitmode),
> > +			  lp->descriptors, lp->descriptors_laddr);
> >  	free_netdev(dev);
> >  
> >  	return 0;
> 
> This is a white-space only change, right? Since this is a fix we should
> avoid making cleanups which are not strictly necessary.
> 

I think it is harmless if it doesn't create any merge conflicts. Any merge 
conflict created by the whitespace change would have happened anyway, 
because all of the changes in this patch are very closely related. That's 
why I was happy to put a 'reviewed-by' tag on this.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

> Is there a way to add a Fixes tag that would not invoke the -stable
> process? And was that what you had in mind?

Christophe Jaillet proposed to complete the exception handling also for this
function implementation.
I find that such a software correction is qualified for this tag.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n183

Corresponding consequences can vary then according to the change management
of involved developers.


> I think 'undo_probe1' is both descriptive and consistent with commit
> 10e3cc180e64 ("net/sonic: Fix a resource leak in an error handling path in
> 'jazz_sonic_probe()'").

I can agree to this view (in principle).

By the way:
The referenced commit contains the tag “Fixes”.
https://lore.kernel.org/patchwork/patch/1231354/
https://lore.kernel.org/lkml/20200427061803.53857-1-christophe.jaillet@wanadoo.fr/


> Your suggestion, 'free_dma' is also good.

Thanks for your positive feedback.


> But coming up with good alternatives is easy.

But the change acceptance can occasionally become harder.


> If every good alternative would be considered there would be no obvious way
> to get a patch merged.

I imagine that some alternatives can result in preferable solutions, can't they?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460

Regards,
Markus

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

> Is there a way to add a Fixes tag that would not invoke the -stable
> process? And was that what you had in mind?

Christophe Jaillet proposed to complete the exception handling also for this
function implementation.
I find that such a software correction is qualified for this tag.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n183

Corresponding consequences can vary then according to the change management
of involved developers.


> I think 'undo_probe1' is both descriptive and consistent with commit
> 10e3cc180e64 ("net/sonic: Fix a resource leak in an error handling path in
> 'jazz_sonic_probe()'").

I can agree to this view (in principle).

By the way:
The referenced commit contains the tag “Fixes”.
https://lore.kernel.org/patchwork/patch/1231354/
https://lore.kernel.org/lkml/20200427061803.53857-1-christophe.jaillet@wanadoo.fr/


> Your suggestion, 'free_dma' is also good.

Thanks for your positive feedback.


> But coming up with good alternatives is easy.

But the change acceptance can occasionally become harder.


> If every good alternative would be considered there would be no obvious way
> to get a patch merged.

I imagine that some alternatives can result in preferable solutions, can't they?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460

Regards,
Markus

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

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

On Sun, 10 May 2020, Markus Elfring wrote:

> Christophe Jaillet proposed to complete the exception handling also for this
> function implementation.
> I find that such a software correction is qualified for this tag.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n183
> 
> Corresponding consequences can vary then according to the change 
> management of involved developers.
> 

Makes sense.

> > I think 'undo_probe1' is both descriptive and consistent with commit 
> > 10e3cc180e64 ("net/sonic: Fix a resource leak in an error handling 
> > path in 'jazz_sonic_probe()'").
> 
> I can agree to this view (in principle).
> 
> By the way:
> The referenced commit contains the tag “Fixes”.
> https://lore.kernel.org/patchwork/patch/1231354/
> https://lore.kernel.org/lkml/20200427061803.53857-1-christophe.jaillet@wanadoo.fr/
> 

Right, I'd forgotten that. Do you know when these bugs were introduced?

> > Your suggestion, 'free_dma' is also good.
> 
> Thanks for your positive feedback.
> 
> 
> > But coming up with good alternatives is easy.
> 
> But the change acceptance can occasionally become harder.
> 

The path to patch acceptance often takes surprising turns.

> 
> > If every good alternative would be considered there would be no 
> > obvious way to get a patch merged.
> 
> I imagine that some alternatives can result in preferable solutions, 
> can't they?

Naming goto labels is just painting another bikeshed. Yes, some 
alternatives are preferable but it takes too long to identify them and 
finding consensus is unlikely anyway, as it's a matter of taste.

Re: [PATCH] net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

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

On Sun, 10 May 2020, Markus Elfring wrote:

> Christophe Jaillet proposed to complete the exception handling also for this
> function implementation.
> I find that such a software correction is qualified for this tag.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n183
> 
> Corresponding consequences can vary then according to the change 
> management of involved developers.
> 

Makes sense.

> > I think 'undo_probe1' is both descriptive and consistent with commit 
> > 10e3cc180e64 ("net/sonic: Fix a resource leak in an error handling 
> > path in 'jazz_sonic_probe()'").
> 
> I can agree to this view (in principle).
> 
> By the way:
> The referenced commit contains the tag “Fixes”.
> https://lore.kernel.org/patchwork/patch/1231354/
> https://lore.kernel.org/lkml/20200427061803.53857-1-christophe.jaillet@wanadoo.fr/
> 

Right, I'd forgotten that. Do you know when these bugs were introduced?

> > Your suggestion, 'free_dma' is also good.
> 
> Thanks for your positive feedback.
> 
> 
> > But coming up with good alternatives is easy.
> 
> But the change acceptance can occasionally become harder.
> 

The path to patch acceptance often takes surprising turns.

> 
> > If every good alternative would be considered there would be no 
> > obvious way to get a patch merged.
> 
> I imagine that some alternatives can result in preferable solutions, 
> can't they?

Naming goto labels is just painting another bikeshed. Yes, some 
alternatives are preferable but it takes too long to identify them and 
finding consensus is unlikely anyway, as it's a matter of taste.

Re: net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

>> https://lore.kernel.org/lkml/20200427061803.53857-1-christophe.jaillet@wanadoo.fr/
>
> Do you know when these bugs were introduced?

I suggest to take another look at a provided tag “Fixes”.
To which commit would you like to refer to for the proposed adjustment of
the function “mac_sonic_platform_probe”?


> Naming goto labels is just painting another bikeshed. Yes, some
> alternatives are preferable but it takes too long to identify them and
> finding consensus is unlikely anyway, as it's a matter of taste.

Would you find numbered labels unwanted according to a possible interpretation
related to “GW-BASIC” identifier selection?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460

Can programming preferences evolve into the direction of “say what the goto does”?

Regards,
Markus

Re: net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

>> https://lore.kernel.org/lkml/20200427061803.53857-1-christophe.jaillet@wanadoo.fr/
>
> Do you know when these bugs were introduced?

I suggest to take another look at a provided tag “Fixes”.
To which commit would you like to refer to for the proposed adjustment of
the function “mac_sonic_platform_probe”?


> Naming goto labels is just painting another bikeshed. Yes, some
> alternatives are preferable but it takes too long to identify them and
> finding consensus is unlikely anyway, as it's a matter of taste.

Would you find numbered labels unwanted according to a possible interpretation
related to “GW-BASIC” identifier selection?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460

Can programming preferences evolve into the direction of “say what the goto does”?

Regards,
Markus

Re: net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

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

On Sun, 10 May 2020, Markus Elfring wrote:

> >
> > Do you know when these bugs were introduced?
> 
> I suggest to take another look at a provided tag “Fixes”.

If you can't determine when the bug was introduced, how can you criticise 
a patch for the lack of a Fixes tag?

> To which commit would you like to refer to for the proposed adjustment 
> of the function “mac_sonic_platform_probe”?
> 

That was my question to you. We seem to be talking past each other. 
Unforunately I only speak English, so if this misunderstanding is to be 
resolved, you're going to have to try harder to make yourself understood.

> > Naming goto labels is just painting another bikeshed. Yes, some 
> > alternatives are preferable but it takes too long to identify them and 
> > finding consensus is unlikely anyway, as it's a matter of taste.
> 
> Would you find numbered labels unwanted according to a possible 
> interpretation related to 'GW-BASIC' identifier selection?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460
> 

My preference is unimportant here. Therefore, your question must be 
rhetorical. I presume that you mean to assert that Christophe's patch 
breaches the style guide.

However, 'sonic_probe1' is the name of a function. The name of the goto 
label 'undo_probe1' reflects the name of the function.

This is not some sequence of GW-BASIC labels referred to in the style 
guide. And neither does the patch add new functions with numbered names.

> Can programming preferences evolve into the direction of “say what the 
> goto does”?
> 

I could agree that macsonic.c has no function resembling "probe1", and 
that portion of the patch could be improved.

Was that the opinion you were trying to express by way of rhetorical 
questions? I can't tell.

Is it possible for a reviewer to effectively criticise C by use of 
English, when his C ability surpasses his English ability?

You needn't answer that question, but please do consider it.

> Regards,
> Markus
> 

Re: net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

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

On Sun, 10 May 2020, Markus Elfring wrote:

> >
> > Do you know when these bugs were introduced?
> 
> I suggest to take another look at a provided tag “Fixes”.

If you can't determine when the bug was introduced, how can you criticise 
a patch for the lack of a Fixes tag?

> To which commit would you like to refer to for the proposed adjustment 
> of the function “mac_sonic_platform_probe”?
> 

That was my question to you. We seem to be talking past each other. 
Unforunately I only speak English, so if this misunderstanding is to be 
resolved, you're going to have to try harder to make yourself understood.

> > Naming goto labels is just painting another bikeshed. Yes, some 
> > alternatives are preferable but it takes too long to identify them and 
> > finding consensus is unlikely anyway, as it's a matter of taste.
> 
> Would you find numbered labels unwanted according to a possible 
> interpretation related to 'GW-BASIC' identifier selection?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460
> 

My preference is unimportant here. Therefore, your question must be 
rhetorical. I presume that you mean to assert that Christophe's patch 
breaches the style guide.

However, 'sonic_probe1' is the name of a function. The name of the goto 
label 'undo_probe1' reflects the name of the function.

This is not some sequence of GW-BASIC labels referred to in the style 
guide. And neither does the patch add new functions with numbered names.

> Can programming preferences evolve into the direction of “say what the 
> goto does”?
> 

I could agree that macsonic.c has no function resembling "probe1", and 
that portion of the patch could be improved.

Was that the opinion you were trying to express by way of rhetorical 
questions? I can't tell.

Is it possible for a reviewer to effectively criticise C by use of 
English, when his C ability surpasses his English ability?

You needn't answer that question, but please do consider it.

> Regards,
> Markus
> 

Re: net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

> If you can't determine when the bug was introduced,

I might be able to determine also this information.


> how can you criticise a patch for the lack of a Fixes tag?

I dared to point two details out for the discussed patch.


>> To which commit would you like to refer to for the proposed adjustment
>> of the function “mac_sonic_platform_probe”?
>
> That was my question to you. We seem to be talking past each other.

We come along different views for this patch review.
Who is going to add a possible reference for this issue?


>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460
>
> My preference is unimportant here.

It is also relevant here because you added the tag “Reviewed-by”.
https://lore.kernel.org/patchwork/comment/1433193/
https://lkml.org/lkml/2020/5/8/1827


> I presume that you mean to assert that Christophe's patch
> breaches the style guide.

I propose to take such a possibility into account.


> However, 'sonic_probe1' is the name of a function.

The discussed source file does not contain such an identifier.
https://elixir.bootlin.com/linux/v5.7-rc5/source/drivers/net/ethernet/natsemi/macsonic.c#L486
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/natsemi/macsonic.c?id=2ef96a5bb12be62ef75b5828c0aab838ebb29cb8#n486


> This is not some sequence of GW-BASIC labels referred to in the style guide.

I recommend to read the current section “7) Centralized exiting of functions”
once more.


>> Can programming preferences evolve into the direction of “say what the
>> goto does”?
>
> I could agree that macsonic.c has no function resembling "probe1",
> and that portion of the patch could be improved.

I find this feedback interesting.


> Was that the opinion you were trying to express by way of rhetorical
> questions? I can't tell.

Some known factors triggered my suggestion to consider the use of
the label “free_dma”.


> Is it possible for a reviewer to effectively criticise C by use of
> English, when his C ability surpasses his English ability?

We come along possibly usual communication challenges.

Regards,
Markus

Re: net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

> If you can't determine when the bug was introduced,

I might be able to determine also this information.


> how can you criticise a patch for the lack of a Fixes tag?

I dared to point two details out for the discussed patch.


>> To which commit would you like to refer to for the proposed adjustment
>> of the function “mac_sonic_platform_probe”?
>
> That was my question to you. We seem to be talking past each other.

We come along different views for this patch review.
Who is going to add a possible reference for this issue?


>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460
>
> My preference is unimportant here.

It is also relevant here because you added the tag “Reviewed-by”.
https://lore.kernel.org/patchwork/comment/1433193/
https://lkml.org/lkml/2020/5/8/1827


> I presume that you mean to assert that Christophe's patch
> breaches the style guide.

I propose to take such a possibility into account.


> However, 'sonic_probe1' is the name of a function.

The discussed source file does not contain such an identifier.
https://elixir.bootlin.com/linux/v5.7-rc5/source/drivers/net/ethernet/natsemi/macsonic.c#L486
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/natsemi/macsonic.c?id=2ef96a5bb12be62ef75b5828c0aab838ebb29cb8#n486


> This is not some sequence of GW-BASIC labels referred to in the style guide.

I recommend to read the current section “7) Centralized exiting of functions”
once more.


>> Can programming preferences evolve into the direction of “say what the
>> goto does”?
>
> I could agree that macsonic.c has no function resembling "probe1",
> and that portion of the patch could be improved.

I find this feedback interesting.


> Was that the opinion you were trying to express by way of rhetorical
> questions? I can't tell.

Some known factors triggered my suggestion to consider the use of
the label “free_dma”.


> Is it possible for a reviewer to effectively criticise C by use of
> English, when his C ability surpasses his English ability?

We come along possibly usual communication challenges.

Regards,
Markus

Re: net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

>> To which commit would you like to refer to for the proposed adjustment
>> of the function “mac_sonic_platform_probe”?
>
> That was my question to you. We seem to be talking past each other.

I have needed a moment to notice your patch as another constructive response
for this code review.

[PATCH v2] net/sonic: Fix some resource leaks in error handling paths
https://lore.kernel.org/r/3eaa58c16dcf313ff7cb873dcff21659b0ea037d.1589158098.git.fthain@telegraphics.com.au/

Regards,
Markus

Re: net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

>> To which commit would you like to refer to for the proposed adjustment
>> of the function “mac_sonic_platform_probe”?
>
> That was my question to you. We seem to be talking past each other.

I have needed a moment to notice your patch as another constructive response
for this code review.

[PATCH v2] net/sonic: Fix some resource leaks in error handling paths
https://lore.kernel.org/r/3eaa58c16dcf313ff7cb873dcff21659b0ea037d.1589158098.git.fthain@telegraphics.com.au/

Regards,
Markus

Re: net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

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


On Mon, 11 May 2020, Markus Elfring wrote:

> > If you can't determine when the bug was introduced,
> 
> I might be able to determine also this information.
> 

This is tantamount to an admission of duplicity.

> 
> > how can you criticise a patch for the lack of a Fixes tag?
> 
> I dared to point two details out for the discussed patch.
> 

You deliberately chose those two details. You appear to be oblivious to 
your own motives.

> 
> >> To which commit would you like to refer to for the proposed 
> >> adjustment of the function “mac_sonic_platform_probe”?
> >
> > That was my question to you. We seem to be talking past each other.
> 
> We come along different views for this patch review. Who is going to add 
> a possible reference for this issue?
> 

Other opinions are not relevant: I was trying to communicate with you.

> 
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460
> 
> >
> > My preference is unimportant here.
> 
> It is also relevant here because you added the tag “Reviewed-by”. 
> https://lore.kernel.org/patchwork/comment/1433193/ 
> https://lkml.org/lkml/2020/5/8/1827
> 

You have quoted my words out-of-context and twisted their meaning to suit 
your purposes.

> 
> > I presume that you mean to assert that Christophe's patch breaches the 
> > style guide.
> 
> I propose to take such a possibility into account.
> 

This "possibility" was among the reasons why the patch was posted to a 
mailing list by its author. That possibility is a given. If you claim this 
possibility as your motivation, you are being foolish or dishonest.

> 
> > However, 'sonic_probe1' is the name of a function.
> 
> The discussed source file does not contain such an identifier. 
> https://elixir.bootlin.com/linux/v5.7-rc5/source/drivers/net/ethernet/natsemi/macsonic.c#L486 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/natsemi/macsonic.c?id=2ef96a5bb12be62ef75b5828c0aab838ebb29cb8#n486
> 

That's what I told you in my previous email. You're welcome.

> 
> > This is not some sequence of GW-BASIC labels referred to in the style 
> > guide.
> 
> I recommend to read the current section “7) Centralized exiting of 
> functions” once more.
> 

Again, you are proposing a bike shed of a different color.

> 
> >> Can programming preferences evolve into the direction of “say what 
> >> the goto does”?
> >
> > I could agree that macsonic.c has no function resembling "probe1", and 
> > that portion of the patch could be improved.
> 
> I find this feedback interesting.
> 
> 
> > Was that the opinion you were trying to express by way of rhetorical 
> > questions? I can't tell.
> 
> Some known factors triggered my suggestion to consider the use of the 
> label “free_dma”.
> 

If you cannot express or convey your "known factors" then they aren't 
useful.

> 
> > Is it possible for a reviewer to effectively criticise C by use of 
> > English, when his C ability surpasses his English ability?
> 
> We come along possibly usual communication challenges.
> 

That looks like a machine translation. I can't make sense of it, sorry.

> Regards,
> Markus
> 

Markus, if you were to write a patch to improve upon coding-style.rst, who 
should review it?

If you are unable to write or review such a patch, how can you hope to 
adjudicate compliance?

Re: net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

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


On Mon, 11 May 2020, Markus Elfring wrote:

> > If you can't determine when the bug was introduced,
> 
> I might be able to determine also this information.
> 

This is tantamount to an admission of duplicity.

> 
> > how can you criticise a patch for the lack of a Fixes tag?
> 
> I dared to point two details out for the discussed patch.
> 

You deliberately chose those two details. You appear to be oblivious to 
your own motives.

> 
> >> To which commit would you like to refer to for the proposed 
> >> adjustment of the function “mac_sonic_platform_probe”?
> >
> > That was my question to you. We seem to be talking past each other.
> 
> We come along different views for this patch review. Who is going to add 
> a possible reference for this issue?
> 

Other opinions are not relevant: I was trying to communicate with you.

> 
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e99332e7b4cda6e60f5b5916cf9943a79dbef902#n460
> 
> >
> > My preference is unimportant here.
> 
> It is also relevant here because you added the tag “Reviewed-by”. 
> https://lore.kernel.org/patchwork/comment/1433193/ 
> https://lkml.org/lkml/2020/5/8/1827
> 

You have quoted my words out-of-context and twisted their meaning to suit 
your purposes.

> 
> > I presume that you mean to assert that Christophe's patch breaches the 
> > style guide.
> 
> I propose to take such a possibility into account.
> 

This "possibility" was among the reasons why the patch was posted to a 
mailing list by its author. That possibility is a given. If you claim this 
possibility as your motivation, you are being foolish or dishonest.

> 
> > However, 'sonic_probe1' is the name of a function.
> 
> The discussed source file does not contain such an identifier. 
> https://elixir.bootlin.com/linux/v5.7-rc5/source/drivers/net/ethernet/natsemi/macsonic.c#L486 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/natsemi/macsonic.c?id=2ef96a5bb12be62ef75b5828c0aab838ebb29cb8#n486
> 

That's what I told you in my previous email. You're welcome.

> 
> > This is not some sequence of GW-BASIC labels referred to in the style 
> > guide.
> 
> I recommend to read the current section “7) Centralized exiting of 
> functions” once more.
> 

Again, you are proposing a bike shed of a different color.

> 
> >> Can programming preferences evolve into the direction of “say what 
> >> the goto does”?
> >
> > I could agree that macsonic.c has no function resembling "probe1", and 
> > that portion of the patch could be improved.
> 
> I find this feedback interesting.
> 
> 
> > Was that the opinion you were trying to express by way of rhetorical 
> > questions? I can't tell.
> 
> Some known factors triggered my suggestion to consider the use of the 
> label “free_dma”.
> 

If you cannot express or convey your "known factors" then they aren't 
useful.

> 
> > Is it possible for a reviewer to effectively criticise C by use of 
> > English, when his C ability surpasses his English ability?
> 
> We come along possibly usual communication challenges.
> 

That looks like a machine translation. I can't make sense of it, sorry.

> Regards,
> Markus
> 

Markus, if you were to write a patch to improve upon coding-style.rst, who 
should review it?

If you are unable to write or review such a patch, how can you hope to 
adjudicate compliance?

Re: net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

> Markus, if you were to write a patch to improve upon coding-style.rst,
> who should review it?

All involved contributors have got chances to provide constructive comments.
I would be curious who will actually dare to contribute further ideas for this area.


> If you are unable to write or review such a patch, how can you hope to
> adjudicate compliance?

I can also try to achieve more improvements here to see how the available
software documentation will evolve.

Regards,
Markus

Re: net/sonic: Fix some resource leaks in error handling paths

[Markus Elfring]

> Markus, if you were to write a patch to improve upon coding-style.rst,
> who should review it?

All involved contributors have got chances to provide constructive comments.
I would be curious who will actually dare to contribute further ideas for this area.


> If you are unable to write or review such a patch, how can you hope to
> adjudicate compliance?

I can also try to achieve more improvements here to see how the available
software documentation will evolve.

Regards,
Markus

Re: net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Tue, 12 May 2020, Markus Elfring wrote:

> > Markus, if you were to write a patch to improve upon coding-style.rst, 
> > who should review it?
> 
> All involved contributors have got chances to provide constructive 
> comments.

But how could someone be elevated to "involved contributor" if their 
patches were to be blocked by arbitrary application of the rules?

> I would be curious who will actually dare to contribute further ideas 
> for this area.
> 

You seem to be uniquely positioned to do that, if only because you cited 
rules which don't appear to support your objection.

> 
> > If you are unable to write or review such a patch, how can you hope to 
> > adjudicate compliance?
> 
> I can also try to achieve more improvements here to see how the 
> available software documentation will evolve.
> 

When the people who write and review the coding standards are the same 
people who write and review the code, the standards devolve (given the 
prevailing incentives).

> Regards, 
> Markus
> 

Re: net/sonic: Fix some resource leaks in error handling paths

[Finn Thain]

On Tue, 12 May 2020, Markus Elfring wrote:

> > Markus, if you were to write a patch to improve upon coding-style.rst, 
> > who should review it?
> 
> All involved contributors have got chances to provide constructive 
> comments.

But how could someone be elevated to "involved contributor" if their 
patches were to be blocked by arbitrary application of the rules?

> I would be curious who will actually dare to contribute further ideas 
> for this area.
> 

You seem to be uniquely positioned to do that, if only because you cited 
rules which don't appear to support your objection.

> 
> > If you are unable to write or review such a patch, how can you hope to 
> > adjudicate compliance?
> 
> I can also try to achieve more improvements here to see how the 
> available software documentation will evolve.
> 

When the people who write and review the coding standards are the same 
people who write and review the code, the standards devolve (given the 
prevailing incentives).

> Regards, 
> Markus
> 

Re: net/sonic: Software evolution around the application of coding standards

[Markus Elfring]

> When the people who write and review the coding standards are the same
> people who write and review the code, the standards devolve (given the
> prevailing incentives).

A coding style is applied also for Linux software. This coding style
supports some alternatives for implementation details.
Deviations from the recommended style are occasionally tolerated.
But some developers care to improve the compliance with the current standard
at various source code places, don't they?

Regards,
Markus

Re: net/sonic: Software evolution around the application of coding standards

[Markus Elfring]

> When the people who write and review the coding standards are the same
> people who write and review the code, the standards devolve (given the
> prevailing incentives).

A coding style is applied also for Linux software. This coding style
supports some alternatives for implementation details.
Deviations from the recommended style are occasionally tolerated.
But some developers care to improve the compliance with the current standard
at various source code places, don't they?

Regards,
Markus

Re: net/sonic: Software evolution around the application of coding standards

[Finn Thain]

On Wed, 13 May 2020, Markus Elfring wrote:

> some developers care to improve the compliance with the current 
> standard at various source code places, don't they?
> 

This thread appears to be circular. Before I abandon it as folly, perhaps 
you would answer one more question. Out of all of the semantic patches in 
scripts/coccinelle, would you care to cast a vote for the best one?

Re: net/sonic: Software evolution around the application of coding standards

[Finn Thain]

On Wed, 13 May 2020, Markus Elfring wrote:

> some developers care to improve the compliance with the current 
> standard at various source code places, don't they?
> 

This thread appears to be circular. Before I abandon it as folly, perhaps 
you would answer one more question. Out of all of the semantic patches in 
scripts/coccinelle, would you care to cast a vote for the best one?