* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.