[PATCH] sh_eth: pm_runtime should not need null operations

[PATCH] sh_eth: pm_runtime should not need null operations

[Ben Dooks]

The driver has a no-op for the pm_runtime callbacks but
the pm_runtime core should correctly ignore drivers without
any pm_rumtime callback ops.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/net/ethernet/renesas/sh_eth.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index b908507..bb93333e 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2998,28 +2998,6 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-static int sh_eth_runtime_nop(struct device *dev)
-{
-	/* Runtime PM callback shared between ->runtime_suspend()
-	 * and ->runtime_resume(). Simply returns success.
-	 *
-	 * This driver re-initializes all registers after
-	 * pm_runtime_get_sync() anyway so there is no need
-	 * to save and restore registers here.
-	 */
-	return 0;
-}
-
-static const struct dev_pm_ops sh_eth_dev_pm_ops = {
-	.runtime_suspend = sh_eth_runtime_nop,
-	.runtime_resume = sh_eth_runtime_nop,
-};
-#define SH_ETH_PM_OPS (&sh_eth_dev_pm_ops)
-#else
-#define SH_ETH_PM_OPS NULL
-#endif
-
 static struct platform_device_id sh_eth_id_table[] = {
 	{ "sh7619-ether", (kernel_ulong_t)&sh7619_data },
 	{ "sh771x-ether", (kernel_ulong_t)&sh771x_data },
@@ -3043,7 +3021,6 @@ static struct platform_driver sh_eth_driver = {
 	.id_table = sh_eth_id_table,
 	.driver = {
 		   .name = CARDNAME,
-		   .pm = SH_ETH_PM_OPS,
 		   .of_match_table = of_match_ptr(sh_eth_match_table),
 	},
 };
-- 
1.9.0

Re: [PATCH] sh_eth: pm_runtime should not need null operations

[Geert Uytterhoeven]

Hi Ben,

On Fri, Mar 21, 2014 at 11:15 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> The driver has a no-op for the pm_runtime callbacks but
> the pm_runtime core should correctly ignore drivers without
> any pm_rumtime callback ops.

The pm_runtime core doesn't ignore non-existing runtime_{suspend,resume}
callbacks, it turns them into a failure withv -ENOSYS.
Only non-existing runtime_idle callbacks are ignored.

rpm_suspend():

        callback = rpm_get_suspend_cb(dev);

        retval = rpm_callback(callback, dev);
        if (retval)
                goto fail;

(rpm_callback() returns -ENOSYS if !callback).

pm_runtime_force_suspend():

        callback = rpm_get_suspend_cb(dev);

        if (!callback) {
                ret = -ENOSYS;
                goto err;
        }

Same for resume.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] sh_eth: pm_runtime should not need null operations

[Ben Dooks]

On 21/03/14 11:30, Geert Uytterhoeven wrote:
> Hi Ben,
>
> On Fri, Mar 21, 2014 at 11:15 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> The driver has a no-op for the pm_runtime callbacks but
>> the pm_runtime core should correctly ignore drivers without
>> any pm_rumtime callback ops.
>
> The pm_runtime core doesn't ignore non-existing runtime_{suspend,resume}
> callbacks, it turns them into a failure withv -ENOSYS.
> Only non-existing runtime_idle callbacks are ignored.

I've added Rafael Wysocki as he may be able to add better
feedback to this issue.

[snip rpm_susend code block]

I got very confused here. The clock_ops sets dev->pm_domain
which over-rides the use of the dev->driver->pm entry as the
primary pm for the device. The code above the bit you snipped
does a ladder looking for the pm_runtime entry it calls and
would stop at finding dev->pm_domain as so:

from drivers/base/power/runtime.c:
     495         if (dev->pm_domain)
     496                 callback = dev->pm_domain->ops.runtime_suspend;
  ...
     502                 callback = dev->bus->pm->runtime_suspend;
     503         else
     504                 callback = NULL;


So for drivers on shmobile with drivers/sh/pm_runtime.c enabled
we would never call the drivers' entry as the ops that this code
introduces just calls the pm_clk calls and does not send the
events on.

If we send the events on, then we would use pm_generic_runtime_suspend()
to send it. This call treats the lack of runtime_pm driver entry as a
do nothing and return 0 which means in this case the code in sh_eth
is not necessary to have any pm_runtime functions.

This means depending on if we have a pm_domain in the path we get
different treatment of NULL runtime pm ops pointer. I am not sure
how to handle this, as IIRC a number of other drivers for Renesas
currently assume that the NULL case is going to be fine for them.

Changing pm_generic_runtime_suspend() to return ENOSYS would end
up breaking davinci and probably a number of other platforms.

So questions:

- Should rpm_suspend() ignore the lack of pm_runtime operations?
- Do we need to add a generic `ignore pm runtime callback`
- Are any other shmobile drivers similarly affected?

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH] sh_eth: pm_runtime should not need null operations

[David Miller]

From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Fri, 21 Mar 2014 11:57:23 +0100

> On 21/03/14 11:30, Geert Uytterhoeven wrote:
>> Hi Ben,
>>
>> On Fri, Mar 21, 2014 at 11:15 AM, Ben Dooks
>> <ben.dooks@codethink.co.uk> wrote:
>>> The driver has a no-op for the pm_runtime callbacks but
>>> the pm_runtime core should correctly ignore drivers without
>>> any pm_rumtime callback ops.
>>
>> The pm_runtime core doesn't ignore non-existing
>> runtime_{suspend,resume}
>> callbacks, it turns them into a failure withv -ENOSYS.
>> Only non-existing runtime_idle callbacks are ignored.
> 
> I've added Rafael Wysocki as he may be able to add better
> feedback to this issue.

This discussion has stalled so I'm marking this patch as
"deferred" in patchwork.

Please resubmit once things are resolved, thanks.