| Message ID | 20180620133100.GA608@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote: > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > where we are expecting to fall through. > > Addresses-Coverity-ID: 1470102 ("Missing break in switch") Any other advantage besides coverity? Can't we address it by marking as "Intentional" on the tool? I'm afraid there will be so many more places to add fallthrough marks.... > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c > index 132fe63..6a40a77 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > @@ -2566,6 +2566,7 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv, > switch (index) { > default: > MISSING_CASE(index); > + /* fall through */ > case 0: > link_clock = 540000; > break; > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 06/20/2018 02:06 PM, Rodrigo Vivi wrote: > On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote: >> In preparation to enabling -Wimplicit-fallthrough, mark switch cases >> where we are expecting to fall through. >> >> Addresses-Coverity-ID: 1470102 ("Missing break in switch") > > Any other advantage besides coverity? > Can't we address it by marking as "Intentional" on the tool? > Yes. The advantage of this is that it will eventually allows to enable -Wimplicit-fallthrough, hence, enabling the compiler to trigger a warning, which will force us to double check if we are actually missing a break before committing the code. The change in the code has nothing to do with the Coverity tool. The tool is only reporting the issue, which, in this case, is a false positive. > I'm afraid there will be so many more places to add fallthrough > marks.... > Oh yeah, there are around 1000 similar places in the whole codebase. There is an ongoing effort to review each case. Months ago, it used to be around 1500 of these cases. Thanks -- Gustavo >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >> --- >> drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c >> index 132fe63..6a40a77 100644 >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c >> @@ -2566,6 +2566,7 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv, >> switch (index) { >> default: >> MISSING_CASE(index); >> + /* fall through */ >> case 0: >> link_clock = 540000; >> break; >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 20 Jun 2018, "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote: > On 06/20/2018 02:06 PM, Rodrigo Vivi wrote: >> On Wed, Jun 20, 2018 at 08:31:00AM -0500, Gustavo A. R. Silva wrote: >>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases >>> where we are expecting to fall through. >>> >>> Addresses-Coverity-ID: 1470102 ("Missing break in switch") >> >> Any other advantage besides coverity? >> Can't we address it by marking as "Intentional" on the tool? >> > > Yes. The advantage of this is that it will eventually allows to enable > -Wimplicit-fallthrough, hence, enabling the compiler to trigger a > warning, which will force us to double check if we are actually missing > a break before committing the code. I applaud the efforts. Since you're doing the comment changes, do you have an idea what -Wimplicit-fallthrough=N level is being considered for kernel? >> I'm afraid there will be so many more places to add fallthrough >> marks.... >> > > Oh yeah, there are around 1000 similar places in the whole codebase. > There is an ongoing effort to review each case. Months ago, it used to > be around 1500 of these cases. We use our own MISSING_CASE() to indicate stuff that's not supposed to happen, or to be implemented, etc. and in many cases the fallthrough is normal. I wonder if we could embed __attribute__ ((fallthrough)) in there to tackle all of these without a comment. BR, Jani.
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c index 132fe63..6a40a77 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -2566,6 +2566,7 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv, switch (index) { default: MISSING_CASE(index); + /* fall through */ case 0: link_clock = 540000; break;
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 1470102 ("Missing break in switch") Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/gpu/drm/i915/intel_dpll_mgr.c | 1 + 1 file changed, 1 insertion(+)