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 <[email protected]>
--- 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
On Mon, 21 May 2007 10:50:42 -0400
Dave Jones <[email protected]> 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
On Mon, May 21, 2007 at 05:15:51PM +0100, Alan Cox wrote:
> On Mon, 21 May 2007 10:50:42 -0400
> Dave Jones <[email protected]> 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
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 <[email protected]> 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 [email protected]
Home Page: http://www.roberthancock.com/
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 <[email protected]>
--- 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
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 <[email protected]>
applied
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 <[email protected]>
---
* 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
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 <[email protected]>
---
* 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
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 <[email protected]>
---
* 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
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 <[email protected]>
applied
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 <[email protected]>
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
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 <[email protected]>
"We have more to fear from the bungling of the incompetent than from
the machinations of the wicked." - from Slashdot
On Mon, 21 May 2007 22:06:36 -0700
Junio C Hamano <[email protected]> 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 <[email protected]>
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.
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 <[email protected]>
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
Bartlomiej Zolnierkiewicz <[email protected]> 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.
Hi,
On Tuesday 29 May 2007, Junio C Hamano wrote:
> Bartlomiej Zolnierkiewicz <[email protected]> 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