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

Attachments:

(No filename) (4.61 kB)
signature.asc (490.00 B)
Digital signature Download all attachments

2011-06-06 09:45:39

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 4/4] mfd: global Suspend and resume support of ehci and ohci

On Sun, Jun 05, 2011 at 03:30:55PM -0400, Alan Stern wrote:
> On Sun, 5 Jun 2011, Felipe Balbi wrote:

> > 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
> very easy to understand "#ifdef" followed by "#endif"; people see them
> 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
> readers would still have to figure out when these tags should be used
> or what advantage they might bring.

The big advantage is that they make it much harder to introduce random
build breakage and they're an awful lot less fiddly to do - it used to
be not so bad when it was just the function pointers that needed to be
defined to NULL but now we need to faff around with both the functions
and the dev_pm_ops.

The annotation approach is already familiar from the init stuff so it
wouldn't be so hard for people to understand.

2011-06-06 16:06:47

by Alan Stern

[permalink] [raw]

Subject: Re: [PATCH 4/4] mfd: global Suspend and resume support of ehci and ohci

On Sun, 5 Jun 2011, Felipe Balbi wrote:

> 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.

True. But with your suggestion, people have to remember to use
__pm_ops and DEFINE_DEV_PM_OPS.

> > 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?

> > 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.

> > 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.

> > 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.

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.

> 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
based on the existing drivers -- even if all the existing drivers get
changed over (which seems unlikely).

Alan Stern

2011-06-06 17:26:08

On Thu, Jun 30, 2011 at 12:50 AM, Kevin Hilman <[email protected]> wrote:
> "Munegowda, Keshava" <[email protected]> writes:
>
>> On Wed, Jun 29, 2011 at 8:52 PM, Munegowda, Keshava
>> <[email protected]> wrote:
>>> On Thu, Jun 2, 2011 at 5:36 AM, Kevin Hilman <[email protected]> wrote:
>>>> Keshava Munegowda <[email protected]> writes:
>>>>
>>>>> From: Keshava Munegowda <[email protected]>
>>>>>
>>>>> The global suspend and resume functions for usbhs core driver
>>>>> are implemented.These routine are called when the global suspend
>>>>> and resume occurs. Before calling these functions, the
>>>>> bus suspend and resume of ehci and ohci drivers are called
>>>>> from runtime pm.
>>>>>
>>>>> Signed-off-by: Keshava Munegowda <[email protected]>
>>>>
>>>> First, from what I can see, this is only a partial implementation of
>>>> runtime PM. ?What I mean is that the runtime PM methods are used only
>>>> during the suspend path. ?The rest of the time the USB host IP block is
>>>> left enabled, even when nothing is connected.
>>>>
>>>> I tested this on my 3530/Overo board, and verified that indeed the
>>>> usbhost powerdomain hits retention on suspend, but while idle, when
>>>> nothing is connected, I would expect the driver could be clever enough
>>>> to use runtime PM (probably using autosuspend timeouts) to disable the
>>>> hardware as well.
>>>>
>>>>> ---
>>>>> ?drivers/mfd/omap-usb-host.c | ?103 +++++++++++++++++++++++++++++++++++++++++++
>>>>> ?1 files changed, 103 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
>>>>> index 43de12a..32d19e2 100644
>>>>> --- a/drivers/mfd/omap-usb-host.c
>>>>> +++ b/drivers/mfd/omap-usb-host.c
>>>>> @@ -146,6 +146,10 @@
>>>>> ?#define is_ehci_hsic_mode(x) (x == OMAP_EHCI_PORT_MODE_HSIC)
>>>>>
>>>>>
>>>>> +/* USBHS state bits */
>>>>> +#define OMAP_USBHS_INIT ? ? ? ? ? ? ?0
>>>>> +#define OMAP_USBHS_SUSPEND ? 4
>>>>
>>>> These additional state bits don't seem to be necessary.
>>>>
>>>> For suspend, just check 'pm_runtime_is_suspended()'
>>>>
>>>> The init flag is only used in the suspend/resume hooks, but the need for
>>>> it is a side effect of not correctly using the runtime PM callbacks.
>>>>
>>>> Remember that the runtime PM get/put hooks have usage counting. ?Only
>>>> when the usage count transitions to/from zero is the actual
>>>> hardware-level enable/disable (via omap_hwmod) being done.
>>>>
>>>> The current code is making the assumption that every call to get/put is
>>>> going to result in an enable/disable of the hardware.
>>>>
>>>> Instead, all of the code that needs to be run only upon actual
>>>> enable/disable of the hardware should be done in the driver's
>>>> runtime_suspend/runtime_resume callbacks. ?These are only called when
>>>> the hardware actually changes state.
>>>>
>>>> Not knowing that much about the EHCI block, upon first glance, it looks
>>>> like mmuch of what is done in usbhs_enable() should actually be done in
>>>> the ->runtime_resume() callback, and similarily, much of what is done in
>>>> usbhs_disable() should be done in the ->runtime_suspend() callback.
>>>
>>> Kevin,
>>> ? do you mean driver->runtime_resume and driver->runtime_resume call backs.
>>> are these call backs from pm_runtime_get_sync and pm_runtime_put_sync?
>>
>> for usb host case , I am seeing that the pm_runtime_get_sync
>>
>>
>> static int rpm_resume(struct device *dev, int rpmflags)
>> {
>> ? ............
>> ?..........
>> ? ? ? if (dev->pwr_domain) {
>> ? ? ? ? ? ? ? callback = dev->pwr_domain->ops.runtime_resume;
>> ? ? ? ? ? ? ? if(!strcmp(dev_name(dev),"usbhs_omap"))
>> ? ? ? ? ? ? ? ? ? ? ? ?pr_err("dev->pwr_domain->ops.runtime_resume");
>> ? ? ? }
>> ? ? ? else if (dev->type && dev->type->pm) {
>> ? ? ? ? ? ? ? callback = dev->type->pm->runtime_resume;
>> ? ? ? ? ? ? ? if(!strcmp(dev_name(dev),"usbhs_omap"))
>> ? ? ? ? ? ? ? ? ? ? ? ?pr_err("dev->type->pm->runtime_resume");
>> ? ? ? }
>> ? ? ? else if (dev->class && dev->class->pm) {
>> ? ? ? ? ? ? ? callback = dev->class->pm->runtime_resume;
>> ? ? ? ? ? ? ? if(!strcmp(dev_name(dev),"usbhs_omap"))
>> ? ? ? ? ? ? ? ? ? ? ? ?pr_err("ev->class->pm->runtime_resume");
>> ? ? ? }
>> ? ? ? else if (dev->bus && dev->bus->pm) {
>> ? ? ? ? ? ? ? callback = dev->bus->pm->runtime_resume;
>> ? ? ? if(!strcmp(dev_name(dev),"usbhs_omap"))
>> ? ? ? ? ? ? ? ?pr_err("dev->bus->pm->runtime_resume");
>> ? ? ? }
>> ? ? ? else
>> ? ? ? ? ? ? ? callback = NULL;
>> }
>>
>>
>> I am seeing that below if statement was hitting true:
>>
>> ? ? ? if (dev->pwr_domain) {
>> ? ? ? ? ? ? ? callback = dev->pwr_domain->ops.runtime_resume;
>> ? ? ? ? ? ? ? if(!strcmp(dev_name(dev),"usbhs_omap"))
>> ? ? ? ? ? ? ? ? ? ? ? ?pr_err("dev->pwr_domain->ops.runtime_resume");
>>
>>
>> due to this; the driver->runtime_resume was not getting called.
>>
>> Any idea on why I am seeing only the dev->pwr_domain is set not
>> dev->bus && dev->bus->pm is hitting here?
>
> Because that's how it was designed. :)
>
> On OMAP, for on-chip devices (omap_devices) we use PM domains
> (pwr_domain) and not the subsystem (bus) to implment runtime PM, and as
> Alan pointed out, PM domains have precedence over subsystem callbacks.
>
> I'm curious why you determined the driver's runtime resume was not
> getting called?
>
> The PM domain callback will call your driver's runtime_resume (assuming
> it exists.)
>
> rpm_resume()
> ? dev->pwr_domain->ops.runtime_resume()
> ? ? ? omap_device_enable()
> ? ? ? pm_generic_runtime_resume()
> ? ? ? ? ?dev->driver->pm->runtime_resume()
>
> Note that the PM domain implementation is done at the omap_device
> level. ?Specifically, see plat-omap/omap_device.c:_od_runtime_resume()
>
> Kevin

Thanks to partha and others

I was an rc issue; I migrated to latest kernel ; its working now.
driver runtime call backs are getting
called now. :-)