* [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough @ 2020-02-13 8:54 Rasmus Villemoes 2020-02-13 12:56 ` Greg Kroah-Hartman ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Rasmus Villemoes @ 2020-02-13 8:54 UTC (permalink / raw) To: Greg Kroah-Hartman, Timur Tabi, Li Yang, Rasmus Villemoes, Gustavo A. R. Silva Cc: Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-13 8:54 [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough Rasmus Villemoes @ 2020-02-13 12:56 ` Greg Kroah-Hartman 2020-02-13 13:35 ` Rasmus Villemoes 2020-02-17 17:12 ` Gustavo A. R. Silva 2020-02-13 21:11 ` Leo Li 2020-02-17 17:28 ` Gustavo A. R. Silva 2 siblings, 2 replies; 14+ messages in thread From: Greg Kroah-Hartman @ 2020-02-13 12:56 UTC (permalink / raw) To: Gustavo A. R. Silva, Rasmus Villemoes Cc: Timur Tabi, Li Yang, Gustavo A. R. Silva, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-13 12:56 ` Greg Kroah-Hartman @ 2020-02-13 13:35 ` Rasmus Villemoes 2020-02-13 15:23 ` [PATCH] checkpatch: Prefer fallthrough; over fallthrough comments Joe Perches 2020-02-17 9:38 ` [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough Greg Kroah-Hartman 2020-02-17 17:12 ` Gustavo A. R. Silva 1 sibling, 2 replies; 14+ messages in thread From: Rasmus Villemoes @ 2020-02-13 13:35 UTC (permalink / raw) To: Greg Kroah-Hartman, Gustavo A. R. Silva Cc: Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] checkpatch: Prefer fallthrough; over fallthrough comments 2020-02-13 13:35 ` Rasmus Villemoes @ 2020-02-13 15:23 ` Joe Perches 2020-02-17 9:38 ` [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough Greg Kroah-Hartman 1 sibling, 0 replies; 14+ messages in thread From: Joe Perches @ 2020-02-13 15:23 UTC (permalink / raw) To: Rasmus Villemoes, Greg Kroah-Hartman, Gustavo A. R. Silva, Andrew Morton Cc: Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel 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 && ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-13 13:35 ` Rasmus Villemoes 2020-02-13 15:23 ` [PATCH] checkpatch: Prefer fallthrough; over fallthrough comments Joe Perches @ 2020-02-17 9:38 ` Greg Kroah-Hartman 2020-02-17 14:12 ` Rasmus Villemoes 1 sibling, 1 reply; 14+ messages in thread From: Greg Kroah-Hartman @ 2020-02-17 9:38 UTC (permalink / raw) To: Rasmus Villemoes Cc: Gustavo A. R. Silva, Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-17 9:38 ` [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough Greg Kroah-Hartman @ 2020-02-17 14:12 ` Rasmus Villemoes 2020-02-17 14:18 ` Greg Kroah-Hartman 0 siblings, 1 reply; 14+ messages in thread From: Rasmus Villemoes @ 2020-02-17 14:12 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Gustavo A. R. Silva, Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-17 14:12 ` Rasmus Villemoes @ 2020-02-17 14:18 ` Greg Kroah-Hartman 2020-02-17 16:15 ` Gustavo A. R. Silva 0 siblings, 1 reply; 14+ messages in thread From: Greg Kroah-Hartman @ 2020-02-17 14:18 UTC (permalink / raw) To: Rasmus Villemoes, Gustavo A. R. Silva Cc: Gustavo A. R. Silva, Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-17 14:18 ` Greg Kroah-Hartman @ 2020-02-17 16:15 ` Gustavo A. R. Silva 2020-02-17 16:29 ` Greg Kroah-Hartman 0 siblings, 1 reply; 14+ messages in thread From: Gustavo A. R. Silva @ 2020-02-17 16:15 UTC (permalink / raw) To: Greg Kroah-Hartman, Rasmus Villemoes Cc: Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-17 16:15 ` Gustavo A. R. Silva @ 2020-02-17 16:29 ` Greg Kroah-Hartman 0 siblings, 0 replies; 14+ messages in thread From: Greg Kroah-Hartman @ 2020-02-17 16:29 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Rasmus Villemoes, Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel, Joe Perches 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-13 12:56 ` Greg Kroah-Hartman 2020-02-13 13:35 ` Rasmus Villemoes @ 2020-02-17 17:12 ` Gustavo A. R. Silva 2020-02-17 17:33 ` Joe Perches 1 sibling, 1 reply; 14+ messages in thread From: Gustavo A. R. Silva @ 2020-02-17 17:12 UTC (permalink / raw) To: Greg Kroah-Hartman, Rasmus Villemoes Cc: Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-17 17:12 ` Gustavo A. R. Silva @ 2020-02-17 17:33 ` Joe Perches 2020-02-17 19:44 ` Rasmus Villemoes 0 siblings, 1 reply; 14+ messages in thread From: Joe Perches @ 2020-02-17 17:33 UTC (permalink / raw) To: Gustavo A. R. Silva, Greg Kroah-Hartman, Rasmus Villemoes Cc: Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel 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>. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-17 17:33 ` Joe Perches @ 2020-02-17 19:44 ` Rasmus Villemoes 0 siblings, 0 replies; 14+ messages in thread From: Rasmus Villemoes @ 2020-02-17 19:44 UTC (permalink / raw) To: Joe Perches, Gustavo A. R. Silva, Greg Kroah-Hartman Cc: Timur Tabi, Li Yang, Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-13 8:54 [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough Rasmus Villemoes 2020-02-13 12:56 ` Greg Kroah-Hartman @ 2020-02-13 21:11 ` Leo Li 2020-02-17 17:28 ` Gustavo A. R. Silva 2 siblings, 0 replies; 14+ messages in thread From: Leo Li @ 2020-02-13 21:11 UTC (permalink / raw) To: Rasmus Villemoes, Greg Kroah-Hartman, Timur Tabi, Gustavo A. R. Silva Cc: Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel > -----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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough 2020-02-13 8:54 [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough Rasmus Villemoes 2020-02-13 12:56 ` Greg Kroah-Hartman 2020-02-13 21:11 ` Leo Li @ 2020-02-17 17:28 ` Gustavo A. R. Silva 2 siblings, 0 replies; 14+ messages in thread From: Gustavo A. R. Silva @ 2020-02-17 17:28 UTC (permalink / raw) To: Rasmus Villemoes, Greg Kroah-Hartman, Timur Tabi, Li Yang Cc: Anton Vorontsov, kbuild test robot, linux-usb, linux-kernel 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; > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-02-17 19:44 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-13 8:54 [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough Rasmus Villemoes 2020-02-13 12:56 ` Greg Kroah-Hartman 2020-02-13 13:35 ` Rasmus Villemoes 2020-02-13 15:23 ` [PATCH] checkpatch: Prefer fallthrough; over fallthrough comments Joe Perches 2020-02-17 9:38 ` [PATCH] usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough Greg Kroah-Hartman 2020-02-17 14:12 ` Rasmus Villemoes 2020-02-17 14:18 ` Greg Kroah-Hartman 2020-02-17 16:15 ` Gustavo A. R. Silva 2020-02-17 16:29 ` Greg Kroah-Hartman 2020-02-17 17:12 ` Gustavo A. R. Silva 2020-02-17 17:33 ` Joe Perches 2020-02-17 19:44 ` Rasmus Villemoes 2020-02-13 21:11 ` Leo Li 2020-02-17 17:28 ` Gustavo A. R. Silva
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).