diff mbox series

ata: disable auto activate if the controller does not support

Message ID 20180514090702.16764-1-yanaijie@huawei.com
State Not Applicable
Delegated to: David Miller
Headers show
Series ata: disable auto activate if the controller does not support | expand

Commit Message

Jason Yan May 14, 2018, 9:07 a.m. UTC 
Some SSDs (such as SAMSUNG PM863 and SM863) enable auto activate feature
by default. Now libata only send set feature command to enable auto
activate feature if the controller and disk both support this feature.
But when the controller do not support this feature then the data transfer
failed. Fix this by sending set feature command to disable this feature
if the controller does not support it.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reported-and-tested-by: Zhou Yupeng <zhouyupeng1@huawei.com>
---
 drivers/ata/libata-core.c | 54 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 16 deletions(-)

Comments

Tejun Heo May 14, 2018, 3:34 p.m. UTC | #1 
On Mon, May 14, 2018 at 05:07:02PM +0800, Jason Yan wrote:
> Some SSDs (such as SAMSUNG PM863 and SM863) enable auto activate feature
> by default. Now libata only send set feature command to enable auto
> activate feature if the controller and disk both support this feature.
> But when the controller do not support this feature then the data transfer
> failed. Fix this by sending set feature command to disable this feature
> if the controller does not support it.

If we know which devices behave this way, I think it'd be better to
apply it only to those devices.  You never know where the dragons
might be lurking with optional features on ATA devices.

Thanks.
diff mbox series

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index cf27f1e1871f..72399c2bd3b1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2278,6 +2278,41 @@  static void ata_dev_config_ncq_prio(struct ata_device *dev)
 
 }
 
+static inline int ata_dev_config_aa(struct ata_device *dev, struct ata_port *ap,
+				 char **aa_desc)
+{
+	unsigned int err_mask;
+	char *desc;
+	u8 enable;
+
+	if (dev->horkage & ATA_HORKAGE_BROKEN_FPDMA_AA)
+		return 0;
+
+	if (!ata_id_has_fpdma_aa(dev->id))
+		return 0;
+
+	if (ap->flags & ATA_FLAG_FPDMA_AA) {
+		enable = SETFEATURES_SATA_ENABLE;
+		desc = "enable";
+	} else {
+		enable = SETFEATURES_SATA_DISABLE;
+		desc = "disalbe";
+	}
+
+	err_mask = ata_dev_set_feature(dev, enable, SATA_FPDMA_AA);
+	if (err_mask) {
+		ata_dev_err(dev, "failed to %s AA (error_mask=0x%x)\n",
+			    desc, err_mask);
+		if (err_mask != AC_ERR_DEV) {
+			dev->horkage |= ATA_HORKAGE_BROKEN_FPDMA_AA;
+			return -EIO;
+		}
+	} else
+		*aa_desc = ", AA";
+
+	return 0;
+}
+
 static int ata_dev_config_ncq(struct ata_device *dev,
 			       char *desc, size_t desc_sz)
 {
@@ -2299,22 +2334,9 @@  static int ata_dev_config_ncq(struct ata_device *dev,
 		dev->flags |= ATA_DFLAG_NCQ;
 	}
 
-	if (!(dev->horkage & ATA_HORKAGE_BROKEN_FPDMA_AA) &&
-		(ap->flags & ATA_FLAG_FPDMA_AA) &&
-		ata_id_has_fpdma_aa(dev->id)) {
-		err_mask = ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
-			SATA_FPDMA_AA);
-		if (err_mask) {
-			ata_dev_err(dev,
-				    "failed to enable AA (error_mask=0x%x)\n",
-				    err_mask);
-			if (err_mask != AC_ERR_DEV) {
-				dev->horkage |= ATA_HORKAGE_BROKEN_FPDMA_AA;
-				return -EIO;
-			}
-		} else
-			aa_desc = ", AA";
-	}
+	err_mask = ata_dev_config_aa(dev, ap, &aa_desc);
+	if (err_mask)
+		return err_mask;
 
 	if (hdepth >= ddepth)
 		snprintf(desc, desc_sz, "NCQ (depth %d)%s", ddepth, aa_desc);