[PATCH v2] i2c: Use u8 type in i2c transfer calls

[PATCH v2] i2c: Use u8 type in i2c transfer calls

[Jason Gerecke]

The 'i2c_transfer_buffer_flags' function (and related inlines) defines its
'buf' argument to be of type 'char*'. This is a poor choice of type given
that most callers actually pass a 'u8*' and that the function itself ends
up just storing the variable to a 'u8*'-typed member of 'struct i2c_msg'
anyway.

Changing the type of the 'buf' argument to 'u8*' vastly reduces the number
of (admittedly usually-silent) Wpointer-sign warnings that are generated
as the types get needlessly juggled back and forth.

At the same time, update the max1363 driver to match the new interface so
we don't introduce a new Wincompatible-function-pointer-types warning.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
---
Changes in v2:
  - Added modifications to the max1363 driver required to avoid warnings

 drivers/i2c/i2c-core-base.c |  2 +-
 drivers/iio/adc/max1363.c   |  8 ++++----
 include/linux/i2c.h         | 14 +++++++-------
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 10f35f942066a..2925507e8626d 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2184,7 +2184,7 @@ EXPORT_SYMBOL(i2c_transfer);
  *
  * Returns negative errno, or else the number of bytes transferred.
  */
-int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
+int i2c_transfer_buffer_flags(const struct i2c_client *client, u8 *buf,
 			      int count, u16 flags)
 {
 	int ret;
diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index eef55ed4814a6..ebe6eb99583da 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -184,9 +184,9 @@ struct max1363_state {
 	struct regulator		*vref;
 	u32				vref_uv;
 	int				(*send)(const struct i2c_client *client,
-						const char *buf, int count);
+						const u8 *buf, int count);
 	int				(*recv)(const struct i2c_client *client,
-						char *buf, int count);
+						u8 *buf, int count);
 };
 
 #define MAX1363_MODE_SINGLE(_num, _mask) {				\
@@ -312,7 +312,7 @@ static const struct max1363_mode
 	return NULL;
 }
 
-static int max1363_smbus_send(const struct i2c_client *client, const char *buf,
+static int max1363_smbus_send(const struct i2c_client *client, const u8 *buf,
 		int count)
 {
 	int i, err;
@@ -323,7 +323,7 @@ static int max1363_smbus_send(const struct i2c_client *client, const char *buf,
 	return err ? err : count;
 }
 
-static int max1363_smbus_recv(const struct i2c_client *client, char *buf,
+static int max1363_smbus_recv(const struct i2c_client *client, u8 *buf,
 		int count)
 {
 	int i, ret;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8eab5017bff30..3a94385f4642c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -64,7 +64,7 @@ const char *i2c_freq_mode_string(u32 bus_freq_hz);
  * @count must be less than 64k since msg.len is u16.
  */
 int i2c_transfer_buffer_flags(const struct i2c_client *client,
-			      char *buf, int count, u16 flags);
+			      u8 *buf, int count, u16 flags);
 
 /**
  * i2c_master_recv - issue a single I2C message in master receive mode
@@ -75,7 +75,7 @@ int i2c_transfer_buffer_flags(const struct i2c_client *client,
  * Returns negative errno, or else the number of bytes read.
  */
 static inline int i2c_master_recv(const struct i2c_client *client,
-				  char *buf, int count)
+				  u8 *buf, int count)
 {
 	return i2c_transfer_buffer_flags(client, buf, count, I2C_M_RD);
 };
@@ -90,7 +90,7 @@ static inline int i2c_master_recv(const struct i2c_client *client,
  * Returns negative errno, or else the number of bytes read.
  */
 static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
-					  char *buf, int count)
+					  u8 *buf, int count)
 {
 	return i2c_transfer_buffer_flags(client, buf, count,
 					 I2C_M_RD | I2C_M_DMA_SAFE);
@@ -105,9 +105,9 @@ static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
  * Returns negative errno, or else the number of bytes written.
  */
 static inline int i2c_master_send(const struct i2c_client *client,
-				  const char *buf, int count)
+				  const u8 *buf, int count)
 {
-	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
+	return i2c_transfer_buffer_flags(client, (u8 *)buf, count, 0);
 };
 
 /**
@@ -120,9 +120,9 @@ static inline int i2c_master_send(const struct i2c_client *client,
  * Returns negative errno, or else the number of bytes written.
  */
 static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
-					  const char *buf, int count)
+					  const u8 *buf, int count)
 {
-	return i2c_transfer_buffer_flags(client, (char *)buf, count,
+	return i2c_transfer_buffer_flags(client, (u8 *)buf, count,
 					 I2C_M_DMA_SAFE);
 };
 
-- 
2.37.1

Re: [PATCH v2] i2c: Use u8 type in i2c transfer calls

[Andy Shevchenko]

On Wed, Aug 3, 2022 at 4:59 PM Jason Gerecke <killertofu@gmail.com> wrote:
>
> The 'i2c_transfer_buffer_flags' function (and related inlines) defines its

We refer to the functions like func() (without any quotes as well).

> 'buf' argument to be of type 'char*'. This is a poor choice of type given

char *

> that most callers actually pass a 'u8*' and that the function itself ends

most of the callers

u8 *

> up just storing the variable to a 'u8*'-typed member of 'struct i2c_msg'

u8 *

> anyway.
>
> Changing the type of the 'buf' argument to 'u8*' vastly reduces the number

u8 *

> of (admittedly usually-silent) Wpointer-sign warnings that are generated

-Wpointer-sign or replace with simple English words.

> as the types get needlessly juggled back and forth.
>
> At the same time, update the max1363 driver to match the new interface so
> we don't introduce a new Wincompatible-function-pointer-types warning.

-Wincompatible-function-pointer-types

...

> Changes in v2:
>   - Added modifications to the max1363 driver required to avoid warnings

Have you really checked _all_ callers of APIs that you have changed here?

For example, drivers/media/usb/em28xx/em28xx-input.c still uses
unsigned char for i2c_master_recv().

I believe you need to create a coccinelle script and run it over the
kernel source tree and then create a patch out of it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] i2c: Use u8 type in i2c transfer calls

[Jason Gerecke]

On Wed, Aug 3, 2022 at 9:47 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Aug 3, 2022 at 4:59 PM Jason Gerecke <killertofu@gmail.com> wrote:
> >
> > The 'i2c_transfer_buffer_flags' function (and related inlines) defines its
>
> We refer to the functions like func() (without any quotes as well).
>
> > 'buf' argument to be of type 'char*'. This is a poor choice of type given
>
> char *
>
> > that most callers actually pass a 'u8*' and that the function itself ends
>
> most of the callers
>
> u8 *
>
> > up just storing the variable to a 'u8*'-typed member of 'struct i2c_msg'
>
> u8 *
>
> > anyway.
> >
> > Changing the type of the 'buf' argument to 'u8*' vastly reduces the number
>
> u8 *
>
> > of (admittedly usually-silent) Wpointer-sign warnings that are generated
>
> -Wpointer-sign or replace with simple English words.
>
> > as the types get needlessly juggled back and forth.
> >
> > At the same time, update the max1363 driver to match the new interface so
> > we don't introduce a new Wincompatible-function-pointer-types warning.
>
> -Wincompatible-function-pointer-types
>
> ...
>
Ack to all.

> > Changes in v2:
> >   - Added modifications to the max1363 driver required to avoid warnings
>
> Have you really checked _all_ callers of APIs that you have changed here?
>
> For example, drivers/media/usb/em28xx/em28xx-input.c still uses
> unsigned char for i2c_master_recv().
>

This particular example shouldn't result in a new warning since
unsigned char and u8 are equivalent types, and u8 is used by the new
API.

Assuming you're referring to callers that are still using *signed*
variables with this API, however, I intentionally ignored them. IIRC,
there were about 400 files using unsigned and about 60 files using
signed. Those 60 files will now generate their own pointer-sign
warnings, but I rationalized it as a still-substantial improvement
over the current state of things.

As for normally-silent warnings *other* than Wpointer-sign (e.g. the
Wincompatible-pointer-types in max1363), I also did not explicitly
check for those. It is possible other warnings may be out there.

> I believe you need to create a coccinelle script and run it over the
> kernel source tree and then create a patch out of it.
>
> --
> With Best Regards,
> Andy Shevchenko

This would definitely be necessary to unify all callers to using
unsigned variables rather than just swapping which callers generate
the pointer-sign warnings.

Jason

Re: [PATCH v2] i2c: Use u8 type in i2c transfer calls

[Wolfram Sang]

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


> > I believe you need to create a coccinelle script and run it over the
> > kernel source tree and then create a patch out of it.
> 
> This would definitely be necessary to unify all callers to using
> unsigned variables rather than just swapping which callers generate
> the pointer-sign warnings.

I am all for using u8 because this is the proper type.

Yet, if we touch this function argument, I'd also like to remove all
inconsistencies once and for all. Removing some warnings here and add
some there is not a good choice IMO. However, how to do this switch of
types cleanly without too much churn, I sadly have no good idea yet.


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

Re: [PATCH v2] i2c: Use u8 type in i2c transfer calls

[Jason Gerecke]

On Wed, Oct 19, 2022 at 1:11 PM Wolfram Sang
<wsa-dev@sang-engineering.com> wrote:
>
>
> > > I believe you need to create a coccinelle script and run it over the
> > > kernel source tree and then create a patch out of it.
> >
> > This would definitely be necessary to unify all callers to using
> > unsigned variables rather than just swapping which callers generate
> > the pointer-sign warnings.
>
> I am all for using u8 because this is the proper type.
>
> Yet, if we touch this function argument, I'd also like to remove all
> inconsistencies once and for all. Removing some warnings here and add
> some there is not a good choice IMO. However, how to do this switch of
> types cleanly without too much churn, I sadly have no good idea yet.
>

I spent a little time trying to put together a Coccinelle script to
take care of everything but I eventually realized the size of the task
was larger than I was comfortable with. In particular, even though I
might be able to put together a script, I worry I don't have a good
way to test the resulting treewide changes to avoid regression.

Re: [PATCH v2] i2c: Use u8 type in i2c transfer calls

[Wolfram Sang]

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


> I spent a little time trying to put together a Coccinelle script to
> take care of everything but I eventually realized the size of the task
> was larger than I was comfortable with. In particular, even though I
> might be able to put together a script, I worry I don't have a good
> way to test the resulting treewide changes to avoid regression.

The coccinelle scripts are one thing. I am quite familiar with it, so I
regard this as "work but doable". My main headache is that I am not sure
about the best way to upstream the result. I'd like to avoid a flag-day
where all drivers across all subsystems need to be converted, but I
don't really see a way around it. Preparing such a branch and make sure
it does not regress is quite some work on a moving target.


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

Re: [PATCH v2] i2c: Use u8 type in i2c transfer calls

[Jonathan Cameron]

On Wed, 19 Oct 2022 23:37:51 +0200
Wolfram Sang <wsa-dev@sang-engineering.com> wrote:

> > I spent a little time trying to put together a Coccinelle script to
> > take care of everything but I eventually realized the size of the task
> > was larger than I was comfortable with. In particular, even though I
> > might be able to put together a script, I worry I don't have a good
> > way to test the resulting treewide changes to avoid regression.  
> 
> The coccinelle scripts are one thing. I am quite familiar with it, so I
> regard this as "work but doable". My main headache is that I am not sure
> about the best way to upstream the result. I'd like to avoid a flag-day
> where all drivers across all subsystems need to be converted, but I
> don't really see a way around it. Preparing such a branch and make sure
> it does not regress is quite some work on a moving target.

Horrendous though it is, you 'could' take it via a void * intermediate
step.   That way all the warnings will disappear (I think).
You then move all the callers to providing u8 * then switch the function
to that.  Could happen over several cycles with coccicheck moaning about
any new entries in the meantime.

Jonathan


> 
> 

Re: [PATCH v2] i2c: Use u8 type in i2c transfer calls

[Jonathan Cameron]

On Wed,  3 Aug 2022 07:59:37 -0700
Jason Gerecke <killertofu@gmail.com> wrote:

> The 'i2c_transfer_buffer_flags' function (and related inlines) defines its
> 'buf' argument to be of type 'char*'. This is a poor choice of type given
> that most callers actually pass a 'u8*' and that the function itself ends
> up just storing the variable to a 'u8*'-typed member of 'struct i2c_msg'
> anyway.
> 
> Changing the type of the 'buf' argument to 'u8*' vastly reduces the number
> of (admittedly usually-silent) Wpointer-sign warnings that are generated
> as the types get needlessly juggled back and forth.
> 
> At the same time, update the max1363 driver to match the new interface so
> we don't introduce a new Wincompatible-function-pointer-types warning.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> Reviewed-by: Ping Cheng <ping.cheng@wacom.com>

With the minor stuff Andy raised tidied up I'm fine with this change.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I'd forgotten all about the oddities of the max1363 :) That brings
back some memories!

Jonathan


> ---
> Changes in v2:
>   - Added modifications to the max1363 driver required to avoid warnings
> 
>  drivers/i2c/i2c-core-base.c |  2 +-
>  drivers/iio/adc/max1363.c   |  8 ++++----
>  include/linux/i2c.h         | 14 +++++++-------
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 10f35f942066a..2925507e8626d 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2184,7 +2184,7 @@ EXPORT_SYMBOL(i2c_transfer);
>   *
>   * Returns negative errno, or else the number of bytes transferred.
>   */
> -int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
> +int i2c_transfer_buffer_flags(const struct i2c_client *client, u8 *buf,
>  			      int count, u16 flags)
>  {
>  	int ret;
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index eef55ed4814a6..ebe6eb99583da 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -184,9 +184,9 @@ struct max1363_state {
>  	struct regulator		*vref;
>  	u32				vref_uv;
>  	int				(*send)(const struct i2c_client *client,
> -						const char *buf, int count);
> +						const u8 *buf, int count);
>  	int				(*recv)(const struct i2c_client *client,
> -						char *buf, int count);
> +						u8 *buf, int count);
>  };
>  
>  #define MAX1363_MODE_SINGLE(_num, _mask) {				\
> @@ -312,7 +312,7 @@ static const struct max1363_mode
>  	return NULL;
>  }
>  
> -static int max1363_smbus_send(const struct i2c_client *client, const char *buf,
> +static int max1363_smbus_send(const struct i2c_client *client, const u8 *buf,
>  		int count)
>  {
>  	int i, err;
> @@ -323,7 +323,7 @@ static int max1363_smbus_send(const struct i2c_client *client, const char *buf,
>  	return err ? err : count;
>  }
>  
> -static int max1363_smbus_recv(const struct i2c_client *client, char *buf,
> +static int max1363_smbus_recv(const struct i2c_client *client, u8 *buf,
>  		int count)
>  {
>  	int i, ret;
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 8eab5017bff30..3a94385f4642c 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -64,7 +64,7 @@ const char *i2c_freq_mode_string(u32 bus_freq_hz);
>   * @count must be less than 64k since msg.len is u16.
>   */
>  int i2c_transfer_buffer_flags(const struct i2c_client *client,
> -			      char *buf, int count, u16 flags);
> +			      u8 *buf, int count, u16 flags);
>  
>  /**
>   * i2c_master_recv - issue a single I2C message in master receive mode
> @@ -75,7 +75,7 @@ int i2c_transfer_buffer_flags(const struct i2c_client *client,
>   * Returns negative errno, or else the number of bytes read.
>   */
>  static inline int i2c_master_recv(const struct i2c_client *client,
> -				  char *buf, int count)
> +				  u8 *buf, int count)
>  {
>  	return i2c_transfer_buffer_flags(client, buf, count, I2C_M_RD);
>  };
> @@ -90,7 +90,7 @@ static inline int i2c_master_recv(const struct i2c_client *client,
>   * Returns negative errno, or else the number of bytes read.
>   */
>  static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
> -					  char *buf, int count)
> +					  u8 *buf, int count)
>  {
>  	return i2c_transfer_buffer_flags(client, buf, count,
>  					 I2C_M_RD | I2C_M_DMA_SAFE);
> @@ -105,9 +105,9 @@ static inline int i2c_master_recv_dmasafe(const struct i2c_client *client,
>   * Returns negative errno, or else the number of bytes written.
>   */
>  static inline int i2c_master_send(const struct i2c_client *client,
> -				  const char *buf, int count)
> +				  const u8 *buf, int count)
>  {
> -	return i2c_transfer_buffer_flags(client, (char *)buf, count, 0);
> +	return i2c_transfer_buffer_flags(client, (u8 *)buf, count, 0);
>  };
>  
>  /**
> @@ -120,9 +120,9 @@ static inline int i2c_master_send(const struct i2c_client *client,
>   * Returns negative errno, or else the number of bytes written.
>   */
>  static inline int i2c_master_send_dmasafe(const struct i2c_client *client,
> -					  const char *buf, int count)
> +					  const u8 *buf, int count)
>  {
> -	return i2c_transfer_buffer_flags(client, (char *)buf, count,
> +	return i2c_transfer_buffer_flags(client, (u8 *)buf, count,
>  					 I2C_M_DMA_SAFE);
>  };
>