Hi,
On Mon, Jun 06, 2011 at 12:06:44PM -0400, Alan Stern wrote:
> > > > 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.
>
> True. But with your suggestion, people have to remember to use
> __pm_ops and DEFINE_DEV_PM_OPS.
Ok, I see your point here.
> > > 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".
>
> But what is the "right thing"? Suppose you want to have conditional
> support for dev_pm_ops whenever CONFIG_PM is enabled and you _also_
> want to have conditional support for runtime PM whenever
> CONFIG_PM_RUNTIME is enabled?
we don't have this today either. Currently everybody does #ifdef
CONFIG_PM, so either all the data is available, or none is and adding
another #ifdef CONFIG_PM_RUNTIME for the runtime_* friends, would just
look even uglier :-)
> > > 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" ;-)
>
> No. Unlike foo_t, they don't obscure important information and they do
> provide a significant gain in simplicity. On the other hand, they also
> provide a certain degree of confusion. Remember all the difficulty we
> had with intialization code sections in the gadget framework.
this is fairly true, but only because the gadget framework isn't really
a framework. It's just an agreement that all UDCs will export a
particular function. It's a great infrastructure for the function
drivers, but not for UDCs, so I think this isn't a great example :-)
> > > 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 ;-)
>
> You can obtain that same guarantee by using #ifdef ... #endif. Even
> better, you can guarantee that the unused data won't be present at all,
> as opposed to loaded and then freed.
I agree with you here, but I give you the same question as you gave me.
How will you have conditional on CONFIG_RUNTIME_PM and CONFIG_PM ? you'd
need two levels of ifdefs.
> > > 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
> > },
> > };
>
> Whereas your way people write:
>
> static int __pm_ops my_driver_suspend(struct device *dev)
> {
> ...
>
> return 0;
> }
> ....
>
> static DEFINE_DEV_PM_OPS(my_driver_pm_ops) = {
> .suspend = my_driver_suspend,
> ...
> };
>
> static struct platform_driver my_driver = {
> ...
> .driver = {
> .pm = &my_driver_pm_ops,
> },
> };
>
> It doesn't seem like a good idea to keep the invalid pointer to
> my_driver_pm_ops, even though it should never get used.
true, I agree.
> An approach that might work better would be for the PM core to define a
> suitable macro:
>
> #ifdef CONFIG_PM
> #define DEV_PM_OPS_REF(my_pm_ops) &(my_pm_ops)
> #else
> #define DEV_PM_OPS_REF(my_pm_ops) NULL
> #endif
>
> Then people could write
>
> static struct platform_driver my_driver = {
> ...
> .driver = {
> .pm = DEV_PM_OPS_REF(my_driver_pm_ops),
> },
> };
>
> without worrying about whether or not my_driver_pm_ops was defined.
> And only one #ifdef block would be needed.
that'd be nice. Something similar to __exit_p() and __devexit_p()
> > 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.
>
> I have to agree that uniformity is to be desired. And it's probably
> already way too late, because we can't prevent new drivers from being
I wouldn't call it late. Such small convertions can be done by simple
scripts, but when patches switching drivers over are rejected [1] then
we loose the opportunity to give good example to newcomers.
> based on the existing drivers -- even if all the existing drivers get
> changed over (which seems unlikely).
Well, it might work out if pm core makes dev_pm_ops only available on
CONFIG_PM builds.
[1] http://marc.info/?l=linux-usb&m=129733927804315&w=2
--
balbi
Attachment:
signature.asc
Description: Digital signature
