[RFC 0/3] libata/ahci unbinding, power down sequence

[RFC 0/3] libata/ahci unbinding, power down sequence

[Brian Norris]

Hello,

I sent out a few questions/suggestions earlier, regarding some trouble I was
having in supporting my SoC SATA controller under the libata / ahci_platform
driver:

    http://article.gmane.org/gmane.linux.ide/52951/match=unbind+rmmod

I think I have identified a problem in libata's handling of hardware activity
at device removal/exit (relevant to my use-case of unbind or rmmod). This patch 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) Modifies libata-core so that ata_host_stop() is not called as a devres
    cleanup function (after all driver-specific functions, including power-off)

The first two patches are relatively harmless and correct, I believe, where the
third patch is more significant and could use good review. I am sending code
because I hear no response to my standalone suggestions :) Please see my
previous email (URL above) and/or patch 3/3 for a more detailed description.

Thanks,
Brian

Brian Norris (3):
  ahci_platform: enable hotplug unbinding
  ahci_platform: convert to module_platform_driver
  libata: don't perform HW activity in devres

 drivers/ata/ahci_platform.c | 16 +++-------------
 drivers/ata/libata-core.c   | 18 +++---------------
 2 files changed, 6 insertions(+), 28 deletions(-)

-- 
1.7.11.3

[RFC 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 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 3/3] libata: don't perform HW activity in devres

[Brian Norris]

devres functions are intended for simplified cleanup of memory and other
software resources on device exit, not for hardware shutdown sequences.
In addition, inducing hardware activity at device removal hamstrings
some drivers (particularly ahci_platform) so that they cannot totally
power off their hardware before removal, as devres cleanup occurs after
the driver's exit() sequence.

More concretely, I experience the following bus error when using rmmod
to remove (and power off) the SATA block on my SoC:

    # rmmod ahci_platform.ko
    Data bus error, epc == e07c0ca0, ra == e07c0d24
    Oops[#1]:
    ...
    Call Trace:
    [<e07c0ca0>] ahci_stop_engine+0x28/0x84 [libahci]
    [<e07c0d24>] ahci_deinit_port+0x28/0xe8 [libahci]
    [<e07c0e08>] ahci_port_stop+0x24/0x64 [libahci]
    [<802dcc28>] ata_host_stop+0x5c/0xc0
    [<802b5390>] release_nodes+0x144/0x244
    [<802b159c>] __device_release_driver+0x68/0xcc
    [<802b1fd8>] driver_detach+0xe8/0xf0
    [<802b13e0>] bus_remove_driver+0x98/0x128
    [<8007b9e4>] sys_delete_module+0x188/0x2d4
    [<8000e6fc>] stack_done+0x20/0x40

This hardware activity pattern seems wrong.

Therefore, I move ata_host_stop() to simply be called as part of the
ata_host_detach() sequence already performed by all SATA drivers at
device exit.

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

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d31ee55..85dee79 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5772,9 +5772,8 @@ int ata_slave_link_init(struct ata_port *ap)
 	return 0;
 }
 
-static void ata_host_stop(struct device *gendev, void *res)
+static void ata_host_stop(struct ata_host *host)
 {
-	struct ata_host *host = dev_get_drvdata(gendev);
 	int i;
 
 	WARN_ON(!(host->flags & ATA_HOST_STARTED));
@@ -5858,7 +5857,6 @@ static void ata_finalize_port_ops(struct ata_port_operations *ops)
  */
 int ata_host_start(struct ata_host *host)
 {
-	int have_stop = 0;
 	void *start_dr = NULL;
 	int i, rc;
 
@@ -5874,18 +5872,6 @@ int ata_host_start(struct ata_host *host)
 
 		if (!host->ops && !ata_port_is_dummy(ap))
 			host->ops = ap->ops;
-
-		if (ap->ops->port_stop)
-			have_stop = 1;
-	}
-
-	if (host->ops->host_stop)
-		have_stop = 1;
-
-	if (have_stop) {
-		start_dr = devres_alloc(ata_host_stop, 0, GFP_KERNEL);
-		if (!start_dr)
-			return -ENOMEM;
 	}
 
 	for (i = 0; i < host->n_ports; i++) {
@@ -6214,6 +6200,8 @@ void ata_host_detach(struct ata_host *host)
 
 	/* the host is dead now, dissociate ACPI */
 	ata_acpi_dissociate(host);
+
+	ata_host_stop(host);
 }
 
 #ifdef CONFIG_PCI
-- 
1.7.11.3

Re: [RFC 3/3] libata: don't perform HW activity in devres

[Tejun Heo]

On Thu, Oct 25, 2012 at 09:56:01AM -0700, Brian Norris wrote:
> devres functions are intended for simplified cleanup of memory and other
> software resources on device exit, not for hardware shutdown sequences.
> In addition, inducing hardware activity at device removal hamstrings
> some drivers (particularly ahci_platform) so that they cannot totally
> power off their hardware before removal, as devres cleanup occurs after
> the driver's exit() sequence.
> 
> More concretely, I experience the following bus error when using rmmod
> to remove (and power off) the SATA block on my SoC:

Shouldn't poweroff happen from ->port/host_stop()?

Thanks.

-- 
tejun

Re: [RFC 3/3] libata: don't perform HW activity in devres

[Brian Norris]

On Thu, Oct 25, 2012 at 10:25 AM, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Oct 25, 2012 at 09:56:01AM -0700, Brian Norris wrote:
>> devres functions are intended for simplified cleanup of memory and other
>> software resources on device exit, not for hardware shutdown sequences.
>> In addition, inducing hardware activity at device removal hamstrings
>> some drivers (particularly ahci_platform) so that they cannot totally
>> power off their hardware before removal, as devres cleanup occurs after
>> the driver's exit() sequence.
>>
>> More concretely, I experience the following bus error when using rmmod
>> to remove (and power off) the SATA block on my SoC:
>
> Shouldn't poweroff happen from ->port/host_stop()?

Hmm, I guess that makes more sense. I was using the ahci_platform
ahci_platform_data->exit() function. Would it be safe to call the
platform init()/exit() functions as part of a
ata_port_operations.host_{start,stop}() hook? These functions aren't
currently implemented at all in ahci_platform, but I don't see why
they couldn't be.

Brian

Re: [RFC 3/3] libata: don't perform HW activity in devres

[Tejun Heo]

Hello,

On Thu, Oct 25, 2012 at 10:41:57AM -0700, Brian Norris wrote:
> Hmm, I guess that makes more sense. I was using the ahci_platform
> ahci_platform_data->exit() function. Would it be safe to call the
> platform init()/exit() functions as part of a
> ata_port_operations.host_{start,stop}() hook? These functions aren't
> currently implemented at all in ahci_platform, but I don't see why
> they couldn't be.

I don't have much idea about the specifics but host_start/stop() are
supposed to perform the functions you're describing.

Thanks.

-- 
tejun