[PATCH 0/3] ata: ahci_brcm: Recover from failures to identify devices

[PATCH 0/3] ata: ahci_brcm: Recover from failures to identify devices

[Florian Fainelli]

Hi Tejun, Kishon,

This patch series implement a recovery mechanism to work around a HW bug
on Broadcom AHCI SATA controller subject to noise triggering a failure to
identify hard drives.

I would like to make this this is okay with you as an approach on how to solve
this.

Thanks!

Florian Fainelli (3):
  ata: Allow having a port recovery callback
  phy: brcm-sata: Implementation calibrate callback
  ata: ahci_brcm: Recover from failures to identify devices

 drivers/ata/ahci_brcm.c              | 83 ++++++++++++++++++++++++++++++++----
 drivers/ata/libata-core.c            |  2 +
 drivers/phy/broadcom/phy-brcm-sata.c | 32 ++++++++++++++
 include/linux/libata.h               |  1 +
 4 files changed, 110 insertions(+), 8 deletions(-)

-- 
2.7.4

[PATCH 0/3] ata: ahci_brcm: Recover from failures to identify devices

[Florian Fainelli]

Hi Tejun, Kishon,

This patch series implement a recovery mechanism to work around a HW bug
on Broadcom AHCI SATA controller subject to noise triggering a failure to
identify hard drives.

I would like to make this this is okay with you as an approach on how to solve
this.

Thanks!

Florian Fainelli (3):
  ata: Allow having a port recovery callback
  phy: brcm-sata: Implementation calibrate callback
  ata: ahci_brcm: Recover from failures to identify devices

 drivers/ata/ahci_brcm.c              | 83 ++++++++++++++++++++++++++++++++----
 drivers/ata/libata-core.c            |  2 +
 drivers/phy/broadcom/phy-brcm-sata.c | 32 ++++++++++++++
 include/linux/libata.h               |  1 +
 4 files changed, 110 insertions(+), 8 deletions(-)

-- 
2.7.4

[PATCH 1/3] ata: Allow having a port recovery callback

[Florian Fainelli]

Some AHCI controllers may need to recovery from a failure to identify a drive,
such as the Broadcom AHCI controller, make that possible at both the
libata-core.c and AHCI controller level such that we can provide such an error
recovery callback.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/ata/libata-core.c | 2 ++
 include/linux/libata.h    | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3c09122bf038..921c2813af07 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2045,6 +2045,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	if (ata_msg_warn(ap))
 		ata_dev_warn(dev, "failed to IDENTIFY (%s, err_mask=0x%x)\n",
 			     reason, err_mask);
+	if (ap->host->ops->port_recovery)
+		ap->host->ops->port_recovery(ap);
 	return rc;
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index ed9826b21c5e..23646a557ab2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -956,6 +956,7 @@ struct ata_port_operations {
 	int  (*port_resume)(struct ata_port *ap);
 	int  (*port_start)(struct ata_port *ap);
 	void (*port_stop)(struct ata_port *ap);
+	void (*port_recovery)(struct ata_port *ap);
 	void (*host_stop)(struct ata_host *host);
 
 #ifdef CONFIG_ATA_SFF
-- 
2.7.4

[PATCH 1/3] ata: Allow having a port recovery callback

[Florian Fainelli]

Some AHCI controllers may need to recovery from a failure to identify a drive,
such as the Broadcom AHCI controller, make that possible at both the
libata-core.c and AHCI controller level such that we can provide such an error
recovery callback.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/ata/libata-core.c | 2 ++
 include/linux/libata.h    | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3c09122bf038..921c2813af07 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2045,6 +2045,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
 	if (ata_msg_warn(ap))
 		ata_dev_warn(dev, "failed to IDENTIFY (%s, err_mask=0x%x)\n",
 			     reason, err_mask);
+	if (ap->host->ops->port_recovery)
+		ap->host->ops->port_recovery(ap);
 	return rc;
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index ed9826b21c5e..23646a557ab2 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -956,6 +956,7 @@ struct ata_port_operations {
 	int  (*port_resume)(struct ata_port *ap);
 	int  (*port_start)(struct ata_port *ap);
 	void (*port_stop)(struct ata_port *ap);
+	void (*port_recovery)(struct ata_port *ap);
 	void (*host_stop)(struct ata_host *host);
 
 #ifdef CONFIG_ATA_SFF
-- 
2.7.4

[PATCH 2/3] phy: brcm-sata: Implementation calibrate callback

[Florian Fainelli]

Implement the calibration callback to allow turning on the Clock-Data Recovery
module clamping when necessary.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/phy/broadcom/phy-brcm-sata.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/phy/broadcom/phy-brcm-sata.c b/drivers/phy/broadcom/phy-brcm-sata.c
index 3f953db70288..8000ce312d95 100644
--- a/drivers/phy/broadcom/phy-brcm-sata.c
+++ b/drivers/phy/broadcom/phy-brcm-sata.c
@@ -150,6 +150,9 @@ enum sata_phy_regs {
 	TXPMD_TX_FREQ_CTRL_CONTROL2_FMIN_MASK	= 0x3ff,
 	TXPMD_TX_FREQ_CTRL_CONTROL3		= 0x84,
 	TXPMD_TX_FREQ_CTRL_CONTROL3_FMAX_MASK	= 0x3ff,
+
+	RXPMD_REG_BANK				= 0x1c0,
+	RXPMD_RX_FREQ_MON_CONTROL1		= 0x87,
 };
 
 enum sata_phy_ctrl_regs {
@@ -505,8 +508,37 @@ static int brcm_sata_phy_init(struct phy *phy)
 	return rc;
 }
 
+static void brcm_stb_sata_calibrate(struct brcm_sata_port *port)
+{
+	void __iomem *base = brcm_sata_pcb_base(port);
+	struct brcm_sata_phy *priv = port->phy_priv;
+	u32 tmp = BIT(8);
+
+	brcm_sata_phy_wr(base, RXPMD_REG_BANK, RXPMD_RX_FREQ_MON_CONTROL1,
+			 ~tmp, tmp);
+}
+
+static int brcm_sata_phy_calibrate(struct phy *phy)
+{
+	struct brcm_sata_port *port = phy_get_drvdata(phy);
+	int rc = -EOPNOTSUPP;
+
+	switch (port->phy_priv->version) {
+	case BRCM_SATA_PHY_STB_28NM:
+	case BRCM_SATA_PHY_STB_40NM:
+		brcm_stb_sata_calibrate(port);
+		rc = 0;
+		break;
+	default:
+		break;
+	};
+
+	return rc;
+}
+
 static const struct phy_ops phy_ops = {
 	.init		= brcm_sata_phy_init,
+	.calibrate	= brcm_sata_phy_calibrate,
 	.owner		= THIS_MODULE,
 };
 
-- 
2.7.4

[PATCH 2/3] phy: brcm-sata: Implementation calibrate callback

[Florian Fainelli]

Implement the calibration callback to allow turning on the Clock-Data Recovery
module clamping when necessary.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/phy/broadcom/phy-brcm-sata.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/phy/broadcom/phy-brcm-sata.c b/drivers/phy/broadcom/phy-brcm-sata.c
index 3f953db70288..8000ce312d95 100644
--- a/drivers/phy/broadcom/phy-brcm-sata.c
+++ b/drivers/phy/broadcom/phy-brcm-sata.c
@@ -150,6 +150,9 @@ enum sata_phy_regs {
 	TXPMD_TX_FREQ_CTRL_CONTROL2_FMIN_MASK	= 0x3ff,
 	TXPMD_TX_FREQ_CTRL_CONTROL3		= 0x84,
 	TXPMD_TX_FREQ_CTRL_CONTROL3_FMAX_MASK	= 0x3ff,
+
+	RXPMD_REG_BANK				= 0x1c0,
+	RXPMD_RX_FREQ_MON_CONTROL1		= 0x87,
 };
 
 enum sata_phy_ctrl_regs {
@@ -505,8 +508,37 @@ static int brcm_sata_phy_init(struct phy *phy)
 	return rc;
 }
 
+static void brcm_stb_sata_calibrate(struct brcm_sata_port *port)
+{
+	void __iomem *base = brcm_sata_pcb_base(port);
+	struct brcm_sata_phy *priv = port->phy_priv;
+	u32 tmp = BIT(8);
+
+	brcm_sata_phy_wr(base, RXPMD_REG_BANK, RXPMD_RX_FREQ_MON_CONTROL1,
+			 ~tmp, tmp);
+}
+
+static int brcm_sata_phy_calibrate(struct phy *phy)
+{
+	struct brcm_sata_port *port = phy_get_drvdata(phy);
+	int rc = -EOPNOTSUPP;
+
+	switch (port->phy_priv->version) {
+	case BRCM_SATA_PHY_STB_28NM:
+	case BRCM_SATA_PHY_STB_40NM:
+		brcm_stb_sata_calibrate(port);
+		rc = 0;
+		break;
+	default:
+		break;
+	};
+
+	return rc;
+}
+
 static const struct phy_ops phy_ops = {
 	.init		= brcm_sata_phy_init,
+	.calibrate	= brcm_sata_phy_calibrate,
 	.owner		= THIS_MODULE,
 };
 
-- 
2.7.4

[PATCH 3/3] ata: ahci_brcm: Recover from failures to identify devices

[Florian Fainelli]

When powering up, the SATA controller may fail to mount the HDD. The SATA
controller will lock up, preventing it from negotiating to a lower speed or
transmitting data. Root cause is power supply noise creating resonance at 6 Ghz
and 3 GHz frequencies, which causes instability in the Clock-Data Recovery
(CDR) frontend module, resulting in false acquisition of the clock at SATA
6G/3G speeds.

The SATA controller may fail to mount the HDD and lock up, requiring a power
cycle. Broadcom chips suspected of being susceptible to this issue include
BCM7445, BCM7439, and BCM7366.

The Kernel implements an error recovery mechanism that resets the SATA PHY and
digital controller when the controller locks up. During this error recovery
process, typically there is less activity on the board and Broadcom STB chip,
so that the power supply is less noisy, thus allowing the SATA controller to
lock correctly.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/ata/ahci_brcm.c | 83 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index ad3b8826ec79..2dfdc2bbe55b 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -96,14 +96,6 @@ struct brcm_ahci_priv {
 	enum brcm_ahci_version version;
 };
 
-static const struct ata_port_info ahci_brcm_port_info = {
-	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
-	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
-	.pio_mask	= ATA_PIO4,
-	.udma_mask	= ATA_UDMA6,
-	.port_ops	= &ahci_platform_ops,
-};
-
 static inline u32 brcm_sata_readreg(void __iomem *addr)
 {
 	/*
@@ -269,6 +261,81 @@ static void brcm_sata_init(struct brcm_ahci_priv *priv)
 	brcm_sata_writereg(data, ctrl);
 }
 
+static void brcm_ahci_port_recovery(struct ata_port *ap)
+{
+	struct ata_host *host = ap->host;
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct brcm_ahci_priv *priv = hpriv->plat_data;
+	void __iomem *mmio = hpriv->mmio;
+	unsigned long flags;
+	int i, rc;
+	u32 ctl;
+
+	/* Disable host interrupts */
+	spin_lock_irqsave(&host->lock, flags);
+	ctl = readl(mmio + HOST_CTL);
+	ctl &= ~HOST_IRQ_EN;
+	writel(ctl, mmio + HOST_CTL);
+	readl(mmio + HOST_CTL); /* flush */
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	/* Perform the SATA PHY reset sequence */
+	brcm_sata_phy_disable(priv, ap->port_no);
+
+	/* Bring the PHY back on */
+	brcm_sata_phy_enable(priv, ap->port_no);
+
+	/* Re-initialize and calibrate the PHY */
+	for (i = 0; i < hpriv->nports; i++) {
+		rc = phy_init(hpriv->phys[i]);
+		if (rc)
+			goto disable_phys;
+
+		rc = phy_calibrate(hpriv->phys[i]);
+		if (rc) {
+			phy_exit(hpriv->phys[i]);
+			goto disable_phys;
+		}
+	}
+
+	/* Re-enable host interrupts */
+	spin_lock_irqsave(&host->lock, flags);
+	ctl = readl(mmio + HOST_CTL);
+	ctl |= HOST_IRQ_EN;
+	writel(ctl, mmio + HOST_CTL);
+	readl(mmio + HOST_CTL); /* flush */
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	return;
+
+disable_phys:
+	while (--i >= 0) {
+		phy_power_off(hpriv->phys[i]);
+		phy_exit(hpriv->phys[i]);
+	}
+}
+
+static void brcm_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	ahci_platform_disable_resources(hpriv);
+}
+
+static struct ata_port_operations ahci_brcm_platform_ops = {
+	.inherits	= &ahci_ops,
+	.host_stop	= brcm_ahci_host_stop,
+	.port_recovery	= brcm_ahci_port_recovery,
+};
+
+static const struct ata_port_info ahci_brcm_port_info = {
+	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
+	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_brcm_platform_ops,
+};
+
 #ifdef CONFIG_PM_SLEEP
 static int brcm_ahci_suspend(struct device *dev)
 {
-- 
2.7.4

[PATCH 3/3] ata: ahci_brcm: Recover from failures to identify devices

[Florian Fainelli]

When powering up, the SATA controller may fail to mount the HDD. The SATA
controller will lock up, preventing it from negotiating to a lower speed or
transmitting data. Root cause is power supply noise creating resonance at 6 Ghz
and 3 GHz frequencies, which causes instability in the Clock-Data Recovery
(CDR) frontend module, resulting in false acquisition of the clock at SATA
6G/3G speeds.

The SATA controller may fail to mount the HDD and lock up, requiring a power
cycle. Broadcom chips suspected of being susceptible to this issue include
BCM7445, BCM7439, and BCM7366.

The Kernel implements an error recovery mechanism that resets the SATA PHY and
digital controller when the controller locks up. During this error recovery
process, typically there is less activity on the board and Broadcom STB chip,
so that the power supply is less noisy, thus allowing the SATA controller to
lock correctly.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/ata/ahci_brcm.c | 83 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index ad3b8826ec79..2dfdc2bbe55b 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -96,14 +96,6 @@ struct brcm_ahci_priv {
 	enum brcm_ahci_version version;
 };
 
-static const struct ata_port_info ahci_brcm_port_info = {
-	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
-	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
-	.pio_mask	= ATA_PIO4,
-	.udma_mask	= ATA_UDMA6,
-	.port_ops	= &ahci_platform_ops,
-};
-
 static inline u32 brcm_sata_readreg(void __iomem *addr)
 {
 	/*
@@ -269,6 +261,81 @@ static void brcm_sata_init(struct brcm_ahci_priv *priv)
 	brcm_sata_writereg(data, ctrl);
 }
 
+static void brcm_ahci_port_recovery(struct ata_port *ap)
+{
+	struct ata_host *host = ap->host;
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct brcm_ahci_priv *priv = hpriv->plat_data;
+	void __iomem *mmio = hpriv->mmio;
+	unsigned long flags;
+	int i, rc;
+	u32 ctl;
+
+	/* Disable host interrupts */
+	spin_lock_irqsave(&host->lock, flags);
+	ctl = readl(mmio + HOST_CTL);
+	ctl &= ~HOST_IRQ_EN;
+	writel(ctl, mmio + HOST_CTL);
+	readl(mmio + HOST_CTL); /* flush */
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	/* Perform the SATA PHY reset sequence */
+	brcm_sata_phy_disable(priv, ap->port_no);
+
+	/* Bring the PHY back on */
+	brcm_sata_phy_enable(priv, ap->port_no);
+
+	/* Re-initialize and calibrate the PHY */
+	for (i = 0; i < hpriv->nports; i++) {
+		rc = phy_init(hpriv->phys[i]);
+		if (rc)
+			goto disable_phys;
+
+		rc = phy_calibrate(hpriv->phys[i]);
+		if (rc) {
+			phy_exit(hpriv->phys[i]);
+			goto disable_phys;
+		}
+	}
+
+	/* Re-enable host interrupts */
+	spin_lock_irqsave(&host->lock, flags);
+	ctl = readl(mmio + HOST_CTL);
+	ctl |= HOST_IRQ_EN;
+	writel(ctl, mmio + HOST_CTL);
+	readl(mmio + HOST_CTL); /* flush */
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	return;
+
+disable_phys:
+	while (--i >= 0) {
+		phy_power_off(hpriv->phys[i]);
+		phy_exit(hpriv->phys[i]);
+	}
+}
+
+static void brcm_ahci_host_stop(struct ata_host *host)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	ahci_platform_disable_resources(hpriv);
+}
+
+static struct ata_port_operations ahci_brcm_platform_ops = {
+	.inherits	= &ahci_ops,
+	.host_stop	= brcm_ahci_host_stop,
+	.port_recovery	= brcm_ahci_port_recovery,
+};
+
+static const struct ata_port_info ahci_brcm_port_info = {
+	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
+	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_brcm_platform_ops,
+};
+
 #ifdef CONFIG_PM_SLEEP
 static int brcm_ahci_suspend(struct device *dev)
 {
-- 
2.7.4

Re: [PATCH 1/3] ata: Allow having a port recovery callback

[Tejun Heo]

Hello,

On Tue, Jan 09, 2018 at 03:04:55PM -0800, Florian Fainelli wrote:
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 3c09122bf038..921c2813af07 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2045,6 +2045,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  	if (ata_msg_warn(ap))
>  		ata_dev_warn(dev, "failed to IDENTIFY (%s, err_mask=0x%x)\n",
>  			     reason, err_mask);
> +	if (ap->host->ops->port_recovery)
> +		ap->host->ops->port_recovery(ap);
>  	return rc;
>  }

This is a really weird spot to add a callback named port_recovery().
Can't the affected driver simply implement its own
ata_port_operations->read_id() operation which does the recovery if
necessary?

Thanks.

-- 
tejun

Re: [PATCH 1/3] ata: Allow having a port recovery callback

[Tejun Heo]

Hello,

On Tue, Jan 09, 2018 at 03:04:55PM -0800, Florian Fainelli wrote:
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 3c09122bf038..921c2813af07 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2045,6 +2045,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>  	if (ata_msg_warn(ap))
>  		ata_dev_warn(dev, "failed to IDENTIFY (%s, err_mask=0x%x)\n",
>  			     reason, err_mask);
> +	if (ap->host->ops->port_recovery)
> +		ap->host->ops->port_recovery(ap);
>  	return rc;
>  }

This is a really weird spot to add a callback named port_recovery().
Can't the affected driver simply implement its own
ata_port_operations->read_id() operation which does the recovery if
necessary?

Thanks.

-- 
tejun

Re: [PATCH 1/3] ata: Allow having a port recovery callback

[Sergei Shtylyov]

Hello!

On 01/10/2018 02:04 AM, Florian Fainelli wrote:

> Some AHCI controllers may need to recovery from a failure to identify a drive,

    Recover.

> such as the Broadcom AHCI controller, make that possible at both the
> libata-core.c and AHCI controller level such that we can provide such an error
> recovery callback.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
[...]

MBR, Sergei

Re: [PATCH 1/3] ata: Allow having a port recovery callback

[Sergei Shtylyov]

Hello!

On 01/10/2018 02:04 AM, Florian Fainelli wrote:

> Some AHCI controllers may need to recovery from a failure to identify a drive,

    Recover.

> such as the Broadcom AHCI controller, make that possible at both the
> libata-core.c and AHCI controller level such that we can provide such an error
> recovery callback.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
[...]

MBR, Sergei

Re: [PATCH 1/3] ata: Allow having a port recovery callback

[Florian Fainelli]

On 01/10/2018 06:25 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Jan 09, 2018 at 03:04:55PM -0800, Florian Fainelli wrote:
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 3c09122bf038..921c2813af07 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2045,6 +2045,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>>  	if (ata_msg_warn(ap))
>>  		ata_dev_warn(dev, "failed to IDENTIFY (%s, err_mask=0x%x)\n",
>>  			     reason, err_mask);
>> +	if (ap->host->ops->port_recovery)
>> +		ap->host->ops->port_recovery(ap);
>>  	return rc;
>>  }
> 
> This is a really weird spot to add a callback named port_recovery().
> Can't the affected driver simply implement its own
> ata_port_operations->read_id() operation which does the recovery if
> necessary?

I did not consider that, but this is actually a great idea, thanks for
the suggestion! I will respin with that being done.
-- 
Florian

Re: [PATCH 1/3] ata: Allow having a port recovery callback

[Florian Fainelli]

On 01/10/2018 06:25 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Jan 09, 2018 at 03:04:55PM -0800, Florian Fainelli wrote:
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 3c09122bf038..921c2813af07 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -2045,6 +2045,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>>  	if (ata_msg_warn(ap))
>>  		ata_dev_warn(dev, "failed to IDENTIFY (%s, err_mask=0x%x)\n",
>>  			     reason, err_mask);
>> +	if (ap->host->ops->port_recovery)
>> +		ap->host->ops->port_recovery(ap);
>>  	return rc;
>>  }
> 
> This is a really weird spot to add a callback named port_recovery().
> Can't the affected driver simply implement its own
> ata_port_operations->read_id() operation which does the recovery if
> necessary?

I did not consider that, but this is actually a great idea, thanks for
the suggestion! I will respin with that being done.
-- 
Florian