From: Erik Andersen <andersen@codepoet.org>
To: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Andries Brouwer <aebr@win.tue.nl>,
Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl>,
axboe@suse.de,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Marcelo Tosatti <marcelo@conectiva.com.br>
Subject: [PATCH] ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE
Date: Sat, 2 Aug 2003 17:34:38 -0600 [thread overview]
Message-ID: <20030802233438.GA7652@codepoet.org> (raw)
In-Reply-To: <1059858379.20305.23.camel@dhcp22.swansea.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]
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--
[-- Attachment #2: fixup-ide-hpa-2.4.patch --]
[-- Type: text/plain, Size: 7092 bytes --]
--- linux/drivers/ide/ide-disk.c.orig 2003-08-02 15:58:32.000000000 -0600
+++ linux/drivers/ide/ide-disk.c 2003-08-02 16:54:17.000000000 -0600
@@ -69,6 +69,7 @@
#include <asm/irq.h>
#include <asm/uaccess.h>
#include <asm/io.h>
+#include <asm/div64.h>
/* FIXME: some day we shouldnt need to look in here! */
@@ -1131,18 +1132,6 @@
#endif /* CONFIG_IDEDISK_STROKE */
/*
- * Tests if the drive supports Host Protected Area feature.
- * Returns true if supported, false otherwise.
- */
-static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
-{
- int flag = (drive->id->cfs_enable_1 & 0x0400) ? 1 : 0;
- if (flag)
- printk("%s: host protected area => %d\n", drive->name, flag);
- return flag;
-}
-
-/*
* Compute drive->capacity, the full capacity of the drive
* Called with drive->id != NULL.
*
@@ -1156,77 +1145,103 @@
* in above order (i.e., if value of higher priority is available,
* reset will be ignored).
*/
-#define IDE_STROKE_LIMIT (32000*1024*2)
static void init_idedisk_capacity (ide_drive_t *drive)
{
- 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;
-
- (void) idedisk_supports_host_protected_area(drive);
-
- 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) {
-#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 */
- }
- 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 */
+ struct hd_driveid *id;
+ unsigned long capacity;
+ unsigned long long capacity_2;
+
+ id = drive->id;
+ capacity = drive->cyl * drive->head * drive->sect;
+ capacity_2 = capacity;
+
+
+ /* Does this drive support the 48-bit Address Feature Set
+ * and does it have that enabled at the moment? */
+ if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
+ //drive speaks 48-bit LBA
+ drive->capacity = (unsigned long) capacity_2 = id->lba_capacity_2;
+ drive->capacity48 = 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->bios_cyl = drive->cyl;
+ drive->select.b.lba = 1;
} else if ((id->capability & 2) && lba_capacity_is_ok(id)) {
- capacity = id->lba_capacity;
+ // drive speaks 28-bit LBA
+ drive->capacity = capacity = id->lba_capacity;
+ drive->capacity48 = 0;
drive->cyl = capacity / (drive->head * drive->sect);
drive->select.b.lba = 1;
+ } else {
+ // drive speaks boring old 28-bit CHS
+ drive->capacity = capacity;
+ drive->capacity48 = 0;
+ drive->select.b.lba = 0;
}
- if (set_max > capacity && capacity > IDE_STROKE_LIMIT) {
+
+ /* If this drive supports the Host Protected Area feature set,
+ * then we may need to change our opinion about the drive's size... */
+ if (id->command_set_1 & 0x0400 && id->cfs_enable_1 & 0x0400)
+ {
+ /* Does this drive support the 48-bit Address Feature Set
+ * and does it have that enabled as well? */
+ if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400))
+ {
+ unsigned long long set_max_ext;
+ set_max_ext = idedisk_read_native_max_address_ext(drive);
+
+ /* If our capacity is smaller than the native capacity,
+ * we have an HPA created using SET MAX ADDRESS EXT... */
+ if (set_max_ext > capacity_2)
+ {
+ /* Sigh. We have to jump through extra hoops to
+ * divide unsigned long longs within the kernel */
+ unsigned long long nativeMb, currentMb;
+ nativeMb = set_max_ext * 512;
+ currentMb = capacity_2 * 512;
+ /* do_div is a macro so we can not safely combine steps */
+ nativeMb = do_div(nativeMb, 1000000);
+ currentMb = do_div(currentMb, 1000000);
+ printk(KERN_INFO "%s: Host Protected Area detected.\n"
+ " current capacity is %lld sectors (%lld MB)\n"
+ " native capacity is %lld sectors (%lld MB)\n",
+ drive->name, capacity_2, 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);
+ set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
+ if (set_max_ext) {
+ drive->capacity48 = drive->id->lba_capacity_2 = set_max_ext;
+ drive->cyl = (unsigned int) set_max_ext / (drive->head * drive->sect);
+ drive->select.b.lba = 1;
+ printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
+ }
#endif /* CONFIG_IDEDISK_STROKE */
- }
+ }
+ } else {
+ unsigned long set_max;
+ set_max = idedisk_read_native_max_address(drive);
- drive->capacity = capacity;
+ if (set_max > capacity)
+ {
+ printk(KERN_INFO "%s: Host Protected Area detected.\n"
+ " current capacity is %ld sectors (%ld MB)\n"
+ " native capacity is %ld sectors (%ld 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 = idedisk_set_max_address(drive, set_max);
+ if (set_max) {
+ drive->capacity = drive->id->lba_capacity = set_max;
+ drive->cyl = set_max / (drive->head * drive->sect);
+ drive->select.b.lba = 1;
+ printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
+ }
+#endif /* CONFIG_IDEDISK_STROKE */
+ }
- 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);
+ }
}
}
[-- Attachment #3: fixup-ide-hpa-2.6.patch --]
[-- Type: text/plain, Size: 7127 bytes --]
--- linux-2.6.0-test2/drivers/ide/ide-disk.c.orig 2003-07-27 11:01:09.000000000 -0600
+++ linux-2.6.0-test2/drivers/ide/ide-disk.c 2003-08-02 17:17:27.000000000 -0600
@@ -67,6 +67,7 @@
#include <asm/irq.h>
#include <asm/uaccess.h>
#include <asm/io.h>
+#include <asm/div64.h>
/* FIXME: some day we shouldn't need to look in here! */
@@ -1066,18 +1067,6 @@
#endif /* CONFIG_IDEDISK_STROKE */
/*
- * Tests if the drive supports Host Protected Area feature.
- * Returns true if supported, false otherwise.
- */
-static inline int idedisk_supports_host_protected_area(ide_drive_t *drive)
-{
- 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;
-}
-
-/*
* Compute drive->capacity, the full capacity of the drive
* Called with drive->id != NULL.
*
@@ -1091,77 +1080,103 @@
* in above order (i.e., if value of higher priority is available,
* reset will be ignored).
*/
-#define IDE_STROKE_LIMIT (32000*1024*2)
static void init_idedisk_capacity (ide_drive_t *drive)
{
- 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;
-
- (void) idedisk_supports_host_protected_area(drive);
-
- 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) {
-#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 */
- }
- 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 */
+ struct hd_driveid *id;
+ unsigned long capacity;
+ unsigned long long capacity_2;
+
+ id = drive->id;
+ capacity = drive->cyl * drive->head * drive->sect;
+ capacity_2 = capacity;
+
+
+ /* Does this drive support the 48-bit Address Feature Set
+ * and does it have that enabled at the moment? */
+ if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400)) {
+ //drive speaks 48-bit LBA
+ drive->capacity = (unsigned long) capacity_2 = id->lba_capacity_2;
+ drive->capacity48 = 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->bios_cyl = drive->cyl;
+ drive->select.b.lba = 1;
} else if ((id->capability & 2) && lba_capacity_is_ok(id)) {
- capacity = id->lba_capacity;
+ // drive speaks 28-bit LBA
+ drive->capacity = capacity = id->lba_capacity;
+ drive->capacity48 = 0;
drive->cyl = capacity / (drive->head * drive->sect);
drive->select.b.lba = 1;
+ } else {
+ // drive speaks boring old 28-bit CHS
+ drive->capacity = capacity;
+ drive->capacity48 = 0;
+ drive->select.b.lba = 0;
}
- if (set_max > capacity && capacity > IDE_STROKE_LIMIT) {
+
+ /* If this drive supports the Host Protected Area feature set,
+ * then we may need to change our opinion about the drive's size... */
+ if (id->command_set_1 & 0x0400 && id->cfs_enable_1 & 0x0400)
+ {
+ /* Does this drive support the 48-bit Address Feature Set
+ * and does it have that enabled as well? */
+ if ((id->command_set_2 & 0x0400) && (id->cfs_enable_2 & 0x0400))
+ {
+ unsigned long long set_max_ext;
+ set_max_ext = idedisk_read_native_max_address_ext(drive);
+
+ /* If our capacity is smaller than the native capacity,
+ * we have an HPA created using SET MAX ADDRESS EXT... */
+ if (set_max_ext > capacity_2)
+ {
+ /* Sigh. We have to jump through extra hoops to
+ * divide unsigned long longs within the kernel */
+ unsigned long long nativeMb, currentMb;
+ nativeMb = set_max_ext * 512;
+ currentMb = capacity_2 * 512;
+ /* do_div is a macro so we can not safely combine steps */
+ nativeMb = do_div(nativeMb, 1000000);
+ currentMb = do_div(currentMb, 1000000);
+ printk(KERN_INFO "%s: Host Protected Area detected.\n"
+ " current capacity is %lld sectors (%lld MB)\n"
+ " native capacity is %lld sectors (%lld MB)\n",
+ drive->name, capacity_2, 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);
+ set_max_ext = idedisk_set_max_address_ext(drive, set_max_ext);
+ if (set_max_ext) {
+ drive->capacity48 = drive->id->lba_capacity_2 = set_max_ext;
+ drive->cyl = (unsigned int) set_max_ext / (drive->head * drive->sect);
+ drive->select.b.lba = 1;
+ printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
+ }
#endif /* CONFIG_IDEDISK_STROKE */
- }
+ }
+ } else {
+ unsigned long set_max;
+ set_max = idedisk_read_native_max_address(drive);
- drive->capacity = capacity;
+ if (set_max > capacity)
+ {
+ printk(KERN_INFO "%s: Host Protected Area detected.\n"
+ " current capacity is %ld sectors (%ld MB)\n"
+ " native capacity is %ld sectors (%ld 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 = idedisk_set_max_address(drive, set_max);
+ if (set_max) {
+ drive->capacity = drive->id->lba_capacity = set_max;
+ drive->cyl = set_max / (drive->head * drive->sect);
+ drive->select.b.lba = 1;
+ printk(KERN_INFO "%s: Host Protected Area Disabled\n", drive->name);
+ }
+#endif /* CONFIG_IDEDISK_STROKE */
+ }
- 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);
+ }
}
}
next prev parent reply other threads:[~2003-08-02 23:34 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2003-08-02 8:42 ide-disk.c rev 1.13 killed CONFIG_IDEDISK_STROKE Erik Andersen
2003-08-02 12:45 ` Andries Brouwer
2003-08-02 13:10 ` Bartlomiej Zolnierkiewicz
2003-08-02 17:42 ` Andries Brouwer
2003-08-02 21:06 ` Alan Cox
2003-08-02 23:34 ` Erik Andersen [this message]
2003-08-03 1:26 ` [PATCH] " Andries Brouwer
2003-08-03 2:12 ` Erik Andersen
2003-08-03 9:52 ` Jens Axboe
[not found] <20030806181142.GD25910@codepoet.org>
2003-08-06 18:32 ` Bartlomiej Zolnierkiewicz
2003-08-06 19:30 ` Erik Andersen
2003-08-06 19:58 ` Bartlomiej Zolnierkiewicz
2003-08-07 1:11 ` Andries Brouwer
2003-08-07 2:31 ` Bartlomiej Zolnierkiewicz
2003-08-07 1:59 Andries.Brouwer
2003-08-07 9:57 Andries.Brouwer
2003-08-07 10:44 ` Bartlomiej Zolnierkiewicz
2003-08-07 11:23 Andries.Brouwer
2003-08-07 11:49 ` Bartlomiej Zolnierkiewicz
2003-08-07 13:22 Andries.Brouwer
2003-08-07 13:50 ` Bartlomiej Zolnierkiewicz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20030802233438.GA7652@codepoet.org \
--to=andersen@codepoet.org \
--cc=B.Zolnierkiewicz@elka.pw.edu.pl \
--cc=aebr@win.tue.nl \
--cc=alan@lxorguk.ukuu.org.uk \
--cc=axboe@suse.de \
--cc=linux-kernel@vger.kernel.org \
--cc=marcelo@conectiva.com.br \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).