[PATCH] ata: ahci: Add shutdown to freeze hardware resources of ahci

[PATCH] ata: ahci: Add shutdown to freeze hardware resources of ahci

[Prabhakar Kushwaha]

device_shutdown() called from reboot/power_shutdown expect all
devices to be shutdown. Same is true for ahci pci driver.
As no shutdown function was implemented ata subsystem remains
always alive and DMA/interrupt still active.

It creates problem during kexec, here "M" bit is cleared to stop
DMA usage. Any further DMA transaction may cause instability and
the hard-disk may even not get detected for second kernel.
One of possible case is periodic file system sync.

So defining ahci pci driver shutdown to freeze hardware (mask
interrupt, stop DMA engine and free DMA resources).

Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
---
 drivers/ata/ahci.c        |  8 ++++++++
 drivers/ata/libata-core.c | 21 +++++++++++++++++++++
 include/linux/libata.h    |  1 +
 3 files changed, 30 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 4bfd1b14b390..31fc934740b6 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -81,6 +81,7 @@ enum board_ids {
 
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
 static void ahci_remove_one(struct pci_dev *dev);
+static void ahci_shutdown_one(struct pci_dev *dev);
 static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
 				 unsigned long deadline);
 static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
@@ -606,6 +607,7 @@ static struct pci_driver ahci_pci_driver = {
 	.id_table		= ahci_pci_tbl,
 	.probe			= ahci_init_one,
 	.remove			= ahci_remove_one,
+	.shutdown		= ahci_shutdown_one,
 	.driver = {
 		.pm		= &ahci_pci_pm_ops,
 	},
@@ -626,6 +628,7 @@ MODULE_PARM_DESC(mobile_lpm_policy, "Default LPM policy for mobile chipsets");
 static void ahci_pci_save_initial_config(struct pci_dev *pdev,
 					 struct ahci_host_priv *hpriv)
 {
+
 	if (pdev->vendor == PCI_VENDOR_ID_JMICRON && pdev->device == 0x2361) {
 		dev_info(&pdev->dev, "JMB361 has only one port\n");
 		hpriv->force_port_map = 1;
@@ -1877,6 +1880,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return 0;
 }
 
+static void ahci_shutdown_one(struct pci_dev *pdev)
+{
+	ata_pci_shutdown_one(pdev);
+}
+
 static void ahci_remove_one(struct pci_dev *pdev)
 {
 	pm_runtime_get_noresume(&pdev->dev);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6f4ab5c5b52d..42c8728f6117 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6767,6 +6767,26 @@ void ata_pci_remove_one(struct pci_dev *pdev)
 	ata_host_detach(host);
 }
 
+void ata_pci_shutdown_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = pci_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		ap->pflags |= ATA_PFLAG_FROZEN;
+
+		/* Disable port interrupts */
+		if (ap->ops->freeze)
+			ap->ops->freeze(ap);
+
+		/* Stop the port DMA engines */
+		if (ap->ops->port_stop)
+			ap->ops->port_stop(ap);
+	}
+}
+
 /* move to PCI subsystem */
 int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits)
 {
@@ -7387,6 +7407,7 @@ EXPORT_SYMBOL_GPL(ata_timing_cycle2mode);
 
 #ifdef CONFIG_PCI
 EXPORT_SYMBOL_GPL(pci_test_config_bits);
+EXPORT_SYMBOL_GPL(ata_pci_shutdown_one);
 EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 #ifdef CONFIG_PM
 EXPORT_SYMBOL_GPL(ata_pci_device_do_suspend);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2dbde119721d..bff539918d82 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1221,6 +1221,7 @@ struct pci_bits {
 };
 
 extern int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits);
+extern void ata_pci_shutdown_one(struct pci_dev *pdev);
 extern void ata_pci_remove_one(struct pci_dev *pdev);
 
 #ifdef CONFIG_PM
-- 
2.17.1

Re: [PATCH] ata: ahci: Add shutdown to freeze hardware resources of ahci

[Bjorn Helgaas]

On Thu, Jan 23, 2020 at 10:41:57AM +0000, Prabhakar Kushwaha wrote:
> device_shutdown() called from reboot/power_shutdown expect all
> devices to be shutdown. Same is true for ahci pci driver.
> As no shutdown function was implemented ata subsystem remains
> always alive and DMA/interrupt still active.
> 
> It creates problem during kexec, here "M" bit is cleared to stop
> DMA usage.

Maybe this is obvious to AHCI folks, but I don't know what "M" bit you
are referring to, and I don't see anything in the patch itself that
looks like an "M" bit.  I thought maybe you meant the "Bus Master
Enable" bit (PCI_COMMAND_MASTER), but I don't see an obvious
connection to that either.

> Any further DMA transaction may cause instability and
> the hard-disk may even not get detected for second kernel.
> One of possible case is periodic file system sync.

I think "may cause instability" and "disk may even not get detected"
is too vague and hand-wavy to really add useful information to the
commit log.

> So defining ahci pci driver shutdown to freeze hardware (mask
> interrupt, stop DMA engine and free DMA resources).
> 
> Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
> ---
>  drivers/ata/ahci.c        |  8 ++++++++
>  drivers/ata/libata-core.c | 21 +++++++++++++++++++++
>  include/linux/libata.h    |  1 +
>  3 files changed, 30 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 4bfd1b14b390..31fc934740b6 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -81,6 +81,7 @@ enum board_ids {
>  
>  static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);
>  static void ahci_remove_one(struct pci_dev *dev);
> +static void ahci_shutdown_one(struct pci_dev *dev);
>  static int ahci_vt8251_hardreset(struct ata_link *link, unsigned int *class,
>  				 unsigned long deadline);
>  static int ahci_avn_hardreset(struct ata_link *link, unsigned int *class,
> @@ -606,6 +607,7 @@ static struct pci_driver ahci_pci_driver = {
>  	.id_table		= ahci_pci_tbl,
>  	.probe			= ahci_init_one,
>  	.remove			= ahci_remove_one,
> +	.shutdown		= ahci_shutdown_one,
>  	.driver = {
>  		.pm		= &ahci_pci_pm_ops,
>  	},
> @@ -626,6 +628,7 @@ MODULE_PARM_DESC(mobile_lpm_policy, "Default LPM policy for mobile chipsets");
>  static void ahci_pci_save_initial_config(struct pci_dev *pdev,
>  					 struct ahci_host_priv *hpriv)
>  {
> +

Spurious whitespace change, please remove.

>  	if (pdev->vendor == PCI_VENDOR_ID_JMICRON && pdev->device == 0x2361) {
>  		dev_info(&pdev->dev, "JMB361 has only one port\n");
>  		hpriv->force_port_map = 1;
> @@ -1877,6 +1880,11 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	return 0;
>  }
>  
> +static void ahci_shutdown_one(struct pci_dev *pdev)
> +{
> +	ata_pci_shutdown_one(pdev);
> +}
> +
>  static void ahci_remove_one(struct pci_dev *pdev)
>  {
>  	pm_runtime_get_noresume(&pdev->dev);
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 6f4ab5c5b52d..42c8728f6117 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6767,6 +6767,26 @@ void ata_pci_remove_one(struct pci_dev *pdev)
>  	ata_host_detach(host);
>  }
>  
> +void ata_pci_shutdown_one(struct pci_dev *pdev)
> +{
> +	struct ata_host *host = pci_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < host->n_ports; i++) {
> +		struct ata_port *ap = host->ports[i];
> +
> +		ap->pflags |= ATA_PFLAG_FROZEN;
> +
> +		/* Disable port interrupts */
> +		if (ap->ops->freeze)
> +			ap->ops->freeze(ap);
> +
> +		/* Stop the port DMA engines */
> +		if (ap->ops->port_stop)
> +			ap->ops->port_stop(ap);
> +	}
> +}
> +
>  /* move to PCI subsystem */
>  int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits)
>  {
> @@ -7387,6 +7407,7 @@ EXPORT_SYMBOL_GPL(ata_timing_cycle2mode);
>  
>  #ifdef CONFIG_PCI
>  EXPORT_SYMBOL_GPL(pci_test_config_bits);
> +EXPORT_SYMBOL_GPL(ata_pci_shutdown_one);
>  EXPORT_SYMBOL_GPL(ata_pci_remove_one);
>  #ifdef CONFIG_PM
>  EXPORT_SYMBOL_GPL(ata_pci_device_do_suspend);
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 2dbde119721d..bff539918d82 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1221,6 +1221,7 @@ struct pci_bits {
>  };
>  
>  extern int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits);
> +extern void ata_pci_shutdown_one(struct pci_dev *pdev);
>  extern void ata_pci_remove_one(struct pci_dev *pdev);
>  
>  #ifdef CONFIG_PM
> -- 
> 2.17.1
> 

Re: [PATCH] ata: ahci: Add shutdown to freeze hardware resources of ahci

[Bjorn Helgaas]

[FYI, your reply was a multipart message (not a simple plain text
message), so I think the mailing lists discarded it and it doesn't
appear in the archives]

On Thu, Jan 23, 2020 at 08:05:04PM +0530, Prabhakar Kushwaha wrote:
> Thanks Bjorn for review. Reply is in-lined.
> 
> On Thu, Jan 23, 2020 at 7:24 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > On Thu, Jan 23, 2020 at 10:41:57AM +0000, Prabhakar Kushwaha wrote:
> > > device_shutdown() called from reboot/power_shutdown expect all
> > > devices to be shutdown. Same is true for ahci pci driver.
> > > As no shutdown function was implemented ata subsystem remains
> > > always alive and DMA/interrupt still active.
> > >
> > > It creates problem during kexec, here "M" bit is cleared to stop
> > > DMA usage.
> >
> > Maybe this is obvious to AHCI folks, but I don't know what "M" bit you
> > are referring to, and I don't see anything in the patch itself that
> > looks like an "M" bit.  I thought maybe you meant the "Bus Master
> > Enable" bit (PCI_COMMAND_MASTER), but I don't see an obvious
> > connection to that either.
> >
> I missed to explain M bit. Thanks for pointing it out.
> M bit is PCI_COMMAND_MASTER bit i.e. Bus Master Enable.

Please don't call it the "M" bit.  Neither the Conventional PCI nor
the PCIe specs call it that, and it could just as easily refer to the
"Memory Space Enable" bit.  Just call it the "Bus Master Enable" bit
like the specs do.

> There are 2 flow in kernel which calls device_shutdown() which inherently
> call pci_device_shutdown()().
> a) reboot flow
> b) kexec -e flow
> 
> issue seen in kexec -e flow where hard-disk is not getting detected in
> second kernel.
> There is special code in pci_device_shutdown() which clear M bit during
> kexec -e i.e. kexec_in_progress flag is set.
> 
> if (kexec_in_progress && (pci_dev->current_state <= PCI_D3hot))
>                 pci_clear_master(pci_dev);
> 
> There should not be any transaction after pci_clear_master() but i am
> seeing filesystem sync function calls after this.
> this is the reason shutdown function is added(this patch) which disable
> interrupt, stop DMA and freeze SATA port before execution of
> pci_clear_master()

I don't see where your patch adds anything that clears Bus Master
Enable.  You're adding ahci_shutdown_one(), so pci_device_shutdown()
will now call it, and maybe something inside ahci_shutdown_one() will
clear Bus Master Enable, but I couldn't find it with a quick look.

pci_device_shutdown() *does* clear it, of course, but only when
kexec_in_progress, and it does that even without this patch.

Just to be clear, I don't object to the patch itself -- that's an AHCI
thing that I don't know much about.  I'm just complaining because it's
not obvious that your commit log describes what the patch actually
does.

ahci_shutdown_one() might indeed stop the AHCI port DMA, but I suspect
it's doing that via the AHCI programming model, not by clearing Bus
Master Enable.  So maybe all you need to do is remove the references
to BME.

> After adding shutdown function, i can see hard-disk getting detected in
> second kernel.
> 
> > > Any further DMA transaction may cause instability and
> > > the hard-disk may even not get detected for second kernel.
> > > One of possible case is periodic file system sync.
> >
> > I think "may cause instability" and "disk may even not get detected"
> > is too vague and hand-wavy to really add useful information to the
> > commit log.
> >
> let me rework on this.. Thanks
> 
> > > So defining ahci pci driver shutdown to freeze hardware (mask
> > > interrupt, stop DMA engine and free DMA resources).