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

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

[Pavel Roskin]

We have save_env now, so we can use the write capability.  This also
fixes the last compiler warning in GRUB.

Sorry, Marco, please ignore the previous message, as it didn't get to
the list.

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.

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

 disk/ata.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/disk/ata.c b/disk/ata.c
index 02c4b06..c9b0498 100644
--- a/disk/ata.c
+++ b/disk/ata.c
@@ -187,6 +187,9 @@ grub_ata_pio_write (struct grub_ata_device *dev, char *buf,
   grub_uint16_t *buf16 = (grub_uint16_t *) buf;
   unsigned int i;
 
+  if (grub_ata_regget (dev, GRUB_ATA_REG_STATUS) & 1)
+    return grub_ata_regget (dev, GRUB_ATA_REG_ERROR);
+
   /* Wait until the device is ready to write.  */
   grub_ata_wait_drq (dev);
 
@@ -562,10 +565,9 @@ grub_ata_readwrite (grub_disk_t disk, grub_disk_addr_t sector,
       /* Write sectors.  */
       grub_ata_regset (dev, GRUB_ATA_REG_CMD, cmd_write);
       grub_ata_wait ();
-      for (sect = 0; sect < batch; sect++)
+      for (sect = 0; sect < (size % batch); sect++)
 	{
-	  if (grub_ata_pio_write (dev, buf,
-				  (size % batch) * GRUB_DISK_SECTOR_SIZE))
+	  if (grub_ata_pio_write (dev, buf, GRUB_DISK_SECTOR_SIZE))
 	    return grub_error (GRUB_ERR_WRITE_ERROR, "ATA write error");
 	  buf += GRUB_DISK_SECTOR_SIZE;
 	}
@@ -705,11 +707,12 @@ grub_ata_write (grub_disk_t disk,
 		grub_size_t size,
 		const char *buf)
 {
-#if 1
-  return GRUB_ERR_NOT_IMPLEMENTED_YET;
-#else
-  return grub_ata_readwrite (disk, sector, size, (char *) buf, 1);
-#endif
+  struct grub_ata_device *dev = (struct grub_ata_device *) disk->data;
+
+  if (! dev->atapi)
+    return grub_ata_readwrite (disk, sector, size, (char *) buf, 1);
+
+  return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "ATAPI write not supported");
 }
 
 static struct grub_disk_dev grub_atadisk_dev =

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

[Robert Millan]

On Wed, Jul 02, 2008 at 08:17:16PM -0400, Pavel Roskin wrote:
> +  if (grub_ata_regget (dev, GRUB_ATA_REG_STATUS) & 1)
> +    return grub_ata_regget (dev, GRUB_ATA_REG_ERROR);

May I suggest a macro to describe this "1"?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What good is a phone call… if you are unable to speak?
(as seen on /.)

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

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> We have save_env now, so we can use the write capability.  This also
> fixes the last compiler warning in GRUB.

Great!

> Sorry, Marco, please ignore the previous message, as it didn't get to
> the list.

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?

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


Great!  Did you test this?

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

--
Marco

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

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

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

[Marco Gerards]

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

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

[Pavel Roskin]

Quoting Marco Gerards <mgerards@xs4all.nl>:

>> 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 :-)

That would be great!

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

I know.  That's why I'll write it from specifications or maybe I'll  
take it from the GNU/Hurd code.

-- 
Regards,
Pavel Roskin

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

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> Quoting Marco Gerards <mgerards@xs4all.nl>:
>
>>> 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 :-)
>
> That would be great!
>
>>> 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.
>
> I know.  That's why I'll write it from specifications or maybe I'll
> take it from the GNU/Hurd code.

Taking it from Specifications will be better.

I think the ATA driver of GNU Mach comes from Linux 2.0 or so.  So
that won't change anything for us ;(.

--
Marco

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

[Pavel Roskin]

On Sun, 2008-07-20 at 20:55 +0200, Marco Gerards wrote:
> Pavel Roskin <proski@gnu.org> writes:

> > I know.  That's why I'll write it from specifications or maybe I'll
> > take it from the GNU/Hurd code.
> 
> Taking it from Specifications will be better.
> 
> I think the ATA driver of GNU Mach comes from Linux 2.0 or so.  So
> that won't change anything for us ;(.

I don't think choosing consistent names could be interpreted as a
copyright violation (except by companies like SCO, but then all bets are
off).

Anyway, if I ever have a chance to touch the GRUB ATA code again, I'll
use FreeBSD as a reference.  Using specification is probably not the
best idea because we need GRUB to work on the real life hardware, and we
need to be prepared to handle known quirks in popular hardware.

-- 
Regards,
Pavel Roskin

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

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> On Sun, 2008-07-20 at 20:55 +0200, Marco Gerards wrote:
>> Pavel Roskin <proski@gnu.org> writes:
>
>> > I know.  That's why I'll write it from specifications or maybe I'll
>> > take it from the GNU/Hurd code.
>> 
>> Taking it from Specifications will be better.
>> 
>> I think the ATA driver of GNU Mach comes from Linux 2.0 or so.  So
>> that won't change anything for us ;(.
>
> I don't think choosing consistent names could be interpreted as a
> copyright violation (except by companies like SCO, but then all bets are
> off).

No, you are right.  But it means that you have a look at the Linux ATA
code.  If you copy Linux names into our code, people could claim that
we looked at Linux and based our code on it.

> Anyway, if I ever have a chance to touch the GRUB ATA code again, I'll
> use FreeBSD as a reference.  Using specification is probably not the
> best idea because we need GRUB to work on the real life hardware, and we
> need to be prepared to handle known quirks in popular hardware.

We were talking about not looking at copyrighted code as a
reference...  But looking at FreeBSD would be better than looking at
Linux if we want to avoid possible copyright problems.

--
Marco

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

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

El lun, 21-07-2008 a las 14:49 +0200, Marco Gerards escribió:
> Pavel Roskin <proski@gnu.org> writes:
> 
> > On Sun, 2008-07-20 at 20:55 +0200, Marco Gerards wrote:
> >> Pavel Roskin <proski@gnu.org> writes:
> >
> >> > I know.  That's why I'll write it from specifications or maybe I'll
> >> > take it from the GNU/Hurd code.
> >> 
> >> Taking it from Specifications will be better.
> >> 
> >> I think the ATA driver of GNU Mach comes from Linux 2.0 or so.  So
> >> that won't change anything for us ;(.
> >
> > I don't think choosing consistent names could be interpreted as a
> > copyright violation (except by companies like SCO, but then all bets are
> > off).
> 
> No, you are right.  But it means that you have a look at the Linux ATA
> code.  If you copy Linux names into our code, people could claim that
> we looked at Linux and based our code on it.
So what? Aren't both Linux and GRUB under the GPL? That _should_ mean
that we can look at their code and put it into GRUB ("create a
derivative work") either as-is or modified.
> 
> > Anyway, if I ever have a chance to touch the GRUB ATA code again, I'll
> > use FreeBSD as a reference.  Using specification is probably not the
> > best idea because we need GRUB to work on the real life hardware, and we
> > need to be prepared to handle known quirks in popular hardware.
> 
> We were talking about not looking at copyrighted code as a
> reference...  But looking at FreeBSD would be better than looking at
> Linux if we want to avoid possible copyright problems.
I still don't understand this: the GPL includes an irrevocable grant as
long as the license is obeyed. As for copyright problems, Linux has had
several clashes (SCO et al), but in each and every instance people has
raised against the attacker, defended Linux and won in court. I say it
"offers" quite good copyright shielding.

Habbit

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

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

[Marco Gerards]

Javier Martín <lordhabbit@gmail.com> writes:

> El lun, 21-07-2008 a las 14:49 +0200, Marco Gerards escribió:
>> Pavel Roskin <proski@gnu.org> writes:
>> 
>> > On Sun, 2008-07-20 at 20:55 +0200, Marco Gerards wrote:
>> >> Pavel Roskin <proski@gnu.org> writes:
>> >
>> >> > I know.  That's why I'll write it from specifications or maybe I'll
>> >> > take it from the GNU/Hurd code.
>> >> 
>> >> Taking it from Specifications will be better.
>> >> 
>> >> I think the ATA driver of GNU Mach comes from Linux 2.0 or so.  So
>> >> that won't change anything for us ;(.
>> >
>> > I don't think choosing consistent names could be interpreted as a
>> > copyright violation (except by companies like SCO, but then all bets are
>> > off).
>> 
>> No, you are right.  But it means that you have a look at the Linux ATA
>> code.  If you copy Linux names into our code, people could claim that
>> we looked at Linux and based our code on it.
> So what? Aren't both Linux and GRUB under the GPL? That _should_ mean
> that we can look at their code and put it into GRUB ("create a
> derivative work") either as-is or modified.

For GRUB 2 we require copyright assignments.
 
>> > Anyway, if I ever have a chance to touch the GRUB ATA code again, I'll
>> > use FreeBSD as a reference.  Using specification is probably not the
>> > best idea because we need GRUB to work on the real life hardware, and we
>> > need to be prepared to handle known quirks in popular hardware.
>> 
>> We were talking about not looking at copyrighted code as a
>> reference...  But looking at FreeBSD would be better than looking at
>> Linux if we want to avoid possible copyright problems.
> I still don't understand this: the GPL includes an irrevocable grant as
> long as the license is obeyed. As for copyright problems, Linux has had
> several clashes (SCO et al), but in each and every instance people has
> raised against the attacker, defended Linux and won in court. I say it
> "offers" quite good copyright shielding.

This isn't about licenses.  This is about copyright.

--
Marco

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

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

El lun, 21-07-2008 a las 17:20 +0200, Marco Gerards escribió:
> Javier Martín <lordhabbit@gmail.com> writes:
> 
> > El lun, 21-07-2008 a las 14:49 +0200, Marco Gerards escribió:
> >> Pavel Roskin <proski@gnu.org> writes:
> >> 
> >> > On Sun, 2008-07-20 at 20:55 +0200, Marco Gerards wrote:
> >> >> Pavel Roskin <proski@gnu.org> writes:
> >> >
> >> >> > I know.  That's why I'll write it from specifications or maybe I'll
> >> >> > take it from the GNU/Hurd code.
> >> >> 
> >> >> Taking it from Specifications will be better.
> >> >> 
> >> >> I think the ATA driver of GNU Mach comes from Linux 2.0 or so.  So
> >> >> that won't change anything for us ;(.
> >> >
> >> > I don't think choosing consistent names could be interpreted as a
> >> > copyright violation (except by companies like SCO, but then all bets are
> >> > off).
> >> 
> >> No, you are right.  But it means that you have a look at the Linux ATA
> >> code.  If you copy Linux names into our code, people could claim that
> >> we looked at Linux and based our code on it.
> > So what? Aren't both Linux and GRUB under the GPL? That _should_ mean
> > that we can look at their code and put it into GRUB ("create a
> > derivative work") either as-is or modified.
> 
> For GRUB 2 we require copyright assignments.
>  
> >> > Anyway, if I ever have a chance to touch the GRUB ATA code again, I'll
> >> > use FreeBSD as a reference.  Using specification is probably not the
> >> > best idea because we need GRUB to work on the real life hardware, and we
> >> > need to be prepared to handle known quirks in popular hardware.
> >> 
> >> We were talking about not looking at copyrighted code as a
> >> reference...  But looking at FreeBSD would be better than looking at
> >> Linux if we want to avoid possible copyright problems.
> > I still don't understand this: the GPL includes an irrevocable grant as
> > long as the license is obeyed. As for copyright problems, Linux has had
> > several clashes (SCO et al), but in each and every instance people has
> > raised against the attacker, defended Linux and won in court. I say it
> > "offers" quite good copyright shielding.
> 
> This isn't about licenses.  This is about copyright.
I know, I know... What I'm asking is _why_ this whole obsession about
copyright assignments. Is there a page in the wiki explaining it?


[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

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

[Marco Gerards]

Javier Martín <lordhabbit@gmail.com> writes:

> El lun, 21-07-2008 a las 17:20 +0200, Marco Gerards escribió:
>> Javier Martín <lordhabbit@gmail.com> writes:
>> 
>> > El lun, 21-07-2008 a las 14:49 +0200, Marco Gerards escribió:
>> >> Pavel Roskin <proski@gnu.org> writes:
>> >> 
>> >> > On Sun, 2008-07-20 at 20:55 +0200, Marco Gerards wrote:
>> >> >> Pavel Roskin <proski@gnu.org> writes:
>> >> >
>> >> >> > I know.  That's why I'll write it from specifications or maybe I'll
>> >> >> > take it from the GNU/Hurd code.
>> >> >> 
>> >> >> Taking it from Specifications will be better.
>> >> >> 
>> >> >> I think the ATA driver of GNU Mach comes from Linux 2.0 or so.  So
>> >> >> that won't change anything for us ;(.
>> >> >
>> >> > I don't think choosing consistent names could be interpreted as a
>> >> > copyright violation (except by companies like SCO, but then all bets are
>> >> > off).
>> >> 
>> >> No, you are right.  But it means that you have a look at the Linux ATA
>> >> code.  If you copy Linux names into our code, people could claim that
>> >> we looked at Linux and based our code on it.
>> > So what? Aren't both Linux and GRUB under the GPL? That _should_ mean
>> > that we can look at their code and put it into GRUB ("create a
>> > derivative work") either as-is or modified.
>> 
>> For GRUB 2 we require copyright assignments.
>>  
>> >> > Anyway, if I ever have a chance to touch the GRUB ATA code again, I'll
>> >> > use FreeBSD as a reference.  Using specification is probably not the
>> >> > best idea because we need GRUB to work on the real life hardware, and we
>> >> > need to be prepared to handle known quirks in popular hardware.
>> >> 
>> >> We were talking about not looking at copyrighted code as a
>> >> reference...  But looking at FreeBSD would be better than looking at
>> >> Linux if we want to avoid possible copyright problems.
>> > I still don't understand this: the GPL includes an irrevocable grant as
>> > long as the license is obeyed. As for copyright problems, Linux has had
>> > several clashes (SCO et al), but in each and every instance people has
>> > raised against the attacker, defended Linux and won in court. I say it
>> > "offers" quite good copyright shielding.
>> 
>> This isn't about licenses.  This is about copyright.
> I know, I know... What I'm asking is _why_ this whole obsession about
> copyright assignments. Is there a page in the wiki explaining it?

I think I explained this already and I do not want to keep repeating
everything I said several times.  There is not wiki page, but there is
a document about this for GNU maintainers:

http://www.gnu.org/prep/maintain/maintain.html#Copyright-Papers

--
Marco

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

[Pavel Roskin]

On Mon, 2008-07-21 at 17:20 +0200, Marco Gerards wrote:
> Javier Martín <lordhabbit@gmail.com> writes:
> 
> > So what? Aren't both Linux and GRUB under the GPL? That _should_ mean
> > that we can look at their code and put it into GRUB ("create a
> > derivative work") either as-is or modified.
> 
> For GRUB 2 we require copyright assignments.

Also, Linux is under GPLv2 and GRUB is under GPLv3.  This means that
Linux developers could object that their code is restricted further than
GPLv2 allows, e.g. there are additional anti-DRM provisions that the
recipients of the GRUB code has to fulfill.

That said, I cannot imagine that copying of variable names could be
treated as a copyright violation.  For instance, Wine reimplements
Windows API, and they just have to use the same names.

-- 
Regards,
Pavel Roskin

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

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> On Mon, 2008-07-21 at 17:20 +0200, Marco Gerards wrote:
>> Javier Martín <lordhabbit@gmail.com> writes:
>> 
>> > So what? Aren't both Linux and GRUB under the GPL? That _should_ mean
>> > that we can look at their code and put it into GRUB ("create a
>> > derivative work") either as-is or modified.
>> 
>> For GRUB 2 we require copyright assignments.
>
> Also, Linux is under GPLv2 and GRUB is under GPLv3.  This means that
> Linux developers could object that their code is restricted further than
> GPLv2 allows, e.g. there are additional anti-DRM provisions that the
> recipients of the GRUB code has to fulfill.
>
> That said, I cannot imagine that copying of variable names could be
> treated as a copyright violation.  For instance, Wine reimplements
> Windows API, and they just have to use the same names.

Please do not put words in my mouth, that is not what I said.  I said
looking at Linux *code* should be avoided.  If you have a look at the
ATA *code*, you might copy their algorithms or whatever.  I am asking
you to be careful.  Looking at code might give you an idea about how
to solve a problem, you might even implement this and legally... well,
I am not sure when you violate copyright or when not.  I cannot tell
if you looked at their code if you opened the files, study the
variable names and how they are used.

--
Marco

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

[Pavel Roskin]

On Mon, 2008-07-21 at 17:53 +0200, Marco Gerards wrote:

> Please do not put words in my mouth, that is not what I said.  I said
> looking at Linux *code* should be avoided.

I agree with you.  I was referring to what I did, not to what you said.

-- 
Regards,
Pavel Roskin

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

[Marco Gerards]

Pavel Roskin <proski@gnu.org> writes:

> On Mon, 2008-07-21 at 17:53 +0200, Marco Gerards wrote:
>
>> Please do not put words in my mouth, that is not what I said.  I said
>> looking at Linux *code* should be avoided.
>
> I agree with you.  I was referring to what I did, not to what you said.

Good :-)

--
Marco