[PATCH v4] ahci: libahci: clear pending interrupt status

[PATCH v4] ahci: libahci: clear pending interrupt status

[Szuying Chen]

When a CRC error occurs, the HBA asserts an interrupt to indicate an
interface fatal error(PxIS.IFS). The ISR clear PxIE and PxIS, then do
error recovery. Before recovery process, HBA receives another SDB FIS
with the error(PxIS.TFES) from device. This can't be serviced due to
PxIE be cleared already. During error recovery process, the HBA can't
issue any new command after setting PxCMD.ST to 1 due to PxIS.TFES
still alive.

According to AHCI 1.3.1 - section 6.2.2 specification. Fatal errors
(signified by the setting of PxIS.HBFS, PxIS.HBDS, PxIS.IFS or PxIS.TFES)
will cause the HBA to enter the ERR:Fatal state. In this state, the HBA
shall not issue any new commands.

To avoid this, introduce the function ahci_port_clear_pending_irq()
to clear pending interrupts before executing a COMRESET. This follows
the AHCI 1.3.1 - section 6.2.2.2 specification.

Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
---
V1->V2: On suggestion by Damien to renamed helper function and modified 
ahci_port_init() to make use of the helper.
V2->V3: On suggestion by Niklas to modify commit log and delete the extra 
describe.
V3->V4: On suggestion by Damien to modify problem statement on commit log.

 drivers/ata/libahci.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index e2bacedf28ef..f1263364fa97 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1256,6 +1256,26 @@ static ssize_t ahci_activity_show(struct ata_device *dev, char *buf)
 	return sprintf(buf, "%d\n", emp->blink_policy);
 }

+static void ahci_port_clear_pending_irq(struct ata_port *ap)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u32 tmp;
+
+	/* clear SError */
+	tmp = readl(port_mmio + PORT_SCR_ERR);
+	dev_dbg(ap->host->dev, "PORT_SCR_ERR 0x%x\n", tmp);
+	writel(tmp, port_mmio + PORT_SCR_ERR);
+
+	/* clear port IRQ */
+	tmp = readl(port_mmio + PORT_IRQ_STAT);
+	dev_dbg(ap->host->dev, "PORT_IRQ_STAT 0x%x\n", tmp);
+	if (tmp)
+		writel(tmp, port_mmio + PORT_IRQ_STAT);
+
+	writel(1 << ap->port_no, hpriv->mmio + HOST_IRQ_STAT);
+}
+
 static void ahci_port_init(struct device *dev, struct ata_port *ap,
 			   int port_no, void __iomem *mmio,
 			   void __iomem *port_mmio)
@@ -1270,18 +1290,7 @@ static void ahci_port_init(struct device *dev, struct ata_port *ap,
 	if (rc)
 		dev_warn(dev, "%s (%d)\n", emsg, rc);

-	/* clear SError */
-	tmp = readl(port_mmio + PORT_SCR_ERR);
-	dev_dbg(dev, "PORT_SCR_ERR 0x%x\n", tmp);
-	writel(tmp, port_mmio + PORT_SCR_ERR);
-
-	/* clear port IRQ */
-	tmp = readl(port_mmio + PORT_IRQ_STAT);
-	dev_dbg(dev, "PORT_IRQ_STAT 0x%x\n", tmp);
-	if (tmp)
-		writel(tmp, port_mmio + PORT_IRQ_STAT);
-
-	writel(1 << port_no, mmio + HOST_IRQ_STAT);
+	ahci_port_clear_pending_irq(ap);

 	/* mark esata ports */
 	tmp = readl(port_mmio + PORT_CMD);
@@ -1603,6 +1612,8 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
 	tf.status = ATA_BUSY;
 	ata_tf_to_fis(&tf, 0, 0, d2h_fis);

+	ahci_port_clear_pending_irq(ap);
+
 	rc = sata_link_hardreset(link, timing, deadline, online,
 				 ahci_check_ready);

--
2.39.2

Re: [PATCH v4] ahci: libahci: clear pending interrupt status

[Niklas Cassel]

On Thu, Sep 07, 2023 at 04:17:10PM +0800, Szuying Chen wrote:
> When a CRC error occurs, the HBA asserts an interrupt to indicate an
> interface fatal error(PxIS.IFS). The ISR clear PxIE and PxIS, then do
> error recovery. Before recovery process, HBA receives another SDB FIS
> with the error(PxIS.TFES) from device. This can't be serviced due to
> PxIE be cleared already. During error recovery process, the HBA can't
> issue any new command after setting PxCMD.ST to 1 due to PxIS.TFES
> still alive.
> 
> According to AHCI 1.3.1 - section 6.2.2 specification. Fatal errors
> (signified by the setting of PxIS.HBFS, PxIS.HBDS, PxIS.IFS or PxIS.TFES)
> will cause the HBA to enter the ERR:Fatal state. In this state, the HBA
> shall not issue any new commands.
> 
> To avoid this, introduce the function ahci_port_clear_pending_irq()
> to clear pending interrupts before executing a COMRESET. This follows
> the AHCI 1.3.1 - section 6.2.2.2 specification.
> 
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> ---
> V1->V2: On suggestion by Damien to renamed helper function and modified 
> ahci_port_init() to make use of the helper.
> V2->V3: On suggestion by Niklas to modify commit log and delete the extra 
> describe.
> V3->V4: On suggestion by Damien to modify problem statement on commit log.
> 
>  drivers/ata/libahci.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index e2bacedf28ef..f1263364fa97 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1256,6 +1256,26 @@ static ssize_t ahci_activity_show(struct ata_device *dev, char *buf)
>  	return sprintf(buf, "%d\n", emp->blink_policy);
>  }
> 
> +static void ahci_port_clear_pending_irq(struct ata_port *ap)
> +{
> +	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	void __iomem *port_mmio = ahci_port_base(ap);
> +	u32 tmp;
> +
> +	/* clear SError */
> +	tmp = readl(port_mmio + PORT_SCR_ERR);
> +	dev_dbg(ap->host->dev, "PORT_SCR_ERR 0x%x\n", tmp);
> +	writel(tmp, port_mmio + PORT_SCR_ERR);
> +
> +	/* clear port IRQ */
> +	tmp = readl(port_mmio + PORT_IRQ_STAT);
> +	dev_dbg(ap->host->dev, "PORT_IRQ_STAT 0x%x\n", tmp);
> +	if (tmp)
> +		writel(tmp, port_mmio + PORT_IRQ_STAT);
> +
> +	writel(1 << ap->port_no, hpriv->mmio + HOST_IRQ_STAT);
> +}
> +
>  static void ahci_port_init(struct device *dev, struct ata_port *ap,
>  			   int port_no, void __iomem *mmio,
>  			   void __iomem *port_mmio)
> @@ -1270,18 +1290,7 @@ static void ahci_port_init(struct device *dev, struct ata_port *ap,
>  	if (rc)
>  		dev_warn(dev, "%s (%d)\n", emsg, rc);
> 
> -	/* clear SError */
> -	tmp = readl(port_mmio + PORT_SCR_ERR);
> -	dev_dbg(dev, "PORT_SCR_ERR 0x%x\n", tmp);
> -	writel(tmp, port_mmio + PORT_SCR_ERR);
> -
> -	/* clear port IRQ */
> -	tmp = readl(port_mmio + PORT_IRQ_STAT);
> -	dev_dbg(dev, "PORT_IRQ_STAT 0x%x\n", tmp);
> -	if (tmp)
> -		writel(tmp, port_mmio + PORT_IRQ_STAT);
> -
> -	writel(1 << port_no, mmio + HOST_IRQ_STAT);
> +	ahci_port_clear_pending_irq(ap);
> 
>  	/* mark esata ports */
>  	tmp = readl(port_mmio + PORT_CMD);
> @@ -1603,6 +1612,8 @@ int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
>  	tf.status = ATA_BUSY;
>  	ata_tf_to_fis(&tf, 0, 0, d2h_fis);
> 
> +	ahci_port_clear_pending_irq(ap);
> +
>  	rc = sata_link_hardreset(link, timing, deadline, online,
>  				 ahci_check_ready);
> 
> --
> 2.39.2
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v4] ahci: libahci: clear pending interrupt status

[Damien Le Moal]

On 9/7/23 17:17, Szuying Chen wrote:
> When a CRC error occurs, the HBA asserts an interrupt to indicate an
> interface fatal error(PxIS.IFS). The ISR clear PxIE and PxIS, then do
> error recovery. Before recovery process, HBA receives another SDB FIS
> with the error(PxIS.TFES) from device. This can't be serviced due to
> PxIE be cleared already. During error recovery process, the HBA can't
> issue any new command after setting PxCMD.ST to 1 due to PxIS.TFES
> still alive.
> 
> According to AHCI 1.3.1 - section 6.2.2 specification. Fatal errors
> (signified by the setting of PxIS.HBFS, PxIS.HBDS, PxIS.IFS or PxIS.TFES)
> will cause the HBA to enter the ERR:Fatal state. In this state, the HBA
> shall not issue any new commands.
> 
> To avoid this, introduce the function ahci_port_clear_pending_irq()
> to clear pending interrupts before executing a COMRESET. This follows
> the AHCI 1.3.1 - section 6.2.2.2 specification.
> 
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>

Applied to for-6.6-fixes with some tweaks to the commit message.
Thanks !

-- 
Damien Le Moal
Western Digital Research