[PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Christophe JAILLET]

Commit 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
has turned a 'pinctrl_register()' into 'devm_pinctrl_register()' in
'pxa2xx_pinctrl_init()'.
However, the corresponding 'pinctrl_unregister()' call in
'pxa2xx_pinctrl_exit()' has not been removed.

This is not an issue, because 'pxa2xx_pinctrl_exit()' is unused.
Remove it now to avoid some wondering in the future and save a few LoC.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
If some some reason the function should be kept, at least it should be
only 'return 0;'
---
 drivers/pinctrl/pxa/pinctrl-pxa2xx.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
index bddf2c5dd3bf..eab029a21643 100644
--- a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
+++ b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
@@ -425,15 +425,6 @@ int pxa2xx_pinctrl_init(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(pxa2xx_pinctrl_init);
 
-int pxa2xx_pinctrl_exit(struct platform_device *pdev)
-{
-	struct pxa_pinctrl *pctl = platform_get_drvdata(pdev);
-
-	pinctrl_unregister(pctl->pctl_dev);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(pxa2xx_pinctrl_exit);
-
 MODULE_AUTHOR("Robert Jarzmik <robert.jarzmik@free.fr>");
 MODULE_DESCRIPTION("Marvell PXA2xx pinctrl driver");
 MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Robert Jarzmik]

Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:

> Commit 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
> has turned a 'pinctrl_register()' into 'devm_pinctrl_register()' in
> 'pxa2xx_pinctrl_init()'.
> However, the corresponding 'pinctrl_unregister()' call in
> 'pxa2xx_pinctrl_exit()' has not been removed.
>
> This is not an issue, because 'pxa2xx_pinctrl_exit()' is unused.
> Remove it now to avoid some wondering in the future and save a few LoC.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Would be even a better patch with a :
Fixes: 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")

Cheers.

--
Robert

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Christophe JAILLET]

Le 01/06/2020 à 10:58, Robert Jarzmik a écrit :
> Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
>
>> Commit 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
>> has turned a 'pinctrl_register()' into 'devm_pinctrl_register()' in
>> 'pxa2xx_pinctrl_init()'.
>> However, the corresponding 'pinctrl_unregister()' call in
>> 'pxa2xx_pinctrl_exit()' has not been removed.
>>
>> This is not an issue, because 'pxa2xx_pinctrl_exit()' is unused.
>> Remove it now to avoid some wondering in the future and save a few LoC.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> Would be even a better patch with a :
> Fixes: 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")

I was wondering it was was needed in this case.
The patch does not really fix anything, as the function is unused. Or it 
fixes things on a theoretical point of view.

CJ


> Cheers.
>
> --
> Robert
>

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Dan Carpenter]

On Mon, Jun 01, 2020 at 01:31:23PM +0200, Christophe JAILLET wrote:
> Le 01/06/2020 à 10:58, Robert Jarzmik a écrit :
> > Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
> > 
> > > Commit 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
> > > has turned a 'pinctrl_register()' into 'devm_pinctrl_register()' in
> > > 'pxa2xx_pinctrl_init()'.
> > > However, the corresponding 'pinctrl_unregister()' call in
> > > 'pxa2xx_pinctrl_exit()' has not been removed.
> > > 
> > > This is not an issue, because 'pxa2xx_pinctrl_exit()' is unused.
> > > Remove it now to avoid some wondering in the future and save a few LoC.
> > > 
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> > 
> > Would be even a better patch with a :
> > Fixes: 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
> 
> I was wondering it was was needed in this case.
> The patch does not really fix anything, as the function is unused. Or it
> fixes things on a theoretical point of view.

There is no concensus...  We should call a vote on this at Kernel
Summit.  :P

regards,
dan carpenter

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Linus Walleij]

On Sun, May 31, 2020 at 9:37 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:

> Commit 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
> has turned a 'pinctrl_register()' into 'devm_pinctrl_register()' in
> 'pxa2xx_pinctrl_init()'.
> However, the corresponding 'pinctrl_unregister()' call in
> 'pxa2xx_pinctrl_exit()' has not been removed.
>
> This is not an issue, because 'pxa2xx_pinctrl_exit()' is unused.
> Remove it now to avoid some wondering in the future and save a few LoC.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Linus Walleij]

On Mon, Jun 1, 2020 at 8:31 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Jun 01, 2020 at 01:31:23PM +0200, Christophe JAILLET wrote:
> > Le 01/06/2020 à 10:58, Robert Jarzmik a écrit :
> > > Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
> > >
> > > > Commit 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
> > > > has turned a 'pinctrl_register()' into 'devm_pinctrl_register()' in
> > > > 'pxa2xx_pinctrl_init()'.
> > > > However, the corresponding 'pinctrl_unregister()' call in
> > > > 'pxa2xx_pinctrl_exit()' has not been removed.
> > > >
> > > > This is not an issue, because 'pxa2xx_pinctrl_exit()' is unused.
> > > > Remove it now to avoid some wondering in the future and save a few LoC.
> > > >
> > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> > >
> > > Would be even a better patch with a :
> > > Fixes: 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
> >
> > I was wondering it was was needed in this case.
> > The patch does not really fix anything, as the function is unused. Or it
> > fixes things on a theoretical point of view.
>
> There is no concensus...  We should call a vote on this at Kernel
> Summit.  :P

Fixes means it fixes something that was wrong in that commit.
That's all. Whether syntactic or semantic or regression or
serious or not does not matter. It is also not compulsory to
add it is just helpful.

If it is a regression or critical bug, we also add Cc: stable.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Dan Carpenter]

On Thu, Jun 04, 2020 at 12:08:49AM +0200, Linus Walleij wrote:
> On Mon, Jun 1, 2020 at 8:31 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Mon, Jun 01, 2020 at 01:31:23PM +0200, Christophe JAILLET wrote:
> > > Le 01/06/2020 à 10:58, Robert Jarzmik a écrit :
> > > > Christophe JAILLET <christophe.jaillet@wanadoo.fr> writes:
> > > >
> > > > > Commit 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
> > > > > has turned a 'pinctrl_register()' into 'devm_pinctrl_register()' in
> > > > > 'pxa2xx_pinctrl_init()'.
> > > > > However, the corresponding 'pinctrl_unregister()' call in
> > > > > 'pxa2xx_pinctrl_exit()' has not been removed.
> > > > >
> > > > > This is not an issue, because 'pxa2xx_pinctrl_exit()' is unused.
> > > > > Remove it now to avoid some wondering in the future and save a few LoC.
> > > > >
> > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > > Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> > > >
> > > > Would be even a better patch with a :
> > > > Fixes: 6d33ee7a0534 ("pinctrl: pxa: Use devm_pinctrl_register() for pinctrl registration")
> > >
> > > I was wondering it was was needed in this case.
> > > The patch does not really fix anything, as the function is unused. Or it
> > > fixes things on a theoretical point of view.
> >
> > There is no concensus...  We should call a vote on this at Kernel
> > Summit.  :P
> 
> Fixes means it fixes something that was wrong in that commit.
> That's all. Whether syntactic or semantic or regression or
> serious or not does not matter. It is also not compulsory to
> add it is just helpful.

Fixes tag should be compulsory for actual bug fixes.  We had a the
Bad Binder exploit last year because commit f5cb779ba163
("ANDROID: binder: remove waitqueue when thread exits.") had no Fixes
tag and wasn't backported to Android kernels.

regards,
dan carpenter

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Joe Perches]

On Thu, 2020-06-04 at 11:31 +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2020 at 12:08:49AM +0200, Linus Walleij wrote:
[]
> > Fixes means it fixes something that was wrong in that commit.
> > That's all. Whether syntactic or semantic or regression or
> > serious or not does not matter. It is also not compulsory to
> > add it is just helpful.
> 
> Fixes tag should be compulsory for actual bug fixes.  We had a the
> Bad Binder exploit last year because commit f5cb779ba163
> ("ANDROID: binder: remove waitqueue when thread exits.") had no Fixes
> tag and wasn't backported to Android kernels.

Fixes tags IMO should be exclusively for actual bug fixes
and should be mandatory.

Perhaps:
---
 Documentation/process/submitting-patches.rst | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 1699b7f8e63a..285a84ae79de 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -636,12 +636,14 @@ idea was not posted in a public forum. That said, if we diligently credit our
 idea reporters, they will, hopefully, be inspired to help us again in the
 future.
 
-A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
-is used to make it easy to determine where a bug originated, which can help
-review a bug fix. This tag also assists the stable kernel team in determining
-which stable kernel versions should receive your fix. This is the preferred
-method for indicating a bug fixed by the patch. See :ref:`describe_changes`
-for more details.
+A Fixes: tag indicates that the patch fixes a "bug". i.e.: a logic defect or
+regression in a previous commit.  A Fixes: tag should not be used to indicate
+that a previous commit had some trivial defect in spelling in the commit log or
+some whitespace defect.  The Fixes: tag is used to make it easy to determine
+where a bug originated, which can help review a bug fix. The Fixes: tag also
+assists the stable kernel team in determining which stable kernel versions
+should receive your fix. This is the preferred method for indicating a bug is
+fixed by the patch.  See :ref:`describe_changes` for more details.
 
 .. _the_canonical_patch_format:
 

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Julia Lawall]

On Thu, 4 Jun 2020, Joe Perches wrote:

> On Thu, 2020-06-04 at 11:31 +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2020 at 12:08:49AM +0200, Linus Walleij wrote:
> []
> > > Fixes means it fixes something that was wrong in that commit.
> > > That's all. Whether syntactic or semantic or regression or
> > > serious or not does not matter. It is also not compulsory to
> > > add it is just helpful.
> >
> > Fixes tag should be compulsory for actual bug fixes.  We had a the
> > Bad Binder exploit last year because commit f5cb779ba163
> > ("ANDROID: binder: remove waitqueue when thread exits.") had no Fixes
> > tag and wasn't backported to Android kernels.
>
> Fixes tags IMO should be exclusively for actual bug fixes
> and should be mandatory.

I'm not sure that it is always possible to determine the specific commit
that a patch fixes.  Some bugs are too old.  Some bugs may arise from an
interaction of issues.  I don't have a concrete example, but I feel uneasy
about mandator and compulsory.  Neither word is in the proposed text,
though.

Should Fixes also be used when the change will make it hard to port other
fixes over it?

julia

>
> Perhaps:
> ---
>  Documentation/process/submitting-patches.rst | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
> index 1699b7f8e63a..285a84ae79de 100644
> --- a/Documentation/process/submitting-patches.rst
> +++ b/Documentation/process/submitting-patches.rst
> @@ -636,12 +636,14 @@ idea was not posted in a public forum. That said, if we diligently credit our
>  idea reporters, they will, hopefully, be inspired to help us again in the
>  future.
>
> -A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
> -is used to make it easy to determine where a bug originated, which can help
> -review a bug fix. This tag also assists the stable kernel team in determining
> -which stable kernel versions should receive your fix. This is the preferred
> -method for indicating a bug fixed by the patch. See :ref:`describe_changes`
> -for more details.
> +A Fixes: tag indicates that the patch fixes a "bug". i.e.: a logic defect or
> +regression in a previous commit.  A Fixes: tag should not be used to indicate
> +that a previous commit had some trivial defect in spelling in the commit log or
> +some whitespace defect.  The Fixes: tag is used to make it easy to determine
> +where a bug originated, which can help review a bug fix. The Fixes: tag also
> +assists the stable kernel team in determining which stable kernel versions
> +should receive your fix. This is the preferred method for indicating a bug is
> +fixed by the patch.  See :ref:`describe_changes` for more details.
>
>  .. _the_canonical_patch_format:
>
>
>

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Joe Perches]

On Thu, 2020-06-04 at 11:52 +0200, Julia Lawall wrote:
> Should Fixes also be used when the change will make it hard to port other
> fixes over it?

If it's a logic defect or regression that's being fixed,
shouldn't the logic defect or regression be fixed as
reasonably soon as possible?

The nature of the fix should ideally be optimal for
backporting, but I believe that should not stop any
consideration for the standalone fix itself.

What do you think?

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Julia Lawall]

On Thu, 4 Jun 2020, Joe Perches wrote:

> On Thu, 2020-06-04 at 11:52 +0200, Julia Lawall wrote:
> > Should Fixes also be used when the change will make it hard to port other
> > fixes over it?
>
> If it's a logic defect or regression that's being fixed,
> shouldn't the logic defect or regression be fixed as
> reasonably soon as possible?

Sure, but I recall seeing some patches that mentioned that the problem had
existed since the beginning of git.  Of course, it should be rare.

>
> The nature of the fix should ideally be optimal for
> backporting, but I believe that should not stop any
> consideration for the standalone fix itself.

I'm not sure to follow this.  Sometimes non-bug fixes that block
backporting a bug fix have to be backported as well.  So the fixes would
again highlight the range of versions affected by the issue.

julia

> What do you think?
>
>

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Joe Perches]

On Thu, 2020-06-04 at 12:33 +0200, Julia Lawall wrote:
> 
> On Thu, 4 Jun 2020, Joe Perches wrote:
> 
> > On Thu, 2020-06-04 at 11:52 +0200, Julia Lawall wrote:
> > > Should Fixes also be used when the change will make it hard to port other
> > > fixes over it?
> > 
> > If it's a logic defect or regression that's being fixed,
> > shouldn't the logic defect or regression be fixed as
> > reasonably soon as possible?
> 
> Sure, but I recall seeing some patches that mentioned that the problem had
> existed since the beginning of git.  Of course, it should be rare.

git history goes back 15 years already.
There are scant few bugs that old.

There is a tree with even older history that Rob Landley
still has here: https://landley.net/kdocs/fullhist/

It does make git blame research a bit easier for those
rare and extremely old defects.

> > The nature of the fix should ideally be optimal for
> > backporting, but I believe that should not stop any
> > consideration for the standalone fix itself.
> 
> I'm not sure to follow this.

I think it comes down to defects in current need to be
fixed.  Describing
the base commit that is being fixed
is useful for backporting.

I believe it's not reasonable to ask the author of a
fix to research how it could or should be backported.

> Sometimes non-bug fixes that block
> backporting a bug fix have to be backported as well.  So the fixes would
> again highlight the range of versions affected by the issue.

Sure, but the non-bug fixes that may also need backporting
to enable easy backports of the actual fix should not be
described in the Fixes: <commit> as those are  generally
easily researched from a command like:

$ git log <commit>.. <files in fix>

by whoever needs to backport.

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Julia Lawall]

On Thu, 4 Jun 2020, Joe Perches wrote:

> On Thu, 2020-06-04 at 12:33 +0200, Julia Lawall wrote:
> >
> > On Thu, 4 Jun 2020, Joe Perches wrote:
> >
> > > On Thu, 2020-06-04 at 11:52 +0200, Julia Lawall wrote:
> > > > Should Fixes also be used when the change will make it hard to port other
> > > > fixes over it?
> > >
> > > If it's a logic defect or regression that's being fixed,
> > > shouldn't the logic defect or regression be fixed as
> > > reasonably soon as possible?
> >
> > Sure, but I recall seeing some patches that mentioned that the problem had
> > existed since the beginning of git.  Of course, it should be rare.
>
> git history goes back 15 years already.
> There are scant few bugs that old.
>
> There is a tree with even older history that Rob Landley
> still has here: https://landley.net/kdocs/fullhist/
>
> It does make git blame research a bit easier for those
> rare and extremely old defects.
>
> > > The nature of the fix should ideally be optimal for
> > > backporting, but I believe that should not stop any
> > > consideration for the standalone fix itself.
> >
> > I'm not sure to follow this.
>
> I think it comes down to defects in current need to be
> fixed.  Describing
> the base commit that is being fixed
> is useful for backporting.
>
> I believe it's not reasonable to ask the author of a
> fix to research how it could or should be backported.
>
> > Sometimes non-bug fixes that block
> > backporting a bug fix have to be backported as well.  So the fixes would
> > again highlight the range of versions affected by the issue.
>
> Sure, but the non-bug fixes that may also need backporting
> to enable easy backports of the actual fix should not be
> described in the Fixes: <commit> as those are  generally
> easily researched from a command like:
>
> $ git log <commit>.. <files in fix>
>
> by whoever needs to backport.

OK, I recall a discussion with Dan where he suggested that some things
that were not actually bug fixes could also merit a Fixes tag.  But it's
probably better if he weighs in directly.

It would be unfortunate if someone doesn't submit a fix because they can't
figure out how to make the Fixes tag properly, though.

For example, when there is a lot of concurrency involved, some of the bugs
reported by syzkaller can be hard to fully understand.  In the case of a
NULL pointer dereference can a patch only be submitted if it is known
where the NULL comes from, and at what time the reason for producing the
NULL was introduced into the kernel?  Adding a NULL test without fully
understanding how the NULL can arise could reasonably be seen as papering
over a real problem.  But sometimes it could be better to paper over the
problem than incur the problem in a critical situation.

But I agree that these are corner cases.  Hopefully if such a NULL test
was submitted with an explanation on why the submitter was not able to
find the source of the problem and why the problem is important, then the
maintainer could provide some guidance that would resolve the situation.

julia

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Dan Carpenter]

On Thu, Jun 04, 2020 at 01:42:12PM +0200, Julia Lawall wrote:
> OK, I recall a discussion with Dan where he suggested that some things
> that were not actually bug fixes could also merit a Fixes tag.  But it's
> probably better if he weighs in directly.

I generally think Fixes should only be used for "real bug" fixes.

The one exception is when I'm reviewing a patch that fixes an "unused
assignment" static checker warning is that I know which commit
introduced the warning.  I don't have strong feelings if it's in the
Fixes tag or if it's just mentioned in the commit message.  But it
helps me when I'm reviewing if I don't have to grovel through git
history myself.  Also I think that it's always a good exercise for
people fixing patches to look at how the bug was introduced.

regards,
dan carpenter

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Joe Perches]

On Thu, 2020-06-04 at 15:30 +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2020 at 01:42:12PM +0200, Julia Lawall wrote:
> > OK, I recall a discussion with Dan where he suggested that some things
> > that were not actually bug fixes could also merit a Fixes tag.  But it's
> > probably better if he weighs in directly.
> 
> I generally think Fixes should only be used for "real bug" fixes.
> 
> The one exception is when I'm reviewing a patch that fixes an "unused
> assignment" static checker warning is that I know which commit
> introduced the warning.  I don't have strong feelings if it's in the
> Fixes tag or if it's just mentioned in the commit message.

My view is that changes that silence compiler warnings are
not fixing bugs and that these changes should generally not
be backported.

Compiler silencing changes marked as fixes can introduce other
defects in working code.

Backporting patches to stable trees should be conservatively
rather than liberally applied.

It seems that the actual backport maintainers disagree though.

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Julia Lawall]

On Thu, 4 Jun 2020, Joe Perches wrote:

> On Thu, 2020-06-04 at 15:30 +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2020 at 01:42:12PM +0200, Julia Lawall wrote:
> > > OK, I recall a discussion with Dan where he suggested that some things
> > > that were not actually bug fixes could also merit a Fixes tag.  But it's
> > > probably better if he weighs in directly.
> >
> > I generally think Fixes should only be used for "real bug" fixes.
> >
> > The one exception is when I'm reviewing a patch that fixes an "unused
> > assignment" static checker warning is that I know which commit
> > introduced the warning.  I don't have strong feelings if it's in the
> > Fixes tag or if it's just mentioned in the commit message.
>
> My view is that changes that silence compiler warnings are
> not fixing bugs and that these changes should generally not
> be backported.
>
> Compiler silencing changes marked as fixes can introduce other
> defects in working code.
>
> Backporting patches to stable trees should be conservatively
> rather than liberally applied.
>
> It seems that the actual backport maintainers disagree though.

But the rule seemed to be "bug fixing patches must contain a Fixes", and
not "backportable patches must contain a Fixes".

Overall, the relationship between backporting and Fixes is not so clear.

Patches that remove something unnecessary might benefit from having a
Fixes, but might not be worth backporting.

julia

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Dan Carpenter]

On Thu, Jun 04, 2020 at 09:08:44AM -0700, Joe Perches wrote:
> On Thu, 2020-06-04 at 15:30 +0300, Dan Carpenter wrote:
> > On Thu, Jun 04, 2020 at 01:42:12PM +0200, Julia Lawall wrote:
> > > OK, I recall a discussion with Dan where he suggested that some things
> > > that were not actually bug fixes could also merit a Fixes tag.  But it's
> > > probably better if he weighs in directly.
> > 
> > I generally think Fixes should only be used for "real bug" fixes.
> > 
> > The one exception is when I'm reviewing a patch that fixes an "unused
> > assignment" static checker warning is that I know which commit
> > introduced the warning.  I don't have strong feelings if it's in the
> > Fixes tag or if it's just mentioned in the commit message.
> 
> My view is that changes that silence compiler warnings are
> not fixing bugs and that these changes should generally not
> be backported.
> 

The Fixes tag is useful for backports but that's not whole the point of
it.  It's also for collecting metrics.  Also sometimes we fix the bug
before the kernel is released so the Fixes tag means we can automatically
ignore those ones when we look at which patches to backport.

I don't care if the "unused assignment" patches use a Fixes tag or just
mention the commit.  Either way the information is there for when I
review the patch.

regards,
dan carpenter

Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken

[Joe Perches]

On Thu, 2020-06-04 at 20:35 +0300, Dan Carpenter wrote:
> On Thu, Jun 04, 2020 at 09:08:44AM -0700, Joe Perches wrote:
> > On Thu, 2020-06-04 at 15:30 +0300, Dan Carpenter wrote:
> > > On Thu, Jun 04, 2020 at 01:42:12PM +0200, Julia Lawall wrote:
> > > > OK, I recall a discussion with Dan where he suggested that some things
> > > > that were not actually bug fixes could also merit a Fixes tag.  But it's
> > > > probably better if he weighs in directly.
> > > 
> > > I generally think Fixes should only be used for "real bug" fixes.
> > > 
> > > The one exception is when I'm reviewing a patch that fixes an "unused
> > > assignment" static checker warning is that I know which commit
> > > introduced the warning.

Sometimes those warnings are introduced by new compiler
versions.

That's why I don't care for -Werror use in Makefiles.

> > > I don't have strong feelings if it's in the
> > > Fixes tag or if it's just mentioned in the commit message.
> > 
> > My view is that changes that silence compiler warnings are
> > not fixing bugs and that these changes should generally not
> > be backported.
> > 
> The Fixes tag is useful for backports but that's not whole the point of
> it.  It's also for collecting metrics.

Hmm, how are these metrics used?

> Also sometimes we fix the bug
> before the kernel is released so the Fixes tag means we can automatically
> ignore those ones when we look at which patches to backport.
> 
> I don't care if the "unused assignment" patches use a Fixes tag or just
> mention the commit.  Either way the information is there for when I
> review the patch.

Perhaps there could/should be some distinction between
"real bug" fixes and trivialities like "unused assignment"

Maybe something like:
	Updates: <commit> ("commit description")
vs
	Fixes: <commit> ("commit description")