libata: glob_match for ata_device_blacklist

PATCH In review Mark Lord 3 participants 23 messages Jul 1, 2010 – Jul 14, 2010 libata

Share & tools

Mark Lord

author

2010-07-01 15:30 UTC #0 lore raw

PATCH 1/3 libata: glob_match for ata_device_blacklist

Replace rudimentry pattern matching with more capable shell-style globbing. This will enable shrinking ata_device_blacklist[] table in subsequent patches, and helps with future editions to the table, such as matching only the end of a firmware revision string etc..

Signed-off-by Mark Lord <[email protected]>

---

Eventually, this function should move out of libata into lib/string.c or similar (?). But since following patches in this series require it for libata,
let's start with it there, and move things later on.

drivers/ata/libata-core.c +56 ~~-20~~

@@ -4413,29 +4413,65 @@
 	{ }
 };
 
-static int strn_pattern_cmp(const char *patt, const char *name, int wildchar)
+/**
+ *	glob_match - match a text string against a glob-style pattern
+ *	@text: the string to be examined
+ *	@pattern: the glob-style pattern to be matched against
+ *
+ *	Either/both of text and pattern can be empty strings.
+ *
+ *	Match text against a glob-style pattern, with wildcards and simple sets:
+ *
+ *		?	matches any single character.
+ *		*	matches any run of characters.
+ *		[xyz]	matches a single character from the set: x, y, or z.
+ *
+ *	Note: hyphenated ranges [0-9] are _not_ supported here.
+ *	The special characters ?, [, or *, can be matched using a set, eg. [*]
+ *
+ *	Example patterns:  "SD1?",  "SD1[012345]",  "*R0",  SD*1?[012]*xx"
+ *
+ *	This function uses one level of recursion per '*' in pattern.
+ *	Since it calls _nothing_ else, and has _no_ explicit local variables,
+ *	this will not cause stack problems for any reasonable use here.
+ *
+ *	RETURNS:
+ *	0 on match, 1 otherwise.
+ */
+static int glob_match (const char *text, const char *pattern)
 {
-	const char *p;
-	int len;
+	do {
+		/* Match single character or a '?' wildcard */
+		if (*text == *pattern || *pattern == '?') {
+			if (!*pattern++)
+				return 0;  /* End of both strings: match */
+		} else {
+			/* Match single char against a '[' bracketed ']' pattern set */
+			if (!*text || *pattern != '[')
+				break;  /* Not a pattern set */
+			while (*++pattern && *pattern != ']' && *text != *pattern);
+			if (!*pattern || *pattern == ']')
+				return 1;  /* No match */
+			while (*pattern && *pattern++ != ']');
+		}
+	} while (*++text && *pattern);
 
-	/*
-	 * 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;
+	/* Match any run of chars against a '*' wildcard */
+	if (*pattern == '*') {
+		if (!*++pattern)
+			return 0;  /* Match: avoid recursion at end of pattern */
+		/* Loop to handle additional pattern chars after the wildcard */
+		while (*text) {
+			if (glob_match(text, pattern) == 0)
+				return 0;  /* Remainder matched */
+			++text;  /* Absorb (match) this char and try again */
 		}
 	}
-
-	return strncmp(patt, name, len);
+	if (!*text && !*pattern)
+		return 0;  /* End of both strings: match */
+	return 1;  /* No match */
 }
-
+
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev)
 {
 	unsigned char model_num[ATA_ID_PROD_LEN + 1];

@@ -4446,10 +4482,10 @@
 	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, '*')) {
+		if (!glob_match(model_num, ad->model_num)) {
 			if (ad->model_rev == NULL)
 				return ad->horkage;
-			if (!strn_pattern_cmp(ad->model_rev, model_rev, '*'))
+			if (!glob_match(model_rev, ad->model_rev))
 				return ad->horkage;
 		}
 		ad++;

Mark Lord

author

2010-07-01 15:31 UTC #1 lore raw

PATCH 2/3 libata: reduce blacklist size

Take advantage of the new glob_match() function to reduce the blacklist size somewhat. There are further savings possible, but these are the most obvious biggies.

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-core.c +6 ~~-54~~

@@ -4298,70 +4298,22 @@
 	{ "OCZ CORE_SSD",	"02.10104",	ATA_HORKAGE_NONCQ },
 
 	/* Seagate NCQ + FLUSH CACHE firmware bug */
-	{ "ST31500341AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31500341AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31500341AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31500341AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31500341AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST31500341AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST31000333AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31000333AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31000333AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31000333AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31000333AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST31000333AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640623AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640623AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640623AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640623AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640623AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST3640623AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640323AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640323AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640323AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640323AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640323AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST3640323AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3320813AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320813AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320813AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320813AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320813AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST3320813AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3320613AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320613AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320613AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320613AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320613AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST3320613AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
 	/* Blacklist entries taken from Silicon Image 3124/3132

Mark Lord

author

2010-07-01 15:31 UTC #2 lore raw

Re: [PATCH 3/3] libata: reduce blacklist size even more

Take further advantage of the new glob_match() function to reduce the blacklist size. There are even more savings possible, but how far do we want to go with this?

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-core.c +3 ~~-10~~

@@ -4254,15 +4254,13 @@
 	{ "WDC AC23200L",	"21.10N21",	ATA_HORKAGE_NODMA },
 	{ "Compaq CRD-8241B", 	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-8400B",		NULL, 		ATA_HORKAGE_NODMA },
-	{ "CRD-8480B",		NULL,		ATA_HORKAGE_NODMA },
-	{ "CRD-8482B",		NULL,		ATA_HORKAGE_NODMA },
+	{ "CRD-848[02]B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-84",		NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B-64",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SANYO CD-ROM CRD",	NULL,		ATA_HORKAGE_NODMA },
 	{ "HITACHI CDR-8",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8335",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8435",	NULL,		ATA_HORKAGE_NODMA },
+	{ "HITACHI CDR-8[34]35",NULL,		ATA_HORKAGE_NODMA },
 	{ "Toshiba CD-ROM XM-6202B", NULL,	ATA_HORKAGE_NODMA },
 	{ "TOSHIBA CD-ROM XM-1702BC", NULL,	ATA_HORKAGE_NODMA },
 	{ "CD-532E-A", 		NULL,		ATA_HORKAGE_NODMA },

@@ -4342,12 +4340,7 @@
 	/* Devices which get the IVB wrong */
 	{ "QUANTUM FIREBALLlct10 05", "A03.0900", ATA_HORKAGE_IVB, },
 	/* Maybe we should just blacklist TSSTcorp... */
-	{ "TSSTcorp CDDVDW SH-S202H", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202H", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB01",	  ATA_HORKAGE_IVB, },
+	{ "TSSTcorp CDDVDW SH-S202[HJN]", "SB0[01]",  ATA_HORKAGE_IVB, },
 
 	/* Devices that do not need bridging limits applied */
 	{ "MTRON MSP-SATA*",		NULL,	ATA_HORKAGE_BRIDGE_OK, },

Mark Lord

author

2010-07-01 15:34 UTC #4 lore raw

In reply to Mark Lord

Quoted 3 lines · Mark Lord

Replace rudimentry pattern matching with more capable shell-style globbing. This will enable shrinking ata_device_blacklist[] table in subsequent patches,

I am working on this, because a future device to be added (not ready yet) to the blacklist table requires matching on only the end of the firmware string. The old pattern matcher cannot handle that case, so..

In the meanwhile, this gives us decent memory savings, and makes the list easier to maintain even for current entries.

Cheers

Mark

James Bottomley

2010-07-01 15:36 UTC #5 lore raw

In reply to Mark Lord

Quoted 4 lines · Mark Lord

Replace rudimentry pattern matching with more capable shell-style globbing. This will enable shrinking ata_device_blacklist[] table in subsequent patches, and helps with future editions to the table, such as matching only the end of a firmware revision string etc..

So we have other identical cases of this list to flag matching ... like in SCSI. How about we move the mechanics to lib and share the code?

James

Mark Lord

author

2010-07-01 15:36 UTC #3 lore raw

PATCH 3/3 libata: reduce blacklist size (resend)

In reply to Mark Lord

Take further advantage of the new glob_match() function to reduce the blacklist size. There are even more savings possible, but how far do we want to go with this?

Signed-off-by Mark Lord <[email protected]>

(resent with subject line fixed)

drivers/ata/libata-core.c +3 ~~-10~~

@@ -4254,15 +4254,13 @@
 	{ "WDC AC23200L",	"21.10N21",	ATA_HORKAGE_NODMA },
 	{ "Compaq CRD-8241B", 	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-8400B",		NULL, 		ATA_HORKAGE_NODMA },
-	{ "CRD-8480B",		NULL,		ATA_HORKAGE_NODMA },
-	{ "CRD-8482B",		NULL,		ATA_HORKAGE_NODMA },
+	{ "CRD-848[02]B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-84",		NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B-64",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SANYO CD-ROM CRD",	NULL,		ATA_HORKAGE_NODMA },
 	{ "HITACHI CDR-8",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8335",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8435",	NULL,		ATA_HORKAGE_NODMA },
+	{ "HITACHI CDR-8[34]35",NULL,		ATA_HORKAGE_NODMA },
 	{ "Toshiba CD-ROM XM-6202B", NULL,	ATA_HORKAGE_NODMA },
 	{ "TOSHIBA CD-ROM XM-1702BC", NULL,	ATA_HORKAGE_NODMA },
 	{ "CD-532E-A", 		NULL,		ATA_HORKAGE_NODMA },

@@ -4342,12 +4340,7 @@
 	/* Devices which get the IVB wrong */
 	{ "QUANTUM FIREBALLlct10 05", "A03.0900", ATA_HORKAGE_IVB, },
 	/* Maybe we should just blacklist TSSTcorp... */
-	{ "TSSTcorp CDDVDW SH-S202H", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202H", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB01",	  ATA_HORKAGE_IVB, },
+	{ "TSSTcorp CDDVDW SH-S202[HJN]", "SB0[01]",  ATA_HORKAGE_IVB, },
 
 	/* Devices that do not need bridging limits applied */
 	{ "MTRON MSP-SATA*",		NULL,	ATA_HORKAGE_BRIDGE_OK, },

Mark Lord

author

2010-07-01 15:44 UTC #6 lore raw

In reply to James Bottomley

Quoted 8 lines · James Bottomley

Quoted 4 lines · Mark Lord
Replace rudimentry pattern matching with more capable shell-style globbing. This will enable shrinking ata_device_blacklist[] table in subsequent patches, and helps with future editions to the table, such as matching only the end of a firmware revision string etc..
So we have other identical cases of this list to flag matching ... like in SCSI. How about we move the mechanics to lib and share the code?

I agree wholeheartedly. But first it has to get upstream. And it has follow-on patches that depend on it, so initially I'd like to push it up via the libata tree. And then.. as in the original post:

Quoted 3 lines

Eventually, this function should move out of libata into lib/string.c or similar (?). But since following patches in this series require it for libata,
let's start with it there, and move things later on.

Cheers!

Jeff Garzik

2010-07-01 19:24 UTC #7 lore raw

In reply to Mark Lord

Quoted 7 lines · Mark Lord

Replace rudimentry pattern matching with more capable shell-style globbing. This will enable shrinking ata_device_blacklist[] table in subsequent patches,
and helps with future editions to the table, such as matching only the end of a firmware revision string etc..

Quoted 7 lines

Signed-off-by Mark Lord <[email protected]>
---
Eventually, this function should move out of libata into lib/string.c or similar (?).
But since following patches in this series require it for libata, let's start with it there, and move things later on.

hmmm, can you resend this series as an attachment? The patch is failing miserably here, both with 'git am' and patch(1). I don't see anything obviously wrong with the patch other than line numbers -- did you diff against the latest upstream kernel (torvalds/linux-2.6.git)?

Adding "-l" to patch(1) causes hunk #2 of patch #1 to succeed, but being the smaller hunk, that's the less useful of the two.

Jeff

[jgarzik@bd libata-dev]$ patch -l --verbose drivers/ata/libata-core.c < .git/rebase-apply/patch Hmm... Looks like a unified diff to me...
The text leading up to this was:
--------------------------

|---
|
|Eventually, this function should move out of libata into lib/string.c or similar (?).
|But since following patches in this series require it for libata,
|let's start with it there, and move things later on.
|
|--- 2.6.34/drivers/ata/libata-core.c	2010-05-16 17:17:36.000000000 -0400
|+++ linux/drivers/ata/libata-core.c	2010-07-01 11:08:10.509159666 -0400

--------------------------
Patching file drivers/ata/libata-core.c using Plan A...
Hunk #1 FAILED at 4413.
Hunk #2 succeeded at 4359 (offset -87 lines).
1 out of 2 hunks FAILED -- saving rejects to file drivers/ata/libata-core.c.rej Hmm... Ignoring the trailing garbage.
done

Mark Lord

author

2010-07-01 21:56 UTC #8 lore raw

Quoted 3 lines · Jeff Garzik

On 07/01/2010 11:30 AM, Mark Lord wrote:
hmmm, can you resend this series as an attachment?

Sure thing. I suspect my old mailer solution is no longer working correctly.

Here, try these (attached).
They are against 2.6.34, though -- I don't have a current Linus tree at the moment. If they still don't apply, let me know and I'll get a tree and rediff them.

Thanks Jeff!

Signed-off-by Mark Lord <[email protected]>

---

drivers/ata/libata-core.c +56 ~~-20~~

@@ -4413,29 +4413,65 @@
 	{ }
 };
 
-static int strn_pattern_cmp(const char *patt, const char *name, int wildchar)
+/**
+ *	glob_match - match a text string against a glob-style pattern
+ *	@text: the string to be examined
+ *	@pattern: the glob-style pattern to be matched against
+ *
+ *	Either/both of text and pattern can be empty strings.
+ *
+ *	Match text against a glob-style pattern, with wildcards and simple sets:
+ *
+ *		?	matches any single character.
+ *		*	matches any run of characters.
+ *		[xyz]	matches a single character from the set: x, y, or z.
+ *
+ *	Note: hyphenated ranges [0-9] are _not_ supported here.
+ *	The special characters ?, [, or *, can be matched using a set, eg. [*]
+ *
+ *	Example patterns:  "SD1?",  "SD1[012345]",  "*R0",  SD*1?[012]*xx"
+ *
+ *	This function uses one level of recursion per '*' in pattern.
+ *	Since it calls _nothing_ else, and has _no_ explicit local variables,
+ *	this will not cause stack problems for any reasonable use here.
+ *
+ *	RETURNS:
+ *	0 on match, 1 otherwise.
+ */
+static int glob_match (const char *text, const char *pattern)
 {
-	const char *p;
-	int len;
+	do {
+		/* Match single character or a '?' wildcard */
+		if (*text == *pattern || *pattern == '?') {
+			if (!*pattern++)
+				return 0;  /* End of both strings: match */
+		} else {
+			/* Match single char against a '[' bracketed ']' pattern set */
+			if (!*text || *pattern != '[')
+				break;  /* Not a pattern set */
+			while (*++pattern && *pattern != ']' && *text != *pattern);
+			if (!*pattern || *pattern == ']')
+				return 1;  /* No match */
+			while (*pattern && *pattern++ != ']');
+		}
+	} while (*++text && *pattern);
 
-	/*
-	 * 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;
+	/* Match any run of chars against a '*' wildcard */
+	if (*pattern == '*') {
+		if (!*++pattern)
+			return 0;  /* Match: avoid recursion at end of pattern */
+		/* Loop to handle additional pattern chars after the wildcard */
+		while (*text) {
+			if (glob_match(text, pattern) == 0)
+				return 0;  /* Remainder matched */
+			++text;  /* Absorb (match) this char and try again */
 		}
 	}
-
-	return strncmp(patt, name, len);
+	if (!*text && !*pattern)
+		return 0;  /* End of both strings: match */
+	return 1;  /* No match */
 }
-
+ 
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev)
 {
 	unsigned char model_num[ATA_ID_PROD_LEN + 1];

@@ -4446,10 +4482,10 @@
 	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, '*')) {
+		if (!glob_match(model_num, ad->model_num)) {
 			if (ad->model_rev == NULL)
 				return ad->horkage;
-			if (!strn_pattern_cmp(ad->model_rev, model_rev, '*'))
+			if (!glob_match(model_rev, ad->model_rev))
 				return ad->horkage;
 		}
 		ad++;

Take advantage of the new glob_match() function to reduce the blacklist size somewhat. There are further savings possible, but these are the most obvious biggies.

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-core.c +6 ~~-54~~

76 lines not on this page · raw message

Take further advantage of the new glob_match() function to reduce the blacklist size. There are even more savings possible, but how far do we want to go with this?

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-core.c +3 ~~-10~~

@@ -4254,15 +4254,13 @@
 	{ "WDC AC23200L",	"21.10N21",	ATA_HORKAGE_NODMA },
 	{ "Compaq CRD-8241B", 	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-8400B",		NULL, 		ATA_HORKAGE_NODMA },
-	{ "CRD-8480B",		NULL,		ATA_HORKAGE_NODMA },
-	{ "CRD-8482B",		NULL,		ATA_HORKAGE_NODMA },
+	{ "CRD-848[02]B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-84",		NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B-64",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SANYO CD-ROM CRD",	NULL,		ATA_HORKAGE_NODMA },
 	{ "HITACHI CDR-8",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8335",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8435",	NULL,		ATA_HORKAGE_NODMA },
+	{ "HITACHI CDR-8[34]35",NULL,		ATA_HORKAGE_NODMA },
 	{ "Toshiba CD-ROM XM-6202B", NULL,	ATA_HORKAGE_NODMA },
 	{ "TOSHIBA CD-ROM XM-1702BC", NULL,	ATA_HORKAGE_NODMA },
 	{ "CD-532E-A", 		NULL,		ATA_HORKAGE_NODMA },

@@ -4342,12 +4340,7 @@
 	/* Devices which get the IVB wrong */
 	{ "QUANTUM FIREBALLlct10 05", "A03.0900", ATA_HORKAGE_IVB, },
 	/* Maybe we should just blacklist TSSTcorp... */
-	{ "TSSTcorp CDDVDW SH-S202H", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202H", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB01",	  ATA_HORKAGE_IVB, },
+	{ "TSSTcorp CDDVDW SH-S202[HJN]", "SB0[01]",  ATA_HORKAGE_IVB, },
 
 	/* Devices that do not need bridging limits applied */
 	{ "MTRON MSP-SATA*",		NULL,	ATA_HORKAGE_BRIDGE_OK, },

Mark Lord

author

2010-07-01 22:09 UTC #9 lore raw

On 01/07/10 05:56 PM, Mark Lord wrote:
..

Quoted 4 lines

Here, try these (attached).
They are against 2.6.34, though -- I don't have a current Linus tree at the moment.
If they still don't apply, let me know and I'll get a tree and rediff them.

Scratch that. I will re-diff them against latest 2.6.35-rc-git and repost here shortly.

Cheers

Mark Lord

author

2010-07-01 22:16 UTC #10 lore raw

PATCH 1/3 libata: glob_match for ata_device_blacklist (v2)

In reply to Mark Lord

Signed-off-by Mark Lord <[email protected]>

---

(re-diff'd against latest -git, and also attached to bypass mailer issues)

drivers/ata/libata-core.c +56 ~~-20~~

@@ -4326,29 +4326,65 @@
 	{ }
 };
 
-static int strn_pattern_cmp(const char *patt, const char *name, int wildchar)
+/**
+ *	glob_match - match a text string against a glob-style pattern
+ *	@text: the string to be examined
+ *	@pattern: the glob-style pattern to be matched against
+ *
+ *	Either/both of text and pattern can be empty strings.
+ *
+ *	Match text against a glob-style pattern, with wildcards and simple sets:
+ *
+ *		?	matches any single character.
+ *		*	matches any run of characters.
+ *		[xyz]	matches a single character from the set: x, y, or z.
+ *
+ *	Note: hyphenated ranges [0-9] are _not_ supported here.
+ *	The special characters ?, [, or *, can be matched using a set, eg. [*]
+ *
+ *	Example patterns:  "SD1?",  "SD1[012345]",  "*R0",  SD*1?[012]*xx"
+ *
+ *	This function uses one level of recursion per '*' in pattern.
+ *	Since it calls _nothing_ else, and has _no_ explicit local variables,
+ *	this will not cause stack problems for any reasonable use here.
+ *
+ *	RETURNS:
+ *	0 on match, 1 otherwise.
+ */
+static int glob_match (const char *text, const char *pattern)
 {
-	const char *p;
-	int len;
+	do {
+		/* Match single character or a '?' wildcard */
+		if (*text == *pattern || *pattern == '?') {
+			if (!*pattern++)
+				return 0;  /* End of both strings: match */
+		} else {
+			/* Match single char against a '[' bracketed ']' pattern set */
+			if (!*text || *pattern != '[')
+				break;  /* Not a pattern set */
+			while (*++pattern && *pattern != ']' && *text != *pattern);
+			if (!*pattern || *pattern == ']')
+				return 1;  /* No match */
+			while (*pattern && *pattern++ != ']');
+		}
+	} while (*++text && *pattern);
 
-	/*
-	 * 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;
+	/* Match any run of chars against a '*' wildcard */
+	if (*pattern == '*') {
+		if (!*++pattern)
+			return 0;  /* Match: avoid recursion at end of pattern */
+		/* Loop to handle additional pattern chars after the wildcard */
+		while (*text) {
+			if (glob_match(text, pattern) == 0)
+				return 0;  /* Remainder matched */
+			++text;  /* Absorb (match) this char and try again */
 		}
 	}
-
-	return strncmp(patt, name, len);
+	if (!*text && !*pattern)
+		return 0;  /* End of both strings: match */
+	return 1;  /* No match */
 }
-
+
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev)
 {
 	unsigned char model_num[ATA_ID_PROD_LEN + 1];

@@ -4359,10 +4395,10 @@
 	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, '*')) {
+		if (!glob_match(model_num, ad->model_num)) {
 			if (ad->model_rev == NULL)
 				return ad->horkage;
-			if (!strn_pattern_cmp(ad->model_rev, model_rev, '*'))
+			if (!glob_match(model_rev, ad->model_rev))
 				return ad->horkage;
 		}
 		ad++;

Signed-off-by Mark Lord <[email protected]>

---

drivers/ata/libata-core.c +56 ~~-20~~

95 lines not on this page · raw message

Mark Lord

author

2010-07-01 22:17 UTC #11 lore raw

PATCH 2/3 libata: reduce blacklist size (v2)

Take advantage of the new glob_match() function to reduce the blacklist size somewhat. There are further savings possible, but these are the most obvious biggies.

Signed-off-by Mark Lord <[email protected]>

---

(re-diff'd against latest -git, and also attached to bypass mailer issues)

drivers/ata/libata-core.c +6 ~~-54~~

@@ -4211,70 +4211,22 @@
 	{ "OCZ CORE_SSD",	"02.10104",	ATA_HORKAGE_NONCQ },
 
 	/* Seagate NCQ + FLUSH CACHE firmware bug */
-	{ "ST31500341AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31500341AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31500341AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31500341AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31500341AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST31500341AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST31000333AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31000333AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31000333AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31000333AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST31000333AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST31000333AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640623AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640623AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640623AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640623AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640623AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST3640623AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640323AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640323AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640323AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640323AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3640323AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST3640323AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3320813AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320813AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320813AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320813AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320813AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST3320813AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3320613AS",	"SD15",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320613AS",	"SD16",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320613AS",	"SD17",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320613AS",	"SD18",		ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-	{ "ST3320613AS",	"SD19",		ATA_HORKAGE_NONCQ |
+	{ "ST3320613AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
 	/* Blacklist entries taken from Silicon Image 3124/3132

Take advantage of the new glob_match() function to reduce the blacklist size somewhat. There are further savings possible, but these are the most obvious biggies.

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-core.c +6 ~~-54~~

76 lines not on this page · raw message

Mark Lord

author

2010-07-01 22:18 UTC #12 lore raw

PATCH 3/3 libata: reduce blacklist size even more (v2)

Take further advantage of the new glob_match() function to reduce the blacklist size. There are even more savings possible, but how far do we want to go with this?

Signed-off-by Mark Lord <[email protected]>

---

(re-diff'd against latest -git, and also attached to bypass mailer issues)

drivers/ata/libata-core.c +3 ~~-10~~

@@ -4167,15 +4167,13 @@
 	{ "WDC AC23200L",	"21.10N21",	ATA_HORKAGE_NODMA },
 	{ "Compaq CRD-8241B", 	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-8400B",		NULL, 		ATA_HORKAGE_NODMA },
-	{ "CRD-8480B",		NULL,		ATA_HORKAGE_NODMA },
-	{ "CRD-8482B",		NULL,		ATA_HORKAGE_NODMA },
+	{ "CRD-848[02]B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-84",		NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B-64",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SANYO CD-ROM CRD",	NULL,		ATA_HORKAGE_NODMA },
 	{ "HITACHI CDR-8",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8335",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8435",	NULL,		ATA_HORKAGE_NODMA },
+	{ "HITACHI CDR-8[34]35",NULL,		ATA_HORKAGE_NODMA },
 	{ "Toshiba CD-ROM XM-6202B", NULL,	ATA_HORKAGE_NODMA },
 	{ "TOSHIBA CD-ROM XM-1702BC", NULL,	ATA_HORKAGE_NODMA },
 	{ "CD-532E-A", 		NULL,		ATA_HORKAGE_NODMA },

@@ -4255,12 +4253,7 @@
 	/* Devices which get the IVB wrong */
 	{ "QUANTUM FIREBALLlct10 05", "A03.0900", ATA_HORKAGE_IVB, },
 	/* Maybe we should just blacklist TSSTcorp... */
-	{ "TSSTcorp CDDVDW SH-S202H", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202H", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB01",	  ATA_HORKAGE_IVB, },
+	{ "TSSTcorp CDDVDW SH-S202[HJN]", "SB0[01]",  ATA_HORKAGE_IVB, },
 
 	/* Devices that do not need bridging limits applied */
 	{ "MTRON MSP-SATA*",		NULL,	ATA_HORKAGE_BRIDGE_OK, },

Take further advantage of the new glob_match() function to reduce the blacklist size. There are even more savings possible, but how far do we want to go with this?

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-core.c +3 ~~-10~~

@@ -4167,15 +4167,13 @@
 	{ "WDC AC23200L",	"21.10N21",	ATA_HORKAGE_NODMA },
 	{ "Compaq CRD-8241B", 	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-8400B",		NULL, 		ATA_HORKAGE_NODMA },
-	{ "CRD-8480B",		NULL,		ATA_HORKAGE_NODMA },
-	{ "CRD-8482B",		NULL,		ATA_HORKAGE_NODMA },
+	{ "CRD-848[02]B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "CRD-84",		NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SanDisk SDP3B-64",	NULL,		ATA_HORKAGE_NODMA },
 	{ "SANYO CD-ROM CRD",	NULL,		ATA_HORKAGE_NODMA },
 	{ "HITACHI CDR-8",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8335",	NULL,		ATA_HORKAGE_NODMA },
-	{ "HITACHI CDR-8435",	NULL,		ATA_HORKAGE_NODMA },
+	{ "HITACHI CDR-8[34]35",NULL,		ATA_HORKAGE_NODMA },
 	{ "Toshiba CD-ROM XM-6202B", NULL,	ATA_HORKAGE_NODMA },
 	{ "TOSHIBA CD-ROM XM-1702BC", NULL,	ATA_HORKAGE_NODMA },
 	{ "CD-532E-A", 		NULL,		ATA_HORKAGE_NODMA },

@@ -4255,12 +4253,7 @@
 	/* Devices which get the IVB wrong */
 	{ "QUANTUM FIREBALLlct10 05", "A03.0900", ATA_HORKAGE_IVB, },
 	/* Maybe we should just blacklist TSSTcorp... */
-	{ "TSSTcorp CDDVDW SH-S202H", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202H", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202J", "SB01",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB00",	  ATA_HORKAGE_IVB, },
-	{ "TSSTcorp CDDVDW SH-S202N", "SB01",	  ATA_HORKAGE_IVB, },
+	{ "TSSTcorp CDDVDW SH-S202[HJN]", "SB0[01]",  ATA_HORKAGE_IVB, },
 
 	/* Devices that do not need bridging limits applied */
 	{ "MTRON MSP-SATA*",		NULL,	ATA_HORKAGE_BRIDGE_OK, },

Jeff Garzik

2010-07-02 07:19 UTC #13 lore raw

Re: [PATCH 3/3] libata: reduce blacklist size even more (v2)

Quoted 9 lines · Mark Lord

Take further advantage of the new glob_match() function to reduce the blacklist size.
There are even more savings possible, but how far do we want to go with this?
Signed-off-by Mark Lord <[email protected]>
---
(re-diff'd against latest -git, and also attached to bypass mailer issues)

applied 1-3

Unfortunately I had to use the attachments, as again, neither git-am nor patch(1) liked the emailed result otherwise.

Jeff

Mark Lord

author

2010-07-05 14:25 UTC #14 lore raw

Re: [PATCH 4/4] libata: allow hyphenated pattern ranges

In reply to Mark Lord

Enable use of hyphenated pattern ranges in glob_match(), similar to how shell globbing works, and how developers might expect things to work.

Signed-off-by Mark Lord <[email protected]>

---

Patch also attached, to get around ongoing mailer issues here. Eventually, glob_match() will move out of libata into lib/string.c or similar.

drivers/ata/libata-core.c +12 -4

@@ -4283,11 +4283,13 @@
  *		?	matches any single character.
  *		*	matches any run of characters.
  *		[xyz]	matches a single character from the set: x, y, or z.
+ *		[a-d]	matches a single character from the range: a, b, c, or d.
+ *		[a-d0-9] matches a single character from either range.
  *
- *	Note: hyphenated ranges [0-9] are _not_ supported here.
- *	The special characters ?, [, or *, can be matched using a set, eg. [*]
+ *	The special characters ?, [, -, or *, can be matched using a set, eg. [*]
+ *	Behaviour with malformed patterns is undefined, though generally reasonable.
  *
- *	Example patterns:  "SD1?",  "SD1[012345]",  "*R0",  SD*1?[012]*xx"
+ *	Example patterns:  "SD1?",  "SD1[0-5]",  "*R0",  SD*1?[012]*xx"
  *
  *	This function uses one level of recursion per '*' in pattern.
  *	Since it calls _nothing_ else, and has _no_ explicit local variables,

@@ -4307,7 +4309,13 @@
 			/* Match single char against a '[' bracketed ']' pattern set */
 			if (!*text || *pattern != '[')
 				break;  /* Not a pattern set */
-			while (*++pattern && *pattern != ']' && *text != *pattern);
+			while (*++pattern && *pattern != ']' && *text != *pattern) {
+				if (*pattern == '-' && *(pattern - 1) != '[')
+					if (*text > *(pattern - 1) && *text < *(pattern + 1)) {
+						++pattern;
+						break;
+					}
+			}
 			if (!*pattern || *pattern == ']')
 				return 1;  /* No match */
 			while (*pattern && *pattern++ != ']');

Enable use of hyphenated pattern ranges in glob_match(), similar to how shell globbing works, and how developers might expect things to work.

Signed-off-by Mark Lord <[email protected]>

---

Patch also attached, to get around ongoing mailer issues here. Eventually, glob_match() will move out of libata into lib/string.c or similar.

drivers/ata/libata-core.c +12 -4

@@ -4283,11 +4283,13 @@
  *		?	matches any single character.
  *		*	matches any run of characters.
  *		[xyz]	matches a single character from the set: x, y, or z.
+ *		[a-d]	matches a single character from the range: a, b, c, or d.
+ *		[a-d0-9] matches a single character from either range.
  *
- *	Note: hyphenated ranges [0-9] are _not_ supported here.
- *	The special characters ?, [, or *, can be matched using a set, eg. [*]
+ *	The special characters ?, [, -, or *, can be matched using a set, eg. [*]
+ *	Behaviour with malformed patterns is undefined, though generally reasonable.
  *
- *	Example patterns:  "SD1?",  "SD1[012345]",  "*R0",  SD*1?[012]*xx"
+ *	Example patterns:  "SD1?",  "SD1[0-5]",  "*R0",  SD*1?[012]*xx"
  *
  *	This function uses one level of recursion per '*' in pattern.
  *	Since it calls _nothing_ else, and has _no_ explicit local variables,

@@ -4307,7 +4309,13 @@
 			/* Match single char against a '[' bracketed ']' pattern set */
 			if (!*text || *pattern != '[')
 				break;  /* Not a pattern set */
-			while (*++pattern && *pattern != ']' && *text != *pattern);
+			while (*++pattern && *pattern != ']' && *text != *pattern) {
+				if (*pattern == '-' && *(pattern - 1) != '[')
+					if (*text > *(pattern - 1) && *text < *(pattern + 1)) {
+						++pattern;
+						break;
+					}
+			}
 			if (!*pattern || *pattern == ']')
 				return 1;  /* No match */
 			while (*pattern && *pattern++ != ']');

Jeff Garzik

2010-07-05 18:33 UTC #15 lore raw

Re: [PATCH 4/4] libata: allow hyphenated pattern ranges

Quoted 9 lines · Mark Lord

Enable use of hyphenated pattern ranges in glob_match(), similar to how shell globbing works, and how developers might expect things to work.
Signed-off-by Mark Lord <[email protected]>
---
Patch also attached, to get around ongoing mailer issues here. Eventually, glob_match() will move out of libata into lib/string.c or similar.

No issue with correctness... but where is the demonstrated need?

Did someone ask for this?

Jeff

Mark Lord

author

2010-07-05 21:47 UTC #16 lore raw

Re: [PATCH 4/4] libata: allow hyphenated pattern ranges

Quoted 7 lines · Jeff Garzik

Quoted 6 lines · Mark Lord
Enable use of hyphenated pattern ranges in glob_match(), similar to how shell globbing works, and how developers might expect things to work.
Signed-off-by Mark Lord <[email protected]>
---

Quoted 7 lines

Quoted 3 lines
Patch also attached, to get around ongoing mailer issues here. Eventually, glob_match() will move out of libata into lib/string.c or similar.
No issue with correctness... but where is the demonstrated need?
Did someone ask for this?

Not specifically. But since this will eventually end up as a kernel-wide library function, somebody somewhere will eventually make the reasonable assumption that it does globbing "in the usual way".

Adding the hyphenated ranges turned out to be almost trivial, so I added it on and posted the patch.

I could also send you follow-on patches to use that feature
in the ata blacklists, if you like, saving a few more bytes hither and dither. :)

Speaking of this stuff.. how do we go about moving this function to /lib/strings.c ? Can that be done via your git tree?

Thanks

Jeff Garzik

2010-07-05 22:20 UTC #17 lore raw

Re: [PATCH 4/4] libata: allow hyphenated pattern ranges

Quoted 21 lines · Mark Lord

Quoted 7 lines · Jeff Garzik
Quoted 6 lines · Mark Lord
Enable use of hyphenated pattern ranges in glob_match(), similar to how shell globbing works, and how developers might expect things to work.
Signed-off-by Mark Lord <[email protected]>
---
Quoted 7 lines
Quoted 3 lines
Patch also attached, to get around ongoing mailer issues here. Eventually, glob_match() will move out of libata into lib/string.c or similar.
No issue with correctness... but where is the demonstrated need?
Did someone ask for this?
..
Not specifically. But since this will eventually end up as a kernel-wide library function, somebody somewhere will eventually make the reasonable assumption that it does globbing "in the usual way".

Quoted 6 lines

Adding the hyphenated ranges turned out to be almost trivial, so I added it on and posted the patch.
I could also send you follow-on patches to use that feature in the ata blacklists, if you like, saving a few more bytes hither and dither. :)

I understand the logic, but typically lean towards rejecting this sort of patch due to "mission creep." It's always easy to make code do One More Thing, but without any actual users, there is little justification.

In this case, if you send a follow-on patch to _use_ this, I would be happy to apply both glob_match() and blacklist patches at that point.

Quoted 3 lines

Speaking of this stuff.. how do we go about moving this function to /lib/strings.c ?
Can that be done via your git tree?

Technically, yes, git can easily do that.

Two comments come to mind, though:

1) ditto on mission creep. lib/string.c is a burden carried by -all- Linux platforms unconditionally, whereas loading libata.ko depends on several conditions. I wouldn't advise moving it until additional usage justifies burdening all platforms -- including non-SATA tiny embedded platforms -- with glob_match() implementation.

It's becoming a real problem with lib/ in general, unfortunately. If two minor sections of the kernel (or two minor, wholly unrelated drivers) need some common code, it gets dumped into lib/ with the associated costs forced upon all.

2) once other kernel users appear, submit the "move to lib/string" patch as a distinct patch in the series of patches to driver XYZ adding a glob_match() caller. I'll happily add an Acked-by on your patch, and it can be submitted either direct to Linus/Andrew, or via the specific subsystem tree that is adding a new glob_match() caller.

I prefer the Acked-by route for patches that are only tangentially related to libata.

Jeff

Mark Lord

author

2010-07-05 22:39 UTC #18 lore raw

Re: [PATCH 4/4] libata: allow hyphenated pattern ranges

Quoted 3 lines · Jeff Garzik

In this case, if you send a follow-on patch to _use_ this, I would be happy to apply both glob_match() and blacklist patches at that point.

Okay, I'll do that. Shortly.

Quoted 5 lines

2) once other kernel users appear, submit the "move to lib/string" patch as a distinct patch in the series of patches to driver XYZ adding a glob_match() caller. I'll happily add an Acked-by on your patch, and it can be submitted either direct to Linus/Andrew, or via the specific subsystem tree that is adding a new glob_match() caller.

James was nudging for me/somebody to do the same sort of thing for SCSI black/white lists. So that'll be the next user, I suppose.

Except those'll have to go via his tree, which currently lacks the glob_match() function. So I suppose perhaps I'll just wait until after 2.6.36 opens for that.

Cheers

Mark Lord

author

2010-07-05 22:50 UTC #19 lore raw

PATCH 5/5 libata: allow hyphenated pattern ranges

In reply to Mark Lord

Take further advantage of the new glob_match() function to reduce the blacklist size, this time making use of "character ranges" in the patterns.

Signed-off-by Mark Lord <[email protected]>

---

(also attached to bypass issues with my b0rked email client).

drivers/ata/libata-core.c +4 ~~-10~~

@@ -4209,22 +4209,16 @@
 	{ "OCZ CORE_SSD",	"02.10104",	ATA_HORKAGE_NONCQ },
 
 	/* Seagate NCQ + FLUSH CACHE firmware bug */
-	{ "ST31500341AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST31500341AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST31000333AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST31000333AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640623AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST3640[36]23AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640323AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-
-	{ "ST3320813AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-
-	{ "ST3320613AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST3320[68]13AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
 	/* Blacklist entries taken from Silicon Image 3124/3132

Take further advantage of the new glob_match() function to reduce the blacklist size, this time making use of "character ranges" in the patterns.

Signed-off-by Mark Lord <[email protected]>

---

(also attached to bypass issues with my b0rked email client).

drivers/ata/libata-core.c +4 ~~-10~~

@@ -4209,22 +4209,16 @@
 	{ "OCZ CORE_SSD",	"02.10104",	ATA_HORKAGE_NONCQ },
 
 	/* Seagate NCQ + FLUSH CACHE firmware bug */
-	{ "ST31500341AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST31500341AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST31000333AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST31000333AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640623AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST3640[36]23AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640323AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-
-	{ "ST3320813AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-
-	{ "ST3320613AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST3320[68]13AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
 	/* Blacklist entries taken from Silicon Image 3124/3132

Mark Lord

author

2010-07-05 22:51 UTC #20 lore raw

Re: [PATCH 5/5] libata: allow hyphenated pattern ranges

Quoted 5 lines · Mark Lord

Take further advantage of the new glob_match() function to reduce the blacklist size,
this time making use of "character ranges" in the patterns.
Signed-off-by Mark Lord <[email protected]>

NAK that one. I'll resubmit a (v2) shortly.

Mark Lord

author

2010-07-05 22:53 UTC #21 lore raw

PATCH 5/5 libata: allow hyphenated pattern ranges (v2)

In reply to Mark Lord

Take further advantage of the new glob_match() function to reduce the blacklist size, this time making use of "character ranges" in the patterns.

Signed-off-by Mark Lord <[email protected]>

---
No change from original post -- false alarm.
(also attached to bypass issues with my b0rked email client).

drivers/ata/libata-core.c +4 ~~-10~~

@@ -4209,22 +4209,16 @@
 	{ "OCZ CORE_SSD",	"02.10104",	ATA_HORKAGE_NONCQ },
 
 	/* Seagate NCQ + FLUSH CACHE firmware bug */
-	{ "ST31500341AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST31500341AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST31000333AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST31000333AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640623AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST3640[36]23AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640323AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-
-	{ "ST3320813AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-
-	{ "ST3320613AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST3320[68]13AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
 	/* Blacklist entries taken from Silicon Image 3124/3132

Take further advantage of the new glob_match() function to reduce the blacklist size, this time making use of "character ranges" in the patterns.

Signed-off-by Mark Lord <[email protected]>

---

(also attached to bypass issues with my b0rked email client).

drivers/ata/libata-core.c +4 ~~-10~~

@@ -4209,22 +4209,16 @@
 	{ "OCZ CORE_SSD",	"02.10104",	ATA_HORKAGE_NONCQ },
 
 	/* Seagate NCQ + FLUSH CACHE firmware bug */
-	{ "ST31500341AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST31500341AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST31000333AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST31000333AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640623AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST3640[36]23AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
-	{ "ST3640323AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-
-	{ "ST3320813AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
-						ATA_HORKAGE_FIRMWARE_WARN },
-
-	{ "ST3320613AS",	"SD1[56789]",	ATA_HORKAGE_NONCQ |
+	{ "ST3320[68]13AS",	"SD1[5-9]",	ATA_HORKAGE_NONCQ |
 						ATA_HORKAGE_FIRMWARE_WARN },
 
 	/* Blacklist entries taken from Silicon Image 3124/3132

Jeff Garzik

2010-07-14 07:54 UTC #22 lore raw

Re: [PATCH 5/5] libata: allow hyphenated pattern ranges (v2)

Quoted 8 lines · Mark Lord

Take further advantage of the new glob_match() function to reduce the blacklist size,
this time making use of "character ranges" in the patterns.
Signed-off-by Mark Lord <[email protected]>
---
No change from original post -- false alarm.
(also attached to bypass issues with my b0rked email client).

applied #4 and #5

had to utilize the attachments, as mail client still does not seem to be cooperating. otherwise, patches applied and lightly tested without a hitch.