PATCH: 2.6.10 - Still mishandles volumes without geometry data

PATCH: 2.6.10 - Still mishandles volumes without geometry data

[Alan Cox]

An IDE device can choose not to report geometry. In this situation the
base 2.6 kernel tries to verify the LBA data despite having nothing to
validate it against and prints it out (although now only as pr_debug).

This patch fixes these problems and has in various forms been in
2.6.9-ac and Fedora Core for a considerable time now

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.10/drivers/ide/ide-disk.c linux-2.6.10/drivers/ide/ide-disk.c
--- linux.vanilla-2.6.10/drivers/ide/ide-disk.c	2004-12-25 21:15:34.000000000 +0000
+++ linux-2.6.10/drivers/ide/ide-disk.c	2004-12-26 21:55:43.084714368 +0000
@@ -84,6 +84,10 @@
 {
 	unsigned long lba_sects, chs_sects, head, tail;
 
+	/* No non-LBA info .. so valid! */
+	if (id->cyls == 0)
+		return 1;
+		
 	/*
 	 * The ATA spec tells large drives to return
 	 * C/H/S = 16383/16/63 independent of their size.
@@ -201,7 +205,8 @@
 		head  = track % drive->head;
 		cyl   = track / drive->head;
 
-		pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
+		if(drive->bios_cyl)
+			pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
 
 		hwif->OUTB(0x00, IDE_FEATURE_REG);
 		hwif->OUTB(nsectors.b.low, IDE_NSECTOR_REG);

Re: PATCH: 2.6.10 - Still mishandles volumes without geometry data

[Bartlomiej Zolnierkiewicz]

On Mon, 27 Dec 2004 13:57:20 +0000, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> An IDE device can choose not to report geometry. In this situation the
> base 2.6 kernel tries to verify the LBA data despite having nothing to
> validate it against and prints it out (although now only as pr_debug).
> 
> This patch fixes these problems and has in various forms been in
> 2.6.9-ac and Fedora Core for a considerable time now
> 
> diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.10/drivers/ide/ide-disk.c linux-2.6.10/drivers/ide/ide-disk.c
> --- linux.vanilla-2.6.10/drivers/ide/ide-disk.c 2004-12-25 21:15:34.000000000 +0000
> +++ linux-2.6.10/drivers/ide/ide-disk.c 2004-12-26 21:55:43.084714368 +0000
> @@ -84,6 +84,10 @@
>  {
>         unsigned long lba_sects, chs_sects, head, tail;
> 
> +       /* No non-LBA info .. so valid! */
> +       if (id->cyls == 0)
> +               return 1;
> +
>         /*
>          * The ATA spec tells large drives to return
>          * C/H/S = 16383/16/63 independent of their size.
> @@ -201,7 +205,8 @@
>                 head  = track % drive->head;
>                 cyl   = track / drive->head;
> 
> -               pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
> +               if(drive->bios_cyl)
> +                       pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
> 
>                 hwif->OUTB(0x00, IDE_FEATURE_REG);
>                 hwif->OUTB(nsectors.b.low, IDE_NSECTOR_REG);

Shouldn't this check be around printk in idedisk_setup() instead?

Re: PATCH: 2.6.10 - Still mishandles volumes without geometry data

[Alan Cox]

> > +
> >         /*
> Shouldn't this check be around printk in idedisk_setup() instead?

Yes that would be useful as well. I'll send you a new patch that catches
that printk as well.

Re: PATCH: 2.6.10 - Still mishandles volumes without geometry data

[Alan Cox]

Ok how about this revision which also silences the boot time CHS data as
Bartlomiej suggested.

--- linux.vanilla-2.6.10/drivers/ide/ide-disk.c	2004-12-25 21:15:34.000000000 +0000
+++ linux-2.6.10/drivers/ide/ide-disk.c	2004-12-28 23:07:13.195925352 +0000
@@ -84,6 +84,10 @@
 {
 	unsigned long lba_sects, chs_sects, head, tail;
 
+	/* No non-LBA info .. so valid! */
+	if (id->cyls == 0)
+		return 1;
+		
 	/*
 	 * The ATA spec tells large drives to return
 	 * C/H/S = 16383/16/63 independent of their size.
@@ -201,7 +205,8 @@
 		head  = track % drive->head;
 		cyl   = track / drive->head;
 
-		pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
+		if(cyl)
+			pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
 
 		hwif->OUTB(0x00, IDE_FEATURE_REG);
 		hwif->OUTB(nsectors.b.low, IDE_NSECTOR_REG);
@@ -1239,8 +1257,9 @@
 	if (id->buf_size)
 		printk (" w/%dKiB Cache", id->buf_size/2);
 
-	printk(", CHS=%d/%d/%d", 
-	       drive->bios_cyl, drive->bios_head, drive->bios_sect);
+	if(drive->bios_cyl)
+		printk(", CHS=%d/%d/%d", 
+			drive->bios_cyl, drive->bios_head, drive->bios_sect);
 	if (drive->using_dma)
 		ide_dma_verbose(drive);
 	printk("\n");

Re: PATCH: 2.6.10 - Still mishandles volumes without geometry data

[Andries Brouwer]

On Tue, Dec 28, 2004 at 10:08:23PM +0000, Alan Cox wrote:
> Ok how about this revision which also silences the boot time CHS data as
> Bartlomiej suggested.
> 
> --- linux.vanilla-2.6.10/drivers/ide/ide-disk.c	2004-12-25 21:15:34.000000000 +0000
> +++ linux-2.6.10/drivers/ide/ide-disk.c	2004-12-28 23:07:13.195925352 +0000
> @@ -84,6 +84,10 @@
>  {
>  	unsigned long lba_sects, chs_sects, head, tail;
>  
> +	/* No non-LBA info .. so valid! */
> +	if (id->cyls == 0)
> +		return 1;
> +		
>  	/*
>  	 * The ATA spec tells large drives to return
>  	 * C/H/S = 16383/16/63 independent of their size.

Reasonable in case this actually occurs. Do you have examples?

> @@ -201,7 +205,8 @@
>  		head  = track % drive->head;
>  		cyl   = track / drive->head;
>  
> -		pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
> +		if(cyl)
> +			pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
>  
>  		hwif->OUTB(0x00, IDE_FEATURE_REG);
>  		hwif->OUTB(nsectors.b.low, IDE_NSECTOR_REG);

Unreasonable. This part must be a misunderstanding.
The cyl here is not the number of cylinders of a disk,
it is the cylinder affected by I/O and may well be zero.

> @@ -1239,8 +1257,9 @@
>  	if (id->buf_size)
>  		printk (" w/%dKiB Cache", id->buf_size/2);
>  
> -	printk(", CHS=%d/%d/%d", 
> -	       drive->bios_cyl, drive->bios_head, drive->bios_sect);
> +	if(drive->bios_cyl)
> +		printk(", CHS=%d/%d/%d", 
> +			drive->bios_cyl, drive->bios_head, drive->bios_sect);
>  	if (drive->using_dma)
>  		ide_dma_verbose(drive);
>  	printk("\n");

Reasonable. (But s/if(/if (/ .)
On the other hand, I like the "CHS=0/0/0" - it makes very clear what is wrong
in case lilo or so has geometry problems.

Andries

Re: PATCH: 2.6.10 - Still mishandles volumes without geometry data

[Alan Cox]

On Mer, 2004-12-29 at 02:52, Andries Brouwer wrote:
> > +	/* No non-LBA info .. so valid! */
> > +	if (id->cyls == 0)
> > +		return 1;
> > +		
> >  	/*
> >  	 * The ATA spec tells large drives to return
> >  	 * C/H/S = 16383/16/63 independent of their size.
> 
> Reasonable in case this actually occurs. Do you have examples?

Yes - raid volumes on IT8212 is one such example.

> > -		pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
> > +		if(cyl)
> > +			pr_debug("%s: CHS=%u/%u/%u\n", drive->name, cyl, head, sect);
> >  
> >  		hwif->OUTB(0x00, IDE_FEATURE_REG);
> >  		hwif->OUTB(nsectors.b.low, IDE_NSECTOR_REG);
> 
> Unreasonable. This part must be a misunderstanding.
> The cyl here is not the number of cylinders of a disk,

Yep - as Bartlomiej correctly noted this was a mismerge when I fixed up
to 2.6.10

> > -	printk(", CHS=%d/%d/%d", 
> > -	       drive->bios_cyl, drive->bios_head, drive->bios_sect);
> > +	if(drive->bios_cyl)
> > +		printk(", CHS=%d/%d/%d", 
> > +			drive->bios_cyl, drive->bios_head, drive->bios_sect);
> >  	if (drive->using_dma)
> >  		ide_dma_verbose(drive);
> >  	printk("\n");
> 
> Reasonable. (But s/if(/if (/ .)
> On the other hand, I like the "CHS=0/0/0" - it makes very clear what is wrong
> in case lilo or so has geometry problems.

0/0/0 is valid in these cases - would it be better if it printed
something else instead for that situation ("No physical geometry, ")
perhaps ?

Re: PATCH: 2.6.10 - Still mishandles volumes without geometry data

[Andries Brouwer]

On Wed, Dec 29, 2004 at 02:36:11PM +0000, Alan Cox wrote:

> > > -	printk(", CHS=%d/%d/%d", 
> > > -	       drive->bios_cyl, drive->bios_head, drive->bios_sect);
> > > +	if(drive->bios_cyl)
> > > +		printk(", CHS=%d/%d/%d", 
> > > +			drive->bios_cyl, drive->bios_head, drive->bios_sect);
> > 
> > Reasonable. (But s/if(/if (/ .)
> > On the other hand, I like the "CHS=0/0/0" - it makes very clear what is wrong
> > in case lilo or so has geometry problems.
> 
> 0/0/0 is valid in these cases - would it be better if it printed
> something else instead for that situation ("No physical geometry, ")
> perhaps ?

But it is not the "physical geometry" (whatever that may be) that is printed.
Not drive->head but drive->bios_head. It is easiest just to leave it.

Andries