Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Markus Elfring]

> I think part of the issue is that the script reports a WARNING

How much does this information influence really the stress tolerance
and change resistance (or acceptance) for the presented collateral evolution?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci


> for something that is definitely correct code,

Can related software improvement possibilities be taken into account
again under other circumstances?


> and could instead be simply toned down.

Does this view mean that the mentioned script for the semantic patch language
should get another chance for integration?


> Anyway, FWIW:
>
> Acked-by: Marc Zyngier <maz@kernel.org>

Would you like to share any more constructive feedback?


Will similar source file mass updates be better picked up
by other well-known Linux developers?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Marc Zyngier]

On Sat, 19 Oct 2019 12:35:49 +0100,
Markus Elfring <Markus.Elfring@web.de> wrote:
> 
> > I think part of the issue is that the script reports a WARNING
> 
> How much does this information influence really the stress tolerance
> and change resistance (or acceptance) for the presented collateral evolution?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci

-ENOPARSE.

> > for something that is definitely correct code,
> 
> Can related software improvement possibilities be taken into account
> again under other circumstances?

These patches provide no improvement whatsoever. As pointed out, they
mostly introduce bugs.

> > and could instead be simply toned down.
> 
> Does this view mean that the mentioned script for the semantic patch language
> should get another chance for integration?

Providing Coccinelle scripts that scream about perfectly valid code is
pointless, and the result is actively harmful.

If said script was providing a correct semantic patch instead of being
an incentive for people to churn untested patches that span the whole
tree, that'd be a different story. But that's not what this is about.

> > Anyway, FWIW:
> >
> > Acked-by: Marc Zyngier <maz@kernel.org>
> 
> Would you like to share any more constructive feedback?

No.

> Will similar source file mass updates be better picked up
> by other well-known Linux developers?

Certainly not for the subsystems I maintain.

	M.

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Joe Perches]

On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote:
> Providing Coccinelle scripts that scream about perfectly valid code is
> pointless, and the result is actively harmful.

Doubtful.

If the new code is smaller object code and correct
than the conversion is worthwhile.

fyi:

There are already ~450 uses of this function and maybe
~800 possible additional conversions.

> If said script was providing a correct semantic patch instead of being
> an incentive for people to churn untested patches that span the whole
> tree, that'd be a different story.

Right.


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Julia Lawall]

> If said script was providing a correct semantic patch instead of being
> an incentive for people to churn untested patches that span the whole
> tree, that'd be a different story. But that's not what this is about.

What is the actual incorrectness with the script?

An option could be to adjust the rule such that it can be run with an
extra command line option, like -D developer but is not run by default by
make coccicheck.

thanks,
julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Markus Elfring]

>>> I think part of the issue is that the script reports a WARNING

Would anybody like to change this category to “INFO”?


>> How much does this information influence really the stress tolerance
>> and change resistance (or acceptance) for the presented collateral evolution?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci
>
> -ENOPARSE.

* Automated processes can trigger also big amounts of possible adjustments.

* The software development capacity will vary for affected components
  during the years.

* Implementing changes is a recurring project management task, isn't it?


>>> for something that is definitely correct code,
>>
>> Can related software improvement possibilities be taken into account
>> again under other circumstances?
>
> These patches provide no improvement whatsoever.

* Do you find information from the description of a corresponding
  commit 7945f929f1a77a1c8887a97ca07f87626858ff42
  ("drivers: provide devm_platform_ioremap_resource()") reasonable?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/base/platform.c

* How do you think about to compare any differences with
  software build results?


> As pointed out, they mostly introduce bugs.

Would you like to check any error statistics in more detail?


> Providing Coccinelle scripts that scream about perfectly valid code is pointless,

They usually point opportunities out for further collateral evolution,
don't they?


> and the result is actively harmful.

You might not like some changes for a while.


> If said script was providing a correct semantic patch

I got the impression that this can also happen often enough.
Would you like to check the concrete transformation failure rate once more?


> instead of being an incentive for people to churn untested patches
> that span the whole tree, that'd be a different story.

Various developers got motivated to achieve something (possible improvements?)
also by the means of available software analysis tools.
Mistakes can then happen as usual during such adjustment attempts.


> But that's not what this is about.

I guess that your software development concerns can be clarified a bit more.

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Marc Zyngier]

On Sun, 20 Oct 2019 06:38:30 +0100,
Julia Lawall <julia.lawall@lip6.fr> wrote:
> 
> > If said script was providing a correct semantic patch instead of being
> > an incentive for people to churn untested patches that span the whole
> > tree, that'd be a different story. But that's not what this is about.
> 
> What is the actual incorrectness with the script?

The first thing is that it spits out a "WARNING", which is almost
universally interpreted as something that needs addressing. In this
case, it really doesn't. The suggested helper is only icing sugar, and
the original code is perfectly fine.

The second thing is that it results in people posting patches they
don't even compile, let alone test. There would be a good chance for
these patches to be correct if the script was directly generating
them, but that's unfortunately not the case.

> An option could be to adjust the rule such that it can be run with an
> extra command line option, like -D developer but is not run by default by
> make coccicheck.

Maybe. I'm not sure this will deter people from running these scripts
and sending broken patches anyway. No matter how many safeguards you
put, people will still post broken patches just because they can.

	M.

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Masahiro Yamada]

On Sun, Oct 20, 2019 at 7:13 AM Joe Perches <joe@perches.com> wrote:
>
> On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote:
> > Providing Coccinelle scripts that scream about perfectly valid code is
> > pointless, and the result is actively harmful.
>
> Doubtful.
>
> If the new code is smaller object code and correct
> than the conversion is worthwhile.

I agree.

We use multi-platform defconfig.
I always appreciate the code refactoring
that reduces the object size.



> fyi:
>
> There are already ~450 uses of this function and maybe
> ~800 possible additional conversions.
>
> > If said script was providing a correct semantic patch instead of being
> > an incentive for people to churn untested patches that span the whole
> > tree, that'd be a different story.
>
> Right.
>
>


Alexandre Belloni used
https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2586@www.loen.fr/
as a reference, but this is not the output from coccicheck.
The patch author just created a wrong patch by hand.

The deleted semantic patch supports MODE=patch,
which creates a correct patch, and is useful.


-- 
Best Regards
Masahiro Yamada
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Markus Elfring]

> I always appreciate the code refactoring
> that reduces the object size.

Would you like to compare effects around conversions for
the mentioned wrapper function any more?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 12:40:52AM +0900, Masahiro Yamada wrote:
> On Sun, Oct 20, 2019 at 7:13 AM Joe Perches <joe@perches.com> wrote:
> > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote:

> Alexandre Belloni used
> https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2586@www.loen.fr/
> as a reference, but this is not the output from coccicheck.
> The patch author just created a wrong patch by hand.

Exactly. Removal of the script is a mistake. Like I said before is a healing
(incorrect by the way!) by symptoms.

> The deleted semantic patch supports MODE=patch,
> which creates a correct patch, and is useful.

Right!

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Julia Lawall]

On Fri, 25 Oct 2019, Andy Shevchenko wrote:

> On Fri, Oct 25, 2019 at 12:40:52AM +0900, Masahiro Yamada wrote:
> > On Sun, Oct 20, 2019 at 7:13 AM Joe Perches <joe@perches.com> wrote:
> > > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote:
>
> > Alexandre Belloni used
> > https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2586@www.loen.fr/
> > as a reference, but this is not the output from coccicheck.
> > The patch author just created a wrong patch by hand.
>
> Exactly. Removal of the script is a mistake. Like I said before is a healing
> (incorrect by the way!) by symptoms.
>
> > The deleted semantic patch supports MODE=patch,
> > which creates a correct patch, and is useful.
>
> Right!

I ran it on the version of Linux that still has the script:

fe7d2c23d748e4206f4bef9330d0dff9abed7411

and managed to compile 341 of the generated files in the time I had
available, and all compiled successfully.  I can let it run again, and see
how it goes for the rest.  Perhaps it would be acceptable if there was no
report, and people would be forced to use the generated patch?

If someone is writing lots of patches on this issue by hand, then perhaps
they don't have make coccicheck to produce patches, and then would
overlook this case completely.

If it would be helpful, I could group the generated patches by maintainer
or by subdirectory and send them out, if it would be easier to review them
all at once.

Anyway, the rule is not in the kernel at the moment.  For it's future, I'm
open to whatever people find best.  Personally, I prefer when same things
are done in the same way - it makes the code easier to understand and
makes it simpler to address other issues when they arise.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Masahiro Yamada]

Hi Julia

On Fri, Oct 25, 2019 at 5:38 PM Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
>
> On Fri, 25 Oct 2019, Andy Shevchenko wrote:
>
> > On Fri, Oct 25, 2019 at 12:40:52AM +0900, Masahiro Yamada wrote:
> > > On Sun, Oct 20, 2019 at 7:13 AM Joe Perches <joe@perches.com> wrote:
> > > > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote:
> >
> > > Alexandre Belloni used
> > > https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2586@www.loen.fr/
> > > as a reference, but this is not the output from coccicheck.
> > > The patch author just created a wrong patch by hand.
> >
> > Exactly. Removal of the script is a mistake. Like I said before is a healing
> > (incorrect by the way!) by symptoms.
> >
> > > The deleted semantic patch supports MODE=patch,
> > > which creates a correct patch, and is useful.
> >
> > Right!
>
> I ran it on the version of Linux that still has the script:
>
> fe7d2c23d748e4206f4bef9330d0dff9abed7411
>
> and managed to compile 341 of the generated files in the time I had
> available, and all compiled successfully.

Yeah, this semantic patch did the correct conversion
as its header part showed the confidence.

// Confidence: High



>  I can let it run again, and see
> how it goes for the rest.  Perhaps it would be acceptable if there was no
> report, and people would be forced to use the generated patch?

I do not think this is the right thing.
MODE=report is the default, and it is fine.

>
> If someone is writing lots of patches on this issue by hand, then perhaps
> they don't have make coccicheck to produce patches, and then would
> overlook this case completely.
>
> If it would be helpful, I could group the generated patches by maintainer
> or by subdirectory and send them out, if it would be easier to review them
> all at once.

Yes, please.

Subsystem maintainers trust you,
so I think it will make things move smoothly.

After converting most of files,
I want 283ea345934d277e30c841c577e0e2142b4bfcae reverted.


>
> Anyway, the rule is not in the kernel at the moment.  For it's future, I'm
> open to whatever people find best.  Personally, I prefer when same things
> are done in the same way - it makes the code easier to understand and
> makes it simpler to address other issues when they arise.


We always did the same things in the same way
except commit 283ea345934d277e30c841c577e0e2142b4bfcae




-- 
Best Regards
Masahiro Yamada
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Julia Lawall]

On Tue, 29 Oct 2019, Masahiro Yamada wrote:

> Hi Julia
>
> On Fri, Oct 25, 2019 at 5:38 PM Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> >
> > On Fri, 25 Oct 2019, Andy Shevchenko wrote:
> >
> > > On Fri, Oct 25, 2019 at 12:40:52AM +0900, Masahiro Yamada wrote:
> > > > On Sun, Oct 20, 2019 at 7:13 AM Joe Perches <joe@perches.com> wrote:
> > > > > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote:
> > >
> > > > Alexandre Belloni used
> > > > https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2586@www.loen.fr/
> > > > as a reference, but this is not the output from coccicheck.
> > > > The patch author just created a wrong patch by hand.
> > >
> > > Exactly. Removal of the script is a mistake. Like I said before is a healing
> > > (incorrect by the way!) by symptoms.
> > >
> > > > The deleted semantic patch supports MODE=patch,
> > > > which creates a correct patch, and is useful.
> > >
> > > Right!
> >
> > I ran it on the version of Linux that still has the script:
> >
> > fe7d2c23d748e4206f4bef9330d0dff9abed7411
> >
> > and managed to compile 341 of the generated files in the time I had
> > available, and all compiled successfully.
>
> Yeah, this semantic patch did the correct conversion
> as its header part showed the confidence.
>
> // Confidence: High
>
>
>
> >  I can let it run again, and see
> > how it goes for the rest.  Perhaps it would be acceptable if there was no
> > report, and people would be forced to use the generated patch?
>
> I do not think this is the right thing.
> MODE=report is the default, and it is fine.
>
> >
> > If someone is writing lots of patches on this issue by hand, then perhaps
> > they don't have make coccicheck to produce patches, and then would
> > overlook this case completely.
> >
> > If it would be helpful, I could group the generated patches by maintainer
> > or by subdirectory and send them out, if it would be easier to review them
> > all at once.
>
> Yes, please.
>
> Subsystem maintainers trust you,
> so I think it will make things move smoothly.
>
> After converting most of files,
> I want 283ea345934d277e30c841c577e0e2142b4bfcae reverted.

OK.  I got 477 of the files to compile directly.  I can send patches on
them, and then look into the issues on the remaining ones (probably
configuration issues).

julia

>
>
> >
> > Anyway, the rule is not in the kernel at the moment.  For it's future, I'm
> > open to whatever people find best.  Personally, I prefer when same things
> > are done in the same way - it makes the code easier to understand and
> > makes it simpler to address other issues when they arise.
>
>
> We always did the same things in the same way
> except commit 283ea345934d277e30c841c577e0e2142b4bfcae
>
>
>
>
> --
> Best Regards
> Masahiro Yamada
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script

[Markus Elfring]

> What are the additional effects?

I suggest to take another look at the commit 7945f929f1a77a1c8887a97ca07f87626858ff42
("drivers: provide devm_platform_ioremap_resource()" from 2019-02-20)
which triggered the discussed software evolution.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/base/platform.c


> What is the end goal of converting all the existing drivers to devm_platform_ioremap_resource?

It was accepted by well-known Linux developers to put two function calls
into another wrapper function.


> This is not an evolution, it is unnecessary churn. Those patches have no
> benefit and eat up very valuable reviewer time.

I am curious if other contributors would like to describe more variants
of software development opinions in affected areas.


>> How will such feedback influence the development and integration of
>> further scripts for the semantic patch language (Coccinelle software)?
>
> There are a few other scripts that have no added value when applied to
> existing code, like ptr_ret.cocci.

Would you like to clarify concerns around such source code transformation
approaches in more detail?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci