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 <[email protected]>
Signed-off-by: Christophe JAILLET <[email protected]>
---
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
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.
?
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.
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).
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!
> 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
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
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.
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
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?
From: Joe Perches <[email protected]>
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.
On Sat, 2020-05-09 at 16:32 -0700, David Miller wrote:
> From: Joe Perches <[email protected]>
> 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" <[email protected]>
L: [email protected]
S: Odd Fixes
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.
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.
> 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/[email protected]/
> 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
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/[email protected]/
>
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.
>> https://lore.kernel.org/lkml/[email protected]/
>
> 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
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
>
> 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
>> 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
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?
> 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
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
>
> 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
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?