[PATCH] coccinelle: api: detect unnecessary le16_to_cpu

[PATCH] coccinelle: api: detect unnecessary le16_to_cpu

[Julia Lawall]

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.

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.

Suggested-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---

The rule could easily be extended with more such functions.  Let me know if
anything else should be taken into account.

 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)

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3345 bytes --]

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@kernel.org>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> 
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Andy Shevchenko]

On Mon, Jul 3, 2017 at 4:36 PM, Sebastian Reichel
<sebastian.reichel@collabora.co.uk> wrote:
> On Sat, Jul 01, 2017 at 09:28:10PM +0200, Julia Lawall wrote:

>  * drivers/gpio/gpio-pca953x.c (line 190-192)

It has double conversion there:
1. LE CPU: Read as LE and converted to LE (no-op), so, just u16
2. BE CPU: Read as BE and converted to LE, makes it __le16

Looks like the conversion is not needed, only get_unaligned() is necessary.

P.S. What about lines 244-245 there? I think they are no-op.
Interesting that those two parts were added in quite different
commits.

-- 
With Best Regards,
Andy Shevchenko

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

Hi,

On Mon, Jul 03, 2017 at 07:37:59PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 3, 2017 at 4:36 PM, Sebastian Reichel
> <sebastian.reichel@collabora.co.uk> wrote:
> > On Sat, Jul 01, 2017 at 09:28:10PM +0200, Julia Lawall wrote:
> 
> >  * drivers/gpio/gpio-pca953x.c (line 190-192)
> 
> It has double conversion there:
> 1. LE CPU: Read as LE and converted to LE (no-op), so, just u16
> 2. BE CPU: Read as BE and converted to LE, makes it __le16
> 
> Looks like the conversion is not needed, only get_unaligned() is necessary.
>
> P.S. What about lines 244-245 there? I think they are no-op.
> Interesting that those two parts were added in quite different
> commits.

val[0] = (u16)ret & 0xFF;
val[1] = (u16)ret >> 8;

looks like cpu_to_be16()?

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Andy Shevchenko]

On Mon, Jul 3, 2017 at 8:14 PM, Sebastian Reichel
<sebastian.reichel@collabora.co.uk> wrote:
> Hi,
>
> On Mon, Jul 03, 2017 at 07:37:59PM +0300, Andy Shevchenko wrote:
>> On Mon, Jul 3, 2017 at 4:36 PM, Sebastian Reichel
>> <sebastian.reichel@collabora.co.uk> wrote:
>> > On Sat, Jul 01, 2017 at 09:28:10PM +0200, Julia Lawall wrote:
>>
>> >  * drivers/gpio/gpio-pca953x.c (line 190-192)
>>
>> It has double conversion there:
>> 1. LE CPU: Read as LE and converted to LE (no-op), so, just u16
>> 2. BE CPU: Read as BE and converted to LE, makes it __le16
>>
>> Looks like the conversion is not needed, only get_unaligned() is necessary.
>>
>> P.S. What about lines 244-245 there? I think they are no-op.
>> Interesting that those two parts were added in quite different
>> commits.
>
> val[0] = (u16)ret & 0xFF;
> val[1] = (u16)ret >> 8;
>
> looks like cpu_to_be16()?

cpu_to_le16(). No-op on LE CPU.

Perhaps they should be replaced by put_unaligned().

-- 
With Best Regards,
Andy Shevchenko

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1158 bytes --]

Hi,

On Mon, Jul 03, 2017 at 08:33:53PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 3, 2017 at 8:14 PM, Sebastian Reichel
> <sebastian.reichel@collabora.co.uk> wrote:
> > On Mon, Jul 03, 2017 at 07:37:59PM +0300, Andy Shevchenko wrote:
> >> On Mon, Jul 3, 2017 at 4:36 PM, Sebastian Reichel
> >> <sebastian.reichel@collabora.co.uk> wrote:
> >> > On Sat, Jul 01, 2017 at 09:28:10PM +0200, Julia Lawall wrote:
> >>
> >> >  * drivers/gpio/gpio-pca953x.c (line 190-192)
> >>
> >> It has double conversion there:
> >> 1. LE CPU: Read as LE and converted to LE (no-op), so, just u16
> >> 2. BE CPU: Read as BE and converted to LE, makes it __le16
> >>
> >> Looks like the conversion is not needed, only get_unaligned() is necessary.
> >>
> >> P.S. What about lines 244-245 there? I think they are no-op.
> >> Interesting that those two parts were added in quite different
> >> commits.
> >
> > val[0] = (u16)ret & 0xFF;
> > val[1] = (u16)ret >> 8;
> >
> > looks like cpu_to_be16()?
> 
> cpu_to_le16(). No-op on LE CPU.

uhm yes of course.

> Perhaps they should be replaced by put_unaligned().

Makes sense to me.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Julia Lawall]

Here is a revised version (not a patch because it doesn't support all of
the various modes) and the results.  It doesn't return anything beyond
what was mentioned in previous mails.

For the following code:

        ret = i2c_smbus_read_word_data(chip->client, reg << 1);
        val[0] = (u16)ret & 0xFF;
        val[1] = (u16)ret >> 8;

do we want to see:

put_unaligned(val,i2c_smbus_read_word_data(chip->client, reg << 1));

julia

---

@@
expression e, x;
@@

* x = i2c_smbus_read_word_data(...)
... when != x = e
* le16_to_cpu(x)

@@
expression e, e1, x, y;
@@

* x = i2c_smbus_read_word_data(...)
... when != x = e
* y = x;
... when != y = e1
* le16_to_cpu(y)

@@
@@

* le16_to_cpu(i2c_smbus_read_word_data(...))

// -------------------------------------------------------------

@@
expression e, e1, e2, x;
type T;
@@

* x = cpu_to_le16(...)
... when != x = e
* i2c_smbus_write_word_data(e1,e2,(T)x)

@@
expression e, e1, e2, e3, x, y;
type T;
@@

* x = cpu_to_le16(...)
... when != x = e
* y = x
... when != y = e3
* i2c_smbus_write_word_data(e1,e2,(T)y)

@@
expression e1,e2;
@@

* i2c_smbus_write_word_data(e1,e2,cpu_to_le16(...))

// -------------------------------------------------------------

@@
expression e1,e2;
typedef u16;
@@

*       e1[0] = (u16)e2 & 0xFF;
	e1[1] = (u16)e2 >> 8;

---

- means line of interest, not line to remove.

diff -u -p a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -187,10 +187,7 @@ static int pca953x_write_regs_8(struct p

 static int pca953x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
 {
-	__le16 word = cpu_to_le16(get_unaligned((u16 *)val));

-	return i2c_smbus_write_word_data(chip->client,
-					 reg << 1, (__force u16)word);
 }

 static int pca957x_write_regs_16(struct pca953x_chip *chip, int reg, u8 *val)
@@ -241,7 +238,6 @@ static int pca953x_read_regs_16(struct p
 	int ret;

 	ret = i2c_smbus_read_word_data(chip->client, reg << 1);
-	val[0] = (u16)ret & 0xFF;
 	val[1] = (u16)ret >> 8;

 	return ret;
diff -u -p a/drivers/macintosh/windfarm_smu_sat.c b/drivers/macintosh/windfarm_smu_sat.c
--- a/drivers/macintosh/windfarm_smu_sat.c
+++ b/drivers/macintosh/windfarm_smu_sat.c
@@ -77,18 +77,15 @@ struct smu_sdbp_header *smu_sat_get_sdb_
 		return NULL;
 	}

-	err = i2c_smbus_read_word_data(sat->i2c, 9);
 	if (err < 0) {
 		printk(KERN_ERR "smu_sat_get_sdb_part rd len error\n");
 		return NULL;
 	}
-	len = err;
 	if (len == 0) {
 		printk(KERN_ERR "smu_sat_get_sdb_part no partition %x\n", id);
 		return NULL;
 	}

-	len = le16_to_cpu(len);
 	len = (len + 3) & ~3;
 	buf = kmalloc(len, GFP_KERNEL);
 	if (buf == NULL)
diff -u -p a/drivers/macintosh/windfarm_lm75_sensor.c b/drivers/macintosh/windfarm_lm75_sensor.c
--- a/drivers/macintosh/windfarm_lm75_sensor.c
+++ b/drivers/macintosh/windfarm_lm75_sensor.c
@@ -68,7 +68,6 @@ static int wf_lm75_get(struct wf_sensor
 	}

 	/* Read temperature register */
-	data = (s32)le16_to_cpu(i2c_smbus_read_word_data(lm->i2c, 0));
 	data <<= 8;
 	*value = data;

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

[Andy Shevchenko]

On Tue, Jul 4, 2017 at 12:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> Here is a revised version (not a patch because it doesn't support all of
> the various modes) and the results.  It doesn't return anything beyond
> what was mentioned in previous mails.
>
> For the following code:
>
>         ret = i2c_smbus_read_word_data(chip->client, reg << 1);
>         val[0] = (u16)ret & 0xFF;
>         val[1] = (u16)ret >> 8;
>
> do we want to see:
>
> put_unaligned(val,i2c_smbus_read_word_data(chip->client, reg << 1));

If and only if the type of the pointer is a byte type (u8 *, char *,
or alike) _and_ we try to use it as a provider for 16-bit value (2
bytes).

-- 
With Best Regards,
Andy Shevchenko

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

[Julia Lawall]

On Tue, 4 Jul 2017, Andy Shevchenko wrote:

> On Tue, Jul 4, 2017 at 12:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > Here is a revised version (not a patch because it doesn't support all of
> > the various modes) and the results.  It doesn't return anything beyond
> > what was mentioned in previous mails.
> >
> > For the following code:
> >
> >         ret = i2c_smbus_read_word_data(chip->client, reg << 1);
> >         val[0] = (u16)ret & 0xFF;
> >         val[1] = (u16)ret >> 8;
> >
> > do we want to see:
> >
> > put_unaligned(val,i2c_smbus_read_word_data(chip->client, reg << 1));
>
> If and only if the type of the pointer is a byte type (u8 *, char *,
> or alike) _and_ we try to use it as a provider for 16-bit value (2
> bytes).

OK, the provider part seems to add more complexity than is worth putting
in the rule, so I will let the developer figure out what should be done.

thanks,
julia