iio: cros_ec_accel_legacy: Mark expected switch fall-throughs

PATCH In review Gustavo A. R. Silva 3 participants 8 messages Oct 8, 2018 – Feb 21, 2019

Subsystems (2)

IIO ChromeOS EC subdrivers

lore

Share & tools

lore

Gustavo A. R. Silva

author

2018-10-08 17:24 UTC #0 lore raw

PATCH 1/1 iio: cros_ec_accel_legacy: Mark expected switch fall-throughs

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 <[email protected]>

---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/iio/accel/cros_ec_accel_legacy.c +2 -0

index 063e89e..d609654 100644
@@ -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;
 		}

Signature · 1 line

2.7.4

Jonathan Cameron

maintainer

2018-10-08 20:31 UTC #1 lore raw

Quoted 5 lines · On Mon, 8 Oct 2018 19:23:32 +0200 "Gustavo A. R. Silva"…

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 <[email protected]>

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

Quoted 19 lines

---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/iio/accel/cros_ec_accel_legacy.c +2 -0

index 063e89e..d609654 100644
@@ -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;
 		}

Gustavo A. R. Silva

author

2019-02-12 21:51 UTC #2 lore raw

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

Quoted 41 lines · Jonathan Cameron

Quoted 5 lines · On Mon, 8 Oct 2018 19:23:32 +0200 "Gustavo A. R. Silva"…
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 <[email protected]>
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
Quoted 19 lines
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/cros_ec_accel_legacy.c +2 -0
index 063e89e..d609654 100644
@@ -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;
 		}

Kees Cook

2019-02-20 18:20 UTC #3 lore raw

In reply to Gustavo A. R. Silva

Quoted 15 lines · Gustavo A. R. Silva

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 <[email protected]>
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/cros_ec_accel_legacy.c +0 -0
index 063e89e..d609654 100644

drivers/iio/accel/cros_ec_accel_legacy.c static int cros_ec_accel_legacy_probe(struct platform_device *pdev) · line 394

Quoted 10 lines

drivers/iio/accel/cros_ec_accel_legacy.c +2 -0

@@ -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.

Signature · 1 line

Kees Cook

Jonathan Cameron

maintainer

2019-02-20 18:34 UTC #4 lore raw

Quoted 31 lines · On Wed, 20 Feb 2019 10:20:39 -0800 Kees Cook <[email protected]> wrote:

Quoted 15 lines · Gustavo A. R. Silva
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 <[email protected]>
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/cros_ec_accel_legacy.c +0 -0
index 063e89e..d609654 100644
drivers/iio/accel/cros_ec_accel_legacy.c static int cros_ec_accel_legacy_probe(struct platform_device *pdev) · line 394
Quoted 10 lines
drivers/iio/accel/cros_ec_accel_legacy.c +2 -0
@@ -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:

Quoted 6 lines

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

Quoted 1 line

Gustavo A. R. Silva

author

2019-02-20 18:47 UTC #5 lore raw

Quoted 34 lines · Jonathan Cameron

Quoted 31 lines · On Wed, 20 Feb 2019 10:20:39 -0800 Kees Cook <[email protected]> wrote:
Quoted 15 lines · Gustavo A. R. Silva
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 <[email protected]>
---
 drivers/iio/accel/cros_ec_accel_legacy.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/iio/accel/cros_ec_accel_legacy.c +0 -0
index 063e89e..d609654 100644
drivers/iio/accel/cros_ec_accel_legacy.c static int cros_ec_accel_legacy_probe(struct platform_device *pdev) · line 394
Quoted 10 lines
drivers/iio/accel/cros_ec_accel_legacy.c +2 -0
@@ -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:

Quoted 6 lines

Quoted 6 lines
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;

Quoted 4 lines

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

Kees Cook

2019-02-21 01:23 UTC #6 lore raw

Quoted 11 lines · Gustavo A. R. Silva

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?

Quoted 4 lines

Quoted 2 lines
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. :)

Signature · 1 line

Kees Cook

Gustavo A. R. Silva

author

2019-02-21 01:48 UTC #7 lore raw

Quoted 9 lines · Kees Cook

Quoted 7 lines · Gustavo A. R. Silva
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;

Quoted 6 lines

Quoted 4 lines
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!

Quoted 2 lines

otherwise, yes. Can you send that patch?

Sure thing.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
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