[PATCH] libata: Un-break ATA blacklist

[George Spelvin]

lib/glob.c provides a new glob_match() function, with arguments in
(pattern, string) order. It replaced a private function with arguments
in (string, pattern) order, but I didn't swap the call site...

The result was the entire ATA blacklist was effectively disabled.

The lesson for today is "I f***ed up *how* badly *how* many months ago?",
er, I mean "Nobody Tests RC Kernels On Legacy Hardware".

This was not a subtle break, but it made it through an entire RC
cycle unreported, presumably because all the people doing testing
have full-featured hardware.

(FWIW, the reason for the argument swap was because fnmatch() does it that
way, and for a while implementing a full fnmatch() was being considered.)

Fixes: 428ac5fc056e0 (libata: Use glob_match from lib/glob.c)
Reported-by: Steven Honeyman <***@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=71371#c21
Signed-off-by: George Spelvin <***@horizon.com>
Cc: <***@vger.kernel.org> # 3.17
---
I'd like to wait to add a Tested-by, but it's also important enough
I want to publicly post a fix ASAP. Mea maxima culpa.

drivers/ata/libata-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index dbdc5d3..8729a2c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4261,10 +4261,10 @@ static unsigned long ata_dev_blacklisted(const struct ata_device *dev)
ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));

while (ad->model_num) {
- if (glob_match(model_num, ad->model_num)) {
+ if (glob_match(ad->model_num, model_num)) {
if (ad->model_rev == NULL)
return ad->horkage;
- if (glob_match(model_rev, ad->model_rev))
+ if (glob_match(ad->model_rev, model_rev))
return ad->horkage;
}
ad++;
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Steven Honeyman]

Post by George Spelvin
lib/glob.c provides a new glob_match() function, with arguments in
(pattern, string) order. It replaced a private function with arguments
in (string, pattern) order, but I didn't swap the call site...
The result was the entire ATA blacklist was effectively disabled.

Oops :)

Post by George Spelvin
The lesson for today is "I f***ed up *how* badly *how* many months ago?",
er, I mean "Nobody Tests RC Kernels On Legacy Hardware".
This was not a subtle break, but it made it through an entire RC
cycle unreported, presumably because all the people doing testing
have full-featured hardware.

Everyone makes mistakes - although I'd be very surprised there are no
testers using the same SSD as mine. It's the most common mSATA drive
around here in the UK at the moment (even if it has a slightly dodgy
firmware!).

Post by George Spelvin
I'd like to wait to add a Tested-by, but it's also important enough
I want to publicly post a fix ASAP

Pesky timezone differences! I was at work when you posted this patch,
but just got home and recompiled. I can confirm this works:
[ 3.845109] ata5.00: disabling queued TRIM support
[ 3.845112] ata5.00: ATA-9: Crucial_CT240M500SSD3, MU05, max UDMA/133

...although the patch didn't apply as a quick copy-paste job from
Gmail ...but that's an unrelated matter :)


Thanks again,
Steven.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[George Spelvin]

lib/glob.c provides a new glob_match() function, with arguments in
(pattern, string) order. It replaced a private function with arguments
in (string, pattern) order, but I didn't swap the call site...

The result was the entire ATA blacklist was effectively disabled.
Anyone have a brown paper bag to spare?

The lesson for today is "I f***ed up *how* badly *how* many months ago?",
er, I mean "RC Kernels Get Lousy Test Coverage".

This was not a subtle break, but it made it through an entire RC
cycle unreported, presumably because all the people doing testing
have shiny bug-free hardware.

(BTW, the original reason for the argument swap was to match fnmatch(3),
as for a while implementing a full fnmatch() was being considered.)

Fixes: 428ac5fc056e0 (libata: Use glob_match from lib/glob.c)
Reported-by: Steven Honeyman <***@gmail.com>
Signed-off-by: George Spelvin <***@horizon.com>
Tested-by: Steven Honeyman <***@gmail.com>
Cc: <***@vger.kernel.org> # 3.17
---
No code changes for v2, just added Tested-by: and a stronger Subject line.

I hope Linus' slow merging this week doesn't stop this getting into
3.17.1 PDQ, since I expect a lot of well-deserved ire from people
trying 3.17.0.

drivers/ata/libata-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index dbdc5d3..8729a2c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4261,10 +4261,10 @@ static unsigned long ata_dev_blacklisted(const struct ata_device *dev)
ata_id_c_string(dev->id, model_rev, ATA_ID_FW_REV, sizeof(model_rev));

while (ad->model_num) {
- if (glob_match(model_num, ad->model_num)) {
+ if (glob_match(ad->model_num, model_num)) {
if (ad->model_rev == NULL)
return ad->horkage;
- if (glob_match(model_rev, ad->model_rev))
+ if (glob_match(ad->model_rev, model_rev))
return ad->horkage;
}
ad++;

[George Spelvin]

Thanks for the ack.

Post by Steven Honeyman
Everyone makes mistakes - although I'd be very surprised there are no
testers using the same SSD as mine. It's the most common mSATA drive
around here in the UK at the moment (even if it has a slightly dodgy
firmware!).

Yeah, but this one is particularly simple *and* particularly public.
I'm feeling a lot of sympathy for the Mars Climate Orbiter people ATM.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Tejun Heo]

Post by George Spelvin
lib/glob.c provides a new glob_match() function, with arguments in
(pattern, string) order. It replaced a private function with arguments
in (string, pattern) order, but I didn't swap the call site...
The result was the entire ATA blacklist was effectively disabled.
The lesson for today is "I f***ed up *how* badly *how* many months ago?",
er, I mean "Nobody Tests RC Kernels On Legacy Hardware".
This was not a subtle break, but it made it through an entire RC
cycle unreported, presumably because all the people doing testing
have full-featured hardware.
(FWIW, the reason for the argument swap was because fnmatch() does it that
way, and for a while implementing a full fnmatch() was being considered.)
Fixes: 428ac5fc056e0 (libata: Use glob_match from lib/glob.c)
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=71371#c21

Oops, lol. Appiled to libata/for-3.17-fixes.

Thanks!

[George Spelvin]

Post by Tejun Heo
Oops, lol. Appiled to libata/for-3.17-fixes.

I'm delighted that you can laugh; I was expecting a more
fulminating response.

The most amusing part is my oscillation between "how the
heck did I miss that?" and "how the heck did the *entire
rest of the world* miss that?"
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html