Add Seagate STT20000A to DMA blacklist.

Add Seagate STT20000A to DMA blacklist.

[Dave Jones]

http://bugzilla.kernel.org/show_bug.cgi?id=1044
has been open for _four_ years with a patch available.
Here's a rediffed version of the same.

Signed-off-by: Dave Jones <davej@redhat.com>

--- linux-2.6/drivers/ide/ide-dma.c~	2007-05-21 10:48:11.000000000 -0400
+++ linux-2.6/drivers/ide/ide-dma.c	2007-05-21 10:48:21.000000000 -0400
@@ -127,7 +127,8 @@ static const struct drive_list_entry dri
 	{ "SAMSUNG CD-ROM SC",	"ALL"		},
 	{ "SanDisk SDP3B-64"	,	"ALL"		},
 	{ "ATAPI CD-ROM DRIVE 40X MAXIMUM",	"ALL"		},
-	{ "_NEC DV5800A",               "ALL"           },  
+	{ "_NEC DV5800A",               "ALL"           },
+	{ "Seagate STT20000A",          "ALL"           },
 	{ NULL			,	NULL		}
 
 };

-- 
http://www.codemonkey.org.uk

Re: Add Seagate STT20000A to DMA blacklist.

[Alan Cox]

On Mon, 21 May 2007 10:50:42 -0400
Dave Jones <davej@redhat.com> wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=1044
> has been open for _four_ years with a patch available.
> Here's a rediffed version of the same.

Please update libata as well when you udpate the blacklists.

Alan

Re: Add Seagate STT20000A to DMA blacklist.

[Dave Jones]

On Mon, May 21, 2007 at 05:15:51PM +0100, Alan Cox wrote:
 > On Mon, 21 May 2007 10:50:42 -0400
 > Dave Jones <davej@redhat.com> wrote:
 > 
 > > http://bugzilla.kernel.org/show_bug.cgi?id=1044
 > > has been open for _four_ years with a patch available.
 > > Here's a rediffed version of the same.
 > 
 > Please update libata as well when you udpate the blacklists.

Sure, point me at the table(s) ?

	Dave

-- 
http://www.codemonkey.org.uk

[PATCH] Match DMA blacklist entries between ide-dma.c and libata-core.c

[Junio C Hamano]

There are a few entries in ata_device_blacklist[] in libata-core.c
marked with HORKAGE_NODMA but are missing from drive_blacklist[]
in ide-dma.c.  This patch makes the lists in sync.

Also remove a duplicated entry for "SanDisk SDP3B-64".

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * Recently Alan Cox responded that libata blacklist needs to be
   kept in sync when Dave Jones added STT20000A to DMA blacklist
   in ide-dma.c
---
 drivers/ide/ide-dma.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
index b77b7d1..ead141e 100644
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -119,15 +119,17 @@ static const struct drive_list_entry drive_blacklist [] = {
 	{ "HITACHI CDR-8335"	,	"ALL"		},
 	{ "HITACHI CDR-8435"	,	"ALL"		},
 	{ "Toshiba CD-ROM XM-6202B"	,	"ALL"		},
+	{ "TOSHIBA CD-ROM XM-1702BC",	"ALL"		},
 	{ "CD-532E-A"		,	"ALL"		},
 	{ "E-IDE CD-ROM CR-840",	"ALL"		},
 	{ "CD-ROM Drive/F5A",	"ALL"		},
 	{ "WPI CDD-820",		"ALL"		},
 	{ "SAMSUNG CD-ROM SC-148C",	"ALL"		},
 	{ "SAMSUNG CD-ROM SC",	"ALL"		},
-	{ "SanDisk SDP3B-64"	,	"ALL"		},
 	{ "ATAPI CD-ROM DRIVE 40X MAXIMUM",	"ALL"		},
 	{ "_NEC DV5800A",               "ALL"           },  
+	{ "SAMSUNG CD-ROM SN-124",	"N001" },
+	{ "Seagate STT20000A",		"ALL" },
 	{ NULL			,	NULL		}
 
 };
-- 
1.5.2.24.g93d4

[PATCH 2/3] Unify dma blacklist in ide-dma.c and libata-core.c

[Junio C Hamano]

This introduces a shared header file that defines the entries
for two dma blacklists in ide-dma.c and libata-core.c to make it
easier to keep them in sync.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * Removes more lines than it adds.  I am not proud of the
   DMA_BLACK_LIST macro in ide-dma.c which relies on the
   compiler doing a sane thing for compile time constant
   expression to initialize the list, but that hopefully can be
   fixed in the next patch.

 drivers/ata/libata-core.c   |   34 ++++------------------------------
 drivers/ide/dma-blacklist.h |   39 +++++++++++++++++++++++++++++++++++++++
 drivers/ide/ide-dma.c       |   33 +++------------------------------
 3 files changed, 46 insertions(+), 60 deletions(-)
 create mode 100644 drivers/ide/dma-blacklist.h

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index a6de57e..93b7fa7 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3739,36 +3739,10 @@ struct ata_blacklist_entry {
 
 static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	/* Devices with DMA related problems under Linux */
-	{ "WDC AC11000H",	NULL,		ATA_HORKAGE_NODMA },
-	{ "WDC AC22100H",	NULL,		ATA_HORKAGE_NODMA },
-	{ "WDC AC32500H",	NULL,		ATA_HORKAGE_NODMA },
-	{ "WDC AC33100H",	NULL,		ATA_HORKAGE_NODMA },
-	{ "WDC AC31600H",	NULL,		ATA_HORKAGE_NODMA },
-	{ "WDC AC32100H",	"24.09P07",	ATA_HORKAGE_NODMA },
-	{ "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-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 },
-	{ "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 },
-	{ "E-IDE CD-ROM CR-840",NULL,		ATA_HORKAGE_NODMA },
-	{ "CD-ROM Drive/F5A",	NULL,		ATA_HORKAGE_NODMA },
-	{ "WPI CDD-820", 	NULL,		ATA_HORKAGE_NODMA },
-	{ "SAMSUNG CD-ROM SC-148C", NULL,	ATA_HORKAGE_NODMA },
-	{ "SAMSUNG CD-ROM SC",	NULL,		ATA_HORKAGE_NODMA },
-	{ "ATAPI CD-ROM DRIVE 40X MAXIMUM",NULL,ATA_HORKAGE_NODMA },
-	{ "_NEC DV5800A", 	NULL,		ATA_HORKAGE_NODMA },
-	{ "SAMSUNG CD-ROM SN-124","N001",	ATA_HORKAGE_NODMA },
-	{ "Seagate STT20000A", NULL,		ATA_HORKAGE_NODMA },
+
+#define DMA_BLACK_LIST(model,rev) { (model), (rev), ATA_HORKAGE_NODMA }
+#include "../ide/dma-blacklist.h"
+#undef DMA_BLACK_LIST
 
 	/* Weird ATAPI devices */
 	{ "TORiSAN DVD-ROM DRD-N216", NULL,	ATA_HORKAGE_MAX_SEC_128 |
diff --git a/drivers/ide/dma-blacklist.h b/drivers/ide/dma-blacklist.h
new file mode 100644
index 0000000..19b4e0c
--- /dev/null
+++ b/drivers/ide/dma-blacklist.h
@@ -0,0 +1,39 @@
+/*
+ * Shared between ide-dma.c::drive_blacklist[] and
+ * ../ata/libata-core.c::ata_device_blacklist[].
+ *
+ * Each of the above users define DMA_BLACK_LIST() macro
+ * which expands these to structure of their liking.
+ */
+
+	DMA_BLACK_LIST("WDC AC11000H", NULL),
+	DMA_BLACK_LIST("WDC AC22100H", NULL),
+	DMA_BLACK_LIST("WDC AC32500H", NULL),
+	DMA_BLACK_LIST("WDC AC33100H", NULL),
+	DMA_BLACK_LIST("WDC AC31600H", NULL),
+	DMA_BLACK_LIST("WDC AC32100H", "24.09P07"),
+	DMA_BLACK_LIST("WDC AC23200L", "21.10N21"),
+	DMA_BLACK_LIST("Compaq CRD-8241B", NULL),
+	DMA_BLACK_LIST("CRD-8400B", NULL),
+	DMA_BLACK_LIST("CRD-8480B", NULL),
+	DMA_BLACK_LIST("CRD-8482B", NULL),
+	DMA_BLACK_LIST("CRD-84", NULL),
+	DMA_BLACK_LIST("SanDisk SDP3B", NULL),
+	DMA_BLACK_LIST("SanDisk SDP3B-64", NULL),
+	DMA_BLACK_LIST("SANYO CD-ROM CRD", NULL),
+	DMA_BLACK_LIST("HITACHI CDR-8", NULL),
+	DMA_BLACK_LIST("HITACHI CDR-8335", NULL),
+	DMA_BLACK_LIST("HITACHI CDR-8435", NULL),
+	DMA_BLACK_LIST("Toshiba CD-ROM XM-6202B", NULL),
+	DMA_BLACK_LIST("TOSHIBA CD-ROM XM-1702BC", NULL),
+	DMA_BLACK_LIST("CD-532E-A", NULL),
+	DMA_BLACK_LIST("E-IDE CD-ROM CR-840", NULL),
+	DMA_BLACK_LIST("CD-ROM Drive/F5A", NULL),
+	DMA_BLACK_LIST("WPI CDD-820", NULL),
+	DMA_BLACK_LIST("SAMSUNG CD-ROM SC-148C", NULL),
+	DMA_BLACK_LIST("SAMSUNG CD-ROM SC", NULL),
+	DMA_BLACK_LIST("ATAPI CD-ROM DRIVE 40X MAXIMUM", NULL),
+	DMA_BLACK_LIST("_NEC DV5800A", NULL),
+	DMA_BLACK_LIST("SAMSUNG CD-ROM SN-124", "N001"),
+	DMA_BLACK_LIST("Seagate STT20000A", NULL),
+
diff --git a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
index ead141e..a6a2074 100644
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -100,36 +100,9 @@ static const struct drive_list_entry drive_whitelist [] = {
 
 static const struct drive_list_entry drive_blacklist [] = {
 
-	{ "WDC AC11000H"	,	"ALL"		},
-	{ "WDC AC22100H"	,	"ALL"		},
-	{ "WDC AC32500H"	,	"ALL"		},
-	{ "WDC AC33100H"	,	"ALL"		},
-	{ "WDC AC31600H"	,	"ALL"		},
-	{ "WDC AC32100H"	,	"24.09P07"	},
-	{ "WDC AC23200L"	,	"21.10N21"	},
-	{ "Compaq CRD-8241B"	,	"ALL"		},
-	{ "CRD-8400B"		,	"ALL"		},
-	{ "CRD-8480B",			"ALL"		},
-	{ "CRD-8482B",			"ALL"		},
- 	{ "CRD-84"		,	"ALL"		},
-	{ "SanDisk SDP3B"	,	"ALL"		},
-	{ "SanDisk SDP3B-64"	,	"ALL"		},
-	{ "SANYO CD-ROM CRD"	,	"ALL"		},
-	{ "HITACHI CDR-8"	,	"ALL"		},
-	{ "HITACHI CDR-8335"	,	"ALL"		},
-	{ "HITACHI CDR-8435"	,	"ALL"		},
-	{ "Toshiba CD-ROM XM-6202B"	,	"ALL"		},
-	{ "TOSHIBA CD-ROM XM-1702BC",	"ALL"		},
-	{ "CD-532E-A"		,	"ALL"		},
-	{ "E-IDE CD-ROM CR-840",	"ALL"		},
-	{ "CD-ROM Drive/F5A",	"ALL"		},
-	{ "WPI CDD-820",		"ALL"		},
-	{ "SAMSUNG CD-ROM SC-148C",	"ALL"		},
-	{ "SAMSUNG CD-ROM SC",	"ALL"		},
-	{ "ATAPI CD-ROM DRIVE 40X MAXIMUM",	"ALL"		},
-	{ "_NEC DV5800A",               "ALL"           },  
-	{ "SAMSUNG CD-ROM SN-124",	"N001" },
-	{ "Seagate STT20000A",		"ALL" },
+#define DMA_BLACK_LIST(model,rev) { (model), (rev==NULL ? "ALL" : (rev)) }
+#include "dma-blacklist.h"
+#undef DMA_BLACK_LIST
 	{ NULL			,	NULL		}
 
 };
-- 
1.5.2.24.g93d4

Re: [PATCH 2/3] Unify dma blacklist in ide-dma.c and libata-core.c

[Bill Davidsen]

Junio C Hamano wrote:
> This introduces a shared header file that defines the entries
> for two dma blacklists in ide-dma.c and libata-core.c to make it
> easier to keep them in sync.
> 
Why wasn't this done this way in the first place? Out of tree 
development for libata or something?

-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot

Re: [PATCH 2/3] Unify dma blacklist in ide-dma.c and libata-core.c

[Alan Cox]

On Mon, 21 May 2007 22:06:36 -0700
Junio C Hamano <junkio@cox.net> wrote:

> This introduces a shared header file that defines the entries
> for two dma blacklists in ide-dma.c and libata-core.c to make it
> easier to keep them in sync.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

NAK

This makes stuff so much more ugly its not funny

> ---
> 
>  * Removes more lines than it adds.  I am not proud of the
>    DMA_BLACK_LIST macro in ide-dma.c which relies on the
>    compiler doing a sane thing for compile time constant
>    expression to initialize the list, but that hopefully can be
>    fixed in the next patch.

The basic concept is sound but it would be better to make sure
the ATA_HORKAGE macros and struct ata_blacklist_entry as well as all the
blacklist stuff was placed together in one file to avoid the horrors you
are proposing and then used by both driver sets.

[PATCH 3/3] Make ide dma blacklist handling a bit saner.

[Junio C Hamano]

Earlier, the matching of (model,rev) in ide-dma black/white list
handling was to consider "ALL" in the table to match any
revision.  This changes the wildcard to NULL.  This way, the
DMA_BLACK_LIST macro used in the previous patch does not have to
use a slightly funky compile time constant expression to convert
NULL to "ALL".

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * I do not really know what I am doing in the mips area, but
   that architecture specific table seems to be used by the same
   ide_in_drive_list() function, so the entries are matched to
   the updated code.

 drivers/ide/ide-dma.c                     |   14 +++++++-------
 include/asm-mips/mach-au1x00/au1xxx_ide.h |   28 ++++++++++++++--------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
index a6a2074..c0b5b10 100644
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -91,16 +91,16 @@
 
 static const struct drive_list_entry drive_whitelist [] = {
 
-	{ "Micropolis 2112A"	,       "ALL"		},
-	{ "CONNER CTMA 4000"	,       "ALL"		},
-	{ "CONNER CTT8000-A"	,       "ALL"		},
-	{ "ST34342A"		,	"ALL"		},
+	{ "Micropolis 2112A"	,       NULL		},
+	{ "CONNER CTMA 4000"	,       NULL		},
+	{ "CONNER CTT8000-A"	,       NULL		},
+	{ "ST34342A"		,	NULL		},
 	{ NULL			,	NULL		}
 };
 
 static const struct drive_list_entry drive_blacklist [] = {
 
-#define DMA_BLACK_LIST(model,rev) { (model), (rev==NULL ? "ALL" : (rev)) }
+#define DMA_BLACK_LIST(model,rev) { (model), (rev) }
 #include "dma-blacklist.h"
 #undef DMA_BLACK_LIST
 	{ NULL			,	NULL		}
@@ -120,8 +120,8 @@ int ide_in_drive_list(struct hd_driveid *id, const struct drive_list_entry *driv
 {
 	for ( ; drive_table->id_model ; drive_table++)
 		if ((!strcmp(drive_table->id_model, id->model)) &&
-		    ((strstr(id->fw_rev, drive_table->id_firmware)) ||
-		     (!strcmp(drive_table->id_firmware, "ALL"))))
+		    (!drive_table->id_firmware ||
+		     strstr(id->fw_rev, drive_table->id_firmware)))
 			return 1;
 	return 0;
 }
diff --git a/include/asm-mips/mach-au1x00/au1xxx_ide.h b/include/asm-mips/mach-au1x00/au1xxx_ide.h
index 8fcae21..4663e8b 100644
--- a/include/asm-mips/mach-au1x00/au1xxx_ide.h
+++ b/include/asm-mips/mach-au1x00/au1xxx_ide.h
@@ -88,26 +88,26 @@ static const struct drive_list_entry dma_white_list [] = {
 /*
  * Hitachi
  */
-        { "HITACHI_DK14FA-20"    ,       "ALL"           },
-        { "HTS726060M9AT00"      ,       "ALL"           },
+        { "HITACHI_DK14FA-20"    ,       NULL            },
+        { "HTS726060M9AT00"      ,       NULL            },
 /*
  * Maxtor
  */
-        { "Maxtor 6E040L0"      ,       "ALL"           },
-        { "Maxtor 6Y080P0"      ,       "ALL"           },
-        { "Maxtor 6Y160P0"      ,       "ALL"           },
+        { "Maxtor 6E040L0"      ,       NULL            },
+        { "Maxtor 6Y080P0"      ,       NULL            },
+        { "Maxtor 6Y160P0"      ,       NULL            },
 /*
  * Seagate
  */
-        { "ST3120026A"          ,       "ALL"           },
-        { "ST320014A"           ,       "ALL"           },
-        { "ST94011A"            ,       "ALL"           },
-        { "ST340016A"           ,       "ALL"           },
+        { "ST3120026A"          ,       NULL            },
+        { "ST320014A"           ,       NULL            },
+        { "ST94011A"            ,       NULL            },
+        { "ST340016A"           ,       NULL            },
 /*
  * Western Digital
  */
-        { "WDC WD400UE-00HCT0"  ,       "ALL"           },
-        { "WDC WD400JB-00JJC0"  ,       "ALL"           },
+        { "WDC WD400UE-00HCT0"  ,       NULL            },
+        { "WDC WD400JB-00JJC0"  ,       NULL            },
         { NULL                  ,       NULL            }
 };
 
@@ -116,9 +116,9 @@ static const struct drive_list_entry dma_black_list [] = {
 /*
  * Western Digital
  */
-        { "WDC WD100EB-00CGH0"  ,       "ALL"           },
-        { "WDC WD200BB-00AUA1"  ,       "ALL"           },
-        { "WDC AC24300L"        ,       "ALL"           },
+        { "WDC WD100EB-00CGH0"  ,       NULL            },
+        { "WDC WD200BB-00AUA1"  ,       NULL            },
+        { "WDC AC24300L"        ,       NULL            },
         { NULL                  ,       NULL            }
 };
 #endif
-- 
1.5.2.24.g93d4

Re: [PATCH 3/3] Make ide dma blacklist handling a bit saner.

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday 22 May 2007, Junio C Hamano wrote:
> Earlier, the matching of (model,rev) in ide-dma black/white list
> handling was to consider "ALL" in the table to match any
> revision.  This changes the wildcard to NULL.  This way, the
> DMA_BLACK_LIST macro used in the previous patch does not have to
> use a slightly funky compile time constant expression to convert
> NULL to "ALL".
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

The change itself looks good but IMO it is worth doing it before patch #2/3
(it would also make it possible for me to merge this patch immediately).

When it comes to patch #2 - Alan's comment may be a bit harsh but he seems
to be right - there should be a common library-like file (ata-blacklist.c
or ata-quirks.c or whatever name you like) containing ata_device_blacklist[].

This would require slight modification of ide_in_drive_list() to teach
it about ATA_HORKAGE_DMA but as I see from this patch this shouldn't be
a problem for you. ;)  Please also note that <linux/ata.h> is used by both
IDE and libata so it should be a good place to put struct ata_blacklist_entry
and ATA_HORKAGE_* macros.

Care to respin both patches?

>  * I do not really know what I am doing in the mips area, but
>    that architecture specific table seems to be used by the same
>    ide_in_drive_list() function, so the entries are matched to
>    the updated code.

Yep, thanks for not breaking AMD Alchemy IDE suppport.

It looks like au1xxx_ide.h should really be in drivers/ide/mips because
it is only included from drivers/ide/mips/au1xxx_ide.c - could somebody
verify this and send me a patch?

Thanks,
Bart

Re: [PATCH 3/3] Make ide dma blacklist handling a bit saner.

[Junio C Hamano]

Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> writes:

> The change itself looks good but IMO it is worth doing it before patch #2/3
> (it would also make it possible for me to merge this patch immediately).

Yes, I should have considered that the earlier #2/3 needs
coordination between you and Jeff.

> When it comes to patch #2 - Alan's comment may be a bit harsh but he seems
> to be right - there should be a common library-like file (ata-blacklist.c
> or ata-quirks.c or whatever name you like) containing ata_device_blacklist[].
>
> This would require slight modification of ide_in_drive_list() to teach
> it about ATA_HORKAGE_DMA ... Please also note that <linux/ata.h> is used by both
> IDE and libata so it should be a good place to put struct ata_blacklist_entry
> and ATA_HORKAGE_* macros.

Thanks for the hint.  Alan is correct to point out that I
cheated. ;-)  If I understand correctly, the change would
involve:

 - create a new file that has ata_device_blacklist[] whose type
   is "struct ata_blacklist_entry" (i.e. matches libata-core),
   by separating the table out of ata/libata-core.c.

   Q1. should that file go to drivers/ata/ or drivers/ide/?

 - make that file depended on when either libata and/or IDE is
   selected.

   Q2. Kconfig dependency rule is needed for this, perhaps.  How
   should that look like?

 - some out-of-tree drivers might be using ide_in_drive_list()
   and relying on it to take "struct drive_list_entry"; create a
   new function, ide_in_device_list(), that takes "struct
   ata_blacklist_entry" as its parameter.

   Q3. Is the 'out-of-tree drivers' a real issue, and if so, is
   the above a reasonable avenue to take?

 - convert in-tree callers to use ide_in_device_list() instead,
   feeding it ata_device_blacklist[], and remove drive_blacklist[]
   from drivers/ide/ide-dma.c.

   Q4. What would you like to do for drive_whitelist[]?

> Care to respin both patches?

Before the questions are answered I cannot respin the earlier
#2/3, but I can certainly respin #3/3.

Re: [PATCH 3/3] Make ide dma blacklist handling a bit saner.

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday 29 May 2007, Junio C Hamano wrote:
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> writes:
> 
> > The change itself looks good but IMO it is worth doing it before patch #2/3
> > (it would also make it possible for me to merge this patch immediately).
> 
> Yes, I should have considered that the earlier #2/3 needs
> coordination between you and Jeff.
> 
> > When it comes to patch #2 - Alan's comment may be a bit harsh but he seems
> > to be right - there should be a common library-like file (ata-blacklist.c
> > or ata-quirks.c or whatever name you like) containing ata_device_blacklist[].
> >
> > This would require slight modification of ide_in_drive_list() to teach
> > it about ATA_HORKAGE_DMA ... Please also note that <linux/ata.h> is used by both
> > IDE and libata so it should be a good place to put struct ata_blacklist_entry
> > and ATA_HORKAGE_* macros.
> 
> Thanks for the hint.  Alan is correct to point out that I
> cheated. ;-)  If I understand correctly, the change would
> involve:
> 
>  - create a new file that has ata_device_blacklist[] whose type
>    is "struct ata_blacklist_entry" (i.e. matches libata-core),
>    by separating the table out of ata/libata-core.c.
> 
>    Q1. should that file go to drivers/ata/ or drivers/ide/?

No strong preference here but I think that it should be drivers/ata/ since it
is updated more frequently (NCQ etc).  Either way there needs to be a BIG FAT
comment at the top of the file explaining that the file is used by both IDE
and SCSI/libata subsystems.

For the new file (i.e. ata-quirks) to be a shared library it needs to be
similar to libraries from lib/ (ie. lib/crc16c).  After some thought I'm
convinced that it is an optimal solution.  It allows the real code sharing
and at the same time it is the simplest when it comes to the build rules.

Of course I can be missing something really obvious which would prevent
this scheme from working...  :)

>  - make that file depended on when either libata and/or IDE is
>    selected.
> 
>    Q2. Kconfig dependency rule is needed for this, perhaps.  How
>    should that look like?

Lets assume that the new config entry will be CONFIG_ATA_QUIRKS
and that it shouldn't be visible during kernel configuration.

* drivers/ata/Kconfig:

menuconfig ATA should select ATA_QUIRKS

...

config ATA_QUIRKS
	tristate
	depends on ATA || BLK_DEV_IDE

* drivers/ide/Kconfig:

config BLK_DEV_IDEDMA should select ATA_QUIRKS

Or something similar... now to the Makefile modifications...

* drivers/ata/Makefile:

Add rule for obj-$(CONFIG_ATA_QUIRKS) at the top of the file.

* drivers/Makefile:

Replace obj-$(CONFIG_ATA) with obj-y (so ata-quirks library
gets compiled even if it is selected only by IDE subsystem).

Since ata-quirks is a stand-alone module the kernel build
system should take care of the rest.

>  - some out-of-tree drivers might be using ide_in_drive_list()
>    and relying on it to take "struct drive_list_entry"; create a
>    new function, ide_in_device_list(), that takes "struct
>    ata_blacklist_entry" as its parameter.
> 
>    Q3. Is the 'out-of-tree drivers' a real issue, and if so, is
>    the above a reasonable avenue to take?

Not a real issue, I'm not aware of any out of tree IDE drivers.

>  - convert in-tree callers to use ide_in_device_list() instead,
>    feeding it ata_device_blacklist[], and remove drive_blacklist[]
>    from drivers/ide/ide-dma.c.
> 
>    Q4. What would you like to do for drive_whitelist[]?

Add ATA_HORKAGE_DMA_OK and add drive_whitelist[] entries back to
ata_device_blacklist[]?  Unless somebody has a better idea...

> > Care to respin both patches?
> 
> Before the questions are answered I cannot respin the earlier
> #2/3, but I can certainly respin #3/3.

Thanks, all three patches were applied.

Bart

[PATCH 1/3] ide_in_drive_list(): accept NULL as the wildcard for firmware revision.

[Junio C Hamano]

Earlier, the matching of (model,rev) in ide-dma black/white list
handling was to consider "ALL" in the table to match any
revision.  This makes NULL to be also an accepted wildcard, and
changes the entries of tables in ide-dma.c to use NULL.

The code still accepts "ALL" as the wildcard, in order to keep
any out-of-tree ide driver that feeds its own table that uses
"ALL" as the wildcard to ide_in_drive_list() function from
breaking.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * This is a respin of earlier "3/3 Make ide dma blacklist handling a bit saner."

 drivers/ide/ide-dma.c |   67 +++++++++++++++++++++++++------------------------
 1 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
index ead141e..230ea12 100644
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -91,45 +91,45 @@
 
 static const struct drive_list_entry drive_whitelist [] = {
 
-	{ "Micropolis 2112A"	,       "ALL"		},
-	{ "CONNER CTMA 4000"	,       "ALL"		},
-	{ "CONNER CTT8000-A"	,       "ALL"		},
-	{ "ST34342A"		,	"ALL"		},
+	{ "Micropolis 2112A"	,       NULL		},
+	{ "CONNER CTMA 4000"	,       NULL		},
+	{ "CONNER CTT8000-A"	,       NULL		},
+	{ "ST34342A"		,	NULL		},
 	{ NULL			,	NULL		}
 };
 
 static const struct drive_list_entry drive_blacklist [] = {
 
-	{ "WDC AC11000H"	,	"ALL"		},
-	{ "WDC AC22100H"	,	"ALL"		},
-	{ "WDC AC32500H"	,	"ALL"		},
-	{ "WDC AC33100H"	,	"ALL"		},
-	{ "WDC AC31600H"	,	"ALL"		},
+	{ "WDC AC11000H"	,	NULL 		},
+	{ "WDC AC22100H"	,	NULL 		},
+	{ "WDC AC32500H"	,	NULL 		},
+	{ "WDC AC33100H"	,	NULL 		},
+	{ "WDC AC31600H"	,	NULL 		},
 	{ "WDC AC32100H"	,	"24.09P07"	},
 	{ "WDC AC23200L"	,	"21.10N21"	},
-	{ "Compaq CRD-8241B"	,	"ALL"		},
-	{ "CRD-8400B"		,	"ALL"		},
-	{ "CRD-8480B",			"ALL"		},
-	{ "CRD-8482B",			"ALL"		},
- 	{ "CRD-84"		,	"ALL"		},
-	{ "SanDisk SDP3B"	,	"ALL"		},
-	{ "SanDisk SDP3B-64"	,	"ALL"		},
-	{ "SANYO CD-ROM CRD"	,	"ALL"		},
-	{ "HITACHI CDR-8"	,	"ALL"		},
-	{ "HITACHI CDR-8335"	,	"ALL"		},
-	{ "HITACHI CDR-8435"	,	"ALL"		},
-	{ "Toshiba CD-ROM XM-6202B"	,	"ALL"		},
-	{ "TOSHIBA CD-ROM XM-1702BC",	"ALL"		},
-	{ "CD-532E-A"		,	"ALL"		},
-	{ "E-IDE CD-ROM CR-840",	"ALL"		},
-	{ "CD-ROM Drive/F5A",	"ALL"		},
-	{ "WPI CDD-820",		"ALL"		},
-	{ "SAMSUNG CD-ROM SC-148C",	"ALL"		},
-	{ "SAMSUNG CD-ROM SC",	"ALL"		},
-	{ "ATAPI CD-ROM DRIVE 40X MAXIMUM",	"ALL"		},
-	{ "_NEC DV5800A",               "ALL"           },  
+	{ "Compaq CRD-8241B"	,	NULL 		},
+	{ "CRD-8400B"		,	NULL 		},
+	{ "CRD-8480B",			NULL 		},
+	{ "CRD-8482B",			NULL 		},
+	{ "CRD-84"		,	NULL 		},
+	{ "SanDisk SDP3B"	,	NULL 		},
+	{ "SanDisk SDP3B-64"	,	NULL 		},
+	{ "SANYO CD-ROM CRD"	,	NULL 		},
+	{ "HITACHI CDR-8"	,	NULL 		},
+	{ "HITACHI CDR-8335"	,	NULL 		},
+	{ "HITACHI CDR-8435"	,	NULL 		},
+	{ "Toshiba CD-ROM XM-6202B"	,	NULL 		},
+	{ "TOSHIBA CD-ROM XM-1702BC",	NULL 		},
+	{ "CD-532E-A"		,	NULL 		},
+	{ "E-IDE CD-ROM CR-840",	NULL 		},
+	{ "CD-ROM Drive/F5A",	NULL 		},
+	{ "WPI CDD-820",		NULL 		},
+	{ "SAMSUNG CD-ROM SC-148C",	NULL 		},
+	{ "SAMSUNG CD-ROM SC",	NULL 		},
+	{ "ATAPI CD-ROM DRIVE 40X MAXIMUM",	NULL 		},
+	{ "_NEC DV5800A",               NULL            },
 	{ "SAMSUNG CD-ROM SN-124",	"N001" },
-	{ "Seagate STT20000A",		"ALL" },
+	{ "Seagate STT20000A",		NULL  },
 	{ NULL			,	NULL		}
 
 };
@@ -147,8 +147,9 @@ int ide_in_drive_list(struct hd_driveid *id, const struct drive_list_entry *driv
 {
 	for ( ; drive_table->id_model ; drive_table++)
 		if ((!strcmp(drive_table->id_model, id->model)) &&
-		    ((strstr(id->fw_rev, drive_table->id_firmware)) ||
-		     (!strcmp(drive_table->id_firmware, "ALL"))))
+		    (!drive_table->id_firmware ||
+		     !strcmp(drive_table->id_firmware, "ALL") || /* to be removed later */
+		     strstr(id->fw_rev, drive_table->id_firmware)))
 			return 1;
 	return 0;
 }
-- 
1.5.2.86.g99b5-dirty

Re: [PATCH 1/3] ide_in_drive_list(): accept NULL as the wildcard for firmware revision.

[Bartlomiej Zolnierkiewicz]

On Tuesday 29 May 2007, Junio C Hamano wrote:
> Earlier, the matching of (model,rev) in ide-dma black/white list
> handling was to consider "ALL" in the table to match any
> revision.  This makes NULL to be also an accepted wildcard, and
> changes the entries of tables in ide-dma.c to use NULL.
> 
> The code still accepts "ALL" as the wildcard, in order to keep
> any out-of-tree ide driver that feeds its own table that uses
> "ALL" as the wildcard to ide_in_drive_list() function from
> breaking.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

applied

[PATCH 2/3] mips au1xxx_ide.h: use NULL as firmware-revision wildcard.

[Junio C Hamano]

This updates the DMA whitelist in MIPS specific au1xxx ide
driver to use NULL instead of "ALL" as the wildcard.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * This is a respin of earlier "3/3 Make ide dma blacklist handling a bit saner."

 include/asm-mips/mach-au1x00/au1xxx_ide.h |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/asm-mips/mach-au1x00/au1xxx_ide.h b/include/asm-mips/mach-au1x00/au1xxx_ide.h
index 8fcae21..4663e8b 100644
--- a/include/asm-mips/mach-au1x00/au1xxx_ide.h
+++ b/include/asm-mips/mach-au1x00/au1xxx_ide.h
@@ -88,26 +88,26 @@ static const struct drive_list_entry dma_white_list [] = {
 /*
  * Hitachi
  */
-        { "HITACHI_DK14FA-20"    ,       "ALL"           },
-        { "HTS726060M9AT00"      ,       "ALL"           },
+        { "HITACHI_DK14FA-20"    ,       NULL            },
+        { "HTS726060M9AT00"      ,       NULL            },
 /*
  * Maxtor
  */
-        { "Maxtor 6E040L0"      ,       "ALL"           },
-        { "Maxtor 6Y080P0"      ,       "ALL"           },
-        { "Maxtor 6Y160P0"      ,       "ALL"           },
+        { "Maxtor 6E040L0"      ,       NULL            },
+        { "Maxtor 6Y080P0"      ,       NULL            },
+        { "Maxtor 6Y160P0"      ,       NULL            },
 /*
  * Seagate
  */
-        { "ST3120026A"          ,       "ALL"           },
-        { "ST320014A"           ,       "ALL"           },
-        { "ST94011A"            ,       "ALL"           },
-        { "ST340016A"           ,       "ALL"           },
+        { "ST3120026A"          ,       NULL            },
+        { "ST320014A"           ,       NULL            },
+        { "ST94011A"            ,       NULL            },
+        { "ST340016A"           ,       NULL            },
 /*
  * Western Digital
  */
-        { "WDC WD400UE-00HCT0"  ,       "ALL"           },
-        { "WDC WD400JB-00JJC0"  ,       "ALL"           },
+        { "WDC WD400UE-00HCT0"  ,       NULL            },
+        { "WDC WD400JB-00JJC0"  ,       NULL            },
         { NULL                  ,       NULL            }
 };
 
@@ -116,9 +116,9 @@ static const struct drive_list_entry dma_black_list [] = {
 /*
  * Western Digital
  */
-        { "WDC WD100EB-00CGH0"  ,       "ALL"           },
-        { "WDC WD200BB-00AUA1"  ,       "ALL"           },
-        { "WDC AC24300L"        ,       "ALL"           },
+        { "WDC WD100EB-00CGH0"  ,       NULL            },
+        { "WDC WD200BB-00AUA1"  ,       NULL            },
+        { "WDC AC24300L"        ,       NULL            },
         { NULL                  ,       NULL            }
 };
 #endif
-- 
1.5.2.86.g99b5-dirty

Re: [PATCH 2/3] mips au1xxx_ide.h: use NULL as firmware-revision wildcard.

[Bartlomiej Zolnierkiewicz]

On Tuesday 29 May 2007, Junio C Hamano wrote:
> This updates the DMA whitelist in MIPS specific au1xxx ide
> driver to use NULL instead of "ALL" as the wildcard.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

applied

[PATCH 3/3] ide_in_drive_list(): "ALL" is not a wildcard anymore.

[Junio C Hamano]

This removes the support to treat "ALL" as a wildcard for
firmware revision.  This is made a separate patch, as it will
break out-of-tree ide drivers that feed its own table that uses
"ALL" as the wildcard to ide_in_drive_list().

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * This is a respin of earlier "3/3 Make ide dma blacklist handling a bit saner."

 drivers/ide/ide-dma.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
index 230ea12..3107b5e 100644
--- a/drivers/ide/ide-dma.c
+++ b/drivers/ide/ide-dma.c
@@ -148,7 +148,6 @@ int ide_in_drive_list(struct hd_driveid *id, const struct drive_list_entry *driv
 	for ( ; drive_table->id_model ; drive_table++)
 		if ((!strcmp(drive_table->id_model, id->model)) &&
 		    (!drive_table->id_firmware ||
-		     !strcmp(drive_table->id_firmware, "ALL") || /* to be removed later */
 		     strstr(id->fw_rev, drive_table->id_firmware)))
 			return 1;
 	return 0;
-- 
1.5.2.86.g99b5-dirty

Re: [PATCH 3/3] ide_in_drive_list(): "ALL" is not a wildcard anymore.

[Bartlomiej Zolnierkiewicz]

On Tuesday 29 May 2007, Junio C Hamano wrote:
> This removes the support to treat "ALL" as a wildcard for
> firmware revision.  This is made a separate patch, as it will
> break out-of-tree ide drivers that feed its own table that uses
> "ALL" as the wildcard to ide_in_drive_list().
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

applied

Re: [PATCH] Match DMA blacklist entries between ide-dma.c and libata-core.c

[Bartlomiej Zolnierkiewicz]

On Tuesday 22 May 2007, Junio C Hamano wrote:
> There are a few entries in ata_device_blacklist[] in libata-core.c
> marked with HORKAGE_NODMA but are missing from drive_blacklist[]
> in ide-dma.c.  This patch makes the lists in sync.
> 
> Also remove a duplicated entry for "SanDisk SDP3B-64".
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

applied

Re: Add Seagate STT20000A to DMA blacklist.

[Bartlomiej Zolnierkiewicz]

On Monday 21 May 2007, Dave Jones wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=1044
> has been open for _four_ years with a patch available.
> Here's a rediffed version of the same.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

Uh, it seems that this fix got stuck in the kernel "black hole"
bugzilla.  Thanks for finding and resurrecting it.  

I've just applied the bigger DMA blacklist update from
Junio C Hamano and it also covers this device.

Bart

Re: Add Seagate STT20000A to DMA blacklist.

[Robert Hancock]

Dave Jones wrote:
> On Mon, May 21, 2007 at 05:15:51PM +0100, Alan Cox wrote:
>  > On Mon, 21 May 2007 10:50:42 -0400
>  > Dave Jones <davej@redhat.com> wrote:
>  > 
>  > > http://bugzilla.kernel.org/show_bug.cgi?id=1044
>  > > has been open for _four_ years with a patch available.
>  > > Here's a rediffed version of the same.
>  > 
>  > Please update libata as well when you udpate the blacklists.
> 
> Sure, point me at the table(s) ?
> 
> 	Dave
> 

ata_device_blacklist in drivers/ata/libata-core.c

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Add Seagate STT20000A to DMA blacklist.

[Dave Jones]

http://bugzilla.kernel.org/show_bug.cgi?id=1044 points out an
additional hard disk that doesn't handle DMA transfers correctly.
This patch is the libata variant of the earlier patch to drivers/ide/

Signed-off-by: Dave Jones <davej@redhat.com>

--- linux-2.6/drivers/ata/libata-core.c~	2007-05-21 14:27:40.000000000 -0400
+++ linux-2.6/drivers/ata/libata-core.c	2007-05-21 14:28:48.000000000 -0400
@@ -3771,6 +3771,7 @@ static const struct ata_blacklist_entry 
 	{ "ATAPI CD-ROM DRIVE 40X MAXIMUM",NULL,ATA_HORKAGE_NODMA },
 	{ "_NEC DV5800A", 	NULL,		ATA_HORKAGE_NODMA },
 	{ "SAMSUNG CD-ROM SN-124","N001",	ATA_HORKAGE_NODMA },
+	{ "Seagate STT20000A", NULL,		ATA_HORKAGE_NODMA },
 
 	/* Weird ATAPI devices */
 	{ "TORiSAN DVD-ROM DRD-N216", NULL,	ATA_HORKAGE_MAX_SEC_128 |

-- 
http://www.codemonkey.org.uk

Re: Add Seagate STT20000A to DMA blacklist.

[Jeff Garzik]

Dave Jones wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=1044 points out an
> additional hard disk that doesn't handle DMA transfers correctly.
> This patch is the libata variant of the earlier patch to drivers/ide/
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

applied