Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu

[Sebastian Reichel]

Hi Julia,

On Sat, Jul 01, 2017 at 09:28:10PM +0200, Julia Lawall wrote:
> As reported by Sebastian Reichel, i2c_smbus_read_word_data() returns native
> endianness for little-endian bus (it basically has builtin
> le16_to_cpu). Calling le16_to_cpu on the result breaks support on big
> endian machines by converting it back.

Thanks, you are fast :)

> This semantic patch give no reports on kernel code currently, but the
> issue is somewhat obscure and has occurred in a sumitted patch, so it could
> be good to have a check for it.

Ok, so problem is not as bad as I feared. I found a few issues with
simple git grep, though:

git grep -C100 i2c_smbus_read_word_data | grep le16_to_cpu
git grep -C100 i2c_smbus_write_word_data | grep cpu_to_le16

It returned just a few files on v4.12 and all of them look buggy
after manual inspection:

 * drivers/macintosh/windfarm_lm75_sensor.c (line 71)
 * drivers/macintosh/windfarm_smu_sat.c (line 80-91)
 * drivers/gpio/gpio-pca953x.c (line 190-192)
 * drivers/power/supply/bq24735-charger.c
   - fixed in linux-next by 48f680c0a9ca
 * drivers/power/supply/sbs-battery.c
   - fixed in linux-next by a1bbec72f9fe

> Suggested-by: Sebastian Reichel <sre@xxxxxxxxxx>
> Signed-off-by: Julia Lawall <Julia.Lawall@xxxxxxx>
> 
> ---
> 
> The rule could easily be extended with more such functions.  Let me know if
> anything else should be taken into account.

I guess the write function should also be covered.

-- Sebastian

>  scripts/coccinelle/api/smbus_word.cocci |   45 ++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/scripts/coccinelle/api/smbus_word.cocci b/scripts/coccinelle/api/smbus_word.cocci
> new file mode 100644
> index 0000000..b167cf0
> --- /dev/null
> +++ b/scripts/coccinelle/api/smbus_word.cocci
> @@ -0,0 +1,45 @@
> +/// i2c_smbus_read_word_data() returns native endianness for little-endian
> +/// bus (it basically has builtin le16_to_cpu). Calling le16_to_cpu on the
> +/// result breaks support on big endian machines by converting it back.
> +///
> +// Confidence: Moderate
> +// Copyright: (C) 2017 Julia Lawall, Inria. GPLv2.
> +// URL: http://coccinelle.lip6.fr/
> +// Options: --no-includes --include-headers
> +// Keywords: i2c_smbus_read_word_data, le16_to_cpu
> +
> +virtual context
> +virtual org
> +virtual report
> +
> +// ----------------------------------------------------------------------------
> +
> +@r depends on context || org || report exists@
> +expression e, x;
> +position j0, j1;
> +@@
> +
> +* x@j0 = i2c_smbus_read_word_data(...)
> +... when != x = e
> +* le16_to_cpu@j1(x)
> +
> +// ----------------------------------------------------------------------------
> +
> +@script:python r_org depends on org@
> +j0 << r.j0;
> +j1 << r.j1;
> +@@
> +
> +msg = "le16_to_cpu not needed on i2c_smbus_read_word_data result."
> +coccilib.org.print_todo(j0[0], msg)
> +coccilib.org.print_link(j1[0], "")
> +
> +// ----------------------------------------------------------------------------
> +
> +@script:python r_report depends on report@
> +j0 << r.j0;
> +j1 << r.j1;
> +@@
> +
> +msg = "le16_to_cpu not needed on i2c_smbus_read_word_data result around line %s." % (j1[0].line)
> +coccilib.report.print_report(j0[0], msg)
>

Attachment: signature.asc
Description: PGP signature