pata_it821x completely broken

On Sun, 13 Jul 2008 14:10:14 +0200
Ondrej Zary <linux@rainbow-software.org> wrote:
[color=blue]
> On Sunday 13 July 2008 13:35:10 Alan Cox wrote:[color=green][color=darkred]
> > > The patch below fixes the IDENTIFY problem for me and makes the RAID
> > > array accessible. Is it OK or is there a better way to do it?[/color]
> >
> > Probably better to fix it in driver. Right now we don't have a way to
> > override the identify reading or post process it cleanly at it821x hacks
> > around that but if that was fixed then the special cases can all hide in
> > the driver where they belong.[/color]
>
> That would be great but it requires some deeper libata and kernel programming
> knowledge.[/color]

Not a lot I think so I've added it to my todo list if nobody else does it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Sunday 13 July 2008 13:47:12 Ondrej Zary wrote:[color=blue]
> On Saturday 12 July 2008 23:42:10 Ondrej Zary wrote:[color=green]
> > On Friday 11 July 2008 22:14:09 Alan Cox wrote:[color=darkred]
> > > > > commenting out the error check after ata_dev_init_params() call in
> > > > > ata_dev_read_id() function (libata-core.c), I got at least the
> > > > > device name. The capacity is 0 so it doesn't work, obviously:
> > >
> > > If you don't read the ID then it wouldn't.
> > >
> > > > I captured the IDENTIFY data from the virtual device. I'm not ATA
> > > > guru but looking at the data, there are zeros at many places where
> > > > something should be. That number starting at 0x78 looks like size of
> > > > the array in sectors (0x4C726C or 0x4C6C72 - the array is built from
> > > > 2.5GB and 6.4GB drives).
> > >
> > > The Ident data for the virtual device is fairly sparse but the specs
> > > don't require a lot of the field are filled in and only the LBA really
> > > matters.[/color]
> >
> > The problem is that ata_id_n_sectors() function:
> >
> > static u64 ata_id_n_sectors(const u16 *id)
> > {
> > if (ata_id_has_lba(id)) {
> > if (ata_id_has_lba48(id))
> > return ata_id_u64(id, 100);
> > else
> > return ata_id_u32(id, 60);
> > } else {
> > if (ata_id_current_chs_valid(id))
> > return ata_id_u32(id, 57);
> > else
> > return id[1] * id[3] * id[6];
> > }
> > }
> >
> > fails to retrieve the LBA48 value.
> >
> >
> > This is because the ata_id_has_lba() test
> >
> > #define ata_id_has_lba(id) ((id)[49] & (1 << 9))
> >
> > fails as the identify data contains only zeros at word 49 (byte 0x62).
> >
> > Another problem is that ata_id_has_lba48() would fail too - that will
> > break array over 2TB (if the controller BIOS and firmware can do it).
> >
> > Looks like this needs to force LBA48 with these virtual drives.[/color]
>
> The patch below fixes the IDENTIFY problem for me and makes the RAID array
> accessible. Is it OK or is there a better way to do it?
>[/color]

New patch below, this time with DMA forced on RAID volumes - as my firmware
does not identify the RAID arrays as DMA capable :(

diff -ur linux-2.6.26/drivers/ata/libata-core.c linux-2.6.26-pentium/drivers/ata/libata-core.c
--- linux-2.6.26/drivers/ata/libata-core.c 2008-07-13 23:51:29.000000000 +0200
+++ linux-2.6.26-pentium/drivers/ata/libata-core.c 2008-07-16 20:50:33.000000000 +0200
@@ -2030,7 +2030,8 @@
* Note that ATA4 says lba is mandatory so the second check
* shoud never trigger.
*/
- if (ata_id_major_version(id) < 4 || !ata_id_has_lba(id)) {
+ if ((ata_id_major_version(id) < 4 || !ata_id_has_lba(id)) &&
+ id[3] != 0 && id[6] != 0) {
err_mask = ata_dev_init_params(dev, id[3], id[6]);
if (err_mask) {
rc = -EIO;
@@ -2195,18 +2196,23 @@
"not be fully accessable.\n");
}

- dev->n_sectors = ata_id_n_sectors(id);
+ if (dev->horkage & ATA_HORKAGE_LBA48_FORCE)
+ dev->n_sectors = ata_id_u64(id, 100);
+ else
+ dev->n_sectors = ata_id_n_sectors(id);

if (dev->id[59] & 0x100)
dev->multi_count = dev->id[59] & 0xff;

- if (ata_id_has_lba(id)) {
+ if (ata_id_has_lba(id) ||
+ dev->horkage & ATA_HORKAGE_LBA48_FORCE) {
const char *lba_desc;
char ncq_desc[20];

lba_desc = "LBA";
dev->flags |= ATA_DFLAG_LBA;
- if (ata_id_has_lba48(id)) {
+ if (ata_id_has_lba48(id) ||
+ dev->horkage & ATA_HORKAGE_LBA48_FORCE) {
dev->flags |= ATA_DFLAG_LBA48;
lba_desc = "LBA48";

@@ -3946,6 +3952,9 @@
{ "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, },
{ "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, },

+ /* Has LBA48 but advertises neither LBA nor LBA48 */
+ { "Integrated Technology Express Inc", NULL, ATA_HORKAGE_LBA48_FORCE, },
+
/* End Marker */
{ }
};
diff -ur linux-2.6.26/drivers/ata/pata_it821x.c linux-2.6.26-pentium/drivers/ata/pata_it821x.c
--- linux-2.6.26/drivers/ata/pata_it821x.c 2008-07-13 23:51:29.000000000 +0200
+++ linux-2.6.26-pentium/drivers/ata/pata_it821x.c 2008-07-22 19:56:14.000000000 +0200
@@ -462,15 +462,19 @@
static int it821x_smart_set_mode(struct ata_link *link, struct ata_device **unused)
{
struct ata_device *dev;
+ unsigned char model_num[ATA_ID_PROD_LEN + 1];

ata_link_for_each_dev(dev, link) {
if (ata_dev_enabled(dev)) {
/* We don't really care */
dev->pio_mode = XFER_PIO_0;
dev->dma_mode = XFER_MW_DMA_0;
+ ata_id_c_string(dev->id, model_num, ATA_ID_PROD,
+ sizeof(model_num));
/* We do need the right mode information for DMA or PIO
and this comes from the current configuration flags */
- if (ata_id_has_dma(dev->id)) {
+ if (ata_id_has_dma(dev->id) ||
+ strstr(model_num, "Integrated Technology Express")) {
ata_dev_printk(dev, KERN_INFO, "configured for DMA\n");
dev->xfer_mode = XFER_MW_DMA_0;
dev->xfer_shift = ATA_SHIFT_MWDMA;
diff -ur linux-2.6.26/include/linux/libata.h linux-2.6.26-pentium/include/linux/libata.h
--- linux-2.6.26/include/linux/libata.h 2008-07-13 23:51:29.000000000 +0200
+++ linux-2.6.26-pentium/include/linux/libata.h 2008-07-16 20:50:33.000000000 +0200
@@ -353,6 +353,7 @@
ATA_HORKAGE_IPM = (1 << 7), /* Link PM problems */
ATA_HORKAGE_IVB = (1 << 8), /* cbl det validity bit bugs */
ATA_HORKAGE_STUCK_ERR = (1 << 9), /* stuck ERR on next PACKET */
+ ATA_HORKAGE_LBA48_FORCE = (1 << 10), /* Has hidden LBA48 */

/* DMA mask for user DMA control: User visible values; DO NOT
renumber */


--
Ondrej Zary
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

> New patch below, this time with DMA forced on RAID volumes - as my firmware[color=blue]
> does not identify the RAID arrays as DMA capable :([/color]

The underlying problem is that the newer libata rewrites the dev->id data
in ways that we can't then patch up. I've actually been hacking on this a
bit today. The existing it821x code was working fine and it does the
needed patching, it just needs to do it in a different place, and that in
turn needs a new ap->ops->read_id() method so it can be hooked.

On the bright side that also allows us to implement a pata_mfm/rll
driver at last.

Alan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Tuesday 22 July 2008 20:10:22 Alan Cox wrote:[color=blue][color=green]
> > New patch below, this time with DMA forced on RAID volumes - as my
> > firmware does not identify the RAID arrays as DMA capable :([/color]
>
> The underlying problem is that the newer libata rewrites the dev->id data
> in ways that we can't then patch up. I've actually been hacking on this a
> bit today. The existing it821x code was working fine and it does the
> needed patching, it just needs to do it in a different place, and that in
> turn needs a new ap->ops->read_id() method so it can be hooked.[/color]

I can test the code once it's done.
I just moved my / to the RAID1 provided by IT8212. The controller has a great
feature that allows converting a drive between RAID1/standalone without data
loss (haven't seen anything like this yet). Also the background rebuild is
nice (as seen on much more expensive RAID controllers), it's running as I'm
typing this.
[color=blue]
>
> On the bright side that also allows us to implement a pata_mfm/rll
> driver at last.
>[/color]

I used to have an old drive that was something like that - 5.25" Seagate 40MB
drive. I sold it including the controller a couple of years ago (it was still
working) so no testing fun for me.
[color=blue]
> Alan[/color]



--
Ondrej Zary
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On 22-07-08 21:16, Ondrej Zary wrote:
[color=blue]
> On Tuesday 22 July 2008 20:10:22 Alan Cox wrote:[/color]
[color=blue][color=green]
>> On the bright side that also allows us to implement a pata_mfm/rll
>> driver at last.[/color]
>
> I used to have an old drive that was something like that - 5.25"
> Seagate 40MB drive. I sold it including the controller a couple of
> years ago (it was still working) so no testing fun for me.[/color]

Looking at a Western Digital "FileCard 30" consisting of a Seagate
ST-125 and WDQC16 8-bit ISA controller. MFM I believe.

I put a sticker on it with: "io=0x320, irq=5, dma=3 615/4/26 (or 614)"

The thing has its own BIOS and if If I'm not mistaken it could only live
in an AT+ with all other drives in the BIOS turned of. This might mean
I'd be capable of testing it from a boot-floppy only but if you
desperately want a tester, I'll try to volunteer ;-)

I remember using DOS "debug" to jump into its BIOS low-level format
routine so I did get it doing something...

Rene.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

> Looking at a Western Digital "FileCard 30" consisting of a Seagate[color=blue]
> ST-125 and WDQC16 8-bit ISA controller. MFM I believe.
>
> I put a sticker on it with: "io=0x320, irq=5, dma=3 615/4/26 (or 614)"[/color]

xt driver should support that including DMA 8)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]