[PATCH] libata: Un-break ATA blacklist

[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 <stevenhoneyman@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=71371#c21
Signed-off-by: George Spelvin <linux@horizon.com>
Cc: <stable@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++;

Re: [PATCH] libata: Un-break ATA blacklist

[Steven Honeyman]

On 7 October 2014 12:26, George Spelvin <linux@horizon.com> wrote:
> 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 :)

> 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!).

> 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.

Re: [PATCH] libata: Un-break ATA blacklist

[Tejun Heo]

On Tue, Oct 07, 2014 at 07:26:38AM -0400, George Spelvin wrote:
> 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 <stevenhoneyman@gmail.com>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=71371#c21
> Signed-off-by: George Spelvin <linux@horizon.com>
> Cc: <stable@vger.kernel.org> # 3.17

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

Thanks!

-- 
tejun

[urgent regression PATCH v2] 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.
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 <stevenhoneyman@gmail.com>
Signed-off-by: George Spelvin <linux@horizon.com>
Tested-by: Steven Honeyman <stevenhoneyman@gmail.com>
Cc: <stable@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++;
-- 
2.1.1

Re: [PATCH] libata: Un-break ATA blacklist

[George Spelvin]

Thanks for the ack.

> 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.

Re: [PATCH] libata: Un-break ATA blacklist

[George Spelvin]

> 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?"