kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
@ 2020-05-31  7:37 Christophe JAILLET
  2020-06-01  8:58 ` Robert Jarzmik
  2020-06-03 22:05 ` Linus Walleij
  0 siblings, 2 replies; 18+ messages in thread
From: Christophe JAILLET @ 2020-05-31  7:37 UTC (permalink / raw)
  To: daniel, haojian.zhuang, robert.jarzmik, linus.walleij
  Cc: linux-gpio, Christophe JAILLET, kernel-janitors, linux-kernel,
	linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-05-31  7:37 [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken Christophe JAILLET
@ 2020-06-01  8:58 ` Robert Jarzmik
  2020-06-01 11:31   ` Christophe JAILLET
  2020-06-03 22:05 ` Linus Walleij
  1 sibling, 1 reply; 18+ messages in thread
From: Robert Jarzmik @ 2020-06-01  8:58 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: linus.walleij, kernel-janitors, linux-kernel, haojian.zhuang,
	linux-gpio, daniel, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-01  8:58 ` Robert Jarzmik
@ 2020-06-01 11:31   ` Christophe JAILLET
  2020-06-01 18:31     ` Dan Carpenter
  0 siblings, 1 reply; 18+ messages in thread
From: Christophe JAILLET @ 2020-06-01 11:31 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: linus.walleij, kernel-janitors, linux-kernel, haojian.zhuang,
	linux-gpio, daniel, linux-arm-kernel

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
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-01 11:31   ` Christophe JAILLET
@ 2020-06-01 18:31     ` Dan Carpenter
  2020-06-03 22:08       ` Linus Walleij
  0 siblings, 1 reply; 18+ messages in thread
From: Dan Carpenter @ 2020-06-01 18:31 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: linus.walleij, kernel-janitors, linux-kernel, haojian.zhuang,
	linux-gpio, daniel, Robert Jarzmik, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-05-31  7:37 [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken Christophe JAILLET
  2020-06-01  8:58 ` Robert Jarzmik
@ 2020-06-03 22:05 ` Linus Walleij
  1 sibling, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2020-06-03 22:05 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: kernel-janitors, linux-kernel, Haojian Zhuang,
	open list:GPIO SUBSYSTEM, Daniel Mack, Robert Jarzmik, Linux ARM

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-01 18:31     ` Dan Carpenter
@ 2020-06-03 22:08       ` Linus Walleij
  2020-06-04  8:31         ` Dan Carpenter
  0 siblings, 1 reply; 18+ messages in thread
From: Linus Walleij @ 2020-06-03 22:08 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: kernel-janitors, linux-kernel, Haojian Zhuang,
	open list:GPIO SUBSYSTEM, Christophe JAILLET, Daniel Mack,
	Robert Jarzmik, Linux ARM

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-03 22:08       ` Linus Walleij
@ 2020-06-04  8:31         ` Dan Carpenter
  2020-06-04  9:17           ` Joe Perches
  0 siblings, 1 reply; 18+ messages in thread
From: Dan Carpenter @ 2020-06-04  8:31 UTC (permalink / raw)
  To: Linus Walleij
  Cc: kernel-janitors, linux-kernel, Haojian Zhuang,
	open list:GPIO SUBSYSTEM, Christophe JAILLET, Daniel Mack,
	Robert Jarzmik, Linux ARM

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04  8:31         ` Dan Carpenter
@ 2020-06-04  9:17           ` Joe Perches
  2020-06-04  9:52             ` Julia Lawall
  0 siblings, 1 reply; 18+ messages in thread
From: Joe Perches @ 2020-06-04  9:17 UTC (permalink / raw)
  To: Dan Carpenter, Linus Walleij
  Cc: kernel-janitors, linux-kernel, Haojian Zhuang,
	open list:GPIO SUBSYSTEM, Christophe JAILLET, Daniel Mack,
	Robert Jarzmik, Linux ARM

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:
 

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04  9:17           ` Joe Perches
@ 2020-06-04  9:52             ` Julia Lawall
  2020-06-04 10:00               ` Joe Perches
  0 siblings, 1 reply; 18+ messages in thread
From: Julia Lawall @ 2020-06-04  9:52 UTC (permalink / raw)
  To: Joe Perches
  Cc: Linux ARM, Linus Walleij, kernel-janitors, linux-kernel,
	Haojian Zhuang, open list:GPIO SUBSYSTEM, Christophe JAILLET,
	Dan Carpenter, Robert Jarzmik, Daniel Mack



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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04  9:52             ` Julia Lawall
@ 2020-06-04 10:00               ` Joe Perches
  2020-06-04 10:33                 ` Julia Lawall
  0 siblings, 1 reply; 18+ messages in thread
From: Joe Perches @ 2020-06-04 10:00 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Linux ARM, Linus Walleij, kernel-janitors, linux-kernel,
	Haojian Zhuang, open list:GPIO SUBSYSTEM, Christophe JAILLET,
	Dan Carpenter, Robert Jarzmik, Daniel Mack

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?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04 10:00               ` Joe Perches
@ 2020-06-04 10:33                 ` Julia Lawall
  2020-06-04 11:08                   ` Joe Perches
  0 siblings, 1 reply; 18+ messages in thread
From: Julia Lawall @ 2020-06-04 10:33 UTC (permalink / raw)
  To: Joe Perches
  Cc: Linux ARM, Linus Walleij, kernel-janitors, linux-kernel,
	Haojian Zhuang, open list:GPIO SUBSYSTEM, Christophe JAILLET,
	Dan Carpenter, Robert Jarzmik, Daniel Mack



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?
>
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04 10:33                 ` Julia Lawall
@ 2020-06-04 11:08                   ` Joe Perches
  2020-06-04 11:42                     ` Julia Lawall
  0 siblings, 1 reply; 18+ messages in thread
From: Joe Perches @ 2020-06-04 11:08 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Linux ARM, Linus Walleij, kernel-janitors, linux-kernel,
	Haojian Zhuang, open list:GPIO SUBSYSTEM, Christophe JAILLET,
	Dan Carpenter, Robert Jarzmik, Daniel Mack

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04 11:08                   ` Joe Perches
@ 2020-06-04 11:42                     ` Julia Lawall
  2020-06-04 12:30                       ` Dan Carpenter
  0 siblings, 1 reply; 18+ messages in thread
From: Julia Lawall @ 2020-06-04 11:42 UTC (permalink / raw)
  To: Joe Perches
  Cc: Linux ARM, Linus Walleij, kernel-janitors, linux-kernel,
	Haojian Zhuang, Julia Lawall, open list:GPIO SUBSYSTEM,
	Christophe JAILLET, Dan Carpenter, Robert Jarzmik, Daniel Mack



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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04 11:42                     ` Julia Lawall
@ 2020-06-04 12:30                       ` Dan Carpenter
  2020-06-04 16:08                         ` Joe Perches
  0 siblings, 1 reply; 18+ messages in thread
From: Dan Carpenter @ 2020-06-04 12:30 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Linus Walleij, kernel-janitors, linux-kernel, Haojian Zhuang,
	open list:GPIO SUBSYSTEM, Christophe JAILLET, Linux ARM,
	Joe Perches, Robert Jarzmik, Daniel Mack

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04 12:30                       ` Dan Carpenter
@ 2020-06-04 16:08                         ` Joe Perches
  2020-06-04 16:29                           ` Julia Lawall
  2020-06-04 17:35                           ` Dan Carpenter
  0 siblings, 2 replies; 18+ messages in thread
From: Joe Perches @ 2020-06-04 16:08 UTC (permalink / raw)
  To: Dan Carpenter, Julia Lawall
  Cc: Linus Walleij, kernel-janitors, linux-kernel, Haojian Zhuang,
	open list:GPIO SUBSYSTEM, Christophe JAILLET, Daniel Mack,
	Robert Jarzmik, Linux ARM

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04 16:08                         ` Joe Perches
@ 2020-06-04 16:29                           ` Julia Lawall
  2020-06-04 17:35                           ` Dan Carpenter
  1 sibling, 0 replies; 18+ messages in thread
From: Julia Lawall @ 2020-06-04 16:29 UTC (permalink / raw)
  To: Joe Perches
  Cc: Linux ARM, Linus Walleij, kernel-janitors, linux-kernel,
	Haojian Zhuang, Julia Lawall, open list:GPIO SUBSYSTEM,
	Christophe JAILLET, Dan Carpenter, Robert Jarzmik, Daniel Mack



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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04 16:08                         ` Joe Perches
  2020-06-04 16:29                           ` Julia Lawall
@ 2020-06-04 17:35                           ` Dan Carpenter
  2020-06-04 18:02                             ` Joe Perches
  1 sibling, 1 reply; 18+ messages in thread
From: Dan Carpenter @ 2020-06-04 17:35 UTC (permalink / raw)
  To: Joe Perches
  Cc: Linus Walleij, kernel-janitors, linux-kernel, Haojian Zhuang,
	Julia Lawall, open list:GPIO SUBSYSTEM, Christophe JAILLET,
	Linux ARM, Robert Jarzmik, Daniel Mack

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken
  2020-06-04 17:35                           ` Dan Carpenter
@ 2020-06-04 18:02                             ` Joe Perches
  0 siblings, 0 replies; 18+ messages in thread
From: Joe Perches @ 2020-06-04 18:02 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Linus Walleij, kernel-janitors, linux-kernel, Haojian Zhuang,
	Julia Lawall, open list:GPIO SUBSYSTEM, Christophe JAILLET,
	Linux ARM, Robert Jarzmik, Daniel Mack

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")

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-06-04 18:02 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-31  7:37 [PATCH] pinctrl: pxa: pxa2xx: Remove 'pxa2xx_pinctrl_exit()' which is unused and broken Christophe JAILLET
2020-06-01  8:58 ` Robert Jarzmik
2020-06-01 11:31   ` Christophe JAILLET
2020-06-01 18:31     ` Dan Carpenter
2020-06-03 22:08       ` Linus Walleij
2020-06-04  8:31         ` Dan Carpenter
2020-06-04  9:17           ` Joe Perches
2020-06-04  9:52             ` Julia Lawall
2020-06-04 10:00               ` Joe Perches
2020-06-04 10:33                 ` Julia Lawall
2020-06-04 11:08                   ` Joe Perches
2020-06-04 11:42                     ` Julia Lawall
2020-06-04 12:30                       ` Dan Carpenter
2020-06-04 16:08                         ` Joe Perches
2020-06-04 16:29                           ` Julia Lawall
2020-06-04 17:35                           ` Dan Carpenter
2020-06-04 18:02                             ` Joe Perches
2020-06-03 22:05 ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).