--- /usr/src/temp/linux-2.4.21/drivers/ide/raid/pdcraid.c 2003-06-13 07:51:34.000000000 -0700
+++ pdcraid.c 2003-07-16 19:03:15.000000000 -0700
@@ -361,8 +361,14 @@
if (ideinfo->sect==0)
return 0;
- lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
- lba = lba * (ideinfo->head*ideinfo->sect);
- lba = lba - ideinfo->sect;
+
+ float lbatemp = 0;
+ float head = ideinfo->head;
+ float sect = ideinfo->sect;
+ float capacity = ideinfo->capacity;
+ lbatemp = (capacity / (head*sect));
+ lbatemp = lbatemp * (head*sect);
+ lbatemp = lbatemp - sect;
+ lba = lbatemp;
return lba;
}
On Thu, 2003-07-17 at 04:26, Walt H wrote:
> compatible with the binary FastTrak.o module. I'm not much of a coder,
> so if this could be done more efficiently than my attached patch, please
> let me know. Please CC any replies. Thanks,
(un)fortionatly it's not valid to use floating point in the kernel.
Could you try the same thing by using u64 as type instead please ?
I don't think this has any to do with the calculation, you probably just have to use:
CONFIG_PDC202XX_FORCE=y
(well, combined with...
CONFIG_BLK_DEV_PDC202XX_NEW=y
CONFIG_BLK_DEV_PDC202XX=y
CONFIG_BLK_DEV_ATARAID=y
CONFIG_BLK_DEV_ATARAID_PDC=y
...and a lot of other options...)
With 2.4.20 the FORCE option had to be set at "n", now it should be set at "y".
> The calc_pdcblock_offset function calculates lba by taking the capacity
> of the drive and dividing it by (head * sector), multiplying the result
> times (head * sector) and subtracting the sector (SPT) count.
> Unfortunately, with the strange geometry reported by the new drive,
> using INTs to store these values will fail.
Avoiding floating-point precision, this would do the same as your calculation:
lba = ideinfo->capacity - ideinfo->sect;
However, I don't think the expression "fails". the way it is expressed now, it will divide something integer-wise and then multiply it with the same value (and then minus sect), thus I assume this was intended: To round the capacity value to the next multiple of (head * start), and then do a minus sect...
I don't think any hardcore programmer ever does a integerwise divide /before/ an integerwise multiply without a very good reason... (And don't give me that "you'd be surprised"!!!)
Arjan van de Ven wrote:
> On Thu, 2003-07-17 at 04:26, Walt H wrote:
>
>>compatible with the binary FastTrak.o module. I'm not much of a coder,
>>so if this could be done more efficiently than my attached patch, please
>>let me know. Please CC any replies. Thanks,
>
>
> (un)fortionatly it's not valid to use floating point in the kernel.
> Could you try the same thing by using u64 as type instead please ?
I've tried it using unsigned long, and it will fail to find the second
drive. The problem is that the FastTrak bios writes the superblock to
the second drive in the same place as if it had the geometry of the
first drive. How it does it, I do not know. What I know is that the
offset for the superblock on both drives lies at: 80418177
On the first drive, you get there like this:
capacity = 80418240, head = 16, sect = 63
lba = capacity / (head * sect) = 79780
lba = lba * (head * sect) = 80418240
lba = lba - sect = 80418177
This one's correct.
On the second drive, it's like this:
capacity = 80418240, head=255, sect = 63
lba = capacity / (head * sect) = 5005 int or 5005.80 float
lba = lba * (head * sect) = 80405325 int or 80418240.01 float
lba = lba - sect = 80405262 int or 80418177 float
If integer results are used, the second drive's offset is returned as
80405262, which is not the offset for the superblock. It lies at the
same location as the first drive.
Insmodding the module compiled with ints as calculations results in this:
Jul 17 07:15:16 waltsathlon kernel: ataraid/disc0/disc: p1 p2 < >
Jul 17 07:15:16 waltsathlon kernel: Drive 0 is 39266 Mb (33 / 0)
Jul 17 07:15:16 waltsathlon kernel: Raid0 array consists of 1 drives.
Jul 17 07:15:16 waltsathlon kernel: Promise Fasttrak(tm) Softwareraid
driver for linux version 0.03beta
While the one with floating calcs finds both drives.
My only guess at this point, is that the FastTrak bios is using
different geometry than what's reported to us. I'm not sure how this
would work, but I've thought about storing the offset of the working
drive to use in the event that the offset calculation fails and the
capacity is identical on additional drives. Seems kinda hacky to me, but
then what do I know :) I'm up for trying things, any other ideas?
-Walt Holman
On Iau, 2003-07-17 at 15:37, Walt H wrote:
> On the second drive, it's like this:
> capacity = 80418240, head=255, sect = 63
> lba = capacity / (head * sect) = 5005 int or 5005.80 float
> lba = lba * (head * sect) = 80405325 int or 80418240.01 float
> lba = lba - sect = 80405262 int or 80418177 float
Would fixed point solve this. Start from capacity <<= 16 and then
do the maths. That would put lba in 65536ths of a sector which
should have the same result as your float maths
On Thu, Jul 17, 2003 at 03:58:38PM +0100, Alan Cox wrote:
> On Iau, 2003-07-17 at 15:37, Walt H wrote:
>
> > On the second drive, it's like this:
> > capacity = 80418240, head=255, sect = 63
> > lba = capacity / (head * sect) = 5005 int or 5005.80 float
> > lba = lba * (head * sect) = 80405325 int or 80418240.01 float
> > lba = lba - sect = 80405262 int or 80418177 float
>
> Would fixed point solve this. Start from capacity <<= 16 and then
> do the maths. That would put lba in 65536ths of a sector which
> should have the same result as your float maths
Ach Alan - I have not seen these earlier posts, but float or
fixed point here is just nonsense.
The purpose of
A = B/C;
A *= C;
can only be to round B down to the largest multiple of C below it.
Using infinite precision just turns this into
A = B;
He needs the first sector of the last cylinder, in a setup where
cylinders have size 16*63 or so, but the surrounding software
thinks that it is 255*63, a mistake.
I don't know anything about these RAIDs, but possibly it would
help to give boot parameters for this disk.
Maybe he is victim of the completely ridiculous
drive->head = 255;
in ide-disk.c.
(We have drive->head, the number of physical heads, and
drive->bios_head, the translation presently used by the BIOS -
or at least that is the intention. It is a bug if the former
is larger than 16. The latter may well be 255.)
Andries
Can't you fix the geometry from fdisk expert mode?
I've done that several times before, when otherwise like-sized disks
appeared with vastly different geometry.
On Thu, Jul 17, 2003 at 11:34:11AM -0400, Jeff Garzik wrote:
> Can't you fix the geometry from fdisk expert mode?
>
> I've done that several times before, when otherwise like-sized disks
> appeared with vastly different geometry.
It is easiest to think that it is meaningless what you say
(in this case). A disk does not have a geometry, and
moreover you cannot change it with fdisk.
However, under some circumstances, some kernels will guess
a (translated) geometry from a DOS-type partition table,
so it is true that under some kernel versions you can use
fdisk to change the kernel's ideas about disk geometry.
A very fragile activity.
[Moreover, there are several kinds of geometry, and the present
authors of ide-disk.c conveniently confused them all.
Concerning pdcraid, I don't know for which of the possible
ideas of geometry it is true that one needs the first sector
of the last cylinder.]
Andries
Andries Brouwer wrote:
> On Thu, Jul 17, 2003 at 03:58:38PM +0100, Alan Cox wrote:
>
>>On Iau, 2003-07-17 at 15:37, Walt H wrote:
>>
>>
>>>On the second drive, it's like this:
>>>capacity = 80418240, head=255, sect = 63
>>>lba = capacity / (head * sect) = 5005 int or 5005.80 float
>>>lba = lba * (head * sect) = 80405325 int or 80418240.01 float
>>>lba = lba - sect = 80405262 int or 80418177 float
>>
>>Would fixed point solve this. Start from capacity <<= 16 and then
>>do the maths. That would put lba in 65536ths of a sector which
>>should have the same result as your float maths
>
>
> Ach Alan - I have not seen these earlier posts, but float or
> fixed point here is just nonsense.
>
> The purpose of
> A = B/C;
> A *= C;
> can only be to round B down to the largest multiple of C below it.
> Using infinite precision just turns this into
> A = B;
>
> He needs the first sector of the last cylinder, in a setup where
> cylinders have size 16*63 or so, but the surrounding software
> thinks that it is 255*63, a mistake.
>
> I don't know anything about these RAIDs, but possibly it would
> help to give boot parameters for this disk.
>
> Maybe he is victim of the completely ridiculous
> drive->head = 255;
> in ide-disk.c.
> (We have drive->head, the number of physical heads, and
> drive->bios_head, the translation presently used by the BIOS -
> or at least that is the intention. It is a bug if the former
> is larger than 16. The latter may well be 255.)
>
> Andries
>
>
OK. Just got home from work. I've tried booting and specifying geometry
via hdg=79780,16,63 hdg=noprobe etc... The geometry is accepted,
however, drive access fails when trying to read the disk. This geometry
is the geometry reported by hde (my old drive without screwy geometry).
The code in calc_pdcblock_offset to calculate the offset is unchanged
in my patch (except the date type conversion to float) and calls
get_info_ptr for geometry. From what I can tell, this call returns the
physical geometry of the disk so using fdisk to fool it doesn't work.
I've also tried using unsigned long to store the calcs, multiplying the
capacity by 100 to start, doing the maths and dividing the final lba by
100. This doesn't work cause it overflows the data type. Won't I have
similar problems using fixed point? At this point, my very limited
knowledge of C keeps me from getting much further.
-Walt
On Thu, Jul 17, 2003 at 07:33:00PM -0700, Walt H wrote:
> OK. Just got home from work. I've tried booting and specifying geometry
> via hdg=79780,16,63 hdg=noprobe etc... The geometry is accepted,
> however, drive access fails when trying to read the disk. This geometry
> is the geometry reported by hde (my old drive without screwy geometry).
> The code in calc_pdcblock_offset to calculate the offset is unchanged
> in my patch (except the date type conversion to float) and calls
> get_info_ptr for geometry.
I don't understand. Did you introduce some float? Remove it immediately.
You just replace
lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
lba = lba * (ideinfo->head*ideinfo->sect);
lba = lba - ideinfo->sect;
by
lba = ideinfo->capacity - 63;
Then everything works for you, I suppose.
Subsequently we wait for other people with the same hardware
and see how the 63 varies as a function of their setup.
(Or maybe you can go into the BIOS and specify different
translations yourself?)
(By the way, didnt your boot parameters lead to ideinfo->head = 16
and ideinfo->sect = 63?)
Andries
Andries Brouwer wrote:
>
> I don't understand. Did you introduce some float? Remove it immediately.
>
> You just replace
>
> lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
> lba = lba * (ideinfo->head*ideinfo->sect);
> lba = lba - ideinfo->sect;
>
> by
>
> lba = ideinfo->capacity - 63;
>
> Then everything works for you, I suppose.
> Subsequently we wait for other people with the same hardware
> and see how the 63 varies as a function of their setup.
> (Or maybe you can go into the BIOS and specify different
> translations yourself?)
>
> (By the way, didnt your boot parameters lead to ideinfo->head = 16
> and ideinfo->sect = 63?)
>
> Andries
>
>
No, you're right. I was just trying to clarify the changes that I had
originally made. It seemed as there may have been some confusion that I
was doing more than the original. In my case, the simplified
ideinfo->capacity - ideinfo->sect should work just fine.
The boot parameters did take. The geometry was reported (correctly?) as
I had passed, but when trying to load the pdcraid module, access to the
disk failed with I/O errors. Seemed as if it was trying to read beyond
the end of the device. I used the identical geometry as reported by the
working drive.
Unfortunately, since this is the embedded FastTrak stuff, the BIOS
doesn't allow me to setup geometry for the drives.
I've just tried the simplified method and it works fine. I'll stick with
that on my end. Thanks for the help,
-Walt
--- /usr/src/temp/linux-2.4.21/drivers/ide/raid/pdcraid.c 2003-06-13 07:51:34.000000000 -0700
+++ pdcraid.c 2003-07-18 06:54:25.000000000 -0700
@@ -361,7 +361,8 @@
if (ideinfo->sect==0)
return 0;
- lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
+/* lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
lba = lba * (ideinfo->head*ideinfo->sect);
- lba = lba - ideinfo->sect;
+ lba = lba - ideinfo->sect; */
+ lba = ideinfo->capacity - ideinfo->sect;
return lba;