| Message ID | 20181008172332.GA29816@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: cros_ec_accel_legacy: Mark expected switch fall-throughs | expand |
On Mon, 8 Oct 2018 19:23:32 +0200 "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote: > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > where we are expecting to fall through. > > Addresses-Coverity-ID: 1397962 ("Missing break in switch") > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> Hi, I'll be honest I'm lost on what the intent of this code actually is... Gwendal - why do we have a loop with this odd switch statement in it. Superficially I think we might as well drop the switch and pull those assignments out of the loop. However, perhaps I'm missing something! Thanks, Jonathan > --- > drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c > index 063e89e..d609654 100644 > --- a/drivers/iio/accel/cros_ec_accel_legacy.c > +++ b/drivers/iio/accel/cros_ec_accel_legacy.c > @@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev) > switch (i) { > case X: > ec_accel_channels[X].scan_index = Y; > + /* fall through */ > case Y: > ec_accel_channels[Y].scan_index = X; > + /* fall through */ > case Z: > ec_accel_channels[Z].scan_index = Z; > }
Hi guys, I was about to submit this patch again, then I realized I had sent it before. So, this is a friendly ping. Thanks -- Gustavo On 10/8/18 3:30 PM, Jonathan Cameron wrote: > On Mon, 8 Oct 2018 19:23:32 +0200 > "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote: > >> In preparation to enabling -Wimplicit-fallthrough, mark switch cases >> where we are expecting to fall through. >> >> Addresses-Coverity-ID: 1397962 ("Missing break in switch") >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > Hi, > > I'll be honest I'm lost on what the intent of this code actually is... > > Gwendal - why do we have a loop with this odd switch statement > in it. Superficially I think we might as well drop the switch > and pull those assignments out of the loop. However, perhaps > I'm missing something! > > Thanks, > > Jonathan > >> --- >> drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c >> index 063e89e..d609654 100644 >> --- a/drivers/iio/accel/cros_ec_accel_legacy.c >> +++ b/drivers/iio/accel/cros_ec_accel_legacy.c >> @@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev) >> switch (i) { >> case X: >> ec_accel_channels[X].scan_index = Y; >> + /* fall through */ >> case Y: >> ec_accel_channels[Y].scan_index = X; >> + /* fall through */ >> case Z: >> ec_accel_channels[Z].scan_index = Z; >> } >
On Mon, Oct 8, 2018 at 10:24 AM Gustavo A. R. Silva <gustavo@embeddedor.com> wrote: > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > where we are expecting to fall through. > > Addresses-Coverity-ID: 1397962 ("Missing break in switch") > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c > index 063e89e..d609654 100644 > --- a/drivers/iio/accel/cros_ec_accel_legacy.c > +++ b/drivers/iio/accel/cros_ec_accel_legacy.c > @@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev) > switch (i) { > case X: > ec_accel_channels[X].scan_index = Y; > + /* fall through */ > case Y: > ec_accel_channels[Y].scan_index = X; > + /* fall through */ > case Z: > ec_accel_channels[Z].scan_index = Z; > } Shouldn't these actually be "break;"s ? It seems like the loop is stepping through X, Y, and Z. The _result_ is accidentally the same: X: set X, Y, and Z Y: set Y and Z Z: set Z result: X, Y, and Z are set correctly. But the code is technically wrong.
On Wed, 20 Feb 2019 10:20:39 -0800 Kees Cook <keescook@chromium.org> wrote: > On Mon, Oct 8, 2018 at 10:24 AM Gustavo A. R. Silva > <gustavo@embeddedor.com> wrote: > > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases > > where we are expecting to fall through. > > > > Addresses-Coverity-ID: 1397962 ("Missing break in switch") > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > > --- > > drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c > > index 063e89e..d609654 100644 > > --- a/drivers/iio/accel/cros_ec_accel_legacy.c > > +++ b/drivers/iio/accel/cros_ec_accel_legacy.c > > @@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev) > > switch (i) { > > case X: > > ec_accel_channels[X].scan_index = Y; > > + /* fall through */ > > case Y: > > ec_accel_channels[Y].scan_index = X; > > + /* fall through */ > > case Z: > > ec_accel_channels[Z].scan_index = Z; > > } > > Shouldn't these actually be "break;"s ? It seems like the loop is > stepping through X, Y, and Z. The _result_ is accidentally the same: > > X: set X, Y, and Z > Y: set Y and Z > Z: set Z > > result: X, Y, and Z are set correctly. But the code is technically wrong. > Agreed, it's 'novel'. Waiting for Gwendal or someone else to come back and check it wasn't meant to be doing something else. Jonathan >
On 2/20/19 12:34 PM, Jonathan Cameron wrote: > On Wed, 20 Feb 2019 10:20:39 -0800 > Kees Cook <keescook@chromium.org> wrote: > >> On Mon, Oct 8, 2018 at 10:24 AM Gustavo A. R. Silva >> <gustavo@embeddedor.com> wrote: >>> >>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases >>> where we are expecting to fall through. >>> >>> Addresses-Coverity-ID: 1397962 ("Missing break in switch") >>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> >>> --- >>> drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c >>> index 063e89e..d609654 100644 >>> --- a/drivers/iio/accel/cros_ec_accel_legacy.c >>> +++ b/drivers/iio/accel/cros_ec_accel_legacy.c >>> @@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev) >>> switch (i) { >>> case X: >>> ec_accel_channels[X].scan_index = Y; >>> + /* fall through */ >>> case Y: >>> ec_accel_channels[Y].scan_index = X; >>> + /* fall through */ >>> case Z: >>> ec_accel_channels[Z].scan_index = Z; >>> } >> >> Shouldn't these actually be "break;"s ? It seems like the loop is >> stepping through X, Y, and Z. The _result_ is accidentally the same: >> >> X: set X, Y, and Z >> Y: set Y and Z >> Z: set Z >> >> result: X, Y, and Z are set correctly. But the code is technically wrong. >> Yeah. Actually, we can even take the switch and for out of the equation, and the code can be rewritten as follows: ec_accel_channels[X].scan_index = Y; ec_accel_channels[Y].scan_index = X; ec_accel_channels[Z].scan_index = Z; if (state->sensor_num == MOTIONSENSE_LOC_LID) state->sign[X] = state->sign[Z] = -1; else state->sign[X] = state->sign[Y] = state->sign[Z] = 1; > > Agreed, it's 'novel'. Waiting for Gwendal or someone else to come > back and check it wasn't meant to be doing something else. > We've been waiting 5 months for Gwendal. :/ Thanks -- Gustavo
On Wed, Feb 20, 2019 at 10:47 AM Gustavo A. R. Silva <gustavo@embeddedor.com> wrote: > Yeah. Actually, we can even take the switch and for out of the equation, > and the code can be rewritten as follows: > > ec_accel_channels[X].scan_index = Y; > ec_accel_channels[Y].scan_index = X; > ec_accel_channels[Z].scan_index = Z; > > if (state->sensor_num == MOTIONSENSE_LOC_LID) > state->sign[X] = state->sign[Z] = -1; > else > state->sign[X] = state->sign[Y] = state->sign[Z] = 1; Actually, should be an unconditional "state->sign[Y] = 1", but otherwise, yes. Can you send that patch? > > Agreed, it's 'novel'. Waiting for Gwendal or someone else to come > > back and check it wasn't meant to be doing something else. > > We've been waiting 5 months for Gwendal. :/ I've looked at this enough. I'm happy to Ack it, if that helps. :)
On 2/20/19 7:23 PM, Kees Cook wrote: > On Wed, Feb 20, 2019 at 10:47 AM Gustavo A. R. Silva > <gustavo@embeddedor.com> wrote: >> Yeah. Actually, we can even take the switch and for out of the equation, >> and the code can be rewritten as follows: >> >> ec_accel_channels[X].scan_index = Y; >> ec_accel_channels[Y].scan_index = X; >> ec_accel_channels[Z].scan_index = Z; >> >> if (state->sensor_num == MOTIONSENSE_LOC_LID) >> state->sign[X] = state->sign[Z] = -1; >> else >> state->sign[X] = state->sign[Y] = state->sign[Z] = 1; > > Actually, should be an unconditional "state->sign[Y] = 1", but You're right. Team work! > otherwise, yes. Can you send that patch? > Sure thing. > >>> Agreed, it's 'novel'. Waiting for Gwendal or someone else to come >>> back and check it wasn't meant to be doing something else. >> >> We've been waiting 5 months for Gwendal. :/ > > I've looked at this enough. I'm happy to Ack it, if that helps. :) > Awesome. :) Thanks -- Gustavo
diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c index 063e89e..d609654 100644 --- a/drivers/iio/accel/cros_ec_accel_legacy.c +++ b/drivers/iio/accel/cros_ec_accel_legacy.c @@ -385,8 +385,10 @@ static int cros_ec_accel_legacy_probe(struct platform_device *pdev) switch (i) { case X: ec_accel_channels[X].scan_index = Y; + /* fall through */ case Y: ec_accel_channels[Y].scan_index = X; + /* fall through */ case Z: ec_accel_channels[Z].scan_index = Z; }
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 1397962 ("Missing break in switch") Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++ 1 file changed, 2 insertions(+)