Patchwork [RFC,v3,03/13] ahci-platform: Fix clk enable/disable unbalance on suspend/resume

login
register
mail settings
Submitter Hans de Goede
Date Jan. 18, 2014, 11:48 p.m.
Message ID <1390088935-7193-4-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/312344/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Hans de Goede - Jan. 18, 2014, 11:48 p.m.
For devices where ahci_platform_data provides suspend() there is an unbalance
in clk enable/disable calls. The suspend path does not disable the clk, but
the resume path enables it. This commit fixes it by always disabling the clk
in the suspend path independent of there being an ahci_platform_data suspend
method.

On all drivers currently using a suspend method, the suspend method always
succeeds, so change its return type to void, to avoid having the introduce
somewhat complex error handling paths.

The disabling of the clock on suspend is a functional change, to ensure this
is ok I've audited all drivers using ahci_platform_data:

arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method
arch/arm/mach-spear/spear1340.c: Does not use the clock framework
drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these
disable / enable another clock, so likely it is ok to disable / enable the
clock at of-node index 0 as well, I've ordered a wandboard to be able to
test these changes.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/mach-spear/spear1340.c | 6 ++----
 drivers/ata/ahci_imx.c          | 4 +---
 drivers/ata/ahci_platform.c     | 2 +-
 include/linux/ahci_platform.h   | 2 +-
 4 files changed, 5 insertions(+), 9 deletions(-)
Tejun Heo - Jan. 19, 2014, 11:14 a.m.
On Sun, Jan 19, 2014 at 12:48:45AM +0100, Hans de Goede wrote:
> For devices where ahci_platform_data provides suspend() there is an unbalance
> in clk enable/disable calls. The suspend path does not disable the clk, but
> the resume path enables it. This commit fixes it by always disabling the clk
> in the suspend path independent of there being an ahci_platform_data suspend
> method.
> 
> On all drivers currently using a suspend method, the suspend method always
> succeeds, so change its return type to void, to avoid having the introduce
> somewhat complex error handling paths.
> 
> The disabling of the clock on suspend is a functional change, to ensure this
> is ok I've audited all drivers using ahci_platform_data:
> 
> arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method
> arch/arm/mach-spear/spear1340.c: Does not use the clock framework
> drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these
> disable / enable another clock, so likely it is ok to disable / enable the
> clock at of-node index 0 as well, I've ordered a wandboard to be able to
> test these changes.

This isn't your fault but similarly to the previous patch, I'd much
prefer if drivers which need custom ops just override the whole
operation and are allowed to use the default platform from their
custom implementations as they see fit.  Allowing partial overrides
seems like an efficient thing to do at the beginning but if you
continue to stack them, you eventually end up with giant pile of
methods where figuring out which code paths are actually executed
takes quite a bit of effort.  I'd really like to avoid that.

Thanks.
Hans de Goede - Jan. 19, 2014, 6:47 p.m.
Hi,

On 01/19/2014 12:14 PM, Tejun Heo wrote:
> On Sun, Jan 19, 2014 at 12:48:45AM +0100, Hans de Goede wrote:
>> For devices where ahci_platform_data provides suspend() there is an unbalance
>> in clk enable/disable calls. The suspend path does not disable the clk, but
>> the resume path enables it. This commit fixes it by always disabling the clk
>> in the suspend path independent of there being an ahci_platform_data suspend
>> method.
>>
>> On all drivers currently using a suspend method, the suspend method always
>> succeeds, so change its return type to void, to avoid having the introduce
>> somewhat complex error handling paths.
>>
>> The disabling of the clock on suspend is a functional change, to ensure this
>> is ok I've audited all drivers using ahci_platform_data:
>>
>> arch/arm/mach-davinci/devices-da8xx.c: Does not use a suspend method
>> arch/arm/mach-spear/spear1340.c: Does not use the clock framework
>> drivers/ata/ahci_imx.c: Does have suspend and resume pdata methods, these
>> disable / enable another clock, so likely it is ok to disable / enable the
>> clock at of-node index 0 as well, I've ordered a wandboard to be able to
>> test these changes.
>
> This isn't your fault but similarly to the previous patch, I'd much
> prefer if drivers which need custom ops just override the whole
> operation and are allowed to use the default platform from their
> custom implementations as they see fit.  Allowing partial overrides
> seems like an efficient thing to do at the beginning but if you
> continue to stack them, you eventually end up with giant pile of
> methods where figuring out which code paths are actually executed
> takes quite a bit of effort.  I'd really like to avoid that.

I disagree, if we were to follow this reasoning then the init and exit
overrides would have to logically also be all or nothing propositions,
currently ahci_probe and ahci_stop do all the clks, regulator and sata-core
setup / teardown needed with the init / exit pdata callbacks doing any
implementation specific register frobbing needed.

As I see it either doing clks, regulator and sata-core things in a common
place makes sense, and then it goes for suspend and resume too, or we
opt for always following the complete override model, and which point
it becomes more sensible to just do a separate platform driver per
ahci implementation.

After this patch, suspend / resume exactly follow init / stop in that
the ahci_platform.c bits take care of the common stuff, while calling
into a platform_data callback for implementation specific setup.

Also if you look at both the imx and sunxi implementations doing things
this way works well in practice.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Jan. 19, 2014, 7:15 p.m.
Hey,

On Sun, Jan 19, 2014 at 07:47:06PM +0100, Hans de Goede wrote:
> As I see it either doing clks, regulator and sata-core things in a common
> place makes sense, and then it goes for suspend and resume too, or we
> opt for always following the complete override model, and which point
> it becomes more sensible to just do a separate platform driver per
> ahci implementation.

It makes sense in light of those specific cases, but there are gonna
be cases where the placement of the callback is slightly wrong and we
end up with ->XXX_ops_pre() and then ->XXX_ops_post() and so on.
Please make the whole op overridable and then export the default op
and use it as library.

Thanks.
Hans de Goede - Jan. 19, 2014, 7:52 p.m.
Hi,

On 01/19/2014 08:15 PM, Tejun Heo wrote:
> Hey,
>
> On Sun, Jan 19, 2014 at 07:47:06PM +0100, Hans de Goede wrote:
>> As I see it either doing clks, regulator and sata-core things in a common
>> place makes sense, and then it goes for suspend and resume too, or we
>> opt for always following the complete override model, and which point
>> it becomes more sensible to just do a separate platform driver per
>> ahci implementation.
>
> It makes sense in light of those specific cases, but there are gonna
> be cases where the placement of the callback is slightly wrong and we
> end up with ->XXX_ops_pre() and then ->XXX_ops_post() and so on.
> Please make the whole op overridable and then export the default op
> and use it as library.

If we were to put a generic implementation in ahci_platform.c and export
it for use from overrides to avoid copy and pasting common bits everywhere,
then we still have the ordering problem you are talking about. How do you
envision all of this fitting together, I can imagine ie a
ahci_platform_resume_controller which has the bits of what is currently
ahci_resume stating at:

"if (dev->power.power_state.event == PM_EVENT_SUSPEND) {"

And having ahci_resume in ahci_platform.c still doing the clk and power enabling
before calling into ahci_platform_resume, and drivers overriding the resume method
need to do their own clk + regulator + whatever setup
before calling into ahci_platform_resume_controller ?

Also how do you see overriding the entire op, does that mean that pdata->suspend
will be deprecated (we will need to keep it around for now to avoid breaking
existing drivers using it), and all of:

ahci_probe
ahci_suspend
ahci_resume

Will get exported from ahci_platform.c and drivers needing to override any of them
will provide their own platform_driver struct, pointing either to their overrides,
or for driver methods they don't need to override to the exported function from
ahci_platform.c ?

This would also work nicely for the of_device_id data stuff, since if drivers
have their own platform_driver struct these bits could just go inside the
driver file instead of being in #ifdef CONFIG_FOO in ahci_platform.c

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Jan. 20, 2014, 12:26 p.m.
Hey,

On Sun, Jan 19, 2014 at 08:52:21PM +0100, Hans de Goede wrote:
> And having ahci_resume in ahci_platform.c still doing the clk and power enabling
> before calling into ahci_platform_resume, and drivers overriding the resume method
> need to do their own clk + regulator + whatever setup
> before calling into ahci_platform_resume_controller ?

If the use cases are significant enough, split the base function to
two parts?  The thing which gets really messy down the road is when
common code and callbacks intertwine arbitrarily.  A driver wants an
early exit after overriding a subpart, so if that callback exists,
it's an early exit.  The next driver wants to override something at
slightly different point but still want to proceed with the rest of
the common code, so it adds a new callback which doesn't do early
exit, and so on.  Pretty soon, when you're looking at a driver, it
becomes really difficult what it actually does.  We *HAD* this problem
over and over again with ide and it was a nightmare.

Providing larger, logical overriding points while providing common lib
routines makes understanding what a given driver does a lot easier.
Also, it forces people to think about how the overall API looks and
whether a split of an existing library which require giving the splits
sensible names and semantics is justifiable or if the case at hand is
an one-off thing which can be better served by just open coding it.

> Will get exported from ahci_platform.c and drivers needing to override any of them
> will provide their own platform_driver struct, pointing either to their overrides,
> or for driver methods they don't need to override to the exported function from
> ahci_platform.c ?

Yeah, makes sense to me.

Thanks.

Patch

diff --git a/arch/arm/mach-spear/spear1340.c b/arch/arm/mach-spear/spear1340.c
index 3fb6834..c4cbbd2 100644
--- a/arch/arm/mach-spear/spear1340.c
+++ b/arch/arm/mach-spear/spear1340.c
@@ -107,14 +107,12 @@  void sata_miphy_exit(struct device *dev)
 	msleep(20);
 }
 
-int sata_suspend(struct device *dev)
+void sata_suspend(struct device *dev)
 {
 	if (dev->power.power_state.event == PM_EVENT_FREEZE)
-		return 0;
+		return;
 
 	sata_miphy_exit(dev);
-
-	return 0;
 }
 
 int sata_resume(struct device *dev)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 3e23e99..30568d3 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -171,7 +171,7 @@  static void imx6q_sata_exit(struct device *dev)
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 }
 
-static int imx_ahci_suspend(struct device *dev)
+static void imx_ahci_suspend(struct device *dev)
 {
 	struct imx_ahci_priv *imxpriv =  dev_get_drvdata(dev->parent);
 
@@ -185,8 +185,6 @@  static int imx_ahci_suspend(struct device *dev)
 				!IMX6Q_GPR13_SATA_MPLL_CLK_EN);
 		clk_disable_unprepare(imxpriv->sata_ref_clk);
 	}
-
-	return 0;
 }
 
 static int imx_ahci_resume(struct device *dev)
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 4b231ba..dc1ef73 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -275,7 +275,7 @@  static int ahci_suspend(struct device *dev)
 		return rc;
 
 	if (pdata && pdata->suspend)
-		return pdata->suspend(dev);
+		pdata->suspend(dev);
 
 	if (!IS_ERR(hpriv->clk))
 		clk_disable_unprepare(hpriv->clk);
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 73a2500..a641cb6 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -23,7 +23,7 @@  struct ata_port_info;
 struct ahci_platform_data {
 	int (*init)(struct device *dev, void __iomem *addr);
 	void (*exit)(struct device *dev);
-	int (*suspend)(struct device *dev);
+	void (*suspend)(struct device *dev);
 	int (*resume)(struct device *dev);
 	const struct ata_port_info *ata_port_info;
 	unsigned int force_port_map;