| Message ID | 20220803145937.698603-1-jason.gerecke@wacom.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i2c: Use u8 type in i2c transfer calls | expand |
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.
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
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); > }; >
> > 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 <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.
> 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.
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 > >
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); };