Re: libata bridge limits

[Jens Axboe <jens.axboe@oracle.com>]

On Tue, Aug 26 2008, Brad Campbell wrote:
> Jens 2 wrote:
> >On Tue, Aug 26 2008, Alan Cox wrote:
> >>>a) Why was this limit put in there? It limits both transfer speed and
> >>>   request size. If it's due to some dodgy drive/bridge, perhaps we
> >>>   should just check for that and only apply the transfer limits when
> >>>   detected (or blacklisted). On the bridge setups I've seen, I've never
> >>>   had problems with killing the limit.
> >>Various old bridges need it - and you can't detect the bridge type.
> >
> >Not generically, but for some devices (like the Mtron) we can.
> >
> >>>b) Put in a whitelist, easy to do for these Mtron drives.
> >>>
> >>>c) Add a parameter to turn it on (or off, depending on the default) for
> >>>   a specific drive.
> >>>
> >>>I'm in favor of a) personally, but I'd like to hear why the check was
> >>>added originally first. Dropping 20-30% of the throughput performance on
> >>>the floor without option seems like a really bad choice.
> 
> The check was originally put there as some nasty bridges caused all sorts 
> of errors when these limits were exceeded, including some random data 
> corruption from memory.
> 
> Those particular bridges were/are still widely available an can't be 
> detected / identified using any other means.

That's a worry, since I don't think we can dynamically switch back in
that case if it has potential data corruption problems.

> >>Can I suggest 
> >>
> >>d) Assume the bridge is ok but teach the SATA error handling code that if
> >>there is a timeout immediately with such a bridge then to flip down to
> >>UDMA5 and knobble the transfer length.
> >
> >That would be nice, assuming that we can rely on safe behaviour (eg not
> >data corrupting badness).
> >
> 
> I was responsible for that original bridge knobble stuff and fortunately I 
> still have the bridges, disks and controllers that triggered the original 
> faults. If someone wants to cook up some code for testing I'd be more than 
> happy to stick this stuff on the bench and beat on it for regression 
> purposes.

Given that this problem should be going away and that it only really
matters on very select devices (like this SSD), I think we should just
add a quick white list for the bridge limits.

Below is a quick'n dirty for that...

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 79e3a8e..fe8033a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2097,9 +2097,70 @@ retry:
 	return rc;
 }
 
+struct ata_blacklist_entry {
+	const char *model_num;
+	const char *model_rev;
+	unsigned long horkage;
+};
+
+static const struct ata_blacklist_entry ata_bridge_whitelist[] = {
+	/*
+	 * The following devices sit behind a bridge, but don't need
+	 * transfer rate or size limits applied.
+	 */
+	{ "Mtron", },
+
+	/* End Marker */
+	{ }
+};
+
+static int strn_pattern_cmp(const char *patt, const char *name, int wildchar)
+{
+	const char *p;
+	int len;
+
+	/*
+	 * check for trailing wildcard: *\0
+	 */
+	p = strchr(patt, wildchar);
+	if (p && ((*(p + 1)) == 0))
+		len = p - patt;
+	else {
+		len = strlen(name);
+		if (!len) {
+			if (!*patt)
+				return 0;
+			return -1;
+		}
+	}
+
+	return strncmp(patt, name, len);
+}
+
+static unsigned int ata_dev_bridge_whitelisted(const struct ata_device *dev)
+{
+	unsigned char model_num[ATA_ID_PROD_LEN + 1];
+	unsigned char model_rev[ATA_ID_FW_REV_LEN + 1];
+	const struct ata_blacklist_entry *ad = ata_bridge_whitelist;
+
+	ata_id_c_string(dev->id, model_num, ATA_ID_PROD, sizeof(model_num));
+	ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));
+
+	while (ad->model_num) {
+		if (!strn_pattern_cmp(ad->model_num, model_num, '*'))
+			return 1;
+		ad++;
+	}
+	return 0;
+}
+
 static inline u8 ata_dev_knobble(struct ata_device *dev)
 {
 	struct ata_port *ap = dev->link->ap;
+
+	if (ata_dev_bridge_whitelisted(dev))
+		return 0;
+
 	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev->id)));
 }
 
@@ -3913,12 +3974,6 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 	return rc;
 }
 
-struct ata_blacklist_entry {
-	const char *model_num;
-	const char *model_rev;
-	unsigned long horkage;
-};
-
 static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	/* Devices with DMA related problems under Linux */
 	{ "WDC AC11000H",	NULL,		ATA_HORKAGE_NODMA },
@@ -4002,29 +4057,6 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	{ }
 };
 
-static int strn_pattern_cmp(const char *patt, const char *name, int wildchar)
-{
-	const char *p;
-	int len;
-
-	/*
-	 * check for trailing wildcard: *\0
-	 */
-	p = strchr(patt, wildchar);
-	if (p && ((*(p + 1)) == 0))
-		len = p - patt;
-	else {
-		len = strlen(name);
-		if (!len) {
-			if (!*patt)
-				return 0;
-			return -1;
-		}
-	}
-
-	return strncmp(patt, name, len);
-}
-
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev)
 {
 	unsigned char model_num[ATA_ID_PROD_LEN + 1];

-- 
Jens Axboe