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

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

[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

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

[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

[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

[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

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