Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1680016 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-03 15:40 +0200
    Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-03 18:40 +0200
      Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-03 19:20 +0200
        Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-03 19:40 +0200
          Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-07-03 20:30 +0200
            Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu Julia Lawall <julia.lawall@lip6.fr> - 2017-07-04 11:20 +0200
              Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-04 12:00 +0200
                Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu Julia Lawall <julia.lawall@lip6.fr> - 2017-07-04 12:00 +0200

#1680016 — Re: [PATCH] coccinelle: api: detect unnecessary le16_to_cpu

[Multipart message — attachments visible in raw view] — view raw

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)
> 

[toc] | [next] | [standalone]

#1680485

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

[toc] | [prev] | [next] | [standalone]

#1680511

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1680513

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

[toc] | [prev] | [next] | [standalone]

#1680523

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1680791

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;

[toc] | [prev] | [next] | [standalone]

#1680824

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

[toc] | [prev] | [next] | [standalone]

#1680826

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web