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

login
register
mail settings
Submitter Brian Norris
Date Oct. 27, 2012, 8:09 p.m.
Message ID <1351368576-5264-4-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/194631/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Brian Norris - Oct. 27, 2012, 8:09 p.m.
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(-)
Tejun Heo - Oct. 29, 2012, 1:37 a.m.
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.
Brian Norris - Nov. 1, 2012, 6:41 a.m.
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.
--
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 - Nov. 1, 2012, 4:17 p.m.
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.

Patch

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