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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
