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

[Marco Gerards <mgerards@xs4all.nl>]

Pavel Roskin <proski@gnu.org> writes:

> 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  <marco@gnu.org>
>
>         * 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.

Ah right :-)

>> > 	(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.

Right, this needs more work.  I will have another look at ATA soon :-)

For me ATA support worked on real hardware.  I will have to try it on
more 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.

No, I cannot check the Linux code.  AFAIK this can cause copyright
problems.  But I agree that more and better error checking is
required.

--
Marco