Re: [PATCH] Enable writing to ATA devices, fix several bugs

[Pavel Roskin]

On Thu, 2008-07-03 at 20:27 +0200, Marco Gerards wrote:

> The more patches I get, the better ;-)
> 
> > ChangeLog:
> >
> >     * disk/ata.c (grub_ata_pio_write): Check status before writing,
> >     like we do in grub_ata_pio_read().
> >
> >     (grub_ata_readwrite): Always write individual sectors.  Fix the
> >     sector count for the remainder.
> 
> Why?

Because we do it elsewhere.  I assume you forgot to convert the code for
writing, but you meant to do it:

r1335 | marco_g | 2007-11-03 08:25:19 -0400 (Sat, 03 Nov 2007) | 6 lines

2007-11-03  Marco Gerards  <address@hidden>

        * disk/ata.c (grub_ata_readwrite): Call grub_ata_pio_read and
        grub_ata_pio_write once for every single sector, instead of for
        multiple sectors.

I guess it's safer.  We can explore some optimization, but first we
should make it reliable.

> >     (grub_ata_write): Enable writing to ATA devices.  Correctly
> >     report error for ATAPI devices.

> Great!  Did you test this?

Yes, I tested this part.  env_save didn't report any error originally,
so I introduced grub_error(), and env_save started reporting the error.
Then I enabled writing and tested it in qemu.

I cannot get the ata module to recognize the hard drive on my test
machine, so more work is needed to test it on the real hardware.

> If you can fix Roberts comment, it would be great!  Can you commit it
> afterwards?

Sure.

If you check Linux include/linux/ata.h, 1 is ATA_ERR, and we really need
ata_ok(), which checks multiple flags.  So it's clearly material for a
separate patch.

-- 
Regards,
Pavel Roskin