* Re: [PATCH] iio:dac: fix i2c_master_send return code misuse
2015-10-27 16:25 ` [PATCH] " Jean-Francois Dagenais
@ 2015-10-27 16:34 ` Jean-François Dagenais
2015-10-31 11:00 ` Jonathan Cameron
2015-11-03 14:27 ` Lars-Peter Clausen
2 siblings, 0 replies; 9+ messages in thread
From: Jean-François Dagenais @ 2015-10-27 16:34 UTC (permalink / raw)
To: linux-iio@vger.kernel.org
Cc: jic23, knaack.h, lars, pmeerw, Michael.Hennerich,
Jean-Francois Dagenais
Note: I've made these changes from searching i2c_master_send in v3.19
-- drivers/iio/dac only.
On Tue, Oct 27, 2015 at 12:25 PM, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
> Contrary to most kernel functions, successful call to
> i2c_master_send returns the number or written bytes,not 0.
>
> However, these callers return the value as is without converting
> the return style to conform to the expected standard.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
> ---
> drivers/iio/dac/ad5064.c | 5 ++++-
> drivers/iio/dac/ad5446.c | 4 +++-
> drivers/iio/dac/max517.c | 6 ++++--
> drivers/iio/dac/max5821.c | 8 ++++++--
> drivers/iio/dac/mcp4725.c | 10 ++++++++--
> 5 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5064.c b/drivers/iio/dac/ad5064.c
> index c067e68..0ff1107 100644
> --- a/drivers/iio/dac/ad5064.c
> +++ b/drivers/iio/dac/ad5064.c
> @@ -598,10 +598,13 @@ static int ad5064_i2c_write(struct ad5064_state *st, unsigned int cmd,
> unsigned int addr, unsigned int val)
> {
> struct i2c_client *i2c = to_i2c_client(st->dev);
> + int res;
>
> st->data.i2c[0] = (cmd << 4) | addr;
> put_unaligned_be16(val, &st->data.i2c[1]);
> - return i2c_master_send(i2c, st->data.i2c, 3);
> + res = i2c_master_send(i2c, st->data.i2c, 3);
> +
> + return res == 3 ? 0 : res;
> }
>
> static int ad5064_i2c_probe(struct i2c_client *i2c,
> diff --git a/drivers/iio/dac/ad5446.c b/drivers/iio/dac/ad5446.c
> index 07e17d7..9a49941 100644
> --- a/drivers/iio/dac/ad5446.c
> +++ b/drivers/iio/dac/ad5446.c
> @@ -512,7 +512,9 @@ static int ad5622_write(struct ad5446_state *st, unsigned val)
> struct i2c_client *client = to_i2c_client(st->dev);
> __be16 data = cpu_to_be16(val);
>
> - return i2c_master_send(client, (char *)&data, sizeof(data));
> + int res = i2c_master_send(client, (char *)&data, sizeof(data));
> +
> + return res == sizeof(data)? 0 : res;
> }
>
> /**
> diff --git a/drivers/iio/dac/max517.c b/drivers/iio/dac/max517.c
> index 5507b39..0f3d81fc 100644
> --- a/drivers/iio/dac/max517.c
> +++ b/drivers/iio/dac/max517.c
> @@ -117,15 +117,17 @@ static int max517_write_raw(struct iio_dev *indio_dev,
> static int max517_suspend(struct device *dev)
> {
> u8 outbuf = COMMAND_PD;
> + int res = i2c_master_send(to_i2c_client(dev), &outbuf, 1);
>
> - return i2c_master_send(to_i2c_client(dev), &outbuf, 1);
> + return res == 1 ? 0 : res;
> }
>
> static int max517_resume(struct device *dev)
> {
> u8 outbuf = 0;
> + int res = i2c_master_send(to_i2c_client(dev), &outbuf, 1);
>
> - return i2c_master_send(to_i2c_client(dev), &outbuf, 1);
> + return res == 1 ? 0 : res;
> }
>
> static SIMPLE_DEV_PM_OPS(max517_pm_ops, max517_suspend, max517_resume);
> diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c
> index 28b8748..56f9252 100644
> --- a/drivers/iio/dac/max5821.c
> +++ b/drivers/iio/dac/max5821.c
> @@ -278,7 +278,9 @@ static int max5821_suspend(struct device *dev)
> MAX5821_EXTENDED_DAC_B |
> MAX5821_EXTENDED_POWER_DOWN_MODE2 };
>
> - return i2c_master_send(to_i2c_client(dev), outbuf, 2);
> + int res = i2c_master_send(to_i2c_client(dev), outbuf, 2);
> +
> + return res == 2 ? 0 : res;
> }
>
> static int max5821_resume(struct device *dev)
> @@ -288,7 +290,9 @@ static int max5821_resume(struct device *dev)
> MAX5821_EXTENDED_DAC_B |
> MAX5821_EXTENDED_POWER_UP };
>
> - return i2c_master_send(to_i2c_client(dev), outbuf, 2);
> + int res = i2c_master_send(to_i2c_client(dev), outbuf, 2);
> +
> + return res == 2 ? 0 : res;
> }
>
> static SIMPLE_DEV_PM_OPS(max5821_pm_ops, max5821_suspend, max5821_resume);
> diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> index 43d1458..166b607 100644
> --- a/drivers/iio/dac/mcp4725.c
> +++ b/drivers/iio/dac/mcp4725.c
> @@ -39,12 +39,15 @@ static int mcp4725_suspend(struct device *dev)
> struct mcp4725_data *data = iio_priv(i2c_get_clientdata(
> to_i2c_client(dev)));
> u8 outbuf[2];
> + int res;
>
> outbuf[0] = (data->powerdown_mode + 1) << 4;
> outbuf[1] = 0;
> data->powerdown = true;
>
> - return i2c_master_send(data->client, outbuf, 2);
> + res = i2c_master_send(data->client, outbuf, 2);
> +
> + return res == 2 ? 0 : res;
> }
>
> static int mcp4725_resume(struct device *dev)
> @@ -52,13 +55,16 @@ static int mcp4725_resume(struct device *dev)
> struct mcp4725_data *data = iio_priv(i2c_get_clientdata(
> to_i2c_client(dev)));
> u8 outbuf[2];
> + int res;
>
> /* restore previous DAC value */
> outbuf[0] = (data->dac_value >> 8) & 0xf;
> outbuf[1] = data->dac_value & 0xff;
> data->powerdown = false;
>
> - return i2c_master_send(data->client, outbuf, 2);
> + res = i2c_master_send(data->client, outbuf, 2);
> +
> + return res == 2 ? 0 : res;
> }
>
> #ifdef CONFIG_PM_SLEEP
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] iio:dac: fix i2c_master_send return code misuse
2015-10-27 16:25 ` [PATCH] " Jean-Francois Dagenais
2015-10-27 16:34 ` Jean-François Dagenais
@ 2015-10-31 11:00 ` Jonathan Cameron
2015-11-03 14:27 ` Lars-Peter Clausen
2 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2015-10-31 11:00 UTC (permalink / raw)
To: Jean-Francois Dagenais, linux-iio
Cc: knaack.h, lars, pmeerw, Michael.Hennerich
On 27/10/15 16:25, Jean-Francois Dagenais wrote:
> Contrary to most kernel functions, successful call to
> i2c_master_send returns the number or written bytes,not 0.
>
> However, these callers return the value as is without converting
> the return style to conform to the expected standard.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Whether these are actually an issue depends on whether the return code
is simply checked as a boolean, or if a less than 0 check is used.
Looks like the ad5064, ad5446 certainly uses it as a boolean.
The max517, max5821 and mcp4725 use them in the suspend and resume callbacks but
do protect against this for everything else. A bit of digging
in the power management code suggests that assumes a 0 return for
good (rather than 0 or positive) so these are indeed an issue.
I'd like to pick up a few Acks however before applying these as
I'm more than a little curious why we didn't get issues in testing
the ad5064 and ad5446 at least (suspend paths often don't work on
a given board for other reasons)
Lars some of these are yours. Are Jean-Francois and I missing something?
Jonathan
> ---
> drivers/iio/dac/ad5064.c | 5 ++++-
> drivers/iio/dac/ad5446.c | 4 +++-
> drivers/iio/dac/max517.c | 6 ++++--
> drivers/iio/dac/max5821.c | 8 ++++++--
> drivers/iio/dac/mcp4725.c | 10 ++++++++--
> 5 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5064.c b/drivers/iio/dac/ad5064.c
> index c067e68..0ff1107 100644
> --- a/drivers/iio/dac/ad5064.c
> +++ b/drivers/iio/dac/ad5064.c
> @@ -598,10 +598,13 @@ static int ad5064_i2c_write(struct ad5064_state *st, unsigned int cmd,
> unsigned int addr, unsigned int val)
> {
> struct i2c_client *i2c = to_i2c_client(st->dev);
> + int res;
>
> st->data.i2c[0] = (cmd << 4) | addr;
> put_unaligned_be16(val, &st->data.i2c[1]);
> - return i2c_master_send(i2c, st->data.i2c, 3);
> + res = i2c_master_send(i2c, st->data.i2c, 3);
> +
> + return res == 3 ? 0 : res;
> }
>
> static int ad5064_i2c_probe(struct i2c_client *i2c,
> diff --git a/drivers/iio/dac/ad5446.c b/drivers/iio/dac/ad5446.c
> index 07e17d7..9a49941 100644
> --- a/drivers/iio/dac/ad5446.c
> +++ b/drivers/iio/dac/ad5446.c
> @@ -512,7 +512,9 @@ static int ad5622_write(struct ad5446_state *st, unsigned val)
> struct i2c_client *client = to_i2c_client(st->dev);
> __be16 data = cpu_to_be16(val);
>
> - return i2c_master_send(client, (char *)&data, sizeof(data));
> + int res = i2c_master_send(client, (char *)&data, sizeof(data));
> +
> + return res == sizeof(data)? 0 : res;
> }
>
> /**
> diff --git a/drivers/iio/dac/max517.c b/drivers/iio/dac/max517.c
> index 5507b39..0f3d81fc 100644
> --- a/drivers/iio/dac/max517.c
> +++ b/drivers/iio/dac/max517.c
> @@ -117,15 +117,17 @@ static int max517_write_raw(struct iio_dev *indio_dev,
> static int max517_suspend(struct device *dev)
> {
> u8 outbuf = COMMAND_PD;
> + int res = i2c_master_send(to_i2c_client(dev), &outbuf, 1);
>
> - return i2c_master_send(to_i2c_client(dev), &outbuf, 1);
> + return res == 1 ? 0 : res;
> }
>
> static int max517_resume(struct device *dev)
> {
> u8 outbuf = 0;
> + int res = i2c_master_send(to_i2c_client(dev), &outbuf, 1);
>
> - return i2c_master_send(to_i2c_client(dev), &outbuf, 1);
> + return res == 1 ? 0 : res;
> }
>
> static SIMPLE_DEV_PM_OPS(max517_pm_ops, max517_suspend, max517_resume);
> diff --git a/drivers/iio/dac/max5821.c b/drivers/iio/dac/max5821.c
> index 28b8748..56f9252 100644
> --- a/drivers/iio/dac/max5821.c
> +++ b/drivers/iio/dac/max5821.c
> @@ -278,7 +278,9 @@ static int max5821_suspend(struct device *dev)
> MAX5821_EXTENDED_DAC_B |
> MAX5821_EXTENDED_POWER_DOWN_MODE2 };
>
> - return i2c_master_send(to_i2c_client(dev), outbuf, 2);
> + int res = i2c_master_send(to_i2c_client(dev), outbuf, 2);
> +
> + return res == 2 ? 0 : res;
> }
>
> static int max5821_resume(struct device *dev)
> @@ -288,7 +290,9 @@ static int max5821_resume(struct device *dev)
> MAX5821_EXTENDED_DAC_B |
> MAX5821_EXTENDED_POWER_UP };
>
> - return i2c_master_send(to_i2c_client(dev), outbuf, 2);
> + int res = i2c_master_send(to_i2c_client(dev), outbuf, 2);
> +
> + return res == 2 ? 0 : res;
> }
>
> static SIMPLE_DEV_PM_OPS(max5821_pm_ops, max5821_suspend, max5821_resume);
> diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> index 43d1458..166b607 100644
> --- a/drivers/iio/dac/mcp4725.c
> +++ b/drivers/iio/dac/mcp4725.c
> @@ -39,12 +39,15 @@ static int mcp4725_suspend(struct device *dev)
> struct mcp4725_data *data = iio_priv(i2c_get_clientdata(
> to_i2c_client(dev)));
> u8 outbuf[2];
> + int res;
>
> outbuf[0] = (data->powerdown_mode + 1) << 4;
> outbuf[1] = 0;
> data->powerdown = true;
>
> - return i2c_master_send(data->client, outbuf, 2);
> + res = i2c_master_send(data->client, outbuf, 2);
> +
> + return res == 2 ? 0 : res;
> }
>
> static int mcp4725_resume(struct device *dev)
> @@ -52,13 +55,16 @@ static int mcp4725_resume(struct device *dev)
> struct mcp4725_data *data = iio_priv(i2c_get_clientdata(
> to_i2c_client(dev)));
> u8 outbuf[2];
> + int res;
>
> /* restore previous DAC value */
> outbuf[0] = (data->dac_value >> 8) & 0xf;
> outbuf[1] = data->dac_value & 0xff;
> data->powerdown = false;
>
> - return i2c_master_send(data->client, outbuf, 2);
> + res = i2c_master_send(data->client, outbuf, 2);
> +
> + return res == 2 ? 0 : res;
> }
>
> #ifdef CONFIG_PM_SLEEP
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] iio:dac: fix i2c_master_send return code misuse
2015-10-27 16:25 ` [PATCH] " Jean-Francois Dagenais
2015-10-27 16:34 ` Jean-François Dagenais
2015-10-31 11:00 ` Jonathan Cameron
@ 2015-11-03 14:27 ` Lars-Peter Clausen
2015-11-04 1:10 ` Dmitry Torokhov
2 siblings, 1 reply; 9+ messages in thread
From: Lars-Peter Clausen @ 2015-11-03 14:27 UTC (permalink / raw)
To: Jean-Francois Dagenais, linux-iio
Cc: jic23, knaack.h, pmeerw, Michael.Hennerich, Dmitry Torokhov
Hi,
On 10/27/2015 05:25 PM, Jean-Francois Dagenais wrote:
> Contrary to most kernel functions, successful call to
> i2c_master_send returns the number or written bytes,not 0.
>
> However, these callers return the value as is without converting
> the return style to conform to the expected standard.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Thanks for the patch.
> ---
> drivers/iio/dac/ad5064.c | 5 ++++-
> drivers/iio/dac/ad5446.c | 4 +++-
> drivers/iio/dac/max517.c | 6 ++++--
> drivers/iio/dac/max5821.c | 8 ++++++--
> drivers/iio/dac/mcp4725.c | 10 ++++++++--
> 5 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5064.c b/drivers/iio/dac/ad5064.c
> index c067e68..0ff1107 100644
> --- a/drivers/iio/dac/ad5064.c
> +++ b/drivers/iio/dac/ad5064.c
> @@ -598,10 +598,13 @@ static int ad5064_i2c_write(struct ad5064_state *st, unsigned int cmd,
> unsigned int addr, unsigned int val)
> {
> struct i2c_client *i2c = to_i2c_client(st->dev);
> + int res;
>
> st->data.i2c[0] = (cmd << 4) | addr;
> put_unaligned_be16(val, &st->data.i2c[1]);
> - return i2c_master_send(i2c, st->data.i2c, 3);
> + res = i2c_master_send(i2c, st->data.i2c, 3);
> +
> + return res == 3 ? 0 : res;
> }
The ad5064 is already fixed in Jonathan's tree.
>
> static int ad5064_i2c_probe(struct i2c_client *i2c,
> diff --git a/drivers/iio/dac/ad5446.c b/drivers/iio/dac/ad5446.c
> index 07e17d7..9a49941 100644
> --- a/drivers/iio/dac/ad5446.c
> +++ b/drivers/iio/dac/ad5446.c
> @@ -512,7 +512,9 @@ static int ad5622_write(struct ad5446_state *st, unsigned val)
> struct i2c_client *client = to_i2c_client(st->dev);
> __be16 data = cpu_to_be16(val);
>
> - return i2c_master_send(client, (char *)&data, sizeof(data));
> + int res = i2c_master_send(client, (char *)&data, sizeof(data));
> +
> + return res == sizeof(data)? 0 : res;
This still returns the wrong if res is between 0 and sizeof(data)-1. You
need something like
if (res == sizeof(data))
return 0
else if (res >= 0)
return -EIO;
else
return res;
Same is true for all the other drivers patched.
I really wish we could fix this in the I2C core to avoid having to copy and
paste the same error handling to each driver.
There was this patch[1], but it looks like it was never followed by a v2.
Dmitry I'm curious, what happened to that patch?
- Lars
[1] https://patchwork.ozlabs.org/patch/220915/
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] iio:dac: fix i2c_master_send return code misuse
2015-11-03 14:27 ` Lars-Peter Clausen
@ 2015-11-04 1:10 ` Dmitry Torokhov
2015-11-08 13:28 ` Jonathan Cameron
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Torokhov @ 2015-11-04 1:10 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Jean-Francois Dagenais, linux-iio, jic23, knaack.h, pmeerw,
Michael.Hennerich
On Tue, Nov 03, 2015 at 03:27:42PM +0100, Lars-Peter Clausen wrote:
> Hi,
>
> On 10/27/2015 05:25 PM, Jean-Francois Dagenais wrote:
> > Contrary to most kernel functions, successful call to
> > i2c_master_send returns the number or written bytes,not 0.
> >
> > However, these callers return the value as is without converting
> > the return style to conform to the expected standard.
> >
> > Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
>
> Thanks for the patch.
>
> > ---
> > drivers/iio/dac/ad5064.c | 5 ++++-
> > drivers/iio/dac/ad5446.c | 4 +++-
> > drivers/iio/dac/max517.c | 6 ++++--
> > drivers/iio/dac/max5821.c | 8 ++++++--
> > drivers/iio/dac/mcp4725.c | 10 ++++++++--
> > 5 files changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/iio/dac/ad5064.c b/drivers/iio/dac/ad5064.c
> > index c067e68..0ff1107 100644
> > --- a/drivers/iio/dac/ad5064.c
> > +++ b/drivers/iio/dac/ad5064.c
> > @@ -598,10 +598,13 @@ static int ad5064_i2c_write(struct ad5064_state *st, unsigned int cmd,
> > unsigned int addr, unsigned int val)
> > {
> > struct i2c_client *i2c = to_i2c_client(st->dev);
> > + int res;
> >
> > st->data.i2c[0] = (cmd << 4) | addr;
> > put_unaligned_be16(val, &st->data.i2c[1]);
> > - return i2c_master_send(i2c, st->data.i2c, 3);
> > + res = i2c_master_send(i2c, st->data.i2c, 3);
> > +
> > + return res == 3 ? 0 : res;
> > }
>
> The ad5064 is already fixed in Jonathan's tree.
>
> >
> > static int ad5064_i2c_probe(struct i2c_client *i2c,
> > diff --git a/drivers/iio/dac/ad5446.c b/drivers/iio/dac/ad5446.c
> > index 07e17d7..9a49941 100644
> > --- a/drivers/iio/dac/ad5446.c
> > +++ b/drivers/iio/dac/ad5446.c
> > @@ -512,7 +512,9 @@ static int ad5622_write(struct ad5446_state *st, unsigned val)
> > struct i2c_client *client = to_i2c_client(st->dev);
> > __be16 data = cpu_to_be16(val);
> >
> > - return i2c_master_send(client, (char *)&data, sizeof(data));
> > + int res = i2c_master_send(client, (char *)&data, sizeof(data));
> > +
> > + return res == sizeof(data)? 0 : res;
>
> This still returns the wrong if res is between 0 and sizeof(data)-1. You
> need something like
>
> if (res == sizeof(data))
> return 0
> else if (res >= 0)
> return -EIO;
> else
> return res;
>
> Same is true for all the other drivers patched.
>
> I really wish we could fix this in the I2C core to avoid having to copy and
> paste the same error handling to each driver.
>
> There was this patch[1], but it looks like it was never followed by a v2.
> Dmitry I'm curious, what happened to that patch?
Fell through the cracks I'm afraid. I think facility like this is still
needed as majority of the drivers do not care about short reads in the
slightest.
--
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] iio:dac: fix i2c_master_send return code misuse
2015-11-04 1:10 ` Dmitry Torokhov
@ 2015-11-08 13:28 ` Jonathan Cameron
2015-11-08 14:10 ` Jean-Francois Dagenais
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2015-11-08 13:28 UTC (permalink / raw)
To: Dmitry Torokhov, Lars-Peter Clausen
Cc: Jean-Francois Dagenais, linux-iio, knaack.h, pmeerw,
Michael.Hennerich
On 04/11/15 01:10, Dmitry Torokhov wrote:
> On Tue, Nov 03, 2015 at 03:27:42PM +0100, Lars-Peter Clausen wrote:
>> Hi,
>>
>> On 10/27/2015 05:25 PM, Jean-Francois Dagenais wrote:
>>> Contrary to most kernel functions, successful call to
>>> i2c_master_send returns the number or written bytes,not 0.
>>>
>>> However, these callers return the value as is without converting
>>> the return style to conform to the expected standard.
>>>
>>> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
>>
>> Thanks for the patch.
>>
>>> ---
>>> drivers/iio/dac/ad5064.c | 5 ++++-
>>> drivers/iio/dac/ad5446.c | 4 +++-
>>> drivers/iio/dac/max517.c | 6 ++++--
>>> drivers/iio/dac/max5821.c | 8 ++++++--
>>> drivers/iio/dac/mcp4725.c | 10 ++++++++--
>>> 5 files changed, 25 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iio/dac/ad5064.c b/drivers/iio/dac/ad5064.c
>>> index c067e68..0ff1107 100644
>>> --- a/drivers/iio/dac/ad5064.c
>>> +++ b/drivers/iio/dac/ad5064.c
>>> @@ -598,10 +598,13 @@ static int ad5064_i2c_write(struct ad5064_state *st, unsigned int cmd,
>>> unsigned int addr, unsigned int val)
>>> {
>>> struct i2c_client *i2c = to_i2c_client(st->dev);
>>> + int res;
>>>
>>> st->data.i2c[0] = (cmd << 4) | addr;
>>> put_unaligned_be16(val, &st->data.i2c[1]);
>>> - return i2c_master_send(i2c, st->data.i2c, 3);
>>> + res = i2c_master_send(i2c, st->data.i2c, 3);
>>> +
>>> + return res == 3 ? 0 : res;
>>> }
>>
>> The ad5064 is already fixed in Jonathan's tree.
>>
>>>
>>> static int ad5064_i2c_probe(struct i2c_client *i2c,
>>> diff --git a/drivers/iio/dac/ad5446.c b/drivers/iio/dac/ad5446.c
>>> index 07e17d7..9a49941 100644
>>> --- a/drivers/iio/dac/ad5446.c
>>> +++ b/drivers/iio/dac/ad5446.c
>>> @@ -512,7 +512,9 @@ static int ad5622_write(struct ad5446_state *st, unsigned val)
>>> struct i2c_client *client = to_i2c_client(st->dev);
>>> __be16 data = cpu_to_be16(val);
>>>
>>> - return i2c_master_send(client, (char *)&data, sizeof(data));
>>> + int res = i2c_master_send(client, (char *)&data, sizeof(data));
>>> +
>>> + return res == sizeof(data)? 0 : res;
>>
>> This still returns the wrong if res is between 0 and sizeof(data)-1. You
>> need something like
>>
>> if (res == sizeof(data))
>> return 0
>> else if (res >= 0)
>> return -EIO;
>> else
>> return res;
>>
>> Same is true for all the other drivers patched.
>>
>> I really wish we could fix this in the I2C core to avoid having to copy and
>> paste the same error handling to each driver.
>>
>> There was this patch[1], but it looks like it was never followed by a v2.
>> Dmitry I'm curious, what happened to that patch?
>
> Fell through the cracks I'm afraid. I think facility like this is still
> needed as majority of the drivers do not care about short reads in the
> slightest.
>
Would be great to pick it up again if you have the time!
Jonathan
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] iio:dac: fix i2c_master_send return code misuse
2015-11-08 13:28 ` Jonathan Cameron
@ 2015-11-08 14:10 ` Jean-Francois Dagenais
2015-11-08 15:10 ` Jonathan Cameron
0 siblings, 1 reply; 9+ messages in thread
From: Jean-Francois Dagenais @ 2015-11-08 14:10 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Lars-Peter Clausen, linux-iio, knaack.h, pmeerw,
Michael.Hennerich
> On Nov 8, 2015, at 8:28 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>
>> Fell through the cracks I'm afraid. I think facility like this is still
>> needed as majority of the drivers do not care about short reads in the
>> slightest.
>>
> Would be great to pick it up again if you have the time!
>
Certainly, the i2c chips handled by ad5446.c do no function at all with the
current state of the driver!
I can resubmit a patch only for ad5446.c or wait for Dmitry's patch to be
applied, then make the proper change in ad5446.c.
For now, our own tree contains my original patch, so we're good... thank you
open source!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] iio:dac: fix i2c_master_send return code misuse
2015-11-08 14:10 ` Jean-Francois Dagenais
@ 2015-11-08 15:10 ` Jonathan Cameron
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2015-11-08 15:10 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Dmitry Torokhov, Lars-Peter Clausen, linux-iio, knaack.h, pmeerw,
Michael.Hennerich
On 08/11/15 14:10, Jean-Francois Dagenais wrote:
>
>> On Nov 8, 2015, at 8:28 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>
>>> Fell through the cracks I'm afraid. I think facility like this is still
>>> needed as majority of the drivers do not care about short reads in the
>>> slightest.
>>>
>> Would be great to pick it up again if you have the time!
>>
>
> Certainly, the i2c chips handled by ad5446.c do no function at all with the
> current state of the driver!
>
> I can resubmit a patch only for ad5446.c or wait for Dmitry's patch to be
> applied, then make the proper change in ad5446.c.
>
> For now, our own tree contains my original patch, so we're good... thank you
> open source!
Where things are actually broken we'll want the fix in stable kernel
trees so ideally we'll have the following:
1) Your current fix applied asap marked for stable.
2) Dmitry's patch resurfaces and allows us to follow up in a cycle or so with
cleanups that move every case over to that, hopefully avoiding any further repeats
of this problem!
If you can role a response to the point Lars raised and send an updated series
that would be great. Next set of fixes to head towards mainline will be next
weekend at the earliest anyway so we have a bit of time to tidy up!
Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread