Re: [PATCH v4.4-rc8] iio: magnetometer: ak8975: Silence 'may be used uninitialized' warning

[Jonathan Cameron]

On 09/01/16 16:51, Srinivas Pandruvada wrote:
> On Sat, 2016-01-09 at 16:25 +0000, Jonathan Cameron wrote:
>>
> On 09/01/16 16:17, Srinivas Pandruvada wrote:
>>> On Sat, 2016-01-09 at 16:00 +0000, Jonathan Cameron wrote:
>>>> On 09/01/16 00:17, tim.gardner@xxxxxxxxxxxxx wrote:
>>>>> From: Tim Gardner <tim.gardner@xxxxxxxxxxxxx>
>>>>>
>>>>> 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];
>>>>>
>>>>> gcc version 5.3.1 20151219 (Ubuntu 5.3.1-4ubuntu1)
>>>>>
>>>>> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
>>>>> Cc: Hartmut Knaack <knaack.h@xxxxxx>
>>>>> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
>>>>> Cc: Peter Meerwald <pmeerw@xxxxxxxxxx>
>>>>> Cc: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
>>>>> Signed-off-by: Tim Gardner <tim.gardner@xxxxxxxxxxxxx>
>>>> Doesn't look to be an actual bug as we either end up with chipset
>>>> being filled
>>>> based on the traditional match table in which case it'll be
>>>> assigned
>>>> or based on the acpi match, which should succeed seeing as we've
>>>> already
>>>> had to have matched one or the other for the probe to match in
>>>> the
>>>> first place.
>>>>
>>>> So probably worth the change to make it easier to tell that it
>>>> should
>>>> be fine
>>>> and suppress the warning.  However, whilst we are here, I note
>>>> that
>>>> *match_acpi_table has a path which returns NULL as the name and
>>>> doesn't assign
>>>> the chipset.  We should be therefore checking if (!name) return 
>>>> -ENOSYS;
>>>> Though maybe another error code would be more appropriate.
>>>>
>>>
>>> Since in this case we are enumerated by ACPI using our match table,
>>> so
>>> name can't be null. The "name" we provided in 
>>> static const struct acpi_device_id ak_acpi_match[] = {..}
>>> Same with the *chipset. Other than suppress warnings, I don't think
>>> it
>>> will cause any real issue.
>> True enough, in which case why are we checking the name?
> 
> We can remove this check for !id
>   id = acpi_match_device(dev->driver->acpi_match_table, dev);
> - if (!id)
> -	return NULL;
>   *chipset = (int)id->driver_data;
Yes, that's the one I meant rather than the name!

I'm not getting the warning Tim is seeing anyway so I'll leave
it to him to confirm if this clears that up as well (so we
don't need the other patch).

Tim, as you are working on this issue, do you want to try the
above and if it works post a patch making that change + adding
a note where the check is removed to say it cannot fail so there
is no need to check?

Thanks,

Jonathan
> 
> Thanks,
> Srinivas
> 
> 
>> I'd be included to drop that check and add a comment.
>> I haven't chased every path, but I think that might deal with the
>> above
>> warning at it's root.
>>> Thanks,
>>> Srinivas 
>>>
>>>> Not sure that error path can actually happen either, but if we
>>>> are
>>>> going to
>>>> bother having the error path out of match_acpi_table then we
>>>> ought to
>>>> actually
>>>> handle it!
>>>>
>>>> Don't suppose you'd mind fixing that one as well whilst here?
>>>>
>>>> Jonathan 
>>>>> ---
>>>>>
>>>>> This seems like a legitimate warning, though gcc should have
>>>>> complained
>>>>> about an earlier use of chipset on line 782.
>>>>>
>>>>>  drivers/iio/magnetometer/ak8975.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/iio/magnetometer/ak8975.c
>>>>> b/drivers/iio/magnetometer/ak8975.c
>>>>> index b13936d..80ec0ce 100644
>>>>> --- a/drivers/iio/magnetometer/ak8975.c
>>>>> +++ b/drivers/iio/magnetometer/ak8975.c
>>>>> @@ -732,7 +732,7 @@ static int ak8975_probe(struct i2c_client
>>>>> *client,
>>>>>  	int eoc_gpio;
>>>>>  	int err;
>>>>>  	const char *name = NULL;
>>>>> -	enum asahi_compass_chipset chipset;
>>>>> +	enum asahi_compass_chipset chipset = AK_MAX_TYPE;
>>>>>  
>>>>>  	/* Grab and set up the supplied GPIO. */
>>>>>  	if (client->dev.platform_data)
>>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux
>>> -iio" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>