Message-ID: <20110605195413.GC18731@legolas.emea.dhcp.ti.com>
Date:	Sun, 5 Jun 2011 22:54:15 +0300
From:	Felipe Balbi <balbi@...com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Felipe Balbi <balbi@...com>, "Rafael J. Wysocki" <rjw@...k.pl>,
	Keshava Munegowda <keshava_mgowda@...com>,
	linux-usb@...r.kernel.org, linux-omap@...r.kernel.org,
	linux-kernel@...r.kernel.org, gadiyar@...com,
	sameo@...ux.intel.com, parthab@...ia.ti.com, tony@...mide.com,
	khilman@...com, b-cousson@...com, paul@...an.com
Subject: Re: [PATCH 4/4] mfd: global Suspend and resume support of ehci and
 ohci

Hi,

On Sun, Jun 05, 2011 at 03:30:55PM -0400, Alan Stern wrote:
> > > > good point. BTW, do we need this #ifdef CONFIG_PM stuff which has been
> > > > popping on most drivers recently ? To me it looks like driver.pm field
> > > > is always available even if PM is disabled, so what's the point ? Saving
> > > > a few bytes ?
> > > 
> > > Basically, yes (you may want to avoid defining the object this points to if
> > > CONFIG_PM is unset).
> > 
> > wouldn't it look nicer to have a specific section for dev_pm_ops which
> > gets automatically freed if CONFIG_PM isn't set ? I mean, there are a
> > few drivers which don't have the ifdeferry around dev_pm_ops anyway.
> > 
> > So, something like:
> > 
> > #define __pm_ops	__section(.pm.ops)
> > 
> > static const struct dev_pm_ops my_driver_pm_ops __pm_ops = {
> > 	.suspend	= my_driver_suspend,
> > 	.resume		= my_driver_resume,
> > 	[ blablabla ]
> > };
> > 
> > to simplify things, you could:
> > 
> > #define DEFINE_DEV_PM_OPS(_ops)		\
> > 	const struct dev_pm_ops _ops __pm_ops
> > 
> > that would mean changes to all linker scripts, though and a utility call
> > that only does anything ifndef CONFIG_PM to free the .pm.ops section.
> 
> In my opinion this would make programming harder, not easier.  It's

I tend to disagree with this statement, see below.

> very easy to understand "#ifdef" followed by "#endif"; people see them

very true... Still everybody has to put them in place.

> all the time.  The new tags you propose would force people to go
> searching through tons of source files to see what they mean, and then

only those who want to see "how things work" would be forced to do that,
other people would be allowed to "assume it's doing the right thing".

> readers would still have to figure out when these tags should be used
> or what advantage they might bring.

not really, if you add a macro which adds that correctly and during
review we only accept drivers using that particular macro, things
wouldn't go bad at all.

> It's a little like "typedef struct foo foo_t;" -- doing this forces

hey c'mon. Then you're saying that all __initdata, __devinitdata,
__initconst and all of those are "typedef struct foo foo_t" ;-)

> people to remember one extra piece of information that serves no real
> purpose except perhaps a minimal reduction in the amount of typing.  

and a guarantee that the unused data will be freed when it's really not
needed ;-)

> Since the limiting factor in kernel programming is human brainpower,
> not source file length, this is a bad tradeoff.  (Not to mention that

OTOH we are going through a big re-factor of the ARM port to reduce the
amount of code. Not that these few characters would change much but my
point is that amount of code also matters. So does uniformity, coding
style, etc...

> it also obscures an important fact: A foo_t is an extended structure
> rather than a single value.)

then it would make sense to have dev_pm_ops only defined when CONFIG_PM
is set to force all drivers stick to a common way of handling this.

Besides, currently, everybody who wants to keep the ifdeferry, needs to
define a macro for &my_dev_pm_ops or have two #ifdef..#endif blocks.

Either you do:

#ifdef CONFIG_PM
static int my_driver_suspend(struct device *dev)
{
	...

	return 0;
}
....

static const struct dev_pm_ops my_driver_pm_ops = {
	.suspend	= my_driver_suspend,
	...
};

#define DEV_PM_OPS	(&my_driver_pm_ops)
#else
#define DEV_PM_OPS	NULL
#endif

static struct platform_driver my_driver = {
	...
	.driver	= {
		.pm = DEV_PM_OPS
	},
};

or you do:

#ifdef CONFIG_PM
static int my_driver_suspend(struct device *dev)
{
	...

	return 0;
}
....

static const struct dev_pm_ops my_driver_pm_ops = {
	.suspend	= my_driver_suspend,
	...
};

#endif

static struct platform_driver my_driver = {
	...
	.driver	= {
#ifdef CONFIG_PM
		.pm = &my_driver_pm_ops,
#endif
	},
};

So, while this is a small thing which is easy to understand, it's still
yet another thing that all drivers have to remember to add. And when
everybody needs to remember that, I'd rather have it done
"automatically" by other means.

I mean, we already free .init.* sections after __init anyway, so what's
the problem in freeing another section ? I don't see it as obfuscation
at all. I see it as if the kernel is smart enough to free all unused
data by itself, without myself having to add ifdefs or freeing it by my
own.

On top of all that, today, we have driver with both ways of ifdefs plus
drivers with no ifdeferry at all, leaving dev_pm_ops floating around for
nothing.

IMHO, if things aren't uniform, we will have small problems, such as
this, proliferate because new drivers are based on other drivers,
generally.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)

Powered by blists - more mailing lists