[RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Daniel Baluta]

When using ACPI, if acpi_match_device fails then chipset enum will be
uninitialized and &ak_def_array[chipset] will point to some bad address.

This fixes the following compilation warning:

drivers/iio/magnetometer/ak8975.c: In function ‘ak8975_probe’:
drivers/iio/magnetometer/ak8975.c:788:14: warning: ‘chipset’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
  data->def = &ak_def_array[chipset];

Reported-by: Octavian Purdila <octavian.purdila@intel.com>
Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
This is a RFC because while I'm pretty sure that chipset should be initialized
with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can live with
a NULL return value of ak8975_match_acpi_device. Current implementation ignores
return value of ak8975_match_acpi_device.

The same situation is for kxcjk-1013, bmc150-accel, bmg160 and possible other
drivers.

 drivers/iio/magnetometer/ak8975.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 0d10a4b..cdf9e77 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -716,6 +716,7 @@ static const char *ak8975_match_acpi_device(struct device *dev,
 {
 	const struct acpi_device_id *id;
 
+	*chipset = AK_MAX_TYPE;
 	id = acpi_match_device(dev->driver->acpi_match_table, dev);
 	if (!id)
 		return NULL;
-- 
1.9.1

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Hartmut Knaack]

Daniel Baluta schrieb am 18.12.2014 um 18:16:
> When using ACPI, if acpi_match_device fails then chipset enum will be
> uninitialized and &ak_def_array[chipset] will point to some bad address.
> 
> This fixes the following compilation warning:
> 
> drivers/iio/magnetometer/ak8975.c: In function ‘ak8975_probe’:
> drivers/iio/magnetometer/ak8975.c:788:14: warning: ‘chipset’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>   data->def =ak_def_array[chipset];
> 
> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> ---
> This is a RFC because while I'm pretty sure that chipset should be initialized
> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can live with
> a NULL return value of ak8975_match_acpi_device. Current implementation ignores
> return value of ak8975_match_acpi_device.
This seems to be the actual problem: these _match_acpi_device functions return
NULL on failure, and this should be checked for.

> 
> The same situation is for kxcjk-1013, bmc150-accel, bmg160 and possible other
> drivers.
> 
>  drivers/iio/magnetometer/ak8975.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 0d10a4b..cdf9e77 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -716,6 +716,7 @@ static const char *ak8975_match_acpi_device(struct device *dev,
>  {
>  	const struct acpi_device_id *id;
>  
> +	*chipset =K_MAX_TYPE;
>  	id =cpi_match_device(dev->driver->acpi_match_table, dev);
>  	if (!id)
>  		return NULL;
> 

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Daniel Baluta]

On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Daniel Baluta schrieb am 18.12.2014 um 18:16:
>> When using ACPI, if acpi_match_device fails then chipset enum will be
>> uninitialized and &ak_def_array[chipset] will point to some bad address.
>>
>> This fixes the following compilation warning:
>>
>> drivers/iio/magnetometer/ak8975.c: In function =E2=80=98ak8975_probe=E2=
=80=99:
>> drivers/iio/magnetometer/ak8975.c:788:14: warning: =E2=80=98chipset=E2=
=80=99 may be used
>> uninitialized in this function [-Wmaybe-uninitialized]
>>   data->def =3Dak_def_array[chipset];
>>
>> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> ---
>> This is a RFC because while I'm pretty sure that chipset should be initi=
alized
>> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can li=
ve with
>> a NULL return value of ak8975_match_acpi_device. Current implementation =
ignores
>> return value of ak8975_match_acpi_device.
> This seems to be the actual problem: these _match_acpi_device functions r=
eturn
> NULL on failure, and this should be checked for.

Ok, so this would acceptable?

diff --git a/drivers/iio/magnetometer/ak8975.c
b/drivers/iio/magnetometer/ak8975.c
index 0d10a4b..68d99e9 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -776,8 +776,9 @@ static int ak8975_probe(struct i2c_client *client,
                name =3D id->name;
        } else if (ACPI_HANDLE(&client->dev))
                name =3D ak8975_match_acpi_device(&client->dev, &chipset);
-       else
-               return -ENOSYS;
+
+       if (!name)
+               return -ENODEV;


I still have some doubts about return code in case of error.

For ak8975 we use -ENOSYS, but for kxcjk-1013 we use -ENODEV.

I will send a patch after we clear this out.

thanks,
Daniel.

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Daniel Baluta]

On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Daniel Baluta schrieb am 18.12.2014 um 18:16:
>> When using ACPI, if acpi_match_device fails then chipset enum will be
>> uninitialized and &ak_def_array[chipset] will point to some bad address.
>>
>> This fixes the following compilation warning:
>>
>> drivers/iio/magnetometer/ak8975.c: In function ‘ak8975_probe’:
>> drivers/iio/magnetometer/ak8975.c:788:14: warning: ‘chipset’ may be used
>> uninitialized in this function [-Wmaybe-uninitialized]
>>   data->def =ak_def_array[chipset];
>>
>> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> ---
>> This is a RFC because while I'm pretty sure that chipset should be initialized
>> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can live with
>> a NULL return value of ak8975_match_acpi_device. Current implementation ignores
>> return value of ak8975_match_acpi_device.
> This seems to be the actual problem: these _match_acpi_device functions return
> NULL on failure, and this should be checked for.

Ok, so this would acceptable?

diff --git a/drivers/iio/magnetometer/ak8975.c
b/drivers/iio/magnetometer/ak8975.c
index 0d10a4b..68d99e9 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -776,8 +776,9 @@ static int ak8975_probe(struct i2c_client *client,
                name = id->name;
        } else if (ACPI_HANDLE(&client->dev))
                name = ak8975_match_acpi_device(&client->dev, &chipset);
-       else
-               return -ENOSYS;
+
+       if (!name)
+               return -ENODEV;


I still have some doubts about return code in case of error.

For ak8975 we use -ENOSYS, but for kxcjk-1013 we use -ENODEV.

I will send a patch after we clear this out.

thanks,
Daniel.

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Srinivas Pandruvada]

On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
> On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> > Daniel Baluta schrieb am 18.12.2014 um 18:16:
> >> When using ACPI, if acpi_match_device fails then chipset enum will be
> >> uninitialized and &ak_def_array[chipset] will point to some bad address.
> >>
I am missing something. You are enumerated over i2c device, which was
created from ACPI PNP resource. There is a valid handle or and the
device has an ACPI companion at the least. If this failing, I have to
check the code for acpi i2c.
Can you check why this check failed? We may have bug in i2c handling.

Thanks,
Srinivas

> >> This fixes the following compilation warning:
> >>
> >> drivers/iio/magnetometer/ak8975.c: In function ‘ak8975_probe’:
> >> drivers/iio/magnetometer/ak8975.c:788:14: warning: ‘chipset’ may be used
> >> uninitialized in this function [-Wmaybe-uninitialized]
> >>   data->def =ak_def_array[chipset];
> >>
> >> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
> >> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> >> ---
> >> This is a RFC because while I'm pretty sure that chipset should be initialized
> >> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can live with
> >> a NULL return value of ak8975_match_acpi_device. Current implementation ignores
> >> return value of ak8975_match_acpi_device.
> > This seems to be the actual problem: these _match_acpi_device functions return
> > NULL on failure, and this should be checked for.
> 
> Ok, so this would acceptable?
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c
> b/drivers/iio/magnetometer/ak8975.c
> index 0d10a4b..68d99e9 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -776,8 +776,9 @@ static int ak8975_probe(struct i2c_client *client,
>                 name = id->name;
>         } else if (ACPI_HANDLE(&client->dev))
>                 name = ak8975_match_acpi_device(&client->dev, &chipset);
> -       else
> -               return -ENOSYS;
> +
> +       if (!name)
> +               return -ENODEV;
> 
> 
> I still have some doubts about return code in case of error.
> 
> For ak8975 we use -ENOSYS, but for kxcjk-1013 we use -ENODEV.
> 
> I will send a patch after we clear this out.
> 
> thanks,
> Daniel.

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Pandruvada, Srinivas]

K01pa2ENCg0KT24gU2F0LCAyMDE0LTEyLTIwIGF0IDEzOjI2IC0wODAwLCBTcmluaXZhcyBQYW5k
cnV2YWRhIHdyb3RlOg0KPiBPbiBTYXQsIDIwMTQtMTItMjAgYXQgMDA6MjUgKzAyMDAsIERhbmll
bCBCYWx1dGEgd3JvdGU6DQo+ID4gT24gU2F0LCBEZWMgMjAsIDIwMTQgYXQgMTI6MTYgQU0sIEhh
cnRtdXQgS25hYWNrIDxrbmFhY2suaEBnbXguZGU+IHdyb3RlOg0KPiA+ID4gRGFuaWVsIEJhbHV0
YSBzY2hyaWViIGFtIDE4LjEyLjIwMTQgdW0gMTg6MTY6DQo+ID4gPj4gV2hlbiB1c2luZyBBQ1BJ
LCBpZiBhY3BpX21hdGNoX2RldmljZSBmYWlscyB0aGVuIGNoaXBzZXQgZW51bSB3aWxsIGJlDQo+
ID4gPj4gdW5pbml0aWFsaXplZCBhbmQgJmFrX2RlZl9hcnJheVtjaGlwc2V0XSB3aWxsIHBvaW50
IHRvIHNvbWUgYmFkIGFkZHJlc3MuDQo+ID4gPj4NCj4gSSBhbSBtaXNzaW5nIHNvbWV0aGluZy4g
WW91IGFyZSBlbnVtZXJhdGVkIG92ZXIgaTJjIGRldmljZSwgd2hpY2ggd2FzDQo+IGNyZWF0ZWQg
ZnJvbSBBQ1BJIFBOUCByZXNvdXJjZS4gVGhlcmUgaXMgYSB2YWxpZCBoYW5kbGUgb3IgYW5kIHRo
ZQ0KPiBkZXZpY2UgaGFzIGFuIEFDUEkgY29tcGFuaW9uIGF0IHRoZSBsZWFzdC4gSWYgdGhpcyBm
YWlsaW5nLCBJIGhhdmUgdG8NCj4gY2hlY2sgdGhlIGNvZGUgZm9yIGFjcGkgaTJjLg0KPiBDYW4g
eW91IGNoZWNrIHdoeSB0aGlzIGNoZWNrIGZhaWxlZD8gV2UgbWF5IGhhdmUgYnVnIGluIGkyYyBo
YW5kbGluZy4NCj4gDQo+IFRoYW5rcywNCj4gU3Jpbml2YXMNCj4gDQo+ID4gPj4gVGhpcyBmaXhl
cyB0aGUgZm9sbG93aW5nIGNvbXBpbGF0aW9uIHdhcm5pbmc6DQo+ID4gPj4NCj4gPiA+PiBkcml2
ZXJzL2lpby9tYWduZXRvbWV0ZXIvYWs4OTc1LmM6IEluIGZ1bmN0aW9uIOKAmGFrODk3NV9wcm9i
ZeKAmToNCj4gPiA+PiBkcml2ZXJzL2lpby9tYWduZXRvbWV0ZXIvYWs4OTc1LmM6Nzg4OjE0OiB3
YXJuaW5nOiDigJhjaGlwc2V04oCZIG1heSBiZSB1c2VkDQo+ID4gPj4gdW5pbml0aWFsaXplZCBp
biB0aGlzIGZ1bmN0aW9uIFstV21heWJlLXVuaW5pdGlhbGl6ZWRdDQo+ID4gPj4gICBkYXRhLT5k
ZWYgPWFrX2RlZl9hcnJheVtjaGlwc2V0XTsNCj4gPiA+Pg0KPiA+ID4+IFJlcG9ydGVkLWJ5OiBP
Y3RhdmlhbiBQdXJkaWxhIDxvY3Rhdmlhbi5wdXJkaWxhQGludGVsLmNvbT4NCj4gPiA+PiBTaWdu
ZWQtb2ZmLWJ5OiBEYW5pZWwgQmFsdXRhIDxkYW5pZWwuYmFsdXRhQGludGVsLmNvbT4NCj4gPiA+
PiAtLS0NCj4gPiA+PiBUaGlzIGlzIGEgUkZDIGJlY2F1c2Ugd2hpbGUgSSdtIHByZXR0eSBzdXJl
IHRoYXQgY2hpcHNldCBzaG91bGQgYmUgaW5pdGlhbGl6ZWQNCj4gPiA+PiB3aXRoIEFLX01BWF9U
WVBFIGluIGFrODk3NV9tYXRjaF9hY3BpX2RldmljZSwgSSBhbSBub3Qgc3VyZSBpZiB3ZSBjYW4g
bGl2ZSB3aXRoDQo+ID4gPj4gYSBOVUxMIHJldHVybiB2YWx1ZSBvZiBhazg5NzVfbWF0Y2hfYWNw
aV9kZXZpY2UuIEN1cnJlbnQgaW1wbGVtZW50YXRpb24gaWdub3Jlcw0KPiA+ID4+IHJldHVybiB2
YWx1ZSBvZiBhazg5NzVfbWF0Y2hfYWNwaV9kZXZpY2UuDQo+ID4gPiBUaGlzIHNlZW1zIHRvIGJl
IHRoZSBhY3R1YWwgcHJvYmxlbTogdGhlc2UgX21hdGNoX2FjcGlfZGV2aWNlIGZ1bmN0aW9ucyBy
ZXR1cm4NCj4gPiA+IE5VTEwgb24gZmFpbHVyZSwgYW5kIHRoaXMgc2hvdWxkIGJlIGNoZWNrZWQg
Zm9yLg0KPiA+IA0KPiA+IE9rLCBzbyB0aGlzIHdvdWxkIGFjY2VwdGFibGU/DQo+ID4gDQo+ID4g
ZGlmZiAtLWdpdCBhL2RyaXZlcnMvaWlvL21hZ25ldG9tZXRlci9hazg5NzUuYw0KPiA+IGIvZHJp
dmVycy9paW8vbWFnbmV0b21ldGVyL2FrODk3NS5jDQo+ID4gaW5kZXggMGQxMGE0Yi4uNjhkOTll
OSAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2lpby9tYWduZXRvbWV0ZXIvYWs4OTc1LmMNCj4g
PiArKysgYi9kcml2ZXJzL2lpby9tYWduZXRvbWV0ZXIvYWs4OTc1LmMNCj4gPiBAQCAtNzc2LDgg
Kzc3Niw5IEBAIHN0YXRpYyBpbnQgYWs4OTc1X3Byb2JlKHN0cnVjdCBpMmNfY2xpZW50ICpjbGll
bnQsDQo+ID4gICAgICAgICAgICAgICAgIG5hbWUgPSBpZC0+bmFtZTsNCj4gPiAgICAgICAgIH0g
ZWxzZSBpZiAoQUNQSV9IQU5ETEUoJmNsaWVudC0+ZGV2KSkNCj4gPiAgICAgICAgICAgICAgICAg
bmFtZSA9IGFrODk3NV9tYXRjaF9hY3BpX2RldmljZSgmY2xpZW50LT5kZXYsICZjaGlwc2V0KTsN
Cj4gPiAtICAgICAgIGVsc2UNCj4gPiAtICAgICAgICAgICAgICAgcmV0dXJuIC1FTk9TWVM7DQo+
ID4gKw0KPiA+ICsgICAgICAgaWYgKCFuYW1lKQ0KPiA+ICsgICAgICAgICAgICAgICByZXR1cm4g
LUVOT0RFVjsNCj4gPiANCj4gPiANCj4gPiBJIHN0aWxsIGhhdmUgc29tZSBkb3VidHMgYWJvdXQg
cmV0dXJuIGNvZGUgaW4gY2FzZSBvZiBlcnJvci4NCj4gPiANCj4gPiBGb3IgYWs4OTc1IHdlIHVz
ZSAtRU5PU1lTLCBidXQgZm9yIGt4Y2prLTEwMTMgd2UgdXNlIC1FTk9ERVYuDQo+ID4gDQo+ID4g
SSB3aWxsIHNlbmQgYSBwYXRjaCBhZnRlciB3ZSBjbGVhciB0aGlzIG91dC4NCj4gPiANCj4gPiB0
aGFua3MsDQo+ID4gRGFuaWVsLg0KPiANCg0K

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2689 bytes --]

+Mika

On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
> > On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> > > Daniel Baluta schrieb am 18.12.2014 um 18:16:
> > >> When using ACPI, if acpi_match_device fails then chipset enum will be
> > >> uninitialized and &ak_def_array[chipset] will point to some bad address.
> > >>
> I am missing something. You are enumerated over i2c device, which was
> created from ACPI PNP resource. There is a valid handle or and the
> device has an ACPI companion at the least. If this failing, I have to
> check the code for acpi i2c.
> Can you check why this check failed? We may have bug in i2c handling.
> 
> Thanks,
> Srinivas
> 
> > >> This fixes the following compilation warning:
> > >>
> > >> drivers/iio/magnetometer/ak8975.c: In function ‘ak8975_probe’:
> > >> drivers/iio/magnetometer/ak8975.c:788:14: warning: ‘chipset’ may be used
> > >> uninitialized in this function [-Wmaybe-uninitialized]
> > >>   data->def =ak_def_array[chipset];
> > >>
> > >> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
> > >> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> > >> ---
> > >> This is a RFC because while I'm pretty sure that chipset should be initialized
> > >> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can live with
> > >> a NULL return value of ak8975_match_acpi_device. Current implementation ignores
> > >> return value of ak8975_match_acpi_device.
> > > This seems to be the actual problem: these _match_acpi_device functions return
> > > NULL on failure, and this should be checked for.
> > 
> > Ok, so this would acceptable?
> > 
> > diff --git a/drivers/iio/magnetometer/ak8975.c
> > b/drivers/iio/magnetometer/ak8975.c
> > index 0d10a4b..68d99e9 100644
> > --- a/drivers/iio/magnetometer/ak8975.c
> > +++ b/drivers/iio/magnetometer/ak8975.c
> > @@ -776,8 +776,9 @@ static int ak8975_probe(struct i2c_client *client,
> >                 name = id->name;
> >         } else if (ACPI_HANDLE(&client->dev))
> >                 name = ak8975_match_acpi_device(&client->dev, &chipset);
> > -       else
> > -               return -ENOSYS;
> > +
> > +       if (!name)
> > +               return -ENODEV;
> > 
> > 
> > I still have some doubts about return code in case of error.
> > 
> > For ak8975 we use -ENOSYS, but for kxcjk-1013 we use -ENODEV.
> > 
> > I will send a patch after we clear this out.
> > 
> > thanks,
> > Daniel.
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Daniel Baluta]

I will have closer look on why acpi_match_device could fail. This patch
was only based on code reading when trying to fix the compiler warning
mentioned in the commit message.

[Sorry for top posting]

On Sat, Dec 20, 2014 at 11:29 PM, Pandruvada, Srinivas
<srinivas.pandruvada@intel.com> wrote:
> +Mika
>
> On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
>> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
>> > On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wro=
te:
>> > > Daniel Baluta schrieb am 18.12.2014 um 18:16:
>> > >> When using ACPI, if acpi_match_device fails then chipset enum will =
be
>> > >> uninitialized and &ak_def_array[chipset] will point to some bad add=
ress.
>> > >>
>> I am missing something. You are enumerated over i2c device, which was
>> created from ACPI PNP resource. There is a valid handle or and the
>> device has an ACPI companion at the least. If this failing, I have to
>> check the code for acpi i2c.
>> Can you check why this check failed? We may have bug in i2c handling.
>>
>> Thanks,
>> Srinivas
>>
>> > >> This fixes the following compilation warning:
>> > >>
>> > >> drivers/iio/magnetometer/ak8975.c: In function =E2=80=98ak8975_prob=
e=E2=80=99:
>> > >> drivers/iio/magnetometer/ak8975.c:788:14: warning: =E2=80=98chipset=
=E2=80=99 may be used
>> > >> uninitialized in this function [-Wmaybe-uninitialized]
>> > >>   data->def =3Dak_def_array[chipset];
>> > >>
>> > >> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
>> > >> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> > >> ---
>> > >> This is a RFC because while I'm pretty sure that chipset should be =
initialized
>> > >> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we c=
an live with
>> > >> a NULL return value of ak8975_match_acpi_device. Current implementa=
tion ignores
>> > >> return value of ak8975_match_acpi_device.
>> > > This seems to be the actual problem: these _match_acpi_device functi=
ons return
>> > > NULL on failure, and this should be checked for.
>> >
>> > Ok, so this would acceptable?
>> >
>> > diff --git a/drivers/iio/magnetometer/ak8975.c
>> > b/drivers/iio/magnetometer/ak8975.c
>> > index 0d10a4b..68d99e9 100644
>> > --- a/drivers/iio/magnetometer/ak8975.c
>> > +++ b/drivers/iio/magnetometer/ak8975.c
>> > @@ -776,8 +776,9 @@ static int ak8975_probe(struct i2c_client *client,
>> >                 name =3D id->name;
>> >         } else if (ACPI_HANDLE(&client->dev))
>> >                 name =3D ak8975_match_acpi_device(&client->dev, &chips=
et);
>> > -       else
>> > -               return -ENOSYS;
>> > +
>> > +       if (!name)
>> > +               return -ENODEV;
>> >
>> >
>> > I still have some doubts about return code in case of error.
>> >
>> > For ak8975 we use -ENOSYS, but for kxcjk-1013 we use -ENODEV.
>> >
>> > I will send a patch after we clear this out.
>> >
>> > thanks,
>> > Daniel.
>>
>

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Daniel Baluta]

I will have closer look on why acpi_match_device could fail. This patch
was only based on code reading when trying to fix the compiler warning
mentioned in the commit message.

[Sorry for top posting]

On Sat, Dec 20, 2014 at 11:29 PM, Pandruvada, Srinivas
<srinivas.pandruvada@intel.com> wrote:
> +Mika
>
> On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
>> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
>> > On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> > > Daniel Baluta schrieb am 18.12.2014 um 18:16:
>> > >> When using ACPI, if acpi_match_device fails then chipset enum will be
>> > >> uninitialized and &ak_def_array[chipset] will point to some bad address.
>> > >>
>> I am missing something. You are enumerated over i2c device, which was
>> created from ACPI PNP resource. There is a valid handle or and the
>> device has an ACPI companion at the least. If this failing, I have to
>> check the code for acpi i2c.
>> Can you check why this check failed? We may have bug in i2c handling.
>>
>> Thanks,
>> Srinivas
>>
>> > >> This fixes the following compilation warning:
>> > >>
>> > >> drivers/iio/magnetometer/ak8975.c: In function ‘ak8975_probe’:
>> > >> drivers/iio/magnetometer/ak8975.c:788:14: warning: ‘chipset’ may be used
>> > >> uninitialized in this function [-Wmaybe-uninitialized]
>> > >>   data->def =ak_def_array[chipset];
>> > >>
>> > >> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
>> > >> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> > >> ---
>> > >> This is a RFC because while I'm pretty sure that chipset should be initialized
>> > >> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can live with
>> > >> a NULL return value of ak8975_match_acpi_device. Current implementation ignores
>> > >> return value of ak8975_match_acpi_device.
>> > > This seems to be the actual problem: these _match_acpi_device functions return
>> > > NULL on failure, and this should be checked for.
>> >
>> > Ok, so this would acceptable?
>> >
>> > diff --git a/drivers/iio/magnetometer/ak8975.c
>> > b/drivers/iio/magnetometer/ak8975.c
>> > index 0d10a4b..68d99e9 100644
>> > --- a/drivers/iio/magnetometer/ak8975.c
>> > +++ b/drivers/iio/magnetometer/ak8975.c
>> > @@ -776,8 +776,9 @@ static int ak8975_probe(struct i2c_client *client,
>> >                 name = id->name;
>> >         } else if (ACPI_HANDLE(&client->dev))
>> >                 name = ak8975_match_acpi_device(&client->dev, &chipset);
>> > -       else
>> > -               return -ENOSYS;
>> > +
>> > +       if (!name)
>> > +               return -ENODEV;
>> >
>> >
>> > I still have some doubts about return code in case of error.
>> >
>> > For ak8975 we use -ENOSYS, but for kxcjk-1013 we use -ENODEV.
>> >
>> > I will send a patch after we clear this out.
>> >
>> > thanks,
>> > Daniel.
>>
>

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Daniel Baluta]

Hello,

On Sat, Dec 20, 2014 at 11:29 PM, Pandruvada, Srinivas
<srinivas.pandruvada@intel.com> wrote:
> +Mika
>
> On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
>> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
>> > On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> > > Daniel Baluta schrieb am 18.12.2014 um 18:16:
>> > >> When using ACPI, if acpi_match_device fails then chipset enum will be
>> > >> uninitialized and &ak_def_array[chipset] will point to some bad address.
>> > >>
>> I am missing something. You are enumerated over i2c device, which was
>> created from ACPI PNP resource. There is a valid handle or and the
>> device has an ACPI companion at the least. If this failing, I have to
>> check the code for acpi i2c.
>> Can you check why this check failed? We may have bug in i2c handling.

You are right about this. Under normal circumstances, if probe is called
then acpi_match_device will not fail. I even tried to remove the
device after probe
but before acpi_match_device, anyhow acpi_match_device was still successful :)

This is more a matter of code correctness.

In ak8975_match_acpi_device we have:

»       const struct acpi_device_id *id;

»       id = acpi_match_device(dev->driver->acpi_match_table, dev);
»       if (!id)
»       »       return NULL;
»       *chipset = (int)id->driver_data;

Compiler complains on the fact that chipset might be uninitialized
if this returns NULL, and we shouldn't ignore this warning even this case
will never happen.

We could use some code injection techniques to force acpi_match_device
to return NULL tough.

>> > >> This fixes the following compilation warning:
>> > >>
>> > >> drivers/iio/magnetometer/ak8975.c: In function ‘ak8975_probe’:
>> > >> drivers/iio/magnetometer/ak8975.c:788:14: warning: ‘chipset’ may be used
>> > >> uninitialized in this function [-Wmaybe-uninitialized]
>> > >>   data->def =ak_def_array[chipset];
>> > >>
>> > >> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
>> > >> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> > >> ---
>> > >> This is a RFC because while I'm pretty sure that chipset should be initialized
>> > >> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can live with
>> > >> a NULL return value of ak8975_match_acpi_device. Current implementation ignores
>> > >> return value of ak8975_match_acpi_device.
>> > > This seems to be the actual problem: these _match_acpi_device functions return
>> > > NULL on failure, and this should be checked for.
>> >
>> > Ok, so this would acceptable?
>> >
>> > diff --git a/drivers/iio/magnetometer/ak8975.c
>> > b/drivers/iio/magnetometer/ak8975.c
>> > index 0d10a4b..68d99e9 100644
>> > --- a/drivers/iio/magnetometer/ak8975.c
>> > +++ b/drivers/iio/magnetometer/ak8975.c
>> > @@ -776,8 +776,9 @@ static int ak8975_probe(struct i2c_client *client,
>> >                 name = id->name;
>> >         } else if (ACPI_HANDLE(&client->dev))
>> >                 name = ak8975_match_acpi_device(&client->dev, &chipset);
>> > -       else
>> > -               return -ENOSYS;
>> > +
>> > +       if (!name)
>> > +               return -ENODEV;
>> >
>> >
>> > I still have some doubts about return code in case of error.
>> >
>> > For ak8975 we use -ENOSYS, but for kxcjk-1013 we use -ENODEV.
>> >
>> > I will send a patch after we clear this out.
>> >
>> > thanks,
>> > Daniel.
>>
>

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Pandruvada, Srinivas]

T24gTW9uLCAyMDE1LTAxLTE5IGF0IDE2OjQwICswMjAwLCBEYW5pZWwgQmFsdXRhIHdyb3RlOiAN
Cj4gSGVsbG8sDQo+IA0KPiBPbiBTYXQsIERlYyAyMCwgMjAxNCBhdCAxMToyOSBQTSwgUGFuZHJ1
dmFkYSwgU3Jpbml2YXMNCj4gPHNyaW5pdmFzLnBhbmRydXZhZGFAaW50ZWwuY29tPiB3cm90ZToN
Cj4gPiArTWlrYQ0KPiA+DQo+ID4gT24gU2F0LCAyMDE0LTEyLTIwIGF0IDEzOjI2IC0wODAwLCBT
cmluaXZhcyBQYW5kcnV2YWRhIHdyb3RlOg0KPiA+PiBPbiBTYXQsIDIwMTQtMTItMjAgYXQgMDA6
MjUgKzAyMDAsIERhbmllbCBCYWx1dGEgd3JvdGU6DQo+ID4+ID4gT24gU2F0LCBEZWMgMjAsIDIw
MTQgYXQgMTI6MTYgQU0sIEhhcnRtdXQgS25hYWNrIDxrbmFhY2suaEBnbXguZGU+IHdyb3RlOg0K
PiA+PiA+ID4gRGFuaWVsIEJhbHV0YSBzY2hyaWViIGFtIDE4LjEyLjIwMTQgdW0gMTg6MTY6DQo+
ID4+ID4gPj4gV2hlbiB1c2luZyBBQ1BJLCBpZiBhY3BpX21hdGNoX2RldmljZSBmYWlscyB0aGVu
IGNoaXBzZXQgZW51bSB3aWxsIGJlDQo+ID4+ID4gPj4gdW5pbml0aWFsaXplZCBhbmQgJmFrX2Rl
Zl9hcnJheVtjaGlwc2V0XSB3aWxsIHBvaW50IHRvIHNvbWUgYmFkIGFkZHJlc3MuDQo+ID4+ID4g
Pj4NCj4gPj4gSSBhbSBtaXNzaW5nIHNvbWV0aGluZy4gWW91IGFyZSBlbnVtZXJhdGVkIG92ZXIg
aTJjIGRldmljZSwgd2hpY2ggd2FzDQo+ID4+IGNyZWF0ZWQgZnJvbSBBQ1BJIFBOUCByZXNvdXJj
ZS4gVGhlcmUgaXMgYSB2YWxpZCBoYW5kbGUgb3IgYW5kIHRoZQ0KPiA+PiBkZXZpY2UgaGFzIGFu
IEFDUEkgY29tcGFuaW9uIGF0IHRoZSBsZWFzdC4gSWYgdGhpcyBmYWlsaW5nLCBJIGhhdmUgdG8N
Cj4gPj4gY2hlY2sgdGhlIGNvZGUgZm9yIGFjcGkgaTJjLg0KPiA+PiBDYW4geW91IGNoZWNrIHdo
eSB0aGlzIGNoZWNrIGZhaWxlZD8gV2UgbWF5IGhhdmUgYnVnIGluIGkyYyBoYW5kbGluZy4NCj4g
DQo+IFlvdSBhcmUgcmlnaHQgYWJvdXQgdGhpcy4gVW5kZXIgbm9ybWFsIGNpcmN1bXN0YW5jZXMs
IGlmIHByb2JlIGlzIGNhbGxlZA0KPiB0aGVuIGFjcGlfbWF0Y2hfZGV2aWNlIHdpbGwgbm90IGZh
aWwuIEkgZXZlbiB0cmllZCB0byByZW1vdmUgdGhlDQo+IGRldmljZSBhZnRlciBwcm9iZQ0KPiBi
dXQgYmVmb3JlIGFjcGlfbWF0Y2hfZGV2aWNlLCBhbnlob3cgYWNwaV9tYXRjaF9kZXZpY2Ugd2Fz
IHN0aWxsIHN1Y2Nlc3NmdWwgOikNCj4gDQo+IFRoaXMgaXMgbW9yZSBhIG1hdHRlciBvZiBjb2Rl
IGNvcnJlY3RuZXNzLg0KPiANCj4gSW4gYWs4OTc1X21hdGNoX2FjcGlfZGV2aWNlIHdlIGhhdmU6
DQo+IA0KPiDCuyAgICAgICBjb25zdCBzdHJ1Y3QgYWNwaV9kZXZpY2VfaWQgKmlkOw0KPiANCj4g
wrsgICAgICAgaWQgPSBhY3BpX21hdGNoX2RldmljZShkZXYtPmRyaXZlci0+YWNwaV9tYXRjaF90
YWJsZSwgZGV2KTsNCj4gwrsgICAgICAgaWYgKCFpZCkNCj4gwrsgICAgICAgwrsgICAgICAgcmV0
dXJuIE5VTEw7DQo+IMK7ICAgICAgICpjaGlwc2V0ID0gKGludClpZC0+ZHJpdmVyX2RhdGE7DQo+
IA0KPiBDb21waWxlciBjb21wbGFpbnMgb24gdGhlIGZhY3QgdGhhdCBjaGlwc2V0IG1pZ2h0IGJl
IHVuaW5pdGlhbGl6ZWQNCj4gaWYgdGhpcyByZXR1cm5zIE5VTEwsIGFuZCB3ZSBzaG91bGRuJ3Qg
aWdub3JlIHRoaXMgd2FybmluZyBldmVuIHRoaXMgY2FzZQ0KPiB3aWxsIG5ldmVyIGhhcHBlbi4N
Cj4gDQpXaWxsIHRoaXMgZml4Pw0KZGF0YS0+Y2hpcHNldCA9IEFLODk3NTsNCmJlZm9yZQ0KYWs4
OTc1X21hdGNoX2FjcGlfZGV2aWNlKCZjbGllbnQtPmRldiwgJmRhdGEtPmNoaXBzZXQpOw0KDQpU
aGFua3MsDQpTcmluaXZhcyANCj4gV2UgY291bGQgdXNlIHNvbWUgY29kZSBpbmplY3Rpb24gdGVj
aG5pcXVlcyB0byBmb3JjZSBhY3BpX21hdGNoX2RldmljZQ0KPiB0byByZXR1cm4gTlVMTCB0b3Vn
aC4NCj4gDQo+ID4+ID4gPj4gVGhpcyBmaXhlcyB0aGUgZm9sbG93aW5nIGNvbXBpbGF0aW9uIHdh
cm5pbmc6DQo+ID4+ID4gPj4NCj4gPj4gPiA+PiBkcml2ZXJzL2lpby9tYWduZXRvbWV0ZXIvYWs4
OTc1LmM6IEluIGZ1bmN0aW9uIOKAmGFrODk3NV9wcm9iZeKAmToNCj4gPj4gPiA+PiBkcml2ZXJz
L2lpby9tYWduZXRvbWV0ZXIvYWs4OTc1LmM6Nzg4OjE0OiB3YXJuaW5nOiDigJhjaGlwc2V04oCZ
IG1heSBiZSB1c2VkDQo+ID4+ID4gPj4gdW5pbml0aWFsaXplZCBpbiB0aGlzIGZ1bmN0aW9uIFst
V21heWJlLXVuaW5pdGlhbGl6ZWRdDQo+ID4+ID4gPj4gICBkYXRhLT5kZWYgPWFrX2RlZl9hcnJh
eVtjaGlwc2V0XTsNCj4gPj4gPiA+Pg0KPiA+PiA+ID4+IFJlcG9ydGVkLWJ5OiBPY3RhdmlhbiBQ
dXJkaWxhIDxvY3Rhdmlhbi5wdXJkaWxhQGludGVsLmNvbT4NCj4gPj4gPiA+PiBTaWduZWQtb2Zm
LWJ5OiBEYW5pZWwgQmFsdXRhIDxkYW5pZWwuYmFsdXRhQGludGVsLmNvbT4NCj4gPj4gPiA+PiAt
LS0NCj4gPj4gPiA+PiBUaGlzIGlzIGEgUkZDIGJlY2F1c2Ugd2hpbGUgSSdtIHByZXR0eSBzdXJl
IHRoYXQgY2hpcHNldCBzaG91bGQgYmUgaW5pdGlhbGl6ZWQNCj4gPj4gPiA+PiB3aXRoIEFLX01B
WF9UWVBFIGluIGFrODk3NV9tYXRjaF9hY3BpX2RldmljZSwgSSBhbSBub3Qgc3VyZSBpZiB3ZSBj
YW4gbGl2ZSB3aXRoDQo+ID4+ID4gPj4gYSBOVUxMIHJldHVybiB2YWx1ZSBvZiBhazg5NzVfbWF0
Y2hfYWNwaV9kZXZpY2UuIEN1cnJlbnQgaW1wbGVtZW50YXRpb24gaWdub3Jlcw0KPiA+PiA+ID4+
IHJldHVybiB2YWx1ZSBvZiBhazg5NzVfbWF0Y2hfYWNwaV9kZXZpY2UuDQo+ID4+ID4gPiBUaGlz
IHNlZW1zIHRvIGJlIHRoZSBhY3R1YWwgcHJvYmxlbTogdGhlc2UgX21hdGNoX2FjcGlfZGV2aWNl
IGZ1bmN0aW9ucyByZXR1cm4NCj4gPj4gPiA+IE5VTEwgb24gZmFpbHVyZSwgYW5kIHRoaXMgc2hv
dWxkIGJlIGNoZWNrZWQgZm9yLg0KPiA+PiA+DQo+ID4+ID4gT2ssIHNvIHRoaXMgd291bGQgYWNj
ZXB0YWJsZT8NCj4gPj4gPg0KPiA+PiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2lpby9tYWduZXRv
bWV0ZXIvYWs4OTc1LmMNCj4gPj4gPiBiL2RyaXZlcnMvaWlvL21hZ25ldG9tZXRlci9hazg5NzUu
Yw0KPiA+PiA+IGluZGV4IDBkMTBhNGIuLjY4ZDk5ZTkgMTAwNjQ0DQo+ID4+ID4gLS0tIGEvZHJp
dmVycy9paW8vbWFnbmV0b21ldGVyL2FrODk3NS5jDQo+ID4+ID4gKysrIGIvZHJpdmVycy9paW8v
bWFnbmV0b21ldGVyL2FrODk3NS5jDQo+ID4+ID4gQEAgLTc3Niw4ICs3NzYsOSBAQCBzdGF0aWMg
aW50IGFrODk3NV9wcm9iZShzdHJ1Y3QgaTJjX2NsaWVudCAqY2xpZW50LA0KPiA+PiA+ICAgICAg
ICAgICAgICAgICBuYW1lID0gaWQtPm5hbWU7DQo+ID4+ID4gICAgICAgICB9IGVsc2UgaWYgKEFD
UElfSEFORExFKCZjbGllbnQtPmRldikpDQo+ID4+ID4gICAgICAgICAgICAgICAgIG5hbWUgPSBh
azg5NzVfbWF0Y2hfYWNwaV9kZXZpY2UoJmNsaWVudC0+ZGV2LCAmY2hpcHNldCk7DQo+ID4+ID4g
LSAgICAgICBlbHNlDQo+ID4+ID4gLSAgICAgICAgICAgICAgIHJldHVybiAtRU5PU1lTOw0KPiA+
PiA+ICsNCj4gPj4gPiArICAgICAgIGlmICghbmFtZSkNCj4gPj4gPiArICAgICAgICAgICAgICAg
cmV0dXJuIC1FTk9ERVY7DQo+ID4+ID4NCj4gPj4gPg0KPiA+PiA+IEkgc3RpbGwgaGF2ZSBzb21l
IGRvdWJ0cyBhYm91dCByZXR1cm4gY29kZSBpbiBjYXNlIG9mIGVycm9yLg0KPiA+PiA+DQo+ID4+
ID4gRm9yIGFrODk3NSB3ZSB1c2UgLUVOT1NZUywgYnV0IGZvciBreGNqay0xMDEzIHdlIHVzZSAt
RU5PREVWLg0KPiA+PiA+DQo+ID4+ID4gSSB3aWxsIHNlbmQgYSBwYXRjaCBhZnRlciB3ZSBjbGVh
ciB0aGlzIG91dC4NCj4gPj4gPg0KPiA+PiA+IHRoYW5rcywNCj4gPj4gPiBEYW5pZWwuDQo+ID4+
DQo+ID4NCg0K

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3949 bytes --]

On Mon, 2015-01-19 at 16:40 +0200, Daniel Baluta wrote: 
> Hello,
> 
> On Sat, Dec 20, 2014 at 11:29 PM, Pandruvada, Srinivas
> <srinivas.pandruvada@intel.com> wrote:
> > +Mika
> >
> > On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
> >> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
> >> > On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> >> > > Daniel Baluta schrieb am 18.12.2014 um 18:16:
> >> > >> When using ACPI, if acpi_match_device fails then chipset enum will be
> >> > >> uninitialized and &ak_def_array[chipset] will point to some bad address.
> >> > >>
> >> I am missing something. You are enumerated over i2c device, which was
> >> created from ACPI PNP resource. There is a valid handle or and the
> >> device has an ACPI companion at the least. If this failing, I have to
> >> check the code for acpi i2c.
> >> Can you check why this check failed? We may have bug in i2c handling.
> 
> You are right about this. Under normal circumstances, if probe is called
> then acpi_match_device will not fail. I even tried to remove the
> device after probe
> but before acpi_match_device, anyhow acpi_match_device was still successful :)
> 
> This is more a matter of code correctness.
> 
> In ak8975_match_acpi_device we have:
> 
> »       const struct acpi_device_id *id;
> 
> »       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> »       if (!id)
> »       »       return NULL;
> »       *chipset = (int)id->driver_data;
> 
> Compiler complains on the fact that chipset might be uninitialized
> if this returns NULL, and we shouldn't ignore this warning even this case
> will never happen.
> 
Will this fix?
data->chipset = AK8975;
before
ak8975_match_acpi_device(&client->dev, &data->chipset);

Thanks,
Srinivas 
> We could use some code injection techniques to force acpi_match_device
> to return NULL tough.
> 
> >> > >> This fixes the following compilation warning:
> >> > >>
> >> > >> drivers/iio/magnetometer/ak8975.c: In function ‘ak8975_probe’:
> >> > >> drivers/iio/magnetometer/ak8975.c:788:14: warning: ‘chipset’ may be used
> >> > >> uninitialized in this function [-Wmaybe-uninitialized]
> >> > >>   data->def =ak_def_array[chipset];
> >> > >>
> >> > >> Reported-by: Octavian Purdila <octavian.purdila@intel.com>
> >> > >> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> >> > >> ---
> >> > >> This is a RFC because while I'm pretty sure that chipset should be initialized
> >> > >> with AK_MAX_TYPE in ak8975_match_acpi_device, I am not sure if we can live with
> >> > >> a NULL return value of ak8975_match_acpi_device. Current implementation ignores
> >> > >> return value of ak8975_match_acpi_device.
> >> > > This seems to be the actual problem: these _match_acpi_device functions return
> >> > > NULL on failure, and this should be checked for.
> >> >
> >> > Ok, so this would acceptable?
> >> >
> >> > diff --git a/drivers/iio/magnetometer/ak8975.c
> >> > b/drivers/iio/magnetometer/ak8975.c
> >> > index 0d10a4b..68d99e9 100644
> >> > --- a/drivers/iio/magnetometer/ak8975.c
> >> > +++ b/drivers/iio/magnetometer/ak8975.c
> >> > @@ -776,8 +776,9 @@ static int ak8975_probe(struct i2c_client *client,
> >> >                 name = id->name;
> >> >         } else if (ACPI_HANDLE(&client->dev))
> >> >                 name = ak8975_match_acpi_device(&client->dev, &chipset);
> >> > -       else
> >> > -               return -ENOSYS;
> >> > +
> >> > +       if (!name)
> >> > +               return -ENODEV;
> >> >
> >> >
> >> > I still have some doubts about return code in case of error.
> >> >
> >> > For ak8975 we use -ENOSYS, but for kxcjk-1013 we use -ENODEV.
> >> >
> >> > I will send a patch after we clear this out.
> >> >
> >> > thanks,
> >> > Daniel.
> >>
> >

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Daniel Baluta]

On Mon, Jan 19, 2015 at 6:44 PM, Pandruvada, Srinivas
<srinivas.pandruvada@intel.com> wrote:
> On Mon, 2015-01-19 at 16:40 +0200, Daniel Baluta wrote:
>> Hello,
>>
>> On Sat, Dec 20, 2014 at 11:29 PM, Pandruvada, Srinivas
>> <srinivas.pandruvada@intel.com> wrote:
>> > +Mika
>> >
>> > On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
>> >> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
>> >> > On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> >> > > Daniel Baluta schrieb am 18.12.2014 um 18:16:
>> >> > >> When using ACPI, if acpi_match_device fails then chipset enum will be
>> >> > >> uninitialized and &ak_def_array[chipset] will point to some bad address.
>> >> > >>
>> >> I am missing something. You are enumerated over i2c device, which was
>> >> created from ACPI PNP resource. There is a valid handle or and the
>> >> device has an ACPI companion at the least. If this failing, I have to
>> >> check the code for acpi i2c.
>> >> Can you check why this check failed? We may have bug in i2c handling.
>>
>> You are right about this. Under normal circumstances, if probe is called
>> then acpi_match_device will not fail. I even tried to remove the
>> device after probe
>> but before acpi_match_device, anyhow acpi_match_device was still successful :)
>>
>> This is more a matter of code correctness.
>>
>> In ak8975_match_acpi_device we have:
>>
>> »       const struct acpi_device_id *id;
>>
>> »       id = acpi_match_device(dev->driver->acpi_match_table, dev);
>> »       if (!id)
>> »       »       return NULL;
>> »       *chipset = (int)id->driver_data;
>>
>> Compiler complains on the fact that chipset might be uninitialized
>> if this returns NULL, and we shouldn't ignore this warning even this case
>> will never happen.
>>
> Will this fix?
> data->chipset = AK8975;
> before
> ak8975_match_acpi_device(&client->dev, &data->chipset);
>

Yes, this is done in the original patch:

+       *chipset = AK_MAX_TYPE;

.. and fixes the warning.

Daniel.

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Pandruvada, Srinivas]

T24gTW9uLCAyMDE1LTAxLTE5IGF0IDE4OjQ5ICswMjAwLCBEYW5pZWwgQmFsdXRhIHdyb3RlOiAN
Cj4gT24gTW9uLCBKYW4gMTksIDIwMTUgYXQgNjo0NCBQTSwgUGFuZHJ1dmFkYSwgU3Jpbml2YXMN
Cj4gPHNyaW5pdmFzLnBhbmRydXZhZGFAaW50ZWwuY29tPiB3cm90ZToNCj4gPiBPbiBNb24sIDIw
MTUtMDEtMTkgYXQgMTY6NDAgKzAyMDAsIERhbmllbCBCYWx1dGEgd3JvdGU6DQo+ID4+IEhlbGxv
LA0KPiA+Pg0KPiA+PiBPbiBTYXQsIERlYyAyMCwgMjAxNCBhdCAxMToyOSBQTSwgUGFuZHJ1dmFk
YSwgU3Jpbml2YXMNCj4gPj4gPHNyaW5pdmFzLnBhbmRydXZhZGFAaW50ZWwuY29tPiB3cm90ZToN
Cj4gPj4gPiArTWlrYQ0KPiA+PiA+DQo+ID4+ID4gT24gU2F0LCAyMDE0LTEyLTIwIGF0IDEzOjI2
IC0wODAwLCBTcmluaXZhcyBQYW5kcnV2YWRhIHdyb3RlOg0KPiA+PiA+PiBPbiBTYXQsIDIwMTQt
MTItMjAgYXQgMDA6MjUgKzAyMDAsIERhbmllbCBCYWx1dGEgd3JvdGU6DQo+ID4+ID4+ID4gT24g
U2F0LCBEZWMgMjAsIDIwMTQgYXQgMTI6MTYgQU0sIEhhcnRtdXQgS25hYWNrIDxrbmFhY2suaEBn
bXguZGU+IHdyb3RlOg0KPiA+PiA+PiA+ID4gRGFuaWVsIEJhbHV0YSBzY2hyaWViIGFtIDE4LjEy
LjIwMTQgdW0gMTg6MTY6DQo+ID4+ID4+ID4gPj4gV2hlbiB1c2luZyBBQ1BJLCBpZiBhY3BpX21h
dGNoX2RldmljZSBmYWlscyB0aGVuIGNoaXBzZXQgZW51bSB3aWxsIGJlDQo+ID4+ID4+ID4gPj4g
dW5pbml0aWFsaXplZCBhbmQgJmFrX2RlZl9hcnJheVtjaGlwc2V0XSB3aWxsIHBvaW50IHRvIHNv
bWUgYmFkIGFkZHJlc3MuDQo+ID4+ID4+ID4gPj4NCj4gPj4gPj4gSSBhbSBtaXNzaW5nIHNvbWV0
aGluZy4gWW91IGFyZSBlbnVtZXJhdGVkIG92ZXIgaTJjIGRldmljZSwgd2hpY2ggd2FzDQo+ID4+
ID4+IGNyZWF0ZWQgZnJvbSBBQ1BJIFBOUCByZXNvdXJjZS4gVGhlcmUgaXMgYSB2YWxpZCBoYW5k
bGUgb3IgYW5kIHRoZQ0KPiA+PiA+PiBkZXZpY2UgaGFzIGFuIEFDUEkgY29tcGFuaW9uIGF0IHRo
ZSBsZWFzdC4gSWYgdGhpcyBmYWlsaW5nLCBJIGhhdmUgdG8NCj4gPj4gPj4gY2hlY2sgdGhlIGNv
ZGUgZm9yIGFjcGkgaTJjLg0KPiA+PiA+PiBDYW4geW91IGNoZWNrIHdoeSB0aGlzIGNoZWNrIGZh
aWxlZD8gV2UgbWF5IGhhdmUgYnVnIGluIGkyYyBoYW5kbGluZy4NCj4gPj4NCj4gPj4gWW91IGFy
ZSByaWdodCBhYm91dCB0aGlzLiBVbmRlciBub3JtYWwgY2lyY3Vtc3RhbmNlcywgaWYgcHJvYmUg
aXMgY2FsbGVkDQo+ID4+IHRoZW4gYWNwaV9tYXRjaF9kZXZpY2Ugd2lsbCBub3QgZmFpbC4gSSBl
dmVuIHRyaWVkIHRvIHJlbW92ZSB0aGUNCj4gPj4gZGV2aWNlIGFmdGVyIHByb2JlDQo+ID4+IGJ1
dCBiZWZvcmUgYWNwaV9tYXRjaF9kZXZpY2UsIGFueWhvdyBhY3BpX21hdGNoX2RldmljZSB3YXMg
c3RpbGwgc3VjY2Vzc2Z1bCA6KQ0KPiA+Pg0KPiA+PiBUaGlzIGlzIG1vcmUgYSBtYXR0ZXIgb2Yg
Y29kZSBjb3JyZWN0bmVzcy4NCj4gPj4NCj4gPj4gSW4gYWs4OTc1X21hdGNoX2FjcGlfZGV2aWNl
IHdlIGhhdmU6DQo+ID4+DQo+ID4+IMK7ICAgICAgIGNvbnN0IHN0cnVjdCBhY3BpX2RldmljZV9p
ZCAqaWQ7DQo+ID4+DQo+ID4+IMK7ICAgICAgIGlkID0gYWNwaV9tYXRjaF9kZXZpY2UoZGV2LT5k
cml2ZXItPmFjcGlfbWF0Y2hfdGFibGUsIGRldik7DQo+ID4+IMK7ICAgICAgIGlmICghaWQpDQo+
ID4+IMK7ICAgICAgIMK7ICAgICAgIHJldHVybiBOVUxMOw0KPiA+PiDCuyAgICAgICAqY2hpcHNl
dCA9IChpbnQpaWQtPmRyaXZlcl9kYXRhOw0KPiA+Pg0KPiA+PiBDb21waWxlciBjb21wbGFpbnMg
b24gdGhlIGZhY3QgdGhhdCBjaGlwc2V0IG1pZ2h0IGJlIHVuaW5pdGlhbGl6ZWQNCj4gPj4gaWYg
dGhpcyByZXR1cm5zIE5VTEwsIGFuZCB3ZSBzaG91bGRuJ3QgaWdub3JlIHRoaXMgd2FybmluZyBl
dmVuIHRoaXMgY2FzZQ0KPiA+PiB3aWxsIG5ldmVyIGhhcHBlbi4NCj4gPj4NCj4gPiBXaWxsIHRo
aXMgZml4Pw0KPiA+IGRhdGEtPmNoaXBzZXQgPSBBSzg5NzU7DQo+ID4gYmVmb3JlDQo+ID4gYWs4
OTc1X21hdGNoX2FjcGlfZGV2aWNlKCZjbGllbnQtPmRldiwgJmRhdGEtPmNoaXBzZXQpOw0KPiA+
DQo+IA0KPiBZZXMsIHRoaXMgaXMgZG9uZSBpbiB0aGUgb3JpZ2luYWwgcGF0Y2g6DQo+IA0KPiAr
ICAgICAgICpjaGlwc2V0ID0gQUtfTUFYX1RZUEU7DQpTaW5jZSBkYXRhIG1lbW9yeSBpcyBub3Qg
emVybyBhbGxvY2VkLCBvdGhlciBtZW1iZXIgb2YgZGF0YSBhcmUgYW55d2F5DQppbml0aWFsaXpl
ZCwgc28gYWRkaW5nIHRoaXMgYWxzbyBtYXkgYmUgYmV0dGVyLiANCj4gDQo+IC4uIGFuZCBmaXhl
cyB0aGUgd2FybmluZy4NCj4gDQo+IERhbmllbC4NCg0K

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2471 bytes --]

On Mon, 2015-01-19 at 18:49 +0200, Daniel Baluta wrote: 
> On Mon, Jan 19, 2015 at 6:44 PM, Pandruvada, Srinivas
> <srinivas.pandruvada@intel.com> wrote:
> > On Mon, 2015-01-19 at 16:40 +0200, Daniel Baluta wrote:
> >> Hello,
> >>
> >> On Sat, Dec 20, 2014 at 11:29 PM, Pandruvada, Srinivas
> >> <srinivas.pandruvada@intel.com> wrote:
> >> > +Mika
> >> >
> >> > On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
> >> >> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
> >> >> > On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> >> >> > > Daniel Baluta schrieb am 18.12.2014 um 18:16:
> >> >> > >> When using ACPI, if acpi_match_device fails then chipset enum will be
> >> >> > >> uninitialized and &ak_def_array[chipset] will point to some bad address.
> >> >> > >>
> >> >> I am missing something. You are enumerated over i2c device, which was
> >> >> created from ACPI PNP resource. There is a valid handle or and the
> >> >> device has an ACPI companion at the least. If this failing, I have to
> >> >> check the code for acpi i2c.
> >> >> Can you check why this check failed? We may have bug in i2c handling.
> >>
> >> You are right about this. Under normal circumstances, if probe is called
> >> then acpi_match_device will not fail. I even tried to remove the
> >> device after probe
> >> but before acpi_match_device, anyhow acpi_match_device was still successful :)
> >>
> >> This is more a matter of code correctness.
> >>
> >> In ak8975_match_acpi_device we have:
> >>
> >> »       const struct acpi_device_id *id;
> >>
> >> »       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> >> »       if (!id)
> >> »       »       return NULL;
> >> »       *chipset = (int)id->driver_data;
> >>
> >> Compiler complains on the fact that chipset might be uninitialized
> >> if this returns NULL, and we shouldn't ignore this warning even this case
> >> will never happen.
> >>
> > Will this fix?
> > data->chipset = AK8975;
> > before
> > ak8975_match_acpi_device(&client->dev, &data->chipset);
> >
> 
> Yes, this is done in the original patch:
> 
> +       *chipset = AK_MAX_TYPE;
Since data memory is not zero alloced, other member of data are anyway
initialized, so adding this also may be better. 
> 
> .. and fixes the warning.
> 
> Daniel.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Hartmut Knaack]

Pandruvada, Srinivas schrieb am 19.01.2015 um 17:56:
> On Mon, 2015-01-19 at 18:49 +0200, Daniel Baluta wrote: 
>> On Mon, Jan 19, 2015 at 6:44 PM, Pandruvada, Srinivas
>> <srinivas.pandruvada@intel.com> wrote:
>>> On Mon, 2015-01-19 at 16:40 +0200, Daniel Baluta wrote:
>>>> Hello,
>>>>
>>>> On Sat, Dec 20, 2014 at 11:29 PM, Pandruvada, Srinivas
>>>> <srinivas.pandruvada@intel.com> wrote:
>>>>> +Mika
>>>>>
>>>>> On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
>>>>>> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
>>>>>>> On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>>>>>> Daniel Baluta schrieb am 18.12.2014 um 18:16:
>>>>>>>>> When using ACPI, if acpi_match_device fails then chipset enum will be
>>>>>>>>> uninitialized and &ak_def_array[chipset] will point to some bad address.
>>>>>>>>>
>>>>>> I am missing something. You are enumerated over i2c device, which was
>>>>>> created from ACPI PNP resource. There is a valid handle or and the
>>>>>> device has an ACPI companion at the least. If this failing, I have to
>>>>>> check the code for acpi i2c.
>>>>>> Can you check why this check failed? We may have bug in i2c handling.
>>>>
>>>> You are right about this. Under normal circumstances, if probe is called
>>>> then acpi_match_device will not fail. I even tried to remove the
>>>> device after probe
>>>> but before acpi_match_device, anyhow acpi_match_device was still successful :)
>>>>
>>>> This is more a matter of code correctness.
>>>>
>>>> In ak8975_match_acpi_device we have:
>>>>
>>>> »       const struct acpi_device_id *id;
>>>>
>>>> »       id = acpi_match_device(dev->driver->acpi_match_table, dev);
>>>> »       if (!id)
>>>> »       »       return NULL;
>>>> »       *chipset = (int)id->driver_data;
>>>>
>>>> Compiler complains on the fact that chipset might be uninitialized
>>>> if this returns NULL, and we shouldn't ignore this warning even this case
>>>> will never happen.
>>>>
>>> Will this fix?
>>> data->chipset = AK8975;
>>> before
>>> ak8975_match_acpi_device(&client->dev, &data->chipset);
>>>

This would fix the compiler warning, but doesn't seem the right solution for
this issue. Quoting the description of acpi_match_device:
"Return a pointer to the first matching ID on success or %NULL on failure."
So, even if it is very unlikely to for it to fail - if it does fail, the
error should be handled as quick as possible. I would favor Daniels solution
to check for a valid assignment of name.

>>
>> Yes, this is done in the original patch:
>>
>> +       *chipset = AK_MAX_TYPE;
> Since data memory is not zero alloced, other member of data are anyway
> initialized, so adding this also may be better. 

If there did not occur an error condition, it will be assigned a value
before being checked for valid ranges. And if there is an error, probe
should be aborted, anyway. So initializing *chipset doesn't seem to add
any benefit IMHO.

>>
>> .. and fixes the warning.
>>
>> Daniel.
> 
> N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> 

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Pandruvada, Srinivas]

T24gU2F0LCAyMDE1LTAxLTI0IGF0IDAwOjM4ICswMTAwLCBIYXJ0bXV0IEtuYWFjayB3cm90ZToN
Cj4gUGFuZHJ1dmFkYSwgU3Jpbml2YXMgc2NocmllYiBhbSAxOS4wMS4yMDE1IHVtIDE3OjU2Og0K
PiA+IE9uIE1vbiwgMjAxNS0wMS0xOSBhdCAxODo0OSArMDIwMCwgRGFuaWVsIEJhbHV0YSB3cm90
ZTogDQo+ID4+IE9uIE1vbiwgSmFuIDE5LCAyMDE1IGF0IDY6NDQgUE0sIFBhbmRydXZhZGEsIFNy
aW5pdmFzDQo+ID4+IDxzcmluaXZhcy5wYW5kcnV2YWRhQGludGVsLmNvbT4gd3JvdGU6DQo+ID4+
PiBPbiBNb24sIDIwMTUtMDEtMTkgYXQgMTY6NDAgKzAyMDAsIERhbmllbCBCYWx1dGEgd3JvdGU6
DQo+ID4+Pj4gSGVsbG8sDQo+ID4+Pj4NCj4gPj4+PiBPbiBTYXQsIERlYyAyMCwgMjAxNCBhdCAx
MToyOSBQTSwgUGFuZHJ1dmFkYSwgU3Jpbml2YXMNCj4gPj4+PiA8c3Jpbml2YXMucGFuZHJ1dmFk
YUBpbnRlbC5jb20+IHdyb3RlOg0KPiA+Pj4+PiArTWlrYQ0KPiA+Pj4+Pg0KPiA+Pj4+PiBPbiBT
YXQsIDIwMTQtMTItMjAgYXQgMTM6MjYgLTA4MDAsIFNyaW5pdmFzIFBhbmRydXZhZGEgd3JvdGU6
DQo+ID4+Pj4+PiBPbiBTYXQsIDIwMTQtMTItMjAgYXQgMDA6MjUgKzAyMDAsIERhbmllbCBCYWx1
dGEgd3JvdGU6DQo+ID4+Pj4+Pj4gT24gU2F0LCBEZWMgMjAsIDIwMTQgYXQgMTI6MTYgQU0sIEhh
cnRtdXQgS25hYWNrIDxrbmFhY2suaEBnbXguZGU+IHdyb3RlOg0KPiA+Pj4+Pj4+PiBEYW5pZWwg
QmFsdXRhIHNjaHJpZWIgYW0gMTguMTIuMjAxNCB1bSAxODoxNjoNCj4gPj4+Pj4+Pj4+IFdoZW4g
dXNpbmcgQUNQSSwgaWYgYWNwaV9tYXRjaF9kZXZpY2UgZmFpbHMgdGhlbiBjaGlwc2V0IGVudW0g
d2lsbCBiZQ0KPiA+Pj4+Pj4+Pj4gdW5pbml0aWFsaXplZCBhbmQgJmFrX2RlZl9hcnJheVtjaGlw
c2V0XSB3aWxsIHBvaW50IHRvIHNvbWUgYmFkIGFkZHJlc3MuDQo+ID4+Pj4+Pj4+Pg0KPiA+Pj4+
Pj4gSSBhbSBtaXNzaW5nIHNvbWV0aGluZy4gWW91IGFyZSBlbnVtZXJhdGVkIG92ZXIgaTJjIGRl
dmljZSwgd2hpY2ggd2FzDQo+ID4+Pj4+PiBjcmVhdGVkIGZyb20gQUNQSSBQTlAgcmVzb3VyY2Uu
IFRoZXJlIGlzIGEgdmFsaWQgaGFuZGxlIG9yIGFuZCB0aGUNCj4gPj4+Pj4+IGRldmljZSBoYXMg
YW4gQUNQSSBjb21wYW5pb24gYXQgdGhlIGxlYXN0LiBJZiB0aGlzIGZhaWxpbmcsIEkgaGF2ZSB0
bw0KPiA+Pj4+Pj4gY2hlY2sgdGhlIGNvZGUgZm9yIGFjcGkgaTJjLg0KPiA+Pj4+Pj4gQ2FuIHlv
dSBjaGVjayB3aHkgdGhpcyBjaGVjayBmYWlsZWQ/IFdlIG1heSBoYXZlIGJ1ZyBpbiBpMmMgaGFu
ZGxpbmcuDQo+ID4+Pj4NCj4gPj4+PiBZb3UgYXJlIHJpZ2h0IGFib3V0IHRoaXMuIFVuZGVyIG5v
cm1hbCBjaXJjdW1zdGFuY2VzLCBpZiBwcm9iZSBpcyBjYWxsZWQNCj4gPj4+PiB0aGVuIGFjcGlf
bWF0Y2hfZGV2aWNlIHdpbGwgbm90IGZhaWwuIEkgZXZlbiB0cmllZCB0byByZW1vdmUgdGhlDQo+
ID4+Pj4gZGV2aWNlIGFmdGVyIHByb2JlDQo+ID4+Pj4gYnV0IGJlZm9yZSBhY3BpX21hdGNoX2Rl
dmljZSwgYW55aG93IGFjcGlfbWF0Y2hfZGV2aWNlIHdhcyBzdGlsbCBzdWNjZXNzZnVsIDopDQo+
ID4+Pj4NCj4gPj4+PiBUaGlzIGlzIG1vcmUgYSBtYXR0ZXIgb2YgY29kZSBjb3JyZWN0bmVzcy4N
Cj4gPj4+Pg0KPiA+Pj4+IEluIGFrODk3NV9tYXRjaF9hY3BpX2RldmljZSB3ZSBoYXZlOg0KPiA+
Pj4+DQo+ID4+Pj4gwrsgICAgICAgY29uc3Qgc3RydWN0IGFjcGlfZGV2aWNlX2lkICppZDsNCj4g
Pj4+Pg0KPiA+Pj4+IMK7ICAgICAgIGlkID0gYWNwaV9tYXRjaF9kZXZpY2UoZGV2LT5kcml2ZXIt
PmFjcGlfbWF0Y2hfdGFibGUsIGRldik7DQo+ID4+Pj4gwrsgICAgICAgaWYgKCFpZCkNCj4gPj4+
PiDCuyAgICAgICDCuyAgICAgICByZXR1cm4gTlVMTDsNCj4gPj4+PiDCuyAgICAgICAqY2hpcHNl
dCA9IChpbnQpaWQtPmRyaXZlcl9kYXRhOw0KPiA+Pj4+DQo+ID4+Pj4gQ29tcGlsZXIgY29tcGxh
aW5zIG9uIHRoZSBmYWN0IHRoYXQgY2hpcHNldCBtaWdodCBiZSB1bmluaXRpYWxpemVkDQo+ID4+
Pj4gaWYgdGhpcyByZXR1cm5zIE5VTEwsIGFuZCB3ZSBzaG91bGRuJ3QgaWdub3JlIHRoaXMgd2Fy
bmluZyBldmVuIHRoaXMgY2FzZQ0KPiA+Pj4+IHdpbGwgbmV2ZXIgaGFwcGVuLg0KPiA+Pj4+DQo+
ID4+PiBXaWxsIHRoaXMgZml4Pw0KPiA+Pj4gZGF0YS0+Y2hpcHNldCA9IEFLODk3NTsNCj4gPj4+
IGJlZm9yZQ0KPiA+Pj4gYWs4OTc1X21hdGNoX2FjcGlfZGV2aWNlKCZjbGllbnQtPmRldiwgJmRh
dGEtPmNoaXBzZXQpOw0KPiA+Pj4NCj4gDQo+IFRoaXMgd291bGQgZml4IHRoZSBjb21waWxlciB3
YXJuaW5nLCBidXQgZG9lc24ndCBzZWVtIHRoZSByaWdodCBzb2x1dGlvbiBmb3INCj4gdGhpcyBp
c3N1ZS4gUXVvdGluZyB0aGUgZGVzY3JpcHRpb24gb2YgYWNwaV9tYXRjaF9kZXZpY2U6DQo+ICJS
ZXR1cm4gYSBwb2ludGVyIHRvIHRoZSBmaXJzdCBtYXRjaGluZyBJRCBvbiBzdWNjZXNzIG9yICVO
VUxMIG9uIGZhaWx1cmUuIg0KPiBTbywgZXZlbiBpZiBpdCBpcyB2ZXJ5IHVubGlrZWx5IHRvIGZv
ciBpdCB0byBmYWlsIC0gaWYgaXQgZG9lcyBmYWlsLCB0aGUNCj4gZXJyb3Igc2hvdWxkIGJlIGhh
bmRsZWQgYXMgcXVpY2sgYXMgcG9zc2libGUuIEkgd291bGQgZmF2b3IgRGFuaWVscyBzb2x1dGlv
bg0KPiB0byBjaGVjayBmb3IgYSB2YWxpZCBhc3NpZ25tZW50IG9mIG5hbWUuDQo+IA0KVGhpcyBz
aG91bGQgbmV2ZXIgZmFpbCBhcyB0aGUgZGV2aWNlIGlzIGVudW1lcmF0ZWQgYnkgdGhpcy4gU28g
aXQNCmRvZXNuJ3QgbWF0dGVyIGFzIGxvbmcgYXMgeW91IHNpbGVudCBjb21waWxlciB3YXJuaW5n
Lg0KPiA+Pg0KPiA+PiBZZXMsIHRoaXMgaXMgZG9uZSBpbiB0aGUgb3JpZ2luYWwgcGF0Y2g6DQo+
ID4+DQo+ID4+ICsgICAgICAgKmNoaXBzZXQgPSBBS19NQVhfVFlQRTsNCj4gPiBTaW5jZSBkYXRh
IG1lbW9yeSBpcyBub3QgemVybyBhbGxvY2VkLCBvdGhlciBtZW1iZXIgb2YgZGF0YSBhcmUgYW55
d2F5DQo+ID4gaW5pdGlhbGl6ZWQsIHNvIGFkZGluZyB0aGlzIGFsc28gbWF5IGJlIGJldHRlci4g
DQo+IA0KPiBJZiB0aGVyZSBkaWQgbm90IG9jY3VyIGFuIGVycm9yIGNvbmRpdGlvbiwgaXQgd2ls
bCBiZSBhc3NpZ25lZCBhIHZhbHVlDQo+IGJlZm9yZSBiZWluZyBjaGVja2VkIGZvciB2YWxpZCBy
YW5nZXMuIEFuZCBpZiB0aGVyZSBpcyBhbiBlcnJvciwgcHJvYmUNCj4gc2hvdWxkIGJlIGFib3J0
ZWQsIGFueXdheS4gU28gaW5pdGlhbGl6aW5nICpjaGlwc2V0IGRvZXNuJ3Qgc2VlbSB0byBhZGQN
Cj4gYW55IGJlbmVmaXQgSU1ITy4NCj4gDQo+ID4+DQo+ID4+IC4uIGFuZCBmaXhlcyB0aGUgd2Fy
bmluZy4NCj4gPj4NCj4gPj4gRGFuaWVsLg0KPiA+IA0KPiA+IE7vv73vv73vv73vv73vv71y77+9
77+9ee+/ve+/ve+/vWLvv71Y77+977+9x6d277+9Xu+/vSneunsubu+/vSvvv73vv73vv73vv717
77+977+9KiLvv73vv71ebu+/vXLvv73vv73vv71677+9Gu+/ve+/vWjvv73vv73vv73vv70m77+9
77+9Hu+/vUfvv73vv73vv71o77+9Ayjvv73pmo7vv73domoi77+977+9Gu+/vRtt77+977+977+9
77+977+9eu+/vd6W77+977+977+9Zu+/ve+/ve+/vWjvv73vv73vv71+77+9bW1sPT0NCj4gPiAN
Cj4gDQoNCg==

Re: [RFC PATCH] iio: ak8975: Make sure chipset is always initialized

[Pandruvada, Srinivas]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3734 bytes --]

On Sat, 2015-01-24 at 00:38 +0100, Hartmut Knaack wrote:
> Pandruvada, Srinivas schrieb am 19.01.2015 um 17:56:
> > On Mon, 2015-01-19 at 18:49 +0200, Daniel Baluta wrote: 
> >> On Mon, Jan 19, 2015 at 6:44 PM, Pandruvada, Srinivas
> >> <srinivas.pandruvada@intel.com> wrote:
> >>> On Mon, 2015-01-19 at 16:40 +0200, Daniel Baluta wrote:
> >>>> Hello,
> >>>>
> >>>> On Sat, Dec 20, 2014 at 11:29 PM, Pandruvada, Srinivas
> >>>> <srinivas.pandruvada@intel.com> wrote:
> >>>>> +Mika
> >>>>>
> >>>>> On Sat, 2014-12-20 at 13:26 -0800, Srinivas Pandruvada wrote:
> >>>>>> On Sat, 2014-12-20 at 00:25 +0200, Daniel Baluta wrote:
> >>>>>>> On Sat, Dec 20, 2014 at 12:16 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> >>>>>>>> Daniel Baluta schrieb am 18.12.2014 um 18:16:
> >>>>>>>>> When using ACPI, if acpi_match_device fails then chipset enum will be
> >>>>>>>>> uninitialized and &ak_def_array[chipset] will point to some bad address.
> >>>>>>>>>
> >>>>>> I am missing something. You are enumerated over i2c device, which was
> >>>>>> created from ACPI PNP resource. There is a valid handle or and the
> >>>>>> device has an ACPI companion at the least. If this failing, I have to
> >>>>>> check the code for acpi i2c.
> >>>>>> Can you check why this check failed? We may have bug in i2c handling.
> >>>>
> >>>> You are right about this. Under normal circumstances, if probe is called
> >>>> then acpi_match_device will not fail. I even tried to remove the
> >>>> device after probe
> >>>> but before acpi_match_device, anyhow acpi_match_device was still successful :)
> >>>>
> >>>> This is more a matter of code correctness.
> >>>>
> >>>> In ak8975_match_acpi_device we have:
> >>>>
> >>>> »       const struct acpi_device_id *id;
> >>>>
> >>>> »       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> >>>> »       if (!id)
> >>>> »       »       return NULL;
> >>>> »       *chipset = (int)id->driver_data;
> >>>>
> >>>> Compiler complains on the fact that chipset might be uninitialized
> >>>> if this returns NULL, and we shouldn't ignore this warning even this case
> >>>> will never happen.
> >>>>
> >>> Will this fix?
> >>> data->chipset = AK8975;
> >>> before
> >>> ak8975_match_acpi_device(&client->dev, &data->chipset);
> >>>
> 
> This would fix the compiler warning, but doesn't seem the right solution for
> this issue. Quoting the description of acpi_match_device:
> "Return a pointer to the first matching ID on success or %NULL on failure."
> So, even if it is very unlikely to for it to fail - if it does fail, the
> error should be handled as quick as possible. I would favor Daniels solution
> to check for a valid assignment of name.
> 
This should never fail as the device is enumerated by this. So it
doesn't matter as long as you silent compiler warning.
> >>
> >> Yes, this is done in the original patch:
> >>
> >> +       *chipset = AK_MAX_TYPE;
> > Since data memory is not zero alloced, other member of data are anyway
> > initialized, so adding this also may be better. 
> 
> If there did not occur an error condition, it will be assigned a value
> before being checked for valid ranges. And if there is an error, probe
> should be aborted, anyway. So initializing *chipset doesn't seem to add
> any benefit IMHO.
> 
> >>
> >> .. and fixes the warning.
> >>
> >> Daniel.
> > 
> > N�����r��y���b�X��ǧv�^�)޺{.n�+����{��*"��^n�r���z�\x1a��h����&��\x1e�G���h�\x03(�階�ݢj"��\x1a�^[m�����z�ޖ���f���h���~�mml==
> > 
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥