| Message ID | 1351368576-5264-4-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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.
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
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(-)