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 <[email protected]>
Reviewed-by: Ping Cheng <[email protected]>
---
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
On Wed, Aug 3, 2022 at 4:59 PM Jason Gerecke <[email protected]> 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
On Wed, Aug 3, 2022 at 9:47 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Aug 3, 2022 at 4:59 PM Jason Gerecke <[email protected]> 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
On Wed, 3 Aug 2022 07:59:37 -0700
Jason Gerecke <[email protected]> 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 <[email protected]>
> Reviewed-by: Ping Cheng <[email protected]>
With the minor stuff Andy raised tidied up I'm fine with this change.
Acked-by: Jonathan Cameron <[email protected]>
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);
> };
>
> > 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.
On Wed, Oct 19, 2022 at 1:11 PM Wolfram Sang
<[email protected]> 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.
> 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.