Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

On Sun 2006-10-01 18:22:28, Heikki Orsila wrote:
> Some nitpicking about the patch follows..
> 
> On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> > +static long 
> > +get_vtg(const char *vdomain)
> > +{
> > +	long ret = 0;
> 
> Unnecessary initialisation.

No, sorry.

> > +static long 
> > +set_vtg(const char *vdomain, int val)
> > +{
> > +	long ret = 0;
> 
> here too. This and 'int i = 0;' happens in many functions.

Wrong again. automatic variables are not zero initialized.

> > +	err = omap_pm_core_verify_opt(opt);
> > +	if (err != 0)
> > +		goto out;
> > +
> > +	return (void *)opt;
> > +out:
> > +	kfree(opt);
> > +	return NULL;
> > +}
> 
> Maybe use if (err != 0) { kfree(opt); return NULL; } because goto out is 
> only used once?

This is actually common kernel idiom, so it is okay to leave like
this. Your other points are ok.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| Some nitpicking about the patch follows..
| 
| On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
| > +static long 
| > +get_vtg(const char *vdomain)
| > +{
| > +	long ret = 0;
| 
| Unnecessary initialisation.
---

Many of us work in environments where initialization is in the coding
standard. It also helps with static analysis tools. CodingStyle seems to
be silent on the point, but points to Kernighan and Ritchie, who say
"These initializations are actually unnecessary, since all are zero, but
it's a good idea to make them explicit anyway."

Any reasonable compiler will avoid doing the initialization twice...

---
| ...
| > +static int cpu_vltg_show(void *md_opt, int *value)
| > +{
| > +	int rc = 0;
| > +	if (md_opt == NULL) {
| > +		if ((*value = get_vtg("v1")) <= 0)
| > +			return -EIO;
| > +	}
| > +	else {
| > +		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
| > +		*value = opt->cpu_vltg;
| > +	}
| > +
| > +	return rc;
| > +}
| 
| int rc is unnecessary because the function always returns 0. This 
| happens in many places.
---

Wonder if he wrote it for a coding standard that requires single return
(so that the "return -EIO" would have been "rc=-EIO") and converted
it...

scott

-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| On Sun, Oct 01, 2006 at 07:10:32PM +0200, Pavel Machek wrote:
| > On Sun 2006-10-01 18:22:28, Heikki Orsila wrote:
| > > Some nitpicking about the patch follows..
| > > 
| > > On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
| > > > +static long 
| > > > +get_vtg(const char *vdomain)
| > > > +{
| > > > +	long ret = 0;
| > > 
| > > Unnecessary initialisation.
| > 
| > No, sorry.
| 
| In get_vtg(), if VOLTAGE_FRAMEWORK is defined then
| 
| 	ret = vtg_get_voltage(v);
| 
| is the first user. If VOLTAGE_FRAMEWORK is not defined, the first user is:
| 
| 	ret = vtg_get_voltage(&vhandle);
| 
| Then "return ret;" follows. I cannot see a path where 
| pre-initialisation of ret does anything useful. If someone removed the
| #else part, the compiler would bark.
---

True, but a good compiler should remove the dead initialization...

scott
-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Heikki Orsila]

On Mon, Oct 02, 2006 at 01:58:33PM -0500, Scott E. Preece wrote:
> It also helps with static analysis tools.

I'd say those analysis tools are pretty useless if they can not handle 
trivial cases like this.

> CodingStyle seems to
> be silent on the point, but points to Kernighan and Ritchie, who say
> "These initializations are actually unnecessary, since all are zero, but
> it's a good idea to make them explicit anyway."

It was a local variable. They are not autoinitialised. Are you perhaps 
mixing this with statics and globals?

- Heikki

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Scott E. Preece]

| From: Heikki Orsila<shd@zakalwe.fi>
| 
| On Mon, Oct 02, 2006 at 01:58:33PM -0500, Scott E. Preece wrote:
| > It also helps with static analysis tools.
| 
| I'd say those analysis tools are pretty useless if they can not handle 
| trivial cases like this.
| 
| > CodingStyle seems to
| > be silent on the point, but points to Kernighan and Ritchie, who say
| > "These initializations are actually unnecessary, since all are zero, but
| > it's a good idea to make them explicit anyway."
| 
| It was a local variable. They are not autoinitialised. Are you perhaps 
| mixing this with statics and globals?
---

Indeed - I answered too quickly.

However, as noted, there's no practical cost to including the
initializations and some of us do work in places where it's normallly
required.

scott
-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] [RFC] OMAP1 PM Core, PM Core Implementation 2/2

[Pavel Machek]

Hi!

> | Some nitpicking about the patch follows..
> | 
> | On Sat, Sep 30, 2006 at 02:24:35AM +0400, Eugeny S. Mints wrote:
> | > +static long 
> | > +get_vtg(const char *vdomain)
> | > +{
> | > +	long ret = 0;
> | 
> | Unnecessary initialisation.
> 
> Many of us work in environments where initialization is in the coding
> standard. 

Well, l-k is not _this_ kind of environment.

> | > +static int cpu_vltg_show(void *md_opt, int *value)
> | > +{
> | > +	int rc = 0;
> | > +	if (md_opt == NULL) {
> | > +		if ((*value = get_vtg("v1")) <= 0)
> | > +			return -EIO;
> | > +	}
> | > +	else {
> | > +		struct pm_core_point *opt = (struct pm_core_point *)md_opt;
> | > +		*value = opt->cpu_vltg;
> | > +	}
> | > +
> | > +	return rc;
> | > +}
> | 
> | int rc is unnecessary because the function always returns 0. This 
> | happens in many places.
> ---
> 
> Wonder if he wrote it for a coding standard that requires single return
> (so that the "return -EIO" would have been "rc=-EIO") and converted
> it...

We sometimes do that in l-k, too. But having rc=-EIO; return rc; with
single return is little extreme. Just fix it, it is easier than
debating codingstyle.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html