[PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Rasmus Villemoes]

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 <lkp@intel.com>
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 <linux@rasmusvillemoes.dk>
---

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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Greg Kroah-Hartman]

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 <lkp@intel.com>
> 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 <linux@rasmusvillemoes.dk>
> ---
> 
> 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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Rasmus Villemoes]

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

[PATCH] checkpatch: Prefer fallthrough; over fallthrough comments

[Joe Perches]

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 <joe@perches.com>
---
 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 &&

RE: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Leo Li]

> -----Original Message-----
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Sent: Thursday, February 13, 2020 2:54 AM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Timur Tabi
> <timur@kernel.org>; Leo Li <leoyang.li@nxp.com>; Rasmus Villemoes
> <linux@rasmusvillemoes.dk>; Gustavo A. R. Silva
> <gustavo@embeddedor.com>
> Cc: Anton Vorontsov <avorontsov@ru.mvista.com>; kbuild test robot
> <lkp@intel.com>; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
> 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 <lkp@intel.com>
> 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 <linux@rasmusvillemoes.dk>

Acked-by: Li Yang <leoyang.li@nxp.com>

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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Rasmus Villemoes]

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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Gustavo A. R. Silva]

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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Gustavo A. R. Silva]

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 <lkp@intel.com>
>> 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

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Gustavo A. R. Silva]

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 <lkp@intel.com>
> 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 <linux@rasmusvillemoes.dk>
> ---
> 
> 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;
> 

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Joe Perches]

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 <lkp@intel.com>
> > > 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>.

Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough

[Rasmus Villemoes]

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 <lkp@intel.com>
>>>> 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