LinuxLists.cc - ide-disk.c rev 1.13 killed CONFIG_IDEDISK

[permalink] [raw]

Subject: Re: ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Sat, Aug 02, 2003 at 02:42:05AM -0600, Erik Andersen wrote:

> Anybody know what the intent of this IDE_STROKE_LIMIT macro is?
> I ask since it completely breaks CONFIG_IDEDISK_STROKE and I can't see
> any reason for it being there. It looks to me like it just needs to be
> ripped right back out.

I agree entirely.

> I have created the following patch which makes this option work
> as expected once again in 2.4.x.

Will you also submit the corresponding 2.6 patch?

> #define IDE_STROKE_LIMIT (32000*1024*2)

I can guess where this limit came from.
The most common way people meet this situation is when they have a
large disk, so large that their BIOS faints when seeing it, so that
they have to use a clipping jumper to fake a smaller disk. Such
jumpers usually clip to about 32 GB (namely 66055248 sectors).
So 65536000 is a lower limit to what one sees with clips.

Maybe it is intended to protect against old disks that do not
understand these new commands. Andre? Bart? Alan?

> + printk(KERN_INFO "%s: Host Protected Area detected.\n"

This text might lead to some confusion: as just remarked, usually
we have a clipping jumper, not a proper Host Protected Area.

#ifdef CONFIG_IDEDISK_STROKE

I am also unhappy about the fact that this is a configuration option,
one of the zillion we have. A boot parameter would be a better choice.

Does anyone know about disks that get unhappy if we just do this
stuff unconditionally (or, to be more precise, do it when the IDENTIFY
output says we can) ?
Maybe not even a boot parameter is needed and we can do the right thing
automatically.

Andries

2003-08-02 13:11:15

[permalink] [raw]

Subject: Re: ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Sat, 2 Aug 2003, Andries Brouwer wrote:

> Maybe it is intended to protect against old disks that do not
> understand these new commands. Andre? Bart? Alan?

Some Samsung disks lock up. Probably we should check if HPA
command set is supported instead of using IDE_STROKE_LIMIT.

--
Bartlomiej

2003-08-02 17:42:33

[permalink] [raw]

Subject: Re: ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Sat, Aug 02, 2003 at 03:10:43PM +0200, Bartlomiej Zolnierkiewicz wrote:

> On Sat, 2 Aug 2003, Andries Brouwer wrote:
>
> > Maybe it is intended to protect against old disks that do not
> > understand these new commands. Andre? Bart? Alan?
>
> Some Samsung disks lock up. Probably we should check if HPA
> command set is supported instead of using IDE_STROKE_LIMIT.

OK, so we have to investigate. This strange test was inserted
in 2.4 and 2.5 via Alan, and google gives me Alan's changelog:

Linux 2.5.66-ac1
o Don't issue WIN_SET_MAX on older drivers (Jens Axboe)
(Breaks some Samsung)

So, now the question is to Jens: what was the situation?
What disk, kernel, identify output?

If possible we would like to remove the test and test the
right bits instead. But if that Samsung disk claims it
supports HPA and doesnt..

Andries

[By the way, google also shows examples where this test
breaks a setup, so removing it might be a good idea
under all circumstances. The usual jumper goes from
above 32GB to 32GB, and from below 32GB to 2GB.
There are also examples of the latter kind solved
by STROKE, but no longer by STROKE + faulty test.]

2003-08-02 21:10:24

by Alan

[permalink] [raw]

Subject: Re: ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Sad, 2003-08-02 at 18:42, Andries Brouwer wrote:
> OK, so we have to investigate. This strange test was inserted
> in 2.4 and 2.5 via Alan, and google gives me Alan's changelog:
>
> Linux 2.5.66-ac1
> o Don't issue WIN_SET_MAX on older drivers (Jens Axboe)
> (Breaks some Samsung)

Some older Samsung drives don't abort WIN_SET_MAX but the firmware
hangs hence the check.

> If possible we would like to remove the test and test the
> right bits instead. But if that Samsung disk claims it
> supports HPA and doesnt..

That would be better if it is the case

2003-08-02 23:34:48

by Erik Andersen

[permalink] [raw]

Subject: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Sat Aug 02, 2003 at 10:06:19PM +0100, Alan Cox wrote:
> On Sad, 2003-08-02 at 18:42, Andries Brouwer wrote:
> > OK, so we have to investigate. This strange test was inserted
> > in 2.4 and 2.5 via Alan, and google gives me Alan's changelog:
> >
> > Linux 2.5.66-ac1
> > o Don't issue WIN_SET_MAX on older drivers (Jens Axboe)
> > (Breaks some Samsung)
>
> Some older Samsung drives don't abort WIN_SET_MAX but the firmware
> hangs hence the check.

Ok, I think I can actually test that one.

<rummages in ye olde box of hardware>

Cool, found it, I have an ancient Samsung SHD-3212A (426MB)
drive that will hopefully show the problem.

<sound of testing in the distance>

Ok, found the problem. The current code (in addition to being
badly written) does not even bother to test if the drive supports
the HPA feature set before issuing a WIN_SET_MAX call. In my
case, it didn't crash my Samsung drive, but it certainly did make
it complain rather loudly.

I have rewritten the init_idedisk_capacity() function and taught
it to behave itself. It is now much cleaner IMHO, and will only
issues SET_MAX* calls to drives that claim they support such
things. I've tested this patch with a 200GB drive, a 120GB
drive, an 80GB drive and my ancient Samsung drive and in each
case (48bit LBA, 28bit LBA, 28bit CHS w/o support for HPA), my
new version appears to the Right Thing(tm).

Attached is a patch vs 2.4.22-pre10, and a patch vs 2.6.0-pre2.
Please apply,

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

Attachments:

(No filename) (1.57 kB)
fixup-ide-hpa-2.4.patch (6.93 kB)
fixup-ide-hpa-2.6.patch (6.96 kB)
Download all attachments

2003-08-03 01:27:04

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Sat, Aug 02, 2003 at 05:34:38PM -0600, Erik Andersen wrote:

> I have rewritten the init_idedisk_capacity() function and taught
> it to behave itself. It is now much cleaner IMHO

Yes, nice cleanup.

Some comments for later - the patch can be applied as it is:

The assignment
drive->select.b.lba = 0/1;
is done in the first half of init_idedisk_capacity().
I don't think the presence or disabling of HPA has any effect
on b.lba, so there should not be such assignments in the
second, HPA, half.

My standard muttering: id->... should not be modified.

In my source I test drive->head * drive->sect for being nonzero
before dividing.

Andries

2003-08-03 02:12:39

by Erik Andersen

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Sun Aug 03, 2003 at 03:26:59AM +0200, Andries Brouwer wrote:
> On Sat, Aug 02, 2003 at 05:34:38PM -0600, Erik Andersen wrote:
>
> > I have rewritten the init_idedisk_capacity() function and taught
> > it to behave itself. It is now much cleaner IMHO
>
> Yes, nice cleanup.

Thanks. :-)

> Some comments for later - the patch can be applied as it is:
>
> The assignment
> drive->select.b.lba = 0/1;
> is done in the first half of init_idedisk_capacity().
> I don't think the presence or disabling of HPA has any effect
> on b.lba, so there should not be such assignments in the
> second, HPA, half.

Yes, you are right. This is garbage leftover from the previous
implementation. It should not change anything, but that would
be a nice additional cleanup.

> My standard muttering: id->... should not be modified.

Agreed, but that is best left for another patch, since I did not
want to walk through the whole ide stack checking for things that
depends on this behavior. Hopefully nothing, but you never know.

> In my source I test drive->head * drive->sect for being nonzero
> before dividing.

That would also be a nice additional cleanup.

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

2003-08-03 09:53:12

by Jens Axboe

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Sat, Aug 02 2003, Erik Andersen wrote:
> On Sat Aug 02, 2003 at 10:06:19PM +0100, Alan Cox wrote:
> > On Sad, 2003-08-02 at 18:42, Andries Brouwer wrote:
> > > OK, so we have to investigate. This strange test was inserted
> > > in 2.4 and 2.5 via Alan, and google gives me Alan's changelog:
> > >
> > > Linux 2.5.66-ac1
> > > o Don't issue WIN_SET_MAX on older drivers (Jens Axboe)
> > > (Breaks some Samsung)
> >
> > Some older Samsung drives don't abort WIN_SET_MAX but the firmware
> > hangs hence the check.
>
> Ok, I think I can actually test that one.
>
> <rummages in ye olde box of hardware>
>
> Cool, found it, I have an ancient Samsung SHD-3212A (426MB)
> drive that will hopefully show the problem.
>
> <sound of testing in the distance>
>
> Ok, found the problem. The current code (in addition to being
> badly written) does not even bother to test if the drive supports
> the HPA feature set before issuing a WIN_SET_MAX call. In my
> case, it didn't crash my Samsung drive, but it certainly did make
> it complain rather loudly.
>
> I have rewritten the init_idedisk_capacity() function and taught
> it to behave itself. It is now much cleaner IMHO, and will only
> issues SET_MAX* calls to drives that claim they support such
> things. I've tested this patch with a 200GB drive, a 120GB
> drive, an 80GB drive and my ancient Samsung drive and in each
> case (48bit LBA, 28bit LBA, 28bit CHS w/o support for HPA), my
> new version appears to the Right Thing(tm).
>
> Attached is a patch vs 2.4.22-pre10, and a patch vs 2.6.0-pre2.
> Please apply,

Very nice Erik, looks good!

--
Jens Axboe

2003-08-06 18:32:53

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Wed, 6 Aug 2003, Erik Andersen wrote:

> > Can you please resend me the patch once its accepted in 2.6 ?
> >
> > Maybe we want to test it a while in -ac, also?
>
> Ok. Though it is being incorporated as part of a much larger
> patch in 2.6. I suppose that will filter back into 2.4 when
> it is ready,

Hi Erik,

I made init_idedisk_capacity() cleanup.
Then I ported your patch and reworked it a bit.

Could you take a look?

Thanks,
--
Bartlomiej

Attachments:

ide-disk-capacity-init-cleanup.patch (3.88 kB)
ide-disk-hpa-fixup.patch (6.51 kB)
Download all attachments

2003-08-06 19:31:05

by Erik Andersen

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Wed Aug 06, 2003 at 08:32:23PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> On Wed, 6 Aug 2003, Erik Andersen wrote:
>
> > > Can you please resend me the patch once its accepted in 2.6 ?
> > >
> > > Maybe we want to test it a while in -ac, also?
> >
> > Ok. Though it is being incorporated as part of a much larger
> > patch in 2.6. I suppose that will filter back into 2.4 when
> > it is ready,
>
> Hi Erik,
>
> I made init_idedisk_capacity() cleanup.
> Then I ported your patch and reworked it a bit.
>
> Could you take a look?

I like your improvements. The only concern I have is you
retained my use of do_div() intact. That needs to change since
it turns out that do_div() directly modifies the numerator
(violating the principle of least surprise). See the recent
thread on "do_div considered harmful". In that thread, Andries
mentions he has made a generic sectors_to_MB() function to
consolidate such things. which I think is a very good idea since
then this only has to be correct once. That would also let you
eliminate the somewhat ugly and not particularly obvious division
plus rounding (X - X/625 + 974)/1950 sequence from
idedisk_check_hpa_lba28(),

-Erik

--
Erik B. Andersen http://codepoet-consulting.com/
--This message was written using 73% post-consumer electrons--

2003-08-06 19:59:03

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Wed, 6 Aug 2003, Erik Andersen wrote:

> On Wed Aug 06, 2003 at 08:32:23PM +0200, Bartlomiej Zolnierkiewicz wrote:
> >
> > On Wed, 6 Aug 2003, Erik Andersen wrote:
> >
> > > > Can you please resend me the patch once its accepted in 2.6 ?
> > > >
> > > > Maybe we want to test it a while in -ac, also?
> > >
> > > Ok. Though it is being incorporated as part of a much larger
> > > patch in 2.6. I suppose that will filter back into 2.4 when
> > > it is ready,
> >
> > Hi Erik,
> >
> > I made init_idedisk_capacity() cleanup.
> > Then I ported your patch and reworked it a bit.
> >
> > Could you take a look?
>
> I like your improvements. The only concern I have is you
> retained my use of do_div() intact. That needs to change since
> it turns out that do_div() directly modifies the numerator
> (violating the principle of least surprise). See the recent
> thread on "do_div considered harmful". In that thread, Andries
> mentions he has made a generic sectors_to_MB() function to
> consolidate such things. which I think is a very good idea since
> then this only has to be correct once. That would also let you
> eliminate the somewhat ugly and not particularly obvious division
> plus rounding (X - X/625 + 974)/1950 sequence from
> idedisk_check_hpa_lba28(),

Oh yes... I forgot about do_div() and sectors_to_MB().
I will correct this before submitting to Linus.

Thanks,
--
Bartlomiej

2003-08-07 01:11:50

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Wed, Aug 06, 2003 at 08:32:23PM +0200, Bartlomiej Zolnierkiewicz wrote:

> diff -puN drivers/ide/ide-disk.c~ide-disk-capacity-init-cleanup drivers/ide/ide-disk.c
> --- linux-2.6.0-test2-bk5/drivers/ide/ide-disk.c~ide-disk-capacity-init-cleanup 2003-08-06 02:48:33.000000000 +0200

Ha - and you didnt even tell me you had this patch out.

Looks good.
You forgot to correct the do_div.
The part I most object to are things like

> + id->lba_capacity_2 = capacity_2 = set_max_ext;

There have been many problems in the past, and it is a bad idea to add
more of this. We should be eliminating all cases.

I mean:

We have info from BIOS, user, disk etc and conclude to a certain geometry.
Sneakily changing what the disk reported is very ugly. I recall a case
where a disk bounced between two capacities because the value that this
computation concluded to was not a fixed point. Also, the user gets an
incorrect report from HDIO_GET_IDENTITY.

So, the clean way is to examine what the disk reported, never change it
(except for the part

--------------------------------------------------------------
void ide_fix_driveid (struct hd_driveid *id)
{
#ifndef __LITTLE_ENDIAN
# ifdef __BIG_ENDIAN
int i;
u16 *shortcast;

shortcast = (u16 *) id;
for (i = 0; i < (sizeof(struct hd_driveid) / 2); i++)
shortcast[i] = __le16_to_cpu(shortcast[i]);
# else
# error "Please fix <asm/byteorder.h>"
# endif
#endif
}
--------------------------------------------------------------

that brings shorts into CPU order)
and store the results elsewhere. In particular, our conclusions
should go into drive->capacity and should not overwrite
id->lba_capacity.

Andries

[will try to find the appropriate fragment of my patch again
for comparison purposes]

2003-08-07 02:00:31

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

From [email protected] Thu Aug 7 03:54:02 2003

Ha - and you didnt even tell me you had this patch out.

Looks good.

[will try to find the appropriate fragment of my patch again
for comparison purposes]

Here it is:

diff -u --recursive --new-file -X /linux/dontdiff a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c Wed Aug 6 23:02:38 2003
+++ b/drivers/ide/ide-disk.c Thu Aug 7 04:49:18 2003
@@ -85,11 +85,6 @@
{
unsigned long lba_sects, chs_sects, head, tail;

- if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
- printk("48-bit Drive: %llu \n", id->lba_capacity_2);
- return 1;
- }
-
/*
* The ATA spec tells large drives to return
* C/H/S = 16383/16/63 independent of their size.
@@ -811,7 +806,7 @@
} else {
u8 cur = hwif->INB(IDE_SELECT_REG);
if (cur & 0x40) { /* using LBA? */
- printk(", LBAsect=%ld", (unsigned long)
+ printk(", LBAsect=%lu", (unsigned long)
((cur&0xf)<<24)
|(hwif->INB(IDE_HCYL_REG)<<16)
|(hwif->INB(IDE_LCYL_REG)<<8)
@@ -981,8 +976,8 @@
args.tfRegister[IDE_SELECT_OFFSET] = 0x40;
args.tfRegister[IDE_COMMAND_OFFSET] = WIN_READ_NATIVE_MAX_EXT;
args.command_type = ide_cmd_type_parser(&args);
- /* submit command request */
- ide_raw_taskfile(drive, &args, NULL);
+ /* submit command request */
+ ide_raw_taskfile(drive, &args, NULL);

/* if OK, compute maximum address value */
if ((args.tfRegister[IDE_STATUS_OFFSET] & 0x01) == 0) {
@@ -1002,6 +997,8 @@
/*
* Sets maximum virtual LBA address of the drive.
* Returns new maximum virtual LBA address (> 0) or 0 on failure.
+ *
+ * Must be immediately preceded by read_native_max.
*/
static unsigned long
idedisk_set_max_address(ide_drive_t *drive, unsigned long addr_req)
@@ -1031,6 +1028,7 @@
return addr_set;
}

+/* Note: must be immediately preceded by read_native_max_ext */
static unsigned long long
idedisk_set_max_address_ext(ide_drive_t *drive, unsigned long long addr_req)
{
@@ -1071,114 +1069,153 @@

static unsigned long long
sectors_to_MB(unsigned long long n) {
- n <<= 9; /* make it bytes */
- do_div(n, 1000000); /* make it MB */
+ n <<= 9; /* make it bytes */
+ do_div(n, 1000000); /* make it MB */
return n;
}

+static inline int
+idedisk_supports_lba(const struct hd_driveid *id)
+{
+ return (id->capability & 2);
+}
+
/*
- * Tests if the drive supports Host Protected Area feature.
- * Returns true if supported, false otherwise.
+ * Bits 10 of command_set_1 and cfs_enable_1 must be equal,
+ * so on non-buggy drives we need test only one.
+ * However, we should also check whether these fields are valid.
*/
-static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
+static inline int
+idedisk_supports_host_protected_area(const struct hd_driveid *id)
{
- int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
- if (flag)
- printk(KERN_INFO "%s: host protected area => %d\n", drive->name, flag);
- return flag;
+ return (id->command_set_1 & 0x0400) && (id->cfs_enable_1 & 0x0400);
}

/*
- * Compute drive->capacity, the full capacity of the drive
- * Called with drive->id != NULL.
- *
- * To compute capacity, this uses either of
- *
- * 1. CHS value set by user (whatever user sets will be trusted)
- * 2. LBA value from target drive (require new ATA feature)
- * 3. LBA value from system BIOS (new one is OK, old one may break)
- * 4. CHS value from system BIOS (traditional style)
- *
- * in above order (i.e., if value of higher priority is available,
- * reset will be ignored).
+ * The same here.
*/
-#define IDE_STROKE_LIMIT (32000*1024*2)
-static void init_idedisk_capacity (ide_drive_t *drive)
+static inline int
+idedisk_supports_lba48(const struct hd_driveid *id)
{
- struct hd_driveid *id = drive->id;
- unsigned long capacity = drive->cyl * drive->head * drive->sect;
- unsigned long set_max = idedisk_read_native_max_address(drive);
- unsigned long long capacity_2 = capacity;
- unsigned long long set_max_ext;
-
- drive->capacity48 = 0;
- drive->select.b.lba = 0;
+ return (id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400);
+}

- (void) idedisk_supports_host_protected_area(drive);
+static inline unsigned long long
+lba48_capacity(const struct hd_driveid *id)
+{
+ return id->lba_capacity_2;
+}

- if (id->cfs_enable_2 & 0x0400) {
- capacity_2 = id->lba_capacity_2;
- drive->head = drive->bios_head = 255;
- drive->sect = drive->bios_sect = 63;
- drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
- drive->select.b.lba = 1;
- set_max_ext = idedisk_read_native_max_address_ext(drive);
- if (set_max_ext > capacity_2 && capacity_2 > IDE_STROKE_LIMIT) {
+/*
+ * See whether some part of the disk was set off as Host Protected Area.
+ * If so, report this, and possible enable access to it.
+ */
+static void
+do_add_hpa(ide_drive_t *drive) {
+ unsigned int capacity;
+ unsigned long set_max;
+
+ capacity = drive->capacity;
+ set_max = idedisk_read_native_max_address(drive);
+
+ if (set_max > capacity) {
+ /* Report */
+ printk(KERN_INFO "%s: Host Protected Area detected.\n"
+ " current capacity is %u sectors (%u MB)\n"
+ " native capacity is %lu sectors (%lu MB)\n",
+ drive->name, capacity,
+ (capacity - capacity/625 + 974)/1950,
+ set_max, (set_max - set_max/625 + 974)/1950);
#ifdef CONFIG_IDEDISK_STROKE
- set_max_ext = idedisk_read_native_max_address_ext(drive);
- set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
- if (set_max_ext) {
- drive->capacity48 = capacity_2 = set_max_ext;
- drive->cyl = (unsigned int) set_max_ext / (drive->head * drive->sect);
- drive->select.b.lba = 1;
- drive->id->lba_capacity_2 = capacity_2;
- }
-#else /* !CONFIG_IDEDISK_STROKE */
- printk(KERN_INFO "%s: setmax_ext LBA %llu, native %llu\n",
- drive->name, set_max_ext, capacity_2);
-#endif /* CONFIG_IDEDISK_STROKE */
+ /* Raise limit */
+ set_max = idedisk_set_max_address(drive, set_max);
+ if (set_max) {
+ drive->capacity = set_max;
+ printk(KERN_INFO "%s: Host Protected Area Disabled\n",
+ drive->name);
}
- drive->cyl = (unsigned int) capacity_2 / (drive->head * drive->sect);
- drive->bios_cyl = drive->cyl;
- drive->capacity48 = capacity_2;
- drive->capacity = (unsigned long) capacity_2;
- return;
- /* Determine capacity, and use LBA if the drive properly supports it */
- } else if ((id->capability & 2) && lba_capacity_is_ok(id)) {
- capacity = id->lba_capacity;
- drive->cyl = capacity / (drive->head * drive->sect);
- drive->select.b.lba = 1;
+#endif
}
+}
+
+static void
+do_add_hpa48(ide_drive_t *drive) {
+ unsigned long long set_max_ext;

- if (set_max > capacity && capacity > IDE_STROKE_LIMIT) {
+ set_max_ext = idedisk_read_native_max_address_ext(drive);
+ if (set_max_ext > drive->capacity48) {
+ unsigned long long nativeMB, currentMB;
+
+ /* Report on additional space */
+ nativeMB = sectors_to_MB(set_max_ext);
+ currentMB = sectors_to_MB(drive->capacity48);
+ printk(KERN_INFO
+ "%s: Host Protected Area detected.\n"
+ " current capacity is %llu sectors (%llu MB)\n"
+ " native capacity is %llu sectors (%llu MB)\n",
+ drive->name, drive->capacity48, currentMB,
+ set_max_ext, nativeMB);
#ifdef CONFIG_IDEDISK_STROKE
- set_max = idedisk_read_native_max_address(drive);
- set_max = idedisk_set_max_address(drive, set_max);
- if (set_max) {
- drive->capacity = capacity = set_max;
- drive->cyl = set_max / (drive->head * drive->sect);
- drive->select.b.lba = 1;
- drive->id->lba_capacity = capacity;
- }
-#else /* !CONFIG_IDEDISK_STROKE */
- printk(KERN_INFO "%s: setmax LBA %lu, native %lu\n",
- drive->name, set_max, capacity);
-#endif /* CONFIG_IDEDISK_STROKE */
+ /* Raise limit */
+ set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
+ if (set_max_ext) {
+ drive->capacity48 = set_max_ext;
+ printk(KERN_INFO
+ "%s: Host Protected Area Disabled\n",
+ drive->name);
+ }
+#endif
}
+}

- drive->capacity = capacity;
+/*
+ * Compute drive->capacity, the amount accessible with CHS/LBA commands,
+ * and drive->capacity48, the amount accessible with LBA48 commands.
+ * Also sets drive->select.b.lba.
+ *
+ * Called with drive->id != NULL.
+ */
+static void init_idedisk_capacity(ide_drive_t *drive)
+{
+ struct hd_driveid *id;
+ unsigned long capacity;
+ unsigned long long capacity48;
+
+ id = drive->id;
+
+ if (idedisk_supports_lba48(id)) {
+ /* drive speaks 48-bit LBA */
+ drive->capacity48 = capacity48 = lba48_capacity(id);
+ capacity = capacity48; /* truncate to 32 bits */
+ if (capacity == capacity48)
+ drive->capacity = capacity;
+ else
+ drive->capacity = 0xffffffff;
+ drive->select.b.lba = 1;
+ } else if (idedisk_supports_lba(id) && lba_capacity_is_ok(id)) {
+ /* drive speaks 28-bit LBA */
+ drive->capacity = capacity = id->lba_capacity;
+ drive->capacity48 = 0;
+ drive->select.b.lba = 1;
+ } else {
+ /* drive speaks CHS only */
+ capacity = drive->cyl * drive->head * drive->sect;
+ drive->capacity = capacity;
+ drive->capacity48 = 0;
+ drive->select.b.lba = 0;
+ }

- if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
- drive->capacity48 = id->lba_capacity_2;
- drive->head = 255;
- drive->sect = 63;
- drive->cyl = (unsigned long)(drive->capacity48) / (drive->head * drive->sect);
+ if (idedisk_supports_host_protected_area(id)) {
+ if (idedisk_supports_lba48(id))
+ do_add_hpa48(drive);
+ else
+ do_add_hpa(drive);
}
}

static sector_t idedisk_capacity (ide_drive_t *drive)
{
- if (drive->id->cfs_enable_2 & 0x0400)
+ if (idedisk_supports_lba48(drive->id))
return (drive->capacity48 - drive->sect0);
return (drive->capacity - drive->sect0);
}
@@ -1469,7 +1506,7 @@
if (HWIF(drive)->addressing)
return 0;

- if (!(drive->id->cfs_enable_2 & 0x0400))
+ if (!idedisk_supports_lba48(drive->id))
return -EIO;
drive->addressing = arg;
return 0;
@@ -1639,19 +1676,29 @@
* by correcting bios_cyls:
*/
capacity = idedisk_capacity (drive);
- if (!drive->forced_geom && drive->bios_sect && drive->bios_head) {
- unsigned int cap0 = capacity; /* truncate to 32 bits */
- unsigned int cylsz, cyl;
-
- if (cap0 != capacity)
- drive->bios_cyl = 65535;
- else {
- cylsz = drive->bios_sect * drive->bios_head;
- cyl = cap0 / cylsz;
- if (cyl > 65535)
- cyl = 65535;
- if (cyl > drive->bios_cyl)
- drive->bios_cyl = cyl;
+ if (!drive->forced_geom) {
+ int lba48 = idedisk_supports_lba48(id);
+
+ if (lba48) {
+ /* compatibility */
+ drive->bios_sect = 63;
+ drive->bios_head = 255;
+ }
+
+ if (drive->bios_sect && drive->bios_head) {
+ unsigned int cap0 = capacity; /* truncate to 32 bits */
+ unsigned int cylsz, cyl;
+
+ if (cap0 != capacity)
+ drive->bios_cyl = 65535;
+ else {
+ cylsz = drive->bios_sect * drive->bios_head;
+ cyl = cap0 / cylsz;
+ if (cyl > 65535)
+ cyl = 65535;
+ if (cyl > drive->bios_cyl || lba48)
+ drive->bios_cyl = cyl;
+ }
}
}
printk(KERN_INFO "%s: %llu sectors (%llu MB)",

2003-08-07 02:31:28

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Thu, 7 Aug 2003, Andries Brouwer wrote:

> On Wed, Aug 06, 2003 at 08:32:23PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> > diff -puN drivers/ide/ide-disk.c~ide-disk-capacity-init-cleanup drivers/ide/ide-disk.c
> > --- linux-2.6.0-test2-bk5/drivers/ide/ide-disk.c~ide-disk-capacity-init-cleanup 2003-08-06 02:48:33.000000000 +0200
>
> Ha - and you didnt even tell me you had this patch out.

:-)

> Looks good.
> You forgot to correct the do_div.

Yep, Erik noticed this already.

> The part I most object to are things like
>
> > + id->lba_capacity_2 = capacity_2 = set_max_ext;
>
> There have been many problems in the past, and it is a bad idea to add
> more of this. We should be eliminating all cases.

What problems? This reflects real change in drive's identify
and I think should be replaced by rereading drive->id from a drive.

> I mean:
>
> We have info from BIOS, user, disk etc and conclude to a certain geometry.

I can't spot place when we get info from a BIOS.
If we get it from a user, user should know what she/he is doing.

We can deal with HPA before our geometry info is set
(by moving code that is in idedisk_setup() right before call to
init_idedisk_capacity()).

> Sneakily changing what the disk reported is very ugly. I recall a case
> where a disk bounced between two capacities because the value that this
> computation concluded to was not a fixed point. Also, the user gets an
> incorrect report from HDIO_GET_IDENTITY.

User gets correct report from HDIO_GET_IDENTIFY as drive's identify was
really changed. Moreover HDIO_GET_IDENTIFY needs fixing to actually
reread drive->id from a drive (similarly like /proc identify was fixed).

> So, the clean way is to examine what the disk reported, never change it

Even if disk's info changes? I don't think so.
--
Bartlomiej

2003-08-07 09:58:04

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

> The part I most object to are things like
>
> > + id->lba_capacity_2 = capacity_2 = set_max_ext;
>
> There have been many problems in the past, and it is a bad idea to add
> more of this. We should be eliminating all cases.

What problems? This reflects real change in drive's identify
and I think should be replaced by rereading drive->id from a drive.

In order to be able to troubleshoot problems we need to be able
to reconstruct the information involved.

One part is the disk identity info that existed at boot time.
It was read by the kernel, and stored. It is returned by the
HDIO_GET_IDENTIFY ioctl, as used in e.g. hdparm -i.

There is also the current disk identity info.
It is found by asking the disk now, and is returned by e.g. hdparm -I.

> We have info from BIOS, user, disk etc and conclude
> to a certain geometry.

I can't spot place when we get info from a BIOS.

See arch/i386/boot/setup.S
(I ripped out ide-geometry.c recently, so the use has diminished.)

> Sneakily changing what the disk reported is very ugly. I recall a case
> where a disk bounced between two capacities because the value that this
> computation concluded to was not a fixed point. Also, the user gets an
> incorrect report from HDIO_GET_IDENTITY.

User gets correct report from HDIO_GET_IDENTIFY as drive's identify was
really changed. Moreover HDIO_GET_IDENTIFY needs fixing to actually
reread drive->id from a drive (similarly like /proc identify was fixed).

There are at least two objections.
First: we do not want the new identity, we want the old.
Second: if we ask the disk for identity again, you'll see that
more than this single field was changed.

If the user only wanted to know the current max size then there are
other means, like BLKGETSIZE.

> So, the clean way is to examine what the disk reported, never change it

Even if disk's info changes? I don't think so.

Yes. The disk geometry data that we use is drive->*
What the disk reported to us is drive->id->*.

Andries

2003-08-07 10:44:58

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Thu, 7 Aug 2003 [email protected] wrote:

> > The part I most object to are things like
> >
> > > + id->lba_capacity_2 = capacity_2 = set_max_ext;
> >
> > There have been many problems in the past, and it is a bad idea to add
> > more of this. We should be eliminating all cases.
>
> What problems? This reflects real change in drive's identify
> and I think should be replaced by rereading drive->id from a drive.
>
> In order to be able to troubleshoot problems we need to be able
> to reconstruct the information involved.

What problems are we talking about? :-)

> One part is the disk identity info that existed at boot time.
> It was read by the kernel, and stored. It is returned by the
> HDIO_GET_IDENTIFY ioctl, as used in e.g. hdparm -i.
>
> There is also the current disk identity info.
> It is found by asking the disk now, and is returned by e.g. hdparm -I.

What is a value of having these two identities
(other than questionable debugging aid which can be implemented
differently)?

> > We have info from BIOS, user, disk etc and conclude
> > to a certain geometry.
>
> I can't spot place when we get info from a BIOS.
>
> See arch/i386/boot/setup.S
> (I ripped out ide-geometry.c recently, so the use has diminished.)

Yep, this is no longer used.

> > Sneakily changing what the disk reported is very ugly. I recall a case
> > where a disk bounced between two capacities because the value that this
> > computation concluded to was not a fixed point. Also, the user gets an
> > incorrect report from HDIO_GET_IDENTITY.
>
> User gets correct report from HDIO_GET_IDENTIFY as drive's identify was
> really changed. Moreover HDIO_GET_IDENTIFY needs fixing to actually
> reread drive->id from a drive (similarly like /proc identify was fixed).
>
> There are at least two objections.
> First: we do not want the new identity, we want the old.

Question remains, what for?

> Second: if we ask the disk for identity again, you'll see that
> more than this single field was changed.

We are updating drive->id for changing DMA modes,
should this be "fixed" as well?

> If the user only wanted to know the current max size then there are
> other means, like BLKGETSIZE.

Yep, I think BLKGETSIZE should be used for such usages.

> > So, the clean way is to examine what the disk reported, never change it
>
> Even if disk's info changes? I don't think so.
>
> Yes. The disk geometry data that we use is drive->*
> What the disk reported to us is drive->id->*.

This is a contradiction if geometry of disk was changed cause
disk reported to us geometry data second time.

--
Bartlomiej

2003-08-07 11:23:14

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

From: Bartlomiej Zolnierkiewicz <[email protected]>

> In order to be able to troubleshoot problems we need to be able
> to reconstruct the information involved.

What problems are we talking about? :-)

I am happy that you think there are no problems.
I spent a considerable time eliminating.
On the other hand, if you don't know from experience
then a simple google search will tell you that this
has been a very painful area in the past.

> One part is the disk identity info that existed at boot time.
> It was read by the kernel, and stored. It is returned by the
> HDIO_GET_IDENTIFY ioctl, as used in e.g. hdparm -i.
>
> There is also the current disk identity info.
> It is found by asking the disk now, and is returned by e.g. hdparm -I.

What is a value of having these two identities

It helps a little in two ways. One, to help people with fdisk/LILO/..
disk geometry problems. Two, to investigate new disks.

It is good to have clean data.

Long ago distributions contained privately modified versions of packages.
Today they use rpms where the pristine sources and the patches are
clearly visible. Much better.

> Second: if we ask the disk for identity again, you'll see that
> more than this single field was changed.

We are updating drive->id for changing DMA modes,
should this be "fixed" as well?

Yes.

drive->id is what the drive says it can do.
If the kernel is satisfied with that then it just uses this data.
If the kernel is of the opinion that it also has to check
what kind of cable, and what kind of controller, then it
does not change drive->id->* but determines the mode it wants to use
and writes that to drive->*.

Moreover, drive->id tells you the situation after the BIOS did its setup.
Sometimes the BIOS knows things the kernel doesnt know. It is good to
have the information. Destroying information serves no useful purpose.

> > So, the clean way is to examine what the disk reported,
> > never change it
>
> Even if disk's info changes? I don't think so.
>
> Yes. The disk geometry data that we use is drive->*
> What the disk reported to us is drive->id->*.

This is a contradiction if geometry of disk was changed cause
disk reported to us geometry data second time.

I am not sure I can parse that.

Andries

-----
>From "man hdparm"

-i Display the identification info that was obtained
from the drive at boot time, if available.
-I Request identification info directly from the
drive.

2003-08-07 11:50:22

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

On Thu, 7 Aug 2003 [email protected] wrote:

> From: Bartlomiej Zolnierkiewicz <[email protected]>
>
> > In order to be able to troubleshoot problems we need to be able
> > to reconstruct the information involved.
>
> What problems are we talking about? :-)
>
> I am happy that you think there are no problems.
> I spent a considerable time eliminating.
> On the other hand, if you don't know from experience
> then a simple google search will tell you that this
> has been a very painful area in the past.

Very informative answer, thanks ;-).

> > One part is the disk identity info that existed at boot time.
> > It was read by the kernel, and stored. It is returned by the
> > HDIO_GET_IDENTIFY ioctl, as used in e.g. hdparm -i.
> >
> > There is also the current disk identity info.
> > It is found by asking the disk now, and is returned by e.g. hdparm -I.
>
> What is a value of having these two identities
>
> It helps a little in two ways. One, to help people with fdisk/LILO/..
> disk geometry problems. Two, to investigate new disks.
>
> It is good to have clean data.

Exacty, but I understand it differently.

> Long ago distributions contained privately modified versions of packages.
> Today they use rpms where the pristine sources and the patches are
> clearly visible. Much better.
>
> > Second: if we ask the disk for identity again, you'll see that
> > more than this single field was changed.
>
> We are updating drive->id for changing DMA modes,
> should this be "fixed" as well?
>
> Yes.
>
> drive->id is what the drive says it can do.

Nope, drive->id is what drive says it can do,
but also what it currently does.

> If the kernel is satisfied with that then it just uses this data.
> If the kernel is of the opinion that it also has to check
> what kind of cable, and what kind of controller, then it
> does not change drive->id->* but determines the mode it wants to use
> and writes that to drive->*.

drive->id also reports *current* active mode
(it changes on a drive itself).

> Moreover, drive->id tells you the situation after the BIOS did its setup.
> Sometimes the BIOS knows things the kernel doesnt know. It is good to
> have the information. Destroying information serves no useful purpose.
>
> > > So, the clean way is to examine what the disk reported,
> > > never change it
> >
> > Even if disk's info changes? I don't think so.
> >
> > Yes. The disk geometry data that we use is drive->*
> > What the disk reported to us is drive->id->*.
>
> This is a contradiction if geometry of disk was changed cause
> disk reported to us geometry data second time.
>
> I am not sure I can parse that.

After issuing SET_MAX_ADDRESS or SET_MAX_ADDRESS_EXT hardware drive->id
is updated and we are using this information, so disk reports to us geometry,
nope?

> -----
> >From "man hdparm"
>
> -i Display the identification info that was obtained
> from the drive at boot time, if available.
> -I Request identification info directly from the
> drive.

This is a show-stopper only for changing HDIO_GET_IDENTITY semantics,
not driver itself (we can have separate drive->boot_id for this).

I am starting to think that drive->id (reflecting actual identity) and
drive->boot_id (saved boot drive->id) is a long-term solution.

--
Bartlomiej

2003-08-07 13:22:11

[permalink] [raw]

Subject: Re: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE

From: Bartlomiej Zolnierkiewicz <[email protected]>

> > > So, the clean way is to examine what the disk reported,
> > > never change it
> >
> > Even if disk's info changes? I don't think so.
> >
> > Yes. The disk geometry data that we use is drive->*
> > What the disk reported to us is drive->id->*.

After issuing SET_MAX_ADDRESS or SET_MAX_ADDRESS_EXT hardware
drive->id is updated

Yes. So far no kernel has asked the disk for an update.
And I don't think we have seen cases where that would have been
necessary.

(I mean, theoretically it would be possible that the disk reported
at first that DMA was supported, while after SET_MAX_ADDRESS this
no longer is supported, e.g. because of some strange problem that
allows DMA only from/to the first 32GB. In practice we have never seen
such things, I think.)

and we are using this information,
so disk reports to us geometry, nope?

Still difficult to parse.

Let me just say some things. Maybe I answer your question, maybe not,
then ask again, as explicitly as possible.

What geometry stuff do we need? Let me sketch roughly, omitting details.

First, there is drive->{head,sect,cyl}.
If the drive does not know LBA, then we use drive->{head,sect} for
actual I/O. If the drive uses LBA we do not use drive->{head,sect,cyl}
at all. It is a bug when drive->head is larger than 16, or drive->sect
larger than 63.

Secondly, there is drive->bios_{head,sect,cyl}.
This is not what we use, but it is what we report when user space asks.
It is for LILO and fdisk. It is a very difficult question what we have
to answer here, but it is irrelevant for the kernel.

Thirdly, there is the total disk size.
The kernel stores this in drive->capacity{48}.
[Yes, that reminds me - having two values here is unnecessary.
One of my following patch fragments eliminates drive->capacity
so that only drive->capacity48 is left.]
What is actually used as total size, and also reported by BLKGETSIZE,
is the return value of current_capacity(), and it is equal to
drive->capacity48 minus the part used by a disk manager.

The kernel does not directly use id->lba_capacity anywhere, except
in the initial setup, in order to compute drive->capacity*.
(So, changing it does not do anything useful.)

> >From "man hdparm"
>
> -i Display the identification info that was obtained
> from the drive at boot time, if available.
> -I Request identification info directly from the
> drive.

This is a show-stopper only for changing HDIO_GET_IDENTITY semantics,
not driver itself (we can have separate drive->boot_id for this).

I am starting to think that drive->id (reflecting actual identity) and
drive->boot_id (saved boot drive->id) is a long-term solution.

Yes, in principle there is nothing wrong with that.

[But I wrote my first operating system on a 4K computer.
Spent hours reducing the size of a driver from 129 to 128 instructions.
Am permanently damaged now, very strongly conditioned against wasting memory.]

Andries

2003-08-07 13:50:39