[RFC v2 0/3] ahci_platform: unbind/rmmod power down sequence

[RFC v2 0/3] ahci_platform: unbind/rmmod power down sequence

[Brian Norris]

Hello,

This is a follow up on a previous questions and RFC series I sent. See here for
some context:

    http://article.gmane.org/gmane.linux.ide/53143
    http://article.gmane.org/gmane.linux.ide/52951

This series:

(1) Allows ahci_platform to unbind a device from the driver. This is useful for
    allowing total power-off of the device, for instance.
(2) Adds ahci_platform ata_port_operations.host_stop() hook, so that
    platform-device exit() can power down the device at the appropriate point
    in the removal sequence.

Thanks to Tejun for the comments, which suggested that ahci_platform (not
libata-core) was broken.

Brian

Brian Norris (3):
  ahci_platform: enable hotplug unbinding
  ahci_platform: convert to module_platform_driver
  ahci_platform: perform platform exit in host_stop() hook

 drivers/ata/ahci_platform.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

-- 
1.7.11.3

[RFC v2 1/3] ahci_platform: enable hotplug unbinding

[Brian Norris]

platform_driver_probe() should be used for registering this driver only
if we want to

    "...remove its run-once probe() infrastructure from memory after the
    driver has bound to the device."

However, we may want to leave the probe infrastructure in place in order
to support binding/unbinding a device dynamically. This is useful, for
instance, as a power management mechanism, where a device can be totally
powered down when unbound (whereas with runtime power management,
powering down the SATA core would incur unacceptable loss of
functionality).

Thus, convert this driver to use platform_driver_register().

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/ata/ahci_platform.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 9e419e1..f467ba8 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -73,7 +73,7 @@ static struct scsi_host_template ahci_platform_sht = {
 	AHCI_SHT("ahci_platform"),
 };
 
-static int __init ahci_probe(struct platform_device *pdev)
+static int __devinit ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct ahci_platform_data *pdata = dev_get_platdata(dev);
@@ -286,6 +286,7 @@ static const struct of_device_id ahci_of_match[] = {
 MODULE_DEVICE_TABLE(of, ahci_of_match);
 
 static struct platform_driver ahci_driver = {
+	.probe = ahci_probe,
 	.remove = __devexit_p(ahci_remove),
 	.driver = {
 		.name = "ahci",
@@ -300,7 +301,7 @@ static struct platform_driver ahci_driver = {
 
 static int __init ahci_init(void)
 {
-	return platform_driver_probe(&ahci_driver, ahci_probe);
+	return platform_driver_register(&ahci_driver);
 }
 module_init(ahci_init);
 
-- 
1.7.11.3

[RFC v2 2/3] ahci_platform: convert to module_platform_driver

[Brian Norris]

The ahci_platform driver can now use the module_platform_driver() macro.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/ata/ahci_platform.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index f467ba8..d9fbd10 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -298,18 +298,7 @@ static struct platform_driver ahci_driver = {
 	},
 	.id_table	= ahci_devtype,
 };
-
-static int __init ahci_init(void)
-{
-	return platform_driver_register(&ahci_driver);
-}
-module_init(ahci_init);
-
-static void __exit ahci_exit(void)
-{
-	platform_driver_unregister(&ahci_driver);
-}
-module_exit(ahci_exit);
+module_platform_driver(ahci_driver);
 
 MODULE_DESCRIPTION("AHCI SATA platform driver");
 MODULE_AUTHOR("Anton Vorontsov <avorontsov@ru.mvista.com>");
-- 
1.7.11.3

[RFC v2 3/3] ahci_platform: perform platform exit in host_stop() hook

[Brian Norris]

AHCI platform devices may provide an exit() routine, via
ahci_platform_data, that powers off the SATA core. Such a routine should
be executed from the ata_port_operations host_stop() hook. That way, the
ATA subsystem can perform any last-minute hardware cleanup (via devres,
for example), then trigger the power-off at the appropriate time.

This patch fixes bus errors triggered during module removal or device
unbinding, seen on an SoC SATA core.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/ata/ahci_platform.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index d9fbd10..dcd23a9 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -23,6 +23,8 @@
 #include <linux/ahci_platform.h>
 #include "ahci.h"
 
+static void ahci_host_stop(struct ata_host *host);
+
 enum ahci_type {
 	AHCI,		/* standard platform ahci */
 	IMX53_AHCI,	/* ahci on i.mx53 */
@@ -45,6 +47,15 @@ static struct platform_device_id ahci_devtype[] = {
 };
 MODULE_DEVICE_TABLE(platform, ahci_devtype);
 
+struct ata_port_operations ahci_platform_ops = {
+	.inherits	= &ahci_ops,
+	.host_stop	= ahci_host_stop,
+};
+
+struct ata_port_operations ahci_platform_retry_srst_ops = {
+	.inherits	= &ahci_pmp_retry_srst_ops,
+	.host_stop	= ahci_host_stop,
+};
 
 static const struct ata_port_info ahci_port_info[] = {
 	/* by features */
@@ -52,20 +63,20 @@ static const struct ata_port_info ahci_port_info[] = {
 		.flags		= AHCI_FLAG_COMMON,
 		.pio_mask	= ATA_PIO4,
 		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_ops,
+		.port_ops	= &ahci_platform_ops,
 	},
 	[IMX53_AHCI] = {
 		.flags		= AHCI_FLAG_COMMON,
 		.pio_mask	= ATA_PIO4,
 		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_pmp_retry_srst_ops,
+		.port_ops	= &ahci_platform_retry_srst_ops,
 	},
 	[STRICT_AHCI] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_DELAY_ENGINE),
 		.flags		= AHCI_FLAG_COMMON,
 		.pio_mask	= ATA_PIO4,
 		.udma_mask	= ATA_UDMA6,
-		.port_ops	= &ahci_ops,
+		.port_ops	= &ahci_platform_ops,
 	},
 };
 
@@ -202,15 +213,20 @@ err0:
 static int __devexit ahci_remove(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct ahci_platform_data *pdata = dev_get_platdata(dev);
 	struct ata_host *host = dev_get_drvdata(dev);
 
 	ata_host_detach(host);
 
+	return 0;
+}
+
+static void ahci_host_stop(struct ata_host *host)
+{
+	struct device *dev = host->dev;
+	struct ahci_platform_data *pdata = dev_get_platdata(dev);
+
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM
-- 
1.7.11.3

Re: [linux-pm] [RFC v2 0/3] ahci_platform: unbind/rmmod power down sequence

[Rafael J. Wysocki]

Hi,

Please don't CC the linux-pm@lists.linux-foundation.org list, which is dead.

Please use linux-pm@vger.kernel.org for future submissions, as documented in
multiple places.

Thanks,
Rafael


On Saturday, October 27, 2012 01:09:33 PM Brian Norris wrote:
> Hello,
> 
> This is a follow up on a previous questions and RFC series I sent. See here for
> some context:
> 
>     http://article.gmane.org/gmane.linux.ide/53143
>     http://article.gmane.org/gmane.linux.ide/52951
> 
> This series:
> 
> (1) Allows ahci_platform to unbind a device from the driver. This is useful for
>     allowing total power-off of the device, for instance.
> (2) Adds ahci_platform ata_port_operations.host_stop() hook, so that
>     platform-device exit() can power down the device at the appropriate point
>     in the removal sequence.
> 
> Thanks to Tejun for the comments, which suggested that ahci_platform (not
> libata-core) was broken.
> 
> Brian
> 
> Brian Norris (3):
>   ahci_platform: enable hotplug unbinding
>   ahci_platform: convert to module_platform_driver
>   ahci_platform: perform platform exit in host_stop() hook
> 
>  drivers/ata/ahci_platform.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC v2 3/3] ahci_platform: perform platform exit in host_stop() hook

[Tejun Heo]

On Sat, Oct 27, 2012 at 01:09:36PM -0700, Brian Norris wrote:
> AHCI platform devices may provide an exit() routine, via
> ahci_platform_data, that powers off the SATA core. Such a routine should
> be executed from the ata_port_operations host_stop() hook. That way, the
> ATA subsystem can perform any last-minute hardware cleanup (via devres,
> for example), then trigger the power-off at the appropriate time.
> 
> This patch fixes bus errors triggered during module removal or device
> unbinding, seen on an SoC SATA core.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

For all three patches,

  Acked-by: Tejun Heo <tj@kernel.org>

If you have some time, it would be nice to introduce
ata_platform_remove_one().  There's no reason to have that implemented
separately in each driver.  It would also be nice to move
remove_one()'s to some higher level port_ops so that individual
drivers don't have to specify them explicitly.

Thanks.

-- 
tejun

Re: [RFC v2 3/3] ahci_platform: perform platform exit in host_stop() hook

[Brian Norris]

Hi Tejun,

On Sun, Oct 28, 2012 at 6:37 PM, Tejun Heo <tj@kernel.org> wrote:
> On Sat, Oct 27, 2012 at 01:09:36PM -0700, Brian Norris wrote:
>> AHCI platform devices may provide an exit() routine, via
>> ahci_platform_data, that powers off the SATA core. Such a routine should
>> be executed from the ata_port_operations host_stop() hook. That way, the
>> ATA subsystem can perform any last-minute hardware cleanup (via devres,
>> for example), then trigger the power-off at the appropriate time.
>>
>> This patch fixes bus errors triggered during module removal or device
>> unbinding, seen on an SoC SATA core.
>>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>
> For all three patches,
>
>   Acked-by: Tejun Heo <tj@kernel.org>

Thanks for the help and the Acked-by. Unfortunately, I embarrassed to
say that I was basing this on an old libata-dev/NEXT branch.
Apparently I had an old github URL still (I thought there was
something fishy... I'd recommend either updating or removing the
github, FWIW):

git://github.com/jgarzik/libata-dev.git

And my test system was actually a 3.3 kernel. So, this patch doesn't
apply to the *real* libata-dev/NEXT. I will rebase and resubmit,
accounting for:

   commit f1e70c2c535923de253eea2021376a936eb8d478
   ata/ahci_platform: Add clock framework support

Incidentally, this fix is probably helpful for my SoC.

> If you have some time, it would be nice to introduce
> ata_platform_remove_one().  There's no reason to have that implemented
> separately in each driver.

OK, I think I have a pretty good set of patches. I have about 8
drivers switched over to a new ata_platform_remove_one(). Should I
submit it with my resend of this series?

>  It would also be nice to move
> remove_one()'s to some higher level port_ops so that individual
> drivers don't have to specify them explicitly.

Hmm, which port op would you recommend? host_stop()? Or a new remove_one() op?

Brian

P.S. I'm seeing some more issues in libata that I still need to dig
further into. When removing or unbinding, libata cannot stop the
driver properly (gets a "START_STOP FAILED" message from the SCSI
subsystem). The problem lies in the fact that ata_dev_disable() is
being called before sd_start_stop_device(), causing the failure. If
this rings a bell as to an obvious issue, let me know... Thanks.

Re: [RFC v2 3/3] ahci_platform: perform platform exit in host_stop() hook

[Tejun Heo]

Hello, Brian.

On Wed, Oct 31, 2012 at 11:41:45PM -0700, Brian Norris wrote:
> > If you have some time, it would be nice to introduce
> > ata_platform_remove_one().  There's no reason to have that implemented
> > separately in each driver.
> 
> OK, I think I have a pretty good set of patches. I have about 8
> drivers switched over to a new ata_platform_remove_one(). Should I
> submit it with my resend of this series?

I think both ways should be fine.

> >  It would also be nice to move
> > remove_one()'s to some higher level port_ops so that individual
> > drivers don't have to specify them explicitly.
> 
> Hmm, which port op would you recommend? host_stop()? Or a new remove_one() op?

Heh, that was me being confused.  Please disregard.  I for some reason
thought .remove is an ata operation.  :)

Thanks.

-- 
tejun