After this was made buildable for something other than PPC32, kbuild
starts warning
drivers/usb/host/fhci-hcd.c:398:8: warning: this statement may fall
through [-Wimplicit-fallthrough=]
I don't know this code, but from the construction (initializing size
with 0 and explicitly using "size +=" in the PIPE_BULK case) I assume
that fallthrough is indeed intended.
Reported-by: kbuild test robot <[email protected]>
Fixes: 5a35435ef4e6 (soc: fsl: qe: remove PPC32 dependency from CONFIG_QUICC_ENGINE)
Fixes: a035d552a93b (Makefile: Globally enable fall-through warning)
Signed-off-by: Rasmus Villemoes <[email protected]>
---
Two different Fixes: Obviously my 5a35435ef4e6 is the one that started
making kbuild complain, but that's just because apparently kbuild
doesn't cover a PPC32+USB_FHCI_HCD .config. Note for -stable folks,
just in case 5.3.y is still maintained somewhere: a035d552a93b
appeared in 5.3, but the #define fallthrough that I'm using here
wasn't introduced until 5.4 (294f69e662d15). So either ignore this,
make it /* fallthrough */, or backport 294f69e662d15 to 5.3.y as well.
drivers/usb/host/fhci-hcd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
index 04733876c9c6..a8e1048278d0 100644
--- a/drivers/usb/host/fhci-hcd.c
+++ b/drivers/usb/host/fhci-hcd.c
@@ -396,6 +396,7 @@ static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
case PIPE_CONTROL:
/* 1 td fro setup,1 for ack */
size = 2;
+ fallthrough;
case PIPE_BULK:
/* one td for every 4096 bytes(can be up to 8k) */
size += urb->transfer_buffer_length / 4096;
--
2.23.0
On Thu, Feb 13, 2020 at 09:54:00AM +0100, Rasmus Villemoes wrote:
> After this was made buildable for something other than PPC32, kbuild
> starts warning
>
> drivers/usb/host/fhci-hcd.c:398:8: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
>
> I don't know this code, but from the construction (initializing size
> with 0 and explicitly using "size +=" in the PIPE_BULK case) I assume
> that fallthrough is indeed intended.
>
> Reported-by: kbuild test robot <[email protected]>
> Fixes: 5a35435ef4e6 (soc: fsl: qe: remove PPC32 dependency from CONFIG_QUICC_ENGINE)
> Fixes: a035d552a93b (Makefile: Globally enable fall-through warning)
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
>
> Two different Fixes: Obviously my 5a35435ef4e6 is the one that started
> making kbuild complain, but that's just because apparently kbuild
> doesn't cover a PPC32+USB_FHCI_HCD .config. Note for -stable folks,
> just in case 5.3.y is still maintained somewhere: a035d552a93b
> appeared in 5.3, but the #define fallthrough that I'm using here
> wasn't introduced until 5.4 (294f69e662d15). So either ignore this,
> make it /* fallthrough */, or backport 294f69e662d15 to 5.3.y as well.
>
> drivers/usb/host/fhci-hcd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
> index 04733876c9c6..a8e1048278d0 100644
> --- a/drivers/usb/host/fhci-hcd.c
> +++ b/drivers/usb/host/fhci-hcd.c
> @@ -396,6 +396,7 @@ static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
> case PIPE_CONTROL:
> /* 1 td fro setup,1 for ack */
> size = 2;
> + fallthrough;
We have an attribute for that?
Shouldn't this be /* fall through */ instead?
Gustavo, what's the best practice here, I count only a few
"fallthrough;" instances in the kernel, although one is in our coding
style document, and thousands of the /* */ version.
thanks,
greg k-h
On 13/02/2020 13.56, Greg Kroah-Hartman wrote:
>> diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
>> index 04733876c9c6..a8e1048278d0 100644
>> --- a/drivers/usb/host/fhci-hcd.c
>> +++ b/drivers/usb/host/fhci-hcd.c
>> @@ -396,6 +396,7 @@ static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
>> case PIPE_CONTROL:
>> /* 1 td fro setup,1 for ack */
>> size = 2;
>> + fallthrough;
>
> We have an attribute for that?
>
> Shouldn't this be /* fall through */ instead?
>
> Gustavo, what's the best practice here, I count only a few
> "fallthrough;" instances in the kernel, although one is in our coding
> style document, and thousands of the /* */ version.
Yes, I went with the attribute/macro due to that, and the history is
that Linus applied Joe's patches directly
(https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/),
so I assumed that meant the Penguin decided that the attribute/macro is
the right thing to do for new code, while existing comment annotations
can be left alone or changed piecemeal as code gets refactored anyway.
Rasmus
commit 294f69e662d1 ("compiler_attributes.h: Add 'fallthrough'
pseudo keyword for switch/case use") added the pseudo keyword
so add a test for it to checkpatch.
Warn on a patch or use --strict for files.
Signed-off-by: Joe Perches <[email protected]>
---
scripts/checkpatch.pl | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f3b8434..5579d7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2286,6 +2286,19 @@ sub pos_last_openparen {
return length(expand_tabs(substr($line, 0, $last_openparen))) + 1;
}
+sub get_raw_comment {
+ my ($line, $rawline) = @_;
+ my $comment = '';
+
+ for my $i (0 .. (length($line) - 1)) {
+ if (substr($line, $i, 1) eq "$;") {
+ $comment .= substr($rawline, $i, 1);
+ }
+ }
+
+ return $comment;
+}
+
sub process {
my $filename = shift;
@@ -2447,6 +2460,7 @@ sub process {
$sline =~ s/$;/ /g; #with comments as spaces
my $rawline = $rawlines[$linenr - 1];
+ my $raw_comment = get_raw_comment($line, $rawline);
# check if it's a mode change, rename or start of a patch
if (!$in_commit_log &&
@@ -6403,6 +6417,28 @@ sub process {
}
}
+# check for /* fallthrough */ like comment, prefer fallthrough;
+ my @fallthroughs = (
+ 'fallthrough',
+ '@fallthrough@',
+ 'lint -fallthrough[ \t]*',
+ 'intentional(?:ly)?[ \t]*fall(?:(?:s | |-)[Tt]|t)hr(?:ough|u|ew)',
+ '(?:else,?\s*)?FALL(?:S | |-)?THR(?:OUGH|U|EW)[ \t.!]*(?:-[^\n\r]*)?',
+ 'Fall(?:(?:s | |-)[Tt]|t)hr(?:ough|u|ew)[ \t.!]*(?:-[^\n\r]*)?',
+ 'fall(?:s | |-)?thr(?:ough|u|ew)[ \t.!]*(?:-[^\n\r]*)?',
+ );
+ if ($raw_comment ne '') {
+ foreach my $ft (@fallthroughs) {
+ if ($raw_comment =~ /$ft/) {
+ my $msg_level = \&WARN;
+ $msg_level = \&CHK if ($file);
+ &{$msg_level}("PREFER_FALLTHROUGH",
+ "Prefer 'fallthrough;' over fallthrough comment\n" . $herecurr);
+ last;
+ }
+ }
+ }
+
# check for switch/default statements without a break;
if ($perl_version_ok &&
defined $stat &&
> -----Original Message-----
> From: Rasmus Villemoes <[email protected]>
> Sent: Thursday, February 13, 2020 2:54 AM
> To: Greg Kroah-Hartman <[email protected]>; Timur Tabi
> <[email protected]>; Leo Li <[email protected]>; Rasmus Villemoes
> <[email protected]>; Gustavo A. R. Silva
> <[email protected]>
> Cc: Anton Vorontsov <[email protected]>; kbuild test robot
> <[email protected]>; [email protected]; [email protected]
> Subject: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case
> with fallthrough
>
> After this was made buildable for something other than PPC32, kbuild starts
> warning
>
> drivers/usb/host/fhci-hcd.c:398:8: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
>
> I don't know this code, but from the construction (initializing size with 0 and
> explicitly using "size +=" in the PIPE_BULK case) I assume that fallthrough is
> indeed intended.
>
> Reported-by: kbuild test robot <[email protected]>
> Fixes: 5a35435ef4e6 (soc: fsl: qe: remove PPC32 dependency from
> CONFIG_QUICC_ENGINE)
> Fixes: a035d552a93b (Makefile: Globally enable fall-through warning)
> Signed-off-by: Rasmus Villemoes <[email protected]>
Acked-by: Li Yang <[email protected]>
> ---
>
> Two different Fixes: Obviously my 5a35435ef4e6 is the one that started
> making kbuild complain, but that's just because apparently kbuild doesn't
> cover a PPC32+USB_FHCI_HCD .config. Note for -stable folks, just in case
> 5.3.y is still maintained somewhere: a035d552a93b appeared in 5.3, but the
> #define fallthrough that I'm using here wasn't introduced until 5.4
> (294f69e662d15). So either ignore this, make it /* fallthrough */, or backport
> 294f69e662d15 to 5.3.y as well.
>
> drivers/usb/host/fhci-hcd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c index
> 04733876c9c6..a8e1048278d0 100644
> --- a/drivers/usb/host/fhci-hcd.c
> +++ b/drivers/usb/host/fhci-hcd.c
> @@ -396,6 +396,7 @@ static int fhci_urb_enqueue(struct usb_hcd *hcd,
> struct urb *urb,
> case PIPE_CONTROL:
> /* 1 td fro setup,1 for ack */
> size = 2;
> + fallthrough;
> case PIPE_BULK:
> /* one td for every 4096 bytes(can be up to 8k) */
> size += urb->transfer_buffer_length / 4096;
> --
> 2.23.0
On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote:
> On 13/02/2020 13.56, Greg Kroah-Hartman wrote:
>
> >> diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
> >> index 04733876c9c6..a8e1048278d0 100644
> >> --- a/drivers/usb/host/fhci-hcd.c
> >> +++ b/drivers/usb/host/fhci-hcd.c
> >> @@ -396,6 +396,7 @@ static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
> >> case PIPE_CONTROL:
> >> /* 1 td fro setup,1 for ack */
> >> size = 2;
> >> + fallthrough;
> >
> > We have an attribute for that?
> >
> > Shouldn't this be /* fall through */ instead?
> >
> > Gustavo, what's the best practice here, I count only a few
> > "fallthrough;" instances in the kernel, although one is in our coding
> > style document, and thousands of the /* */ version.
>
> Yes, I went with the attribute/macro due to that, and the history is
> that Linus applied Joe's patches directly
> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/),
> so I assumed that meant the Penguin decided that the attribute/macro is
> the right thing to do for new code, while existing comment annotations
> can be left alone or changed piecemeal as code gets refactored anyway.
But, to be fair, Gustavo went and fixed up thousands of these, with the
/* */ version, not the attribute.
Gustavo, can coverity notice the "fallthrough;" attribute properly? I
don't want to start adding things that end up triggering
false-positives.
thanks,
greg k-h
On 17/02/2020 10.38, Greg Kroah-Hartman wrote:
> On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote:
>> On 13/02/2020 13.56, Greg Kroah-Hartman wrote:
>>
>>> Shouldn't this be /* fall through */ instead?
>>>
>>> Gustavo, what's the best practice here, I count only a few
>>> "fallthrough;" instances in the kernel, although one is in our coding
>>> style document, and thousands of the /* */ version.
>>
>> Yes, I went with the attribute/macro due to that, and the history is
>> that Linus applied Joe's patches directly
>> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/),
>> so I assumed that meant the Penguin decided that the attribute/macro is
>> the right thing to do for new code, while existing comment annotations
>> can be left alone or changed piecemeal as code gets refactored anyway.
>
> But, to be fair, Gustavo went and fixed up thousands of these, with the
> /* */ version, not the attribute.
>
> Gustavo, can coverity notice the "fallthrough;" attribute properly? I
> don't want to start adding things that end up triggering
> false-positives.
I'm not Gustavo, and I don't know the answer, but 1.5 years ago some guy
named greg k-h suggested that coverity does grok the fallthrough attribute:
https://patchwork.kernel.org/cover/10651357/#22279095
Rasmus
On Mon, Feb 17, 2020 at 03:12:21PM +0100, Rasmus Villemoes wrote:
> On 17/02/2020 10.38, Greg Kroah-Hartman wrote:
> > On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote:
> >> On 13/02/2020 13.56, Greg Kroah-Hartman wrote:
> >>
> >>> Shouldn't this be /* fall through */ instead?
> >>>
> >>> Gustavo, what's the best practice here, I count only a few
> >>> "fallthrough;" instances in the kernel, although one is in our coding
> >>> style document, and thousands of the /* */ version.
> >>
> >> Yes, I went with the attribute/macro due to that, and the history is
> >> that Linus applied Joe's patches directly
> >> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/),
> >> so I assumed that meant the Penguin decided that the attribute/macro is
> >> the right thing to do for new code, while existing comment annotations
> >> can be left alone or changed piecemeal as code gets refactored anyway.
> >
> > But, to be fair, Gustavo went and fixed up thousands of these, with the
> > /* */ version, not the attribute.
> >
> > Gustavo, can coverity notice the "fallthrough;" attribute properly? I
> > don't want to start adding things that end up triggering
> > false-positives.
>
> I'm not Gustavo, and I don't know the answer, but 1.5 years ago some guy
> named greg k-h suggested that coverity does grok the fallthrough attribute:
>
> https://patchwork.kernel.org/cover/10651357/#22279095
I wouldn't trust anything that bum says :)
Ok, I don't remember saying that at all, but I'll wait a day or two to
get Gustavo's opinion befor applying the patch.
thanks,
greg k-h
Hi!
Sorry for the late reply. I wasn't aware of this thread until now.
Please, see my comments below...
On 2/17/20 08:18, Greg Kroah-Hartman wrote:
> On Mon, Feb 17, 2020 at 03:12:21PM +0100, Rasmus Villemoes wrote:
>> On 17/02/2020 10.38, Greg Kroah-Hartman wrote:
>>> On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote:
>>>> On 13/02/2020 13.56, Greg Kroah-Hartman wrote:
>>>>
>>>>> Shouldn't this be /* fall through */ instead?
>>>>>
>>>>> Gustavo, what's the best practice here, I count only a few
>>>>> "fallthrough;" instances in the kernel, although one is in our coding
>>>>> style document, and thousands of the /* */ version.
>>>>
>>>> Yes, I went with the attribute/macro due to that, and the history is
>>>> that Linus applied Joe's patches directly
>>>> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/),
>>>> so I assumed that meant the Penguin decided that the attribute/macro is
>>>> the right thing to do for new code, while existing comment annotations
>>>> can be left alone or changed piecemeal as code gets refactored anyway.
>>>
>>> But, to be fair, Gustavo went and fixed up thousands of these, with the
>>> /* */ version, not the attribute.
>>>
>>> Gustavo, can coverity notice the "fallthrough;" attribute properly? I
>>> don't want to start adding things that end up triggering
>>> false-positives.
>>
>> I'm not Gustavo, and I don't know the answer, but 1.5 years ago some guy
>> named greg k-h suggested that coverity does grok the fallthrough attribute:
>>
>> https://patchwork.kernel.org/cover/10651357/#22279095
>
> I wouldn't trust anything that bum says :)
>
> Ok, I don't remember saying that at all, but I'll wait a day or two to
> get Gustavo's opinion befor applying the patch.
>
We are good to go with the 'fallthrough' pseudo keyword. Linus is OK with
that.
The comment annotations will eventually be transformed to "fallthrough;"
Thanks
--
Gustavo
On Mon, Feb 17, 2020 at 10:15:09AM -0600, Gustavo A. R. Silva wrote:
> Hi!
>
> Sorry for the late reply. I wasn't aware of this thread until now.
>
> Please, see my comments below...
>
> On 2/17/20 08:18, Greg Kroah-Hartman wrote:
> > On Mon, Feb 17, 2020 at 03:12:21PM +0100, Rasmus Villemoes wrote:
> >> On 17/02/2020 10.38, Greg Kroah-Hartman wrote:
> >>> On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote:
> >>>> On 13/02/2020 13.56, Greg Kroah-Hartman wrote:
> >>>>
> >>>>> Shouldn't this be /* fall through */ instead?
> >>>>>
> >>>>> Gustavo, what's the best practice here, I count only a few
> >>>>> "fallthrough;" instances in the kernel, although one is in our coding
> >>>>> style document, and thousands of the /* */ version.
> >>>>
> >>>> Yes, I went with the attribute/macro due to that, and the history is
> >>>> that Linus applied Joe's patches directly
> >>>> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/),
> >>>> so I assumed that meant the Penguin decided that the attribute/macro is
> >>>> the right thing to do for new code, while existing comment annotations
> >>>> can be left alone or changed piecemeal as code gets refactored anyway.
> >>>
> >>> But, to be fair, Gustavo went and fixed up thousands of these, with the
> >>> /* */ version, not the attribute.
> >>>
> >>> Gustavo, can coverity notice the "fallthrough;" attribute properly? I
> >>> don't want to start adding things that end up triggering
> >>> false-positives.
> >>
> >> I'm not Gustavo, and I don't know the answer, but 1.5 years ago some guy
> >> named greg k-h suggested that coverity does grok the fallthrough attribute:
> >>
> >> https://patchwork.kernel.org/cover/10651357/#22279095
> >
> > I wouldn't trust anything that bum says :)
> >
> > Ok, I don't remember saying that at all, but I'll wait a day or two to
> > get Gustavo's opinion befor applying the patch.
> >
>
> We are good to go with the 'fallthrough' pseudo keyword. Linus is OK with
> that.
>
> The comment annotations will eventually be transformed to "fallthrough;"
Ok, thanks for the confirmation, will queue this up.
greg k-h
On 2/13/20 06:56, Greg Kroah-Hartman wrote:
> On Thu, Feb 13, 2020 at 09:54:00AM +0100, Rasmus Villemoes wrote:
>> After this was made buildable for something other than PPC32, kbuild
>> starts warning
>>
>> drivers/usb/host/fhci-hcd.c:398:8: warning: this statement may fall
>> through [-Wimplicit-fallthrough=]
>>
>> I don't know this code, but from the construction (initializing size
>> with 0 and explicitly using "size +=" in the PIPE_BULK case) I assume
>> that fallthrough is indeed intended.
>>
>> Reported-by: kbuild test robot <[email protected]>
>> Fixes: 5a35435ef4e6 (soc: fsl: qe: remove PPC32 dependency from CONFIG_QUICC_ENGINE)
>> Fixes: a035d552a93b (Makefile: Globally enable fall-through warning)
By the way, the "Fixes" tag above makes no sense. There is nothing wrong about
that commit. It just enabled the fall-through warning globally. Why would you
"fix" that?
Thanks
--
Gustavo
On 2/13/20 02:54, Rasmus Villemoes wrote:
> After this was made buildable for something other than PPC32, kbuild
> starts warning
>
> drivers/usb/host/fhci-hcd.c:398:8: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
>
> I don't know this code, but from the construction (initializing size
> with 0 and explicitly using "size +=" in the PIPE_BULK case) I assume
> that fallthrough is indeed intended.
>
> Reported-by: kbuild test robot <[email protected]>
> Fixes: 5a35435ef4e6 (soc: fsl: qe: remove PPC32 dependency from CONFIG_QUICC_ENGINE)
> Fixes: a035d552a93b (Makefile: Globally enable fall-through warning)
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
>
> Two different Fixes: Obviously my 5a35435ef4e6 is the one that started
> making kbuild complain, but that's just because apparently kbuild
> doesn't cover a PPC32+USB_FHCI_HCD .config. Note for -stable folks,
> just in case 5.3.y is still maintained somewhere: a035d552a93b
> appeared in 5.3, but the #define fallthrough that I'm using here
> wasn't introduced until 5.4 (294f69e662d15). So either ignore this,
> make it /* fallthrough */, or backport 294f69e662d15 to 5.3.y as well.
>
This patch should not be considered for -stable at all.
Thanks
--
Gustavo
> drivers/usb/host/fhci-hcd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
> index 04733876c9c6..a8e1048278d0 100644
> --- a/drivers/usb/host/fhci-hcd.c
> +++ b/drivers/usb/host/fhci-hcd.c
> @@ -396,6 +396,7 @@ static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
> case PIPE_CONTROL:
> /* 1 td fro setup,1 for ack */
> size = 2;
> + fallthrough;
> case PIPE_BULK:
> /* one td for every 4096 bytes(can be up to 8k) */
> size += urb->transfer_buffer_length / 4096;
>
On Mon, 2020-02-17 at 11:12 -0600, Gustavo A. R. Silva wrote:
>
> On 2/13/20 06:56, Greg Kroah-Hartman wrote:
> > On Thu, Feb 13, 2020 at 09:54:00AM +0100, Rasmus Villemoes wrote:
> > > After this was made buildable for something other than PPC32, kbuild
> > > starts warning
> > >
> > > drivers/usb/host/fhci-hcd.c:398:8: warning: this statement may fall
> > > through [-Wimplicit-fallthrough=]
> > >
> > > I don't know this code, but from the construction (initializing size
> > > with 0 and explicitly using "size +=" in the PIPE_BULK case) I assume
> > > that fallthrough is indeed intended.
> > >
> > > Reported-by: kbuild test robot <[email protected]>
> > > Fixes: 5a35435ef4e6 (soc: fsl: qe: remove PPC32 dependency from CONFIG_QUICC_ENGINE)
> > > Fixes: a035d552a93b (Makefile: Globally enable fall-through warning)
>
> By the way, the "Fixes" tag above makes no sense. There is nothing wrong about
> that commit. It just enabled the fall-through warning globally. Why would you
> "fix" that?"
There could be some effort made to better specify when "Fixes:"
tags should be used.
Right now the "Fixes:" tag is used far too often for changes
like
whitespace only or trivial typos corrections.
And those changes can get backported.
I believe "Fixes:" should be used only when changes have some
runtime impact. "Fixes:" should not be used for changes that
just silence compiler warnings using W=<123>.
On 17/02/2020 18.33, Joe Perches wrote:
> On Mon, 2020-02-17 at 11:12 -0600, Gustavo A. R. Silva wrote:
>>
>>>> Reported-by: kbuild test robot <[email protected]>
>>>> Fixes: 5a35435ef4e6 (soc: fsl: qe: remove PPC32 dependency from CONFIG_QUICC_ENGINE)
>>>> Fixes: a035d552a93b (Makefile: Globally enable fall-through warning)
>>
>> By the way, the "Fixes" tag above makes no sense. There is nothing wrong about
>> that commit. It just enabled the fall-through warning globally. Why would you
>> "fix" that?"
Depends on whether you consider a change that introduces a warning in an
otherwise warning-free build a regression or not. That commit claimed
Now that all the fall-through warnings have been addressed in the
kernel, enable the fall-through warning globally.
but as I explained below the fold, any CONFIG_PPC32+CONFIG_USB_FHCI_HCD
.config grew a warning due to a035d552a93b. So at least in that sense
there is something wrong about that commit - the above claim is simply
false. Please note that I don't expect anybody to ever be able to
actually cover everything before doing something like what a035d552a93b
does, so I'm not complaining, just explaining.
Then I introduced a change which made that code compile for a ppc64
allmodconfig, which apparently 0day does cover, which is why I added
that other tag.
> There could be some effort made to better specify when "Fixes:"
> tags should be used.
Indeed. I explicitly chose not to cc stable because I don't think it's
for -stable. But in case somebody (or Sasha's ML) decides it is, I went
out of my way to include relevant commits and an explanation for the
somewhat odd dual Fixes:. So no, I don't think Fixes implies or should
imply Cc stable - and I think this is all consistent with
submitting-patches.rst:
Patches that fix a severe bug in a released kernel should be directed
toward the stable maintainers...
and
A Fixes: tag indicates that the patch fixes an issue in a previous commit.
Nothing says that Fixes is reserved for -stable material.
> I believe "Fixes:" should be used only when changes have some
> runtime impact.
Perhaps. But it's hard to make the rules completely rigid - suppose
commit A does fix a real bug and is backported, however, in some configs
it introduces some warnings; that gets fixed by B which doesn't change
generated code. Should B be backported, or should the -stable tree(s)
live with those warnings?
"Fixes:" should not be used for changes that
> just silence compiler warnings using W=<123>.
I tend to agree, but that's completely irrelevant in this case, as this
is not a warning that only appears for W=<123>.
Rasmus