Re: [PATCH V3 05/19] OMAP3+: voltage: use IS_ERR_OR_NULL

[Nishanth Menon <nm@ti.com>]

David Cohen wrote, on 03/05/2011 11:06 PM:
[..]
>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>> index 53c399f..76bcaee 100644
>> --- a/arch/arm/mach-omap2/voltage.c
>> +++ b/arch/arm/mach-omap2/voltage.c
>> @@ -682,7 +682,7 @@ unsigned long omap_voltage_get_nom_volt(struct voltagedomain *voltdm)
>>   {
>>         struct omap_vdd_info *vdd;
>>
>> -       if (!voltdm || IS_ERR(voltdm)) {
>> +       if (IS_ERR_OR_NULL(voltdm)) {
>
> I have one comment here. voltdm is received as parameter and it's
> already checked by IS_ERR(). Is there any specific reason for that?
yes:
1. omap_voltage_domain_lookup can return ERR_PTR() in certain conditions
2. the !voltdm is coz of the usage of these APIs from external to 
voltage.c (sr, pm, dvfs etc) - it is possible (and has happend) when 
mistakes have been made and NULL pointers passed as voltdm.


> IS_ERR() doesn't suppose to be a macro to check if the pointer is
> valid or not, but to know if there's an invalid value with error code
> in the pointer value. It's useful when you have a function which
> returns a pointer but it can return an error code when it fails.
> Please, note that IS_ERR() won't detect invalid pointers which does
> not represent an error code, so it's not correct to rely on it for
> this purpose.
> IMO, instead of change to IS_ERR_OR_NULL(), you could only remove
> IS_ERR(). The same apply for the other cases.
no, I am not convinced with your argument.

omap_voltage_get_nom_volt(omap_voltage_domain_lookup("me"));
IS_ERR is useful in this case. If I were to remove vdata, 
PTR_ERR(something)== vdata and vdata !=NULL and it will try to 
dereference and crash.

my_dumb_func(){
	struct voltagedomain *vdata = NULL;
	if (cpu_is_omap3630()_) {
		vdata = omap_voltage_domain_lookup("mpu")
	}
	/* forgot to add other cpu types or a else case */
	/* do other things */
	me_volt=omap_voltage_get_nom_volt(vdata);
	/* do things with me_volt */
}

Sorry, but i think the check, even if seems superfluous is sane IMHO. 
even if the above code worked on 3630, it'd fail on o4/3430 without the 
check, it can even crash. and given that we've all seen our fair share 
of developers who develop for one platform without consideration that 
the rest of the world uses others as well... I do feel cases like above 
example might infact be a reality.

[...]

-- 
Regards,
Nishanth Menon