| Message ID | 20200213085401.27862-1-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 28926994e5d7f049807119c48bd7b94c2d15fc95 |
| Headers | show |
| Series | usb: host: fhci-hcd: annotate PIPE_CONTROL switch case with fallthrough | expand |
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
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
> -----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
On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote: > On 13/02/2020 13.56, Greg Kroah-Hartman wrote: > > >> diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c > >> index 04733876c9c6..a8e1048278d0 100644 > >> --- a/drivers/usb/host/fhci-hcd.c > >> +++ b/drivers/usb/host/fhci-hcd.c > >> @@ -396,6 +396,7 @@ static int fhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, > >> case PIPE_CONTROL: > >> /* 1 td fro setup,1 for ack */ > >> size = 2; > >> + fallthrough; > > > > We have an attribute for that? > > > > Shouldn't this be /* fall through */ instead? > > > > Gustavo, what's the best practice here, I count only a few > > "fallthrough;" instances in the kernel, although one is in our coding > > style document, and thousands of the /* */ version. > > Yes, I went with the attribute/macro due to that, and the history is > that Linus applied Joe's patches directly > (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/), > so I assumed that meant the Penguin decided that the attribute/macro is > the right thing to do for new code, while existing comment annotations > can be left alone or changed piecemeal as code gets refactored anyway. But, to be fair, Gustavo went and fixed up thousands of these, with the /* */ version, not the attribute. Gustavo, can coverity notice the "fallthrough;" attribute properly? I don't want to start adding things that end up triggering false-positives. thanks, greg k-h
On 17/02/2020 10.38, Greg Kroah-Hartman wrote: > On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote: >> On 13/02/2020 13.56, Greg Kroah-Hartman wrote: >> >>> Shouldn't this be /* fall through */ instead? >>> >>> Gustavo, what's the best practice here, I count only a few >>> "fallthrough;" instances in the kernel, although one is in our coding >>> style document, and thousands of the /* */ version. >> >> Yes, I went with the attribute/macro due to that, and the history is >> that Linus applied Joe's patches directly >> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/), >> so I assumed that meant the Penguin decided that the attribute/macro is >> the right thing to do for new code, while existing comment annotations >> can be left alone or changed piecemeal as code gets refactored anyway. > > But, to be fair, Gustavo went and fixed up thousands of these, with the > /* */ version, not the attribute. > > Gustavo, can coverity notice the "fallthrough;" attribute properly? I > don't want to start adding things that end up triggering > false-positives. I'm not Gustavo, and I don't know the answer, but 1.5 years ago some guy named greg k-h suggested that coverity does grok the fallthrough attribute: https://patchwork.kernel.org/cover/10651357/#22279095 Rasmus
On Mon, Feb 17, 2020 at 03:12:21PM +0100, Rasmus Villemoes wrote: > On 17/02/2020 10.38, Greg Kroah-Hartman wrote: > > On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote: > >> On 13/02/2020 13.56, Greg Kroah-Hartman wrote: > >> > >>> Shouldn't this be /* fall through */ instead? > >>> > >>> Gustavo, what's the best practice here, I count only a few > >>> "fallthrough;" instances in the kernel, although one is in our coding > >>> style document, and thousands of the /* */ version. > >> > >> Yes, I went with the attribute/macro due to that, and the history is > >> that Linus applied Joe's patches directly > >> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/), > >> so I assumed that meant the Penguin decided that the attribute/macro is > >> the right thing to do for new code, while existing comment annotations > >> can be left alone or changed piecemeal as code gets refactored anyway. > > > > But, to be fair, Gustavo went and fixed up thousands of these, with the > > /* */ version, not the attribute. > > > > Gustavo, can coverity notice the "fallthrough;" attribute properly? I > > don't want to start adding things that end up triggering > > false-positives. > > I'm not Gustavo, and I don't know the answer, but 1.5 years ago some guy > named greg k-h suggested that coverity does grok the fallthrough attribute: > > https://patchwork.kernel.org/cover/10651357/#22279095 I wouldn't trust anything that bum says :) Ok, I don't remember saying that at all, but I'll wait a day or two to get Gustavo's opinion befor applying the patch. thanks, greg k-h
Hi! Sorry for the late reply. I wasn't aware of this thread until now. Please, see my comments below... On 2/17/20 08:18, Greg Kroah-Hartman wrote: > On Mon, Feb 17, 2020 at 03:12:21PM +0100, Rasmus Villemoes wrote: >> On 17/02/2020 10.38, Greg Kroah-Hartman wrote: >>> On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote: >>>> On 13/02/2020 13.56, Greg Kroah-Hartman wrote: >>>> >>>>> Shouldn't this be /* fall through */ instead? >>>>> >>>>> Gustavo, what's the best practice here, I count only a few >>>>> "fallthrough;" instances in the kernel, although one is in our coding >>>>> style document, and thousands of the /* */ version. >>>> >>>> Yes, I went with the attribute/macro due to that, and the history is >>>> that Linus applied Joe's patches directly >>>> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/), >>>> so I assumed that meant the Penguin decided that the attribute/macro is >>>> the right thing to do for new code, while existing comment annotations >>>> can be left alone or changed piecemeal as code gets refactored anyway. >>> >>> But, to be fair, Gustavo went and fixed up thousands of these, with the >>> /* */ version, not the attribute. >>> >>> Gustavo, can coverity notice the "fallthrough;" attribute properly? I >>> don't want to start adding things that end up triggering >>> false-positives. >> >> I'm not Gustavo, and I don't know the answer, but 1.5 years ago some guy >> named greg k-h suggested that coverity does grok the fallthrough attribute: >> >> https://patchwork.kernel.org/cover/10651357/#22279095 > > I wouldn't trust anything that bum says :) > > Ok, I don't remember saying that at all, but I'll wait a day or two to > get Gustavo's opinion befor applying the patch. > We are good to go with the 'fallthrough' pseudo keyword. Linus is OK with that. The comment annotations will eventually be transformed to "fallthrough;" Thanks -- Gustavo
On Mon, Feb 17, 2020 at 10:15:09AM -0600, Gustavo A. R. Silva wrote: > Hi! > > Sorry for the late reply. I wasn't aware of this thread until now. > > Please, see my comments below... > > On 2/17/20 08:18, Greg Kroah-Hartman wrote: > > On Mon, Feb 17, 2020 at 03:12:21PM +0100, Rasmus Villemoes wrote: > >> On 17/02/2020 10.38, Greg Kroah-Hartman wrote: > >>> On Thu, Feb 13, 2020 at 02:35:18PM +0100, Rasmus Villemoes wrote: > >>>> On 13/02/2020 13.56, Greg Kroah-Hartman wrote: > >>>> > >>>>> Shouldn't this be /* fall through */ instead? > >>>>> > >>>>> Gustavo, what's the best practice here, I count only a few > >>>>> "fallthrough;" instances in the kernel, although one is in our coding > >>>>> style document, and thousands of the /* */ version. > >>>> > >>>> Yes, I went with the attribute/macro due to that, and the history is > >>>> that Linus applied Joe's patches directly > >>>> (https://lore.kernel.org/lkml/CAHk-=whOF8heTGz5tfzYUBp_UQQzSWNJ_50M7-ECXkfFRDQWFA@mail.gmail.com/), > >>>> so I assumed that meant the Penguin decided that the attribute/macro is > >>>> the right thing to do for new code, while existing comment annotations > >>>> can be left alone or changed piecemeal as code gets refactored anyway. > >>> > >>> But, to be fair, Gustavo went and fixed up thousands of these, with the > >>> /* */ version, not the attribute. > >>> > >>> Gustavo, can coverity notice the "fallthrough;" attribute properly? I > >>> don't want to start adding things that end up triggering > >>> false-positives. > >> > >> I'm not Gustavo, and I don't know the answer, but 1.5 years ago some guy > >> named greg k-h suggested that coverity does grok the fallthrough attribute: > >> > >> https://patchwork.kernel.org/cover/10651357/#22279095 > > > > I wouldn't trust anything that bum says :) > > > > Ok, I don't remember saying that at all, but I'll wait a day or two to > > get Gustavo's opinion befor applying the patch. > > > > We are good to go with the 'fallthrough' pseudo keyword. Linus is OK with > that. > > The comment annotations will eventually be transformed to "fallthrough;" Ok, thanks for the confirmation, will queue this up. greg k-h
On 2/13/20 06:56, Greg Kroah-Hartman wrote: > On Thu, Feb 13, 2020 at 09:54:00AM +0100, Rasmus Villemoes wrote: >> After this was made buildable for something other than PPC32, kbuild >> starts warning >> >> drivers/usb/host/fhci-hcd.c:398:8: warning: this statement may fall >> through [-Wimplicit-fallthrough=] >> >> I don't know this code, but from the construction (initializing size >> with 0 and explicitly using "size +=" in the PIPE_BULK case) I assume >> that fallthrough is indeed intended. >> >> Reported-by: kbuild test robot <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
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; >
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>.
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
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;
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(+)