ata read doesn't work after 504 bytes of a sector

[protegee6155]

Hi, following my unsuccessful adventure with irq base ata driver implementation, i have desided to implement a polling base ata driver. After some tests of my current implementation, it seems that my read implementation doesn't read correctly the sector's bytes following (and included) the 504-th byte currectly (e.g. returning all as null bytes, although in disk other data is present).

may someone help me find the problem? i am really out of ideas.

My polling base ata driver:

Code: Select all

#include "drivers/ata_driver.h"
#include "kernel/print.h"
#include "io/port.h"

/*
 * Static helper functions
 */
static void delay_400ns(ata_drive_t *drive);
static uint8_t read_status_reg(ata_drive_t *drive);
static uint8_t ata_read28_one_sector_request(ata_drive_t *drive, uint32_t sector, uint8_t *buffer);
static uint32_t ata_response_handler(cpu_status_t *regs);

static void delay_400ns(ata_drive_t *drive) {
    inb(drive->device_id.io_base + ATA_REG_ALTSTATUS);
    inb(drive->device_id.io_base + ATA_REG_ALTSTATUS);
    inb(drive->device_id.io_base + ATA_REG_ALTSTATUS);
    inb(drive->device_id.io_base + ATA_REG_ALTSTATUS);
}

static uint8_t read_status_reg(ata_drive_t *drive) {
    uint8_t status;

    status = inb(drive->device_id.io_base + ATA_REG_STATUS);

    return status;
}

static uint8_t ata_read28_one_sector_request(ata_drive_t *drive, uint32_t sector, uint8_t *buffer) {
    outb(drive->device_id.io_base + ATA_REG_HDDEVSEL, 0xE0 | (drive->device_id.master<<4) | ((sector>>24)&0x0F));
    outb(drive->device_id.io_base + ATA_REG_FEATURES, 0);  // send Null (0) to the feature register (don't know why)
    outb(drive->device_id.io_base + ATA_REG_SECCOUNT, 1);
    outb(drive->device_id.io_base + ATA_REG_LBA_LOW,  sector & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_MID, (sector >> 8) & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_HIGH,(sector >>16) & 0xFF);

    /* send the command */
    outb(drive->device_id.io_base + ATA_REG_COMMAND, ATA_CMD_READ_PIO);

    /* check if disk is ready */
    ata_wait_bsy_clear(drive);
    /* "You may also need to ignore ERR and DF the first four times that you read the Status, if you are polling" */
    delay_400ns(drive);
    ata_wait_drq_ready(drive);

    /* check if we got an error */
    if (ata_check_err(drive))
        return ata_get_err(drive);

    for(int i = 0; i < ATA_SECTOR_SIZE/2; i++) {
		uint16_t data = inw(drive->device_id.io_base + ATA_REG_DATA);
		((uint16_t *)buffer)[i] = data;
	}

    /* Note for polling PIO drivers: After transferring the last uint16_t of a PIO data block to the data IO port, 
     * give the drive a 400ns delay to reset its DRQ bit (and possibly set BSY again, 
     * while emptying/filling its buffer to/from the drive).
     */
    delay_400ns(drive);

    return 0;
}

static uint8_t ata_write28_one_sector_request(ata_drive_t *drive, uint32_t sector, uint8_t *buffer) {
    outb(drive->device_id.io_base + ATA_REG_HDDEVSEL, 0xE0 | (drive->device_id.master<<4) | ((sector>>24)&0x0F));
    outb(drive->device_id.io_base + ATA_REG_FEATURES, 0);  // send Null (0) to the feature register (don't know why)
    outb(drive->device_id.io_base + ATA_REG_SECCOUNT, 1);
    outb(drive->device_id.io_base + ATA_REG_LBA_LOW,  sector & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_MID, (sector >> 8) & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_HIGH,(sector >>16) & 0xFF);

    /* send the command */
    outb(drive->device_id.io_base + ATA_REG_COMMAND, ATA_CMD_WRITE_PIO);

    /* check if disk is ready */
    ata_wait_bsy_clear(drive);
    /* "You may also need to ignore ERR and DF the first four times that you read the Status, if you are polling" */
    delay_400ns(drive);
    ata_wait_drq_ready(drive);

    /* check if we got an error */
    uint8_t err = ata_get_err(drive);
    if (err != 0) return ata_get_err(drive);

    for(int i = 0; i < ATA_SECTOR_SIZE/2; i++) {
        outw(drive->device_id.io_base + ATA_REG_DATA, ((uint16_t *)buffer)[i]);
        asm volatile("jmp .+2"); // short mandatory delay
	}

    /* may or may not be needed, not so sure */
    delay_400ns(drive);

    return 0;
}

static uint32_t ata_response_handler(cpu_status_t *regs) {
    return 0;
}

/*
 * Helper functions
 */
void ata_wait_bsy_clear(ata_drive_t *drive) {
    uint8_t status;

    do {
        status = read_status_reg(drive);
    } while (status & ATA_SR_BSY);
}

void ata_wait_drq_ready(ata_drive_t *drive) {
    uint8_t status;

    do {
        status = read_status_reg(drive);

        if (status & ATA_SR_ERR)
            return;

    } while (!(status & ATA_SR_DRQ));
}

uint8_t ata_check_err(ata_drive_t *drive) {
    uint8_t status;

    status = read_status_reg(drive);

    return status & ATA_SR_ERR;
}

uint8_t ata_get_err(ata_drive_t *drive) {
    uint8_t err;

    err = inb(drive->device_id.io_base + ATA_REG_ERROR);
    
    return err;
}

/* 
 * Driver functions
 */

 void ata_driver_init() {
    register_interrupt_handler(46, ata_response_handler);
    register_interrupt_handler(47, ata_response_handler);
 }

uint8_t ata_read28_request(ata_drive_t *drive, uint32_t sector, uint8_t count, uint8_t *buffer) {
    uint8_t err;

    for (uint32_t i = 0; i < count; i++) {
        err = ata_read28_one_sector_request(drive, sector + i, buffer);
        if (err != 0) return err;

        buffer += ATA_SECTOR_SIZE;
    }

    return 0;
}

uint8_t ata_write28_request(ata_drive_t *drive, uint32_t sector, uint8_t count, uint8_t *buffer) {
    uint8_t err;

    for (uint32_t i = 0; i < count; i++) {
        err = ata_write28_one_sector_request(drive, sector + i, buffer);
        if (err != 0) return err;

        buffer += ATA_SECTOR_SIZE;
    }

    return ata_flush_cache(drive);
}

uint8_t ata_send_identify_command(ata_drive_t *drive, identify_device_data_t *buffer) {
    outb(drive->device_id.io_base + ATA_REG_HDDEVSEL, 0xE0 | (drive->device_id.master << 4));
    outb(drive->device_id.io_base + ATA_REG_LBA_LOW,  0);
    outb(drive->device_id.io_base + ATA_REG_LBA_MID,  0);
    outb(drive->device_id.io_base + ATA_REG_LBA_HIGH, 0);

    /* send the command */
    outb(drive->device_id.io_base + ATA_REG_COMMAND, ATA_CMD_IDENTIFY);

    uint8_t status = read_status_reg(drive);
    if (status == 0) {
        printf("ATA: No device detected on the port\n");
        return 1;
    }

    /* wait for drive to not be busy */
    ata_wait_bsy_clear(drive);

    uint8_t LBAmid = inb(drive->device_id.io_base + ATA_REG_LBA_MID);
    uint8_t LBAhi  = inb(drive->device_id.io_base + ATA_REG_LBA_HIGH);

    if (LBAmid != 0 || LBAhi != 0) {
        printf("ATA: Device is not ATA compatible\n");
        return 1;
    }

    ata_wait_drq_ready(drive);
    if (ata_check_err(drive)) {
        printf("ATA: IDENTIFY failed, error flag raised\n");
        return ata_get_err(drive);
    }

    // Read IDENTIFY data
    for (uint32_t i = 0; i < ATA_SECTOR_SIZE/2; i++)
        ((uint16_t *)buffer)[i] = inw(drive->device_id.io_base + ATA_REG_DATA);

    /* may or may not be needed, not so sure */
    delay_400ns(drive);
}

uint8_t ata_flush_cache(ata_drive_t *drive) {
    outb(drive->device_id.io_base + ATA_REG_HDDEVSEL, 0xE0 | (drive->device_id.master << 4));
    outb(drive->device_id.io_base + ATA_REG_COMMAND, ATA_CMD_FLUSH);

    /* "sending the 0xE7 command to the Command Register (then waiting for BSY to clear)" */
    ata_wait_bsy_clear(drive);

    /* check if we got an error */
    uint8_t err = ata_check_err(drive);
    if (err != 0) return ata_get_err(drive);

    return 0;
}

/* 
 * Print helper functions 
 */
void print_identify_device_data(const identify_device_data_t *id) {
    printf("=== ATA IDENTIFY DEVICE DATA ===\n");

    printf("Device type: %s\n",
           id->GeneralConfiguration.DeviceType ? "Non-disk device" : "ATA Hard Disk");

    printf("LBA28 Sector count: %u (0x%x)\n",
           id->UserAddressableSectors, id->UserAddressableSectors);

    printf("LBA48 Sector count: %s\n",
           id->Max48BitLBA[0] ? "Supported" : "Not supported");

    printf("UDMA supported modes: ");
    for (int i = 0; i < 8; i++)
        if (id->UltraDMASupport & (1 << i))
            printf("%d ", i);

    printf("\n80-wire cable detected: %s\n",
           (id->HardwareResetResult & (1 << 11)) ? "Yes" : "No");

    printf("================================\n");
}

the tester code:

Code: Select all

void ata_test_write_read_3_sectors(ata_drive_t *drive, uint32_t start_sector)
{
    uint8_t write_buf[ATA_SECTOR_SIZE * 3];
    uint8_t read_buf[ATA_SECTOR_SIZE * 3];

    memset(write_buf, 0, sizeof(write_buf));
    memset(read_buf, 0, sizeof(read_buf));

    /* Fill each sector with a different pattern */
    for (uint32_t i = 0; i < ATA_SECTOR_SIZE; i++) {
        write_buf[i] = 0xAA;                         /* sector 0 */
        write_buf[ATA_SECTOR_SIZE + i] = 0xBB;       /* sector 1 */
        write_buf[ATA_SECTOR_SIZE * 2 + i] = 0xCC;   /* sector 2 */
    }

    printf("ATA TEST: writing 3 sectors at LBA %u\n", start_sector);

    uint8_t err = ata_write28_request(drive, start_sector, 3, write_buf);
    if (err != 0) {
        printf("ATA TEST: write failed, err=%u\n", err);
        return;
    }

    err = ata_flush_cache(drive);
    if (err != 0) {
        printf("ATA TEST: flush failed, err=%u\n", err);
        return;
    }

    printf("ATA TEST: reading 3 sectors back\n");

    err = ata_read28_request(drive, start_sector, 3, read_buf);
    if (err != 0) {
        printf("ATA TEST: read failed, err=%u\n", err);
        return;
    }

    for (uint32_t i = 0; i < sizeof(write_buf); i++) {
        if (write_buf[i] != read_buf[i]) {
            printf("ATA TEST: mismatch at byte %u\n", i);
            printf("expected=0x%x actual=0x%x\n",
                   write_buf[i],
                   read_buf[i]);
            return;
        }
    }

    printf("ATA TEST: PASS - 3 sectors written and read correctly\n");
}

[Octocontrabass]

following my unsuccessful adventure with irq base ata driver implementation

You're giving up already? The only problem is that you aren't acknowledging the IRQ correctly. You never did show me which part of your code reads the status register to acknowledge the IRQ...

i have desided to implement a polling base ata driver.

This is a bad idea.

Code: Select all

    inb(drive->device_id.io_base + ATA_REG_ALTSTATUS);

Are you sure you're reading the correct register here?

[protegee6155]

You're giving up already?

This is a bad idea.

Well, when one is out of hope he may take drastic measures. XD
I would accualy prefer to finish my polling driver, before anything else, since i need to hand in this project somewhat soon, also, from what i have undestood, writing an polling driver should be easier (if i am wrong i am willing to go back and try again, although i have no idea of to fix the problems of my irq base driver).

Code: Select all

Are you sure you're reading the correct register here?

although not the problem . this this is incorrect, it should be relative to base port.

Code: Select all

The only problem is that you aren't acknowledging the IRQ correctly. You never did show me which part of your code reads the status register to acknowledge the IRQ...

from what i have understood, to acknowledge the IRQ you only need to read the status register.

[protegee6155]

Now it seems that i get the same error as before (before as in irq base driver). the secound sector of the write command never comes.
this is my current driver implimentation

You never did show me which part of your code reads the status register to acknowledge the IRQ...

Is there a need to also ackowledge the irq (by reading the status register) in an poll base implementation too?
is reading the status register enough.

e.g. is this enough?

Code: Select all

static uint32_t ata_response_handler(cpu_status_t *regs) {
    read_status_reg(current_working_drive);
    return 0;
}

[Octocontrabass]

I would accualy prefer to finish my polling driver, before anything else, since i need to hand in this project somewhat soon, also, from what i have undestood, writing an polling driver should be easier (if i am wrong i am willing to go back and try again, although i have no idea of to fix the problems of my irq base driver).

Since you have a deadline, the polling driver might be easier to finish for now.

yes, this is correct, it is only for making delay.

Sure, but only if you're reading the correct register. I see you already fixed this bug.

from what i have understood, to acknowledge the IRQ you only need to read the status register.

Correct.

Now it seems that i get the same error as before (before as in irq base driver). the secound sector of the write command never comes.

Where does it get stuck? What is your code waiting for that never happens?

Is there a need to also ackowledge the irq (by reading the status register) in an poll base implementation too?

There are no IRQs when you're polling.

[protegee6155]

There are no IRQs when you're polling.

are you sure? from what i understood in any case (if you use the default settings) for each sector ready to read, and for each sector writen, an interrupt is issued.

Where does it get stuck? What is your code waiting for that never happens?

Unfurtenetly it seems that i get two different results depending on the enviroment (a debugged one, or a non debugged one).
In the debugged version (which was debugged using gdb), nothing is blocked, and evething seems to work correctly.
in the non debugged version (which was debugged only with printf), i seem to get stuck in ata_wait_drq_ready (of the sector 51, so it the secound sector written to):

Code: Select all

static uint8_t ata_write28_one_sector_request(ata_drive_t *drive, uint32_t sector, uint8_t *buffer) {
    outb(drive->device_id.io_base + ATA_REG_HDDEVSEL, 0xE0 | (drive->device_id.master<<4) | ((sector>>24)&0x0F));
    outb(drive->device_id.io_base + ATA_REG_FEATURES, 0);  // send Null (0) to the feature register (don't know why)
    outb(drive->device_id.io_base + ATA_REG_SECCOUNT, 1);
    outb(drive->device_id.io_base + ATA_REG_LBA_LOW,  sector & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_MID, (sector >> 8) & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_HIGH,(sector >>16) & 0xFF);

    /* send the command */
    outb(drive->device_id.io_base + ATA_REG_COMMAND, ATA_CMD_WRITE_PIO);

    printf("before writing status check %d\n", sector);
    /* check if disk is ready */
    ata_wait_bsy_clear(drive);
    /* "You may also need to ignore ERR and DF the first four times that you read the Status, if you are polling" */
    printf("after writing status check %d\n", sector);

    printf("before data ready check %d\n", sector); <-------- this is printed to the screan
    delay_400ns(drive);
    ata_wait_drq_ready(drive); <--------- i get stuck in here
    printf("before after check %d\n", sector);  
   

    /* check if we got an error */
    uint8_t err = ata_get_err(drive);
    if (err != 0) return ata_get_err(drive);

    for(int i = 0; i < ATA_SECTOR_SIZE/2; i++) {
        outw(drive->device_id.io_base + ATA_REG_DATA, ((uint16_t *)buffer)[i]);
        asm volatile("jmp .+2"); // short mandatory delay
	}

    /* may or may not be needed, not so sure */
    delay_400ns(drive);

    return 0;
}

[Octocontrabass]

are you sure? from what i understood in any case (if you use the default settings) for each sector ready to read, and for each sector writen, an interrupt is issued.

But you've disabled those IRQs because you're polling. Right?

in the non debugged version (which was debugged only with printf), i seem to get stuck in ata_wait_drq_ready (of the sector 51, so it the secound sector written to):

I don't see any reason why it would get stuck there. Try running QEMU with "--trace ide_*" and see if the log shows anything strange around the point where it gets stuck.

[protegee6155]

But you've disabled those IRQs because you're polling. Right?

No, why would i disable IRQs before polling?

I don't see any reason why it would get stuck there. Try running QEMU with "--trace ide_*" and see if the log shows anything strange around the point where it gets stuck.

Ok! i will try it tommorow (It is quite late here, so i will try it tommorow), i will update on the results, thanks!!

btw, this is the whole project.

[protegee6155]

Hi, it got it to work! thanks to this.

ata_wait_drq_ready(drive); seems to not be nessesery (or not correct for the implimentation).

the correct order of execution for ready/write commands are:
1. set device
2. set 0 fetures
3. set sector count
4. set sector address
5. wait for drive to not be busy (busy bit clear)
6. wait for drive to be ready (drive ready bit set)
7. send command
8. wait for drive to not be busy (busy bit clear)
9. get/write data from drive

here are my one sector implementation:

Code: Select all

static uint8_t ata_read28_one_sector_request(ata_drive_t *drive, uint32_t sector, uint8_t *buffer) {
    outb(drive->device_id.io_base + ATA_REG_HDDEVSEL, 0xE0 | (drive->device_id.master<<4) | ((sector>>24)&0x0F));
    outb(drive->device_id.io_base + ATA_REG_FEATURES, 0);  // send Null (0) to the feature register (don't know why)
    outb(drive->device_id.io_base + ATA_REG_SECCOUNT, 1);
    outb(drive->device_id.io_base + ATA_REG_LBA_LOW,  sector & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_MID, (sector >> 8) & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_HIGH,(sector >>16) & 0xFF);

    /* check if disk is not busy and ready */
    ata_wait_not_busy(drive);
    ata_wait_drive_ready(drive);

    /* send the command */
    outb(drive->device_id.io_base + ATA_REG_COMMAND, ATA_CMD_READ_PIO);
    ata_wait_not_busy(drive);

    /* check if we got an error */
    if (ata_check_err(drive))
        return ata_get_err(drive);

    for(int i = 0; i < ATA_SECTOR_SIZE/2; i++) {
		uint16_t data = inw(drive->device_id.io_base + ATA_REG_DATA);
		((uint16_t *)buffer)[i] = data;
	}

    /* Note for polling PIO drivers: After transferring the last uint16_t of a PIO data block to the data IO port, 
     * give the drive a 400ns delay to reset its DRQ bit (and possibly set BSY again, 
     * while emptying/filling its buffer to/from the drive).
     */
    delay_400ns(drive);

    return 0;
}

static uint8_t ata_write28_one_sector_request(ata_drive_t *drive, uint32_t sector, uint8_t *buffer) {
    outb(drive->device_id.io_base + ATA_REG_HDDEVSEL, 0xE0 | (drive->device_id.master<<4) | ((sector>>24)&0x0F));
    outb(drive->device_id.io_base + ATA_REG_FEATURES, 0);  // send Null (0) to the feature register (don't know why)
    outb(drive->device_id.io_base + ATA_REG_SECCOUNT, 1);
    outb(drive->device_id.io_base + ATA_REG_LBA_LOW,  sector & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_MID, (sector >> 8) & 0xFF);
    outb(drive->device_id.io_base + ATA_REG_LBA_HIGH,(sector >>16) & 0xFF);

    /* check if disk is not busy and ready */
    ata_wait_not_busy(drive);
    ata_wait_drive_ready(drive);

    /* send the command */
    outb(drive->device_id.io_base + ATA_REG_COMMAND, ATA_CMD_WRITE_PIO);

    /* check if disk is not busy and ready */
    ata_wait_not_busy(drive);

    /* check if we got an error */
    if (ata_get_err(drive)) return ata_get_err(drive);

    for(int i = 0; i < ATA_SECTOR_SIZE/2; i++) {
        outw(drive->device_id.io_base + ATA_REG_DATA, ((uint16_t *)buffer)[i]);
        asm volatile("jmp .+2"); // short mandatory delay
	}

    ata_wait_not_busy(drive);

    return 0;
}

[Octocontrabass]

ata_wait_drq_ready(drive); seems to not be nessesery (or not correct for the implimentation).

It should be necessary when you're polling the drive, but I guess it's not worth worrying about if you're going to replace your driver with one that's properly interrupt-driven later.